Dart-Code / Dart-Code

Dart and Flutter support for VS Code
https://dartcode.org/
MIT License
1.49k stars 304 forks source link

Lints in analysis_options.yaml without a pubspec.yaml #2019

Open dcharkes opened 5 years ago

dcharkes commented 5 years ago

Question: Are lints defined in analysis_options.yaml only supposed to work when there is a pubspec.yaml right next to it? I only see lints show up in my editor in that situation.

However, if I run the dartanalyzer from the commandline it reports lints from the first analysis_options.yaml it finds in a parent folder.

DanTup commented 5 years ago

I can't repro this - I created an analysis_options.yaml one level up from my pubspec and enabled the avoid_as lint, and it still shows:

Screenshot 2019-09-30 at 3 42 18 pm

Can you give a specific example of what you're doing, and what the structure looks like in your explorer side bar? Thanks!

DanTup commented 5 years ago

I tried opening the foo folder directly too (so the analysis_options.yaml is outside of the visible/open folder) but it also still works.

dcharkes commented 5 years ago

I think the issue might be which analysis_options.yaml files are taken into account for a specific file when running dartanalyzer on a specific path. It is not the most specific analysis_options.yaml apparently.

It is the most specific analysis_options.yaml when calling dartanalyzer directly on that file, but it is not when calling dartanalyzer on an ancestor folder.

Maybe someone from the analyzer team can clarify this behavior. /cc @bwilkerson

@DanTup we run the analyzer on the opened folder (to analyze all files at the same time) instead of running the analyzer per file? That would be consistent with the command line behavior I see.

Two examples:

Example 1

If I change pkg/vm/analysis_options.yaml to

analyzer:
  exclude:
    - testcases/**
    - tool/**
linter:
  rules:
    - prefer_final_fields
    - prefer_final_in_for_each
    - prefer_final_locals

the IDE shows non-final fields/variables in for example pkg/vm/lib/transformations/mixin_deduplication.dart.

However, if I do not make the above change, and only create pkg/vm/lib/transformations/analysis_options.yaml and let it contain

linter:
  rules:
    - prefer_final_fields
    - prefer_final_in_for_each
    - prefer_final_locals

the same file does not get lints on non-finals.

Running on the command line on the file produces lints:

$ dartanalyzer ./pkg/vm/lib/transformations/mixin_deduplication.dart 
Analyzing pkg/vm/lib/transformations/mixin_deduplication.dart...
  lint • Prefer final in for-each loop variable if reference is not reassigned. • pkg/vm/lib/transformations/mixin_deduplication.dart:49:14 • prefer_final_in_for_each
...

However, running dartanalyzer on pkg/vm does not

$ dartanalyzer pkg/vm
Analyzing pkg/vm...
No issues found!

Example 2

Generated root analysis_options.yaml excludes the tests folder:

analyzer:
  exclude:
    - docs/newsletter/20171103/**
    - out/**
    - pkg/front_end/testcases/**
    - runtime/**
    - samples-dev/swarm/**
    - sdk/lib/**
    - sdk_nnbd/lib/**
    - tests/**
    - third_party/observatory_pub_packages/**
    - third_party/pkg/**
    - third_party/pkg_tested/dart_style/**
    - third_party/tcmalloc/**
    - tools/apps/update_homebrew/**
    - tools/dart2js/**
    - tools/dom/**
    - tools/sdks/dart-sdk/lib/**
    - tools/status_clean.dart
    - xcodebuild/**

And then a modified tests/ffi/analysis_options.yaml includes the subfolder tests/ffi:

analyzer:
  exclude:
    # Do analyze this subfolder in the tests/ even if tests/ is fully excluded.
linter:
  rules:
    - prefer_final_fields
    - prefer_final_in_for_each
    - prefer_final_locals

When running the dartanalyzer on a file from the command line you walk up until you find an analysis_options.yaml:

$ dartanalyzer ./tests/ffi/data_test.dart
  lint • Prefer final for variable declarations if they are not reassigned. • tests/ffi/data_test.dart:57:18 • prefer_final_locals
...

Curiously dartanalyzer ./ and dartanalyzer tests excludes tests/ all together, but dartanalyzer tests/ffi does analyze tests/ffi/*.dart.

DanTup commented 5 years ago

@DanTup we run the analyzer on the opened folder (to analyze all files at the same time) instead of running the analyzer per file? That would be consistent with the command line behavior I see.

In short, yes. Though things work slightly different in the editors - we don't invoke the analyzer directly to get errors, but rather run it in a server mode and tell it which folders to analyze (the "analysis roots"). This mode handles all of the language functionality (code completion, hovers, outline, etc.) and streams the errors to us (this also allows us to get errors for unsaved files - since we can tell the server as you edit your files, and it can analyze the unsaved buffer).

Which analysis_options files are used is decided by the server. I don't know all the details, but when you analyse a specific file I guess it would start searching there and go up the tree, whereas given a folder it might go the other way (search down from there). This might explain the difference. I think @bwilkerson has explained this somewhere in an issue before, but can't find it now. This might be related code though, which seems to consider a folder a root if it contains either file:

https://github.com/dart-lang/sdk/blob/22857411807ed3609847d32eb96341f34053cf59/pkg/analyzer/lib/src/dart/analysis/context_locator.dart#L239-L243

If the search direction is different, this might therefore end up with different roots (and different options). I don't know what the rules are around analysis_options lower down the tree though - Brian is probably best placed to answer this.

bwilkerson commented 5 years ago

The goal is for analyzer to use the closest analysis options file and the closest packages file when analyzing any given Dart file. The class ContextLocator was written to implement those semantics.

Historically, however, we implemented a different semantics, one in which the analysis options and packages files were determined based on the presence of a pubspec.yaml file. While the issues with that approach are clear today, that was the semantic desired at the time.

Unfortunately, the work to convert all of our tooling over to use the new semantics was put on hold and remains incomplete. This has left us in the unfortunate position that the tools all work slightly differently from one another and none of them fully implements the semantics we would like to provide.