Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not start the Dart analysis server from the root if Dart projects are in subfolders #1295

Closed
dcharkes opened this issue Nov 12, 2018 · 27 comments

Comments

Projects
None yet
4 participants
@dcharkes
Copy link

commented Nov 12, 2018

This plugin always runs the analysis server from the root, resulting in a never terminating or hanging analysis in the dart-sdk. This is because the analyzer does not support analysis from the root of the dart-sdk project.

@dcharkes, thanks for the report! Generally, the use case of starting the analysis server from the root of the sdk is not supported.

@sjindel-google mentioned the Emacs plugin does not have the same issue, because it uses the .packages or pubspec.yaml files to determine from which folder to start analysis.

Would it be possible to get this behaviour in the VSCode plugin?

How to reproduce

  1. clone https://github.com/dart-lang/sdk
  2. open VSCode
  3. open the root folder of dart-lang/sdk
  4. enable Dart Code plugin

Analysis does not terminate, and generates 10000+ errors.

Expected behaviour

Analysis terminates normally (with a bunch of errors, as this is not a normal Dart project).

@sjindel-google

This comment has been minimized.

Copy link

commented Nov 12, 2018

I would actually expect there to be no errors if it's limiting the scope of analysis to a subdirectory which is part of our toolchain (e.g. pkg/vm).

@DanTup

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

@bwilkerson @devoncarew What are your thoughts on this? We could find all the package roots in the Dart Plugin and send each as an analysis root, but since the server normally handles this for us it feels a bit out of place to do it here (and if we move to LSP, we won't have the option of doing it on the client).

(@dcharkes I don't know if it helps in the meantime, but when I work on the analysis server I use the multi-root functionality in VS Code to open multiple SDK packages at once.. If you click File -> Open and then Shift-click multiple folders in the pkg folder, it'll open each one as its own workspace folder, which will set an analysis root on each one and - depending on the cause - may avoid this issue)

@bwilkerson

This comment has been minimized.

Copy link

commented Nov 12, 2018

'sjindel-google' mentioned the Emacs plugin does not have the same issue, because it uses the .packages or pubspec.yaml files to determine from which folder to start analysis.

Unfortunately, that won't help with the SDK repository because the .packages file is at the root of the repository, not in each of the packages.

If Emacs doesn't have this problem then it must be doing something other than looking for the .packages file (which would also match my understanding of what it's doing).

Analysis does not terminate, ...

I hope you mean that it does not terminate in a reasonable amount of time. If there's an infinite loop in the analysis then we need to find and fix it.

... and generates 10000+ errors.

I'm not surprised. The SDK repository contains a lot of files that are not normal Dart but are used by the VM, dart2js and ddc to help implement the core libraries. It also contains a lot of test files that, by design, contain syntactic and semantic errors.

One option would be for us to add an analysis_options.yaml file at the root of the SDK repository that excluded the directories for which analysis should not be performed (including the two cases given above).

I would actually expect there to be no errors if it's limiting the scope of analysis to a subdirectory which is part of our toolchain (e.g. pkg/vm).

If a user opens one of the directories under pkg then I would expect everything to be fine. I, and many other people, do that all the time and never have problems.

@sjindel-google

This comment has been minimized.

Copy link

commented Nov 12, 2018

Analysis does not terminate, ...

I hope you mean that it does not terminate in a reasonable amount of time. If there's an infinite loop in the analysis then we need to find and fix it.

There is an infinite loop: dart-lang/sdk#34925
As I understand, it will not be fixed until some future refactoring is complete.

@bwilkerson

This comment has been minimized.

Copy link

commented Nov 12, 2018

Ah, right. That one.

@DanTup

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

Based on:

Unfortunately, that won't help with the SDK repository because the .packages file is at the root of the repository, not in each of the packages.

I'm not sure if there's anything I can do in Dart Code here (and I'm also not sure if it'd help if there's a bug causing infinite loops when analyzing some of the code)?

If it only happens for some specific code, then using VS Code's multi-root workspaces may be the best fix. If you save a .code-workspace file in the repo, VS Code will also prompt you to switch to it if you open the root folder.

@dcharkes

This comment has been minimized.

Copy link
Author

commented Nov 14, 2018

I would actually expect there to be no errors if it's limiting the scope of analysis to a subdirectory which is part of our toolchain (e.g. pkg/vm).

(@dcharkes I don't know if it helps in the meantime, but when I work on the analysis server I use the multi-root functionality in VS Code to open multiple SDK packages at once.. If you click File -> Open and then Shift-click multiple folders in the pkg folder, it'll open each one as its own workspace folder, which will set an analysis root on each one and - depending on the cause - may avoid this issue)

There are .dart files with @patch in the folders I open in VS Code, as I'm editing the the .cc files right next to them. These will report errors.

So opening specific folders to help the Dart analyzer will not work in this scenario.

Maybe another solution would be to be able to whitelist or blacklist which files are analyzed?

Or an option to disable Dart analysis completely, that way this scenario would still benefit from Dart syntax highlighting.

@DanTup

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

Or an option to disable Dart analysis completely, that way this scenario would still benefit from Dart syntax highlighting.

Disabling the analyzer would also break all the other functionality (navigation, outline, formatting, completion, etc.) - is that what you're asking for? It seems a bit extreme.

I think the best option is definitely to fix the analyzer, but if that's going to take a while, another slightly less extreme fix could be that we let you override the analysis roots for a project. Eg. in the "Workspace Settings" JSON file, you specify the paths you want set as roots:

screen shot 2018-11-14 at 2 42 16 pm

Then in the extension we just use that array in preference to the one we normally calculate if it exists. That's not a big change (and one that doesn't feel super-specific to working around this issue) so if it sounds promising I could send you a build to test it out this week.

@DanTup

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

(and I guess, if you want, you could set that to an empty array, but I don't know exactly what would/wouldn't work as you start opening Dart files if you did that - it might do what you want, but you might find errors hanging around due to the issue mentioned at dart-lang/sdk#25551 (comment))

@bwilkerson

This comment has been minimized.

Copy link

commented Nov 14, 2018

There are .dart files with @patch in the folders I open in VS Code ...

Are these valid Dart files, or are they a special format that the VM uses but analyzer doesn't (yet) know about?

@sjindel-google

This comment has been minimized.

Copy link

commented Nov 14, 2018

They are internal files for the VM and (I think) Dart2JS. They allow backends to override classes and methods defined in the core libraries with implementation-specific versions, as well as to add methods to core library classes.

@dcharkes

This comment has been minimized.

Copy link
Author

commented Nov 14, 2018

another slightly less extreme fix could be that we let you override the analysis roots for a project. Eg. in the "Workspace Settings" JSON file, you specify the paths you want set as roots

This would work for packages under pkg. Actually, opening pkg itself does not result in any analysis errors. And opening the sdk root also does not result in any Dart errors in the pkg folder. So it seems that the analysis root mechanism already works.

I'm not surprised. The SDK repository contains a lot of files that are not normal Dart but are used by the VM, dart2js and ddc to help implement the core libraries. It also contains a lot of test files that, by design, contain syntactic and semantic errors.

Indeed all errors are in:

  • runtime/, which among others contains these _patch.dart files for overriding things in the core libraries - which the analyzer doesn't know about
  • tests/ which contains on purpose failing tests - while the analyzer doesn't know about test expectations
  • tools/dom/ (unsure what's going on here)
  • sdks/dart-sdk/lib/ which contains the standard library - which throws funny errors such as A value of type 'String (/Users/me/dart-sdk/sdk/xcodebuild/DebugX64/dart-sdk/lib/core/string.dart)' can't be assigned to a variable of type 'String (/Users/me/dart-sdk/sdk/sdk/lib/core/string.dart)'. in lib/core/uri.dart:4000

So maybe being able to blacklist these folders from analysis as suggested by @bwilkerson is the best option.

Disabling the analyzer would also break all the other functionality (navigation, outline, formatting, completion, etc.) - is that what you're asking for? It seems a bit extreme.

I know, but currently I do not even have syntax highlighting because I have to disable the Dart plugin.

@DanTup

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

Ok, us me know how it goes.

@bwilkerson

This comment has been minimized.

Copy link

commented Nov 14, 2018

runtime/, which among others contains these _patch.dart files ...

Analyzer could treat these files specially, but that wouldn't fix the other issues. Alternately, have we considered the possibility of naming those files something like core.patch?

sdks/dart-sdk/lib/ which contains the standard library ...

The problem here is that analyzer doesn't know that it ought to use these files as the source of the SDK, so they're treated the same as if some user sat down and tried to define a class named String (or whatever). It effectively sees two definitions of String from two different libraries, one in the SDK and one in some random file.

We could fix this, but it's never been high enough priority to get done.

@dcharkes

This comment has been minimized.

Copy link
Author

commented Nov 14, 2018

One option would be for us to add an analysis_options.yaml file at the root of the SDK repository that excluded the directories for which analysis should not be performed (including the two cases given above).

Putting analysis_options.yaml in the root with the following contents works.

analyzer:
  exclude:
    - docs/newsletter/20171103/**
    - out/**
    - runtime/**
    - samples-dev/swarm/**
    - sdk/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/**

@bwilkerson, is the above what you would expect to be required?

It looks like some folders already manage which files need to be analyzed with their own analysis_option.yaml excludes:

$ find * -name analysis_options.yaml | xargs grep "exclude:"
pkg/vm/analysis_options.yaml
pkg/dev_compiler/analysis_options.yaml
pkg/analyzer_cli/test/data/exclude_test_project/analysis_options.yaml
pkg/analyzer_cli/analysis_options.yaml
pkg/kernel/analysis_options.yaml
pkg/front_end/analysis_options.yaml
runtime/observatory/analysis_options.yaml
tests/compiler/dart2js/analysis_options.yaml
third_party/pkg/linter/analysis_options.yaml
third_party/pkg/dartdoc/testing/test_package_flutter_plugin/analysis_options.yaml
third_party/pkg/dartdoc/analysis_options.yaml

But most folders do not manage their own files to be excluded from the analyzer.

The question is if we add such a file to the root, would we break any developer workflow? Or can we argue that no-one uses the analyzer from the sdk root?

@dcharkes

This comment has been minimized.

Copy link
Author

commented Nov 14, 2018

(Note: I presume this would also fix this analyzer problem in IntelliJ/CLion, because it is analyzer specific, not IDE specific.)

@bwilkerson

This comment has been minimized.

Copy link

commented Nov 14, 2018

is the above what you would expect to be required?

I'm a little surprised that we need to exclude the packages in third_party, but that list looks generally like what I'd expect.

The question is if we add such a file to the root, would we break any developer workflow?

I don't know. I do know that there are likely to be some strong opinions about whether we ought to have an analysis options file at the root of the repo. :-)

Or can we argue that no-one uses the analyzer from the sdk root?

Well, given how broken it is ... :-)

It looks like some folders already manage which files need to be analyzed ...

Interesting. Currently we look in the folder that was opened and if it contains an options file we use it. If not, then we repeat the process in the parent folder until we reach the root of the file system. That means that if the user opens a subdirectory that contains an options file we'll ignore the one at the top level. (There is a way to chain options files, so we could get around this.)

Also, we currently ignore options files in subfolders. That means that if the sdk root directory is opened we will ignore the options file you listed. (There are plans to change this in the future so that nested options files are not ignored, but that's probably a few months out.)

@dcharkes

This comment has been minimized.

Copy link
Author

commented Nov 15, 2018

Or can we argue that no-one uses the analyzer from the sdk root?

Well, given how broken it is ... :-)

Okay I'll make a CL for this

there are likely to be some strong opinions

But if no one uses the analyzer from the root, they are only affected by the fact that there exists one extra file in the root. I'd say that is not much of a concern. :-)

@dcharkes

This comment has been minimized.

Copy link
Author

commented Nov 15, 2018

The analyzer try jobs for this CL fail, for example:

FAILED: dart2analyzer-none release_x64 co19_2/LibTest/isolate/Isolate/spawn_A02_t05
Expected: Pass
Actual: MissingCompileTimeError
Missing expected compile error.

--- Command "dart2analyzer" (took 1ms):
DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/dart-sdk/bin/dartanalyzer --sync-async --ignore-unrecognized-flags --packages=/b/s/w/ir/cache/builder/sdk/.packages --format=machine --no-hints /b/s/w/ir/cache/builder/sdk/tests/co19_2/src/LibTest/isolate/Isolate/spawn_A02_t05.dart

exit code:
1

stderr:
No dart files found at: /b/s/w/ir/cache/builder/sdk/tests/co19_2/src/LibTest/isolate/Isolate/spawn_A02_t05.dart

--- Re-run this test:
python tools/test.py -m release -n analyzer-linux --write-logs co19_2/LibTest/isolate/Isolate/spawn_A02_t05

I can't reproduce these crashes locally, python tools/test.py -m debug -n analyzer-macos co19_2/LibTest/isolate/Isolate/spawn_A02_t05 succeeds.

@bwilkerson any reason adding analysis_options.yaml at the root would trigger No dart files found?

@bwilkerson

This comment has been minimized.

Copy link

commented Nov 15, 2018

I suspect that when analyzer is asked to analyze the test file it finds the analysis options file in the root directory, and that because the file excludes the test file the analyzer decides that there are no files to analyze. Seems like the command-line tool should analyze explicitly mentioned files even if they're excluded, but I'm guessing it doesn't work that way today.

@dcharkes

This comment has been minimized.

Copy link
Author

commented Nov 16, 2018

Would that be easy to fix?

If not, maybe manually adding analysis_options.yaml to the VM workflow is the best solution.

@bwilkerson

This comment has been minimized.

Copy link

commented Nov 16, 2018

It should be fairly easy; it's just a question of when the work would be scheduled. A temporary solution might be best for now.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Jan 4, 2019

[tools] repurpose generate_compile_commands.py to generate_idefiles.py
and let it generate a configuration file for the c++ analyzers and the Dart analyzer

Workaround for: Dart-Code/Dart-Code#1295
Change-Id: I6d1d8100649116c2fc8325cf73c4bfc11f9eacb3
Reviewed-on: https://dart-review.googlesource.com/c/88061
Reviewed-by: Stevie Strickland <sstrickl@google.com>
@dcharkes

This comment has been minimized.

Copy link
Author

commented Jan 4, 2019

I landed a workaround in dart-lang/sdk. If anyone is stumbling on this issue, run tools/generate_idefiles.py to generate an analysis_options.yaml.

@DanTup

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

Great! Is there anything to do in Dart Code for this? I think all of the possible fixes would be in the SDK repo in some form, so I don't know if there's much do to this in this repo with this issue?

@dcharkes

This comment has been minimized.

Copy link
Author

commented Jan 4, 2019

I don't think so. So we can close this issue.

@bwilkerson should we open an analyzer issue? Or will it never be scheduled? In that case we should just stick to the workaround.

@DanTup

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

Ok thanks, I'll close this and leave it to you two to decide if there should be an analyzer issue. Thanks!

@bwilkerson

This comment has been minimized.

Copy link

commented Jan 4, 2019

... should we open an analyzer issue? Or will it never be scheduled?

I don't actually control the scheduling of analyzer issues, so I can't say for sure. I think it's worth opening an issue so that, if nothing else, we have some history in a place where we might be able to find it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.