search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.08k stars 1.56k forks source link

Analyzer Plugin incorrectly linting dependency packages #50749

Open pattobrien opened 1 year ago

pattobrien commented 1 year ago

See example project in sidecar repo for reproducible project.

When an analyzer plugin (such as sidecar) is enabled for a package, lints will appear for dependency projects too (such as my_backend_client in packages folder), even though sidecar plugin was not explicitly enabled for the dependency.

Expected Behavior: from @bwilkerson (source):

If a plugin is passed any analysis contexts for which it shouldn't be run, then that's a bug. It wasn't intended to work that way. The design was that each plugin would be passed exactly the list of analysis contexts for which the plugin had been enabled and nothing else.

Dart SDK version: 2.18.2 (stable) (Tue Sep 27 13:24:11 2022 +0200) on "macos_arm64"

incendial commented 1 year ago

Not sure if this one is new, but might be https://github.com/dart-lang/sdk/issues/49404 (since you have a bit outdated SDK version as well). The fix for DCM was https://github.com/dart-code-checker/dart-code-metrics/blob/master/lib/src/analyzer_plugin/analyzer_plugin.dart#L61

pattobrien commented 1 year ago

hmm no I've been using analyzer_plugin v0.11.1, and the issue is slightly different (extra AnalysisContexts in the collection).

but do you not have the issue with DCM? because in that case it must be an implementation issue on my end.

incendial commented 1 year ago

Hm, I initially thought you were referring to "same level packages". But it's about nested, my bad.

I think we have this behaviour too (I'm actually not sure this is a bug, coz people might want to set up the analysis config for the root package and then it will analyze all nested too), but I'll check and return with the exact answer.

pattobrien commented 1 year ago

Okay, thanks for looking into it! I agree that I thought it was working to spec, for the same reason you said, but according to this discussion from the analyzer team its not supposed to work that way:

If a plugin is passed any analysis contexts for which it shouldn't be run, then that's a bug. It wasn't intended to work that way. The design was that each plugin would be passed exactly the list of analysis contexts for which the plugin had been enabled and nothing else.

I do think that explicitly enabling plugins for each package is at least intuitive and would match how the official linter rules work too, as long as nested packages can inherit plugins from an imported analysis config from the root package - though I haven't tested if this is the case.

bwilkerson commented 1 year ago

... then that's a bug.

But note that I never said that there weren't bugs in this unsupported prototype.

... as long as nested packages can inherit plugins from an imported analysis config from the root package ...

Given a directory D to be analyzed, we walk the directory structure (starting in D and moving toward the root of the file system) looking for an analysis_options.yaml file. When we find it we stop looking and use that file to control analysis in the directory D.

So, yes, if the first analysis_options.yaml file we find when searching from in the directory D containing the nested package is in the root package, then the nested package will "inherit" it.

srawlins commented 1 year ago

Sounds like this is expected and intended.

pattobrien commented 1 year ago

Sorry for the delay in responding (holidays and such).

But note that I never said that there weren't bugs in this unsupported prototype.

Right, and my expectations for filing any issues are low accordingly.

Sounds like this is expected and intended.

I don't believe so. Currently AnalysisErrors appear in a codebase even when a package doesn't explicitly enable the corresponding plugin (inherited from nearby analysis_options.yaml), yet the same doesn't happen for Assists. My assumption (and more importantly: my plugin's users' assumptions) should be: wherever lints appear, so should assists.

I set up a simple plugin to demonstrate the issue for you. There's a Lint warning that will trigger on all analyzable files, and an assist action named "Test Assist" that will appear for any text in the same files. Both the lint and quick assist appear for files in the example target package (which has the plugin explicitly enabled). However, for the package under example/packages/my_dependency/ (which doesn't explicitly enable the plugin), the lint appears but the quick assist does not. By "explicitly enable", I mean that there's an analysis_options.yaml file at the root, and that the files contains the plugin under analyzer > plugins.

In order to get quick assists to appear for a given target package, you must explicitly enable the plugin in every target's analysis_options.yaml file. The same is not true for lints. This will be a source of confusion for end users of any plugin.