[compiler-v2] Externalize the linter architecture #15138
Conversation
⏱️ 1h 4m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vineethk
force-pushed
the
vk/linter-externalize
branch
2 times, most recently
from
8c7392f to
ea90b83
Compare
vineethk
changed the title
vineethk
requested review from
gregnazario,
banool,
davidiw,
movekevin and
wrwg
as code owners
vineethk
requested review from
rahxephon89,
fEst1ck and
brmataptos
and removed request for
davidiw,
gregnazario,
banool and
movekevin
| fn visit_expr_post(&mut self, _env: &GlobalEnv, _expr: &ExpData) {} | ||
|
|
||
| /// Report the `msg` highlighting the `loc`. | ||
| fn report(&self, env: &GlobalEnv, loc: &Loc, msg: &str) { |
There was a problem hiding this comment.
maybe it is an overkill but method get_name and report are common among ExpChecker and StacklessBytecodeChecker, I am wondering whether it is worth to extract them into a super trait?
There was a problem hiding this comment.
You might even use a single trait with all functions and just stub out the ones that aren't useful.
There was a problem hiding this comment.
- If we extract
get_name()into a super trait, then each lint would have to implement two traits (either the exp checker or stackless bytecode checker, and then this trait withget_name). I feel like this is currently an overkill, but can be revisited as these traits evolve to support more functionality. - Implementors of these two traits are called in different places, e.g., each expression as we are visiting them for exp checkers, and each function for stackless bytecode checkers. Putting them together conflates two different concepts, IMO.
| │ | ||
| 212 │ *&mut u | ||
| │ ^^^^^^^ | ||
| │ | ||
| = To suppress this warning, annotate the function/module with the attribute `#[lint::skip(needless_deref_ref)]`. | ||
|
|
||
|
|
||
| Diagnostics: |
There was a problem hiding this comment.
So we're suppressing all normal compiler warnings here? What about a compiler error?
There was a problem hiding this comment.
Actually, please add a test to show what happens with different kinds of compiler errors (parser, type-check).
There was a problem hiding this comment.
No, we are not suppressing all normal compiler warnings here. This warning just ended up moving elsewhere in the expected file, but is still present. The difference is due to the way compiler-v2's testsuite prints out warnings vs how the compiler API prints out the warnings (the lint crate uses the compiler API).
There are already some tests with compiler errors here: e.g., third_party/move/tools/move-linter/tests/model_ast_lints/bad_lint_attribute_01.exp.
| @@ -112,7 +113,7 @@ impl CliCommand<&'static str> for LintPackage { | |||
|
|
|||
| async fn execute(self) -> CliTypedResult<&'static str> { | |||
| let move_options = MovePackageDir { | |||
| compiler_version: Some(CompilerVersion::V2_0), | |||
| compiler_version: Some(CompilerVersion::latest_stable()), | |||
There was a problem hiding this comment.
Good idea, but we maybe should have a separate PR to fix these.
There was a problem hiding this comment.
It is always good to fix code on the fly, specifically such trivialities.
There was a problem hiding this comment.
No, because the problem may be more widespread. Unless you have time to go fix all the use cases, you've just swept one case under the rug. If you do this, do a thorough search or file an issue to follow up and do it.
You also will have a commit message that says this line was changed to "Externalize the linter architecture". Is that useful for future devs? No.
SWE is communication, not just twiddling dials to make it work.
There was a problem hiding this comment.
For this PR, keeping this change.
Will follow up with another PR that does a more wide-spread fix across the code base.
There was a problem hiding this comment.
FYI, I filed an issues #15178 to track this.
| .get_exp_checkers() | ||
| .into_iter() | ||
| .map(|c| c.get_name()) | ||
| .collect::<Vec<_>>() |
There was a problem hiding this comment.
Is this an efficiency hack? You can actually apply .join(", ") directly to the iterator, though I think I've seen some recommendation that collecting into a vector first is actually faster.
There was a problem hiding this comment.
We cannot apply .join(", ") directly to the iterator.
There was a problem hiding this comment.
https://www.reddit.com/r/rust/comments/6q4uqc/help_whats_the_best_way_to_join_an_iterator_of/, maybe it's in itertools so not visible by default.
| fn visit_expr_post(&mut self, _env: &GlobalEnv, _expr: &ExpData) {} | ||
|
|
||
| /// Report the `msg` highlighting the `loc`. | ||
| fn report(&self, env: &GlobalEnv, loc: &Loc, msg: &str) { |
There was a problem hiding this comment.
You might even use a single trait with all functions and just stub out the ones that aren't useful.
| .and_then(|bp| bp.compile_no_exit(&compiler_config.clone(), &mut Vec::new())) | ||
| { | ||
| (true, _) => match BuildPlan::create(resolved_package).and_then(|bp| { | ||
| bp.compile_no_exit(&compiler_config.clone(), vec![], &mut Vec::new()) |
There was a problem hiding this comment.
The juxtaposition of two different ways to create an empty vector is kind of odd. How about
.., vec![], &mut vec![])
vineethk
force-pushed
the
vk/linter-externalize
branch
from
ea90b83 to
b9b5b53
Compare
vineethk
force-pushed
the
vk/linter-externalize
branch
from
b9b5b53 to
fa4e527
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
The goal of this PR is to make the compiler v2 accept an option to run externally provided lint checks. The compiler provides necessary traits for external implementors of the lint checks (on AST and stackless bytecode).
Further, all the lint checks inside the compiler are now moved out to a new (third-party) crate called
move-linter, which is expected to contain aptos move specific lint checks. The corresponding tests have also been moved.The aptos CLI lint tool (
aptos move lint) invokes the compiler v2 with the checks insidemove-lintercrate: thus, the existing behavior is unchanged. In the future, it could implement some of the extended checks and even additional checks (such as those particular to aptos framework and ecosystem) and pass along those checks as well, in addition to the checks inmove-linter.A follow up PR will introduce
lint.tomlto be able to configure the linter, which would be passed to the linter factory making up the lint check instances.Other implementation options considered
CompilerConfig,BuildOptionsetc. to further minimize API changes. However, this requires implementing serialization/deserialization,PartialEqand other traits for dynamic trait objects.buildbehavior (dependency resolution, compiler driver, etc) for including external checks - this results in a lot of code duplication, which could hamper package system revamping.After attempting both these options, I settled on changing a handful of APIs (only those needed, we could expand to more on a need basis) to explicitly pass external checks where necessary.
How Has This Been Tested?
move-lintercrate)aptos move lintCLI command.Generally speaking, none of the user-visible behavior should have changed from this PR.
Type of Change
Which Components or Systems Does This Change Impact?