Flaky error when resolving (valid) annotations in analyzer

[jakemac53]

Example failure: https://travis-ci.org/github/dart-lang/angular/jobs/734348689

This is experienced consistently on travis, and approximately 1/3 of the time for me locally (glinux).

Repro instructions (unfortunately I can't make a small repro):

1. Clone the angular repo https://github.com/dart-lang/angular
2. Checkout the latest_dependencies branch
3. Cd into the angular_forms dir
4. Run pub upgrade
5. Run pub run build_runner build - you may need to do this several times before seeing the failure (run pub run build_runner clean to clear your cache between runs).

Debugging instructions:

You can run pub run build_runner generate-build-script which will just generate the dart script and print its location - then you can run that script (passing build as the argument still) and debug it using your ide or whatever tool you use.

Collected information so far

I tried running on several versions of analyzer - this repros on 0.40.1 and above, but not 0.39.17 (0.40.0 isn't solvable via pub solves). So I am pretty sure this was introduced in either 0.40.0 or 0.40.1 of analyzer.

In order to test on 0.39.17 you will also end up downgrading build_resolvers (and the analyzer constraint in other packages), but there were no meaningful changes in build_resolvers regarding how the analyzer is used, so I don't believe it is a build_resolvers issue.

cc @kevmoo can you set prioritization?

[kevmoo]

Labeling P1 – this is a regression! CC @devoncarew @scheglov @bwilkerson

[kevmoo]

Really annoying: it never reproduces on my Mac. 🤷

[jakemac53]

Hmm actually I was wrong about there being no substantive changes in the build_resolvers code - it is possible this is at least related to the change in build_resolvers - specifically this PR dart-lang/build#2817.

I have tried now running with analyzer 0.40.1 and the old build_resolvers 1.3.11 and I can't repro the issue.

[jakemac53]

Ok! I think I have at least narrowed down which change in the dart-lang/build#2817 caused this - I am fairly sure it is specifically this change https://github.com/dart-lang/build/pull/2817/files#diff-87577255efaaf8017c6478e01573eea3R315 (hard to be 100% sure due to flakyness).

This should have no real effect, so it does seem like a lurking analyzer bug, triggered now by the fact that we more lazily resolve libraries (we wait until somebody actually asks for a LibraryElement - where we used to eagerly resolve the entrypoint library when you grab a resolver).

[kevmoo]

@scheglov – would you take a look here?

[scheglov]

I did the steps outlined in the initial comment, and get the following error.
Is this what this issue about?

scheglov@scheglov-macbookpro2:~/dart/angular/angular_forms (latest_dependencies)$ pub run build_runner build
Precompiling executable... (13.3s)
Precompiled build_runner:build_runner.
[INFO] Generating build script completed, took 490ms
[INFO] Creating build script snapshot... completed, took 16.4s
[INFO] Building new asset graph completed, took 2.0s
[INFO] Checking for unexpected pre-existing outputs. completed, took 2ms
[INFO] Generating SDK summary completed, took 3.1s
[SEVERE] angular:angular on package:angular/src/core/linker/component_factory.dart:
line 29, column 6 of package:angular/src/core/linker/component_factory.dart: Could not resolve "@experimental"
   ╷
29 │ bool debugUsesDefaultChangeDetection(ComponentRef componentRef) {
   │      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
Try the following when diagnosing the problem:
  * Check your IDE or with the dartanalyzer for errors or warnings
  * Check your "import" statements for missing or incorrect URLs

If you are still stuck, file an issue and include this error message:
https://github.com/dart-lang/angular/issues/new

[kevmoo]

Yup!

[scheglov]

FWIW, after I ran pub upgrade in all angular, angular_ast, angular_compiler, angular_router, and angular_test; and then removed all but package_config.json in angular_forms/.dart_tool, then I can run pub run build_runner build successfully. This raises the question of how the analyzer is invoked, and whether it can find right package_config.json files.

[natebosch]

then I can run pub run build_runner build successfully.

I believe the issue Jake was seeing was inconsistent. If you pub run build_runner clean and then pub run build_runner build a few times in a row it may surface again. I think Jake told me it generally happened within the first 3-5 tries.

This raises the question of how the analyzer is invoked, and whether it can find right package_config.json files.

We create a Packages instances for the AnalysisDriver from a PackageConfig:
https://github.com/dart-lang/build/blob/631119ff00e1e21ca74bd062dab4785f9103dfc5/build_resolvers/lib/src/analysis_driver.dart#L66-L84

The PackageConfig we load is from the same file as the current isolate uses:
https://github.com/dart-lang/build/blob/631119ff00e1e21ca74bd062dab4785f9103dfc5/build_resolvers/lib/src/resolver.dart#L304-L305

[scheglov]

Yes, I think you are right - I re-ran pub run build_runner clean; pub run build_runner build several times without .dart_tool anywhere but in angular_forms and after 2-3 successful runs I get such error run.

[scheglov]

OK, I added some debug output to the analyzer, and think that it might be working as expected.

The pertinent debug output:

[directive: import 'package:meta/meta.dart';][uri: package:meta/meta.dart][path: /meta/lib/meta.dart]
  [referencedFile: [id: 100][rid: 101]package:meta/meta.dart = /meta/lib/meta.dart][exists: false]
    [exists2: true]

What this means is that when we were resolving a Dart file, we found that the file that corresponds to package:meta/meta.dart does not exist. The reason, I suspect, is that we have already tried to read /meta/lib/meta.dart before it existed, and recorded this fact into the FileState model in the analyzer. There is AnalysisDriver.changeFile(path) that should be invoked when the client knows that the file changed, this instructs the driver to re-read the file. Otherwise, every file stays in the state which it was when it was the last (might be the first) time it was refreshed.

Another important piece of information is that AnalysisDriver.parseFile() reads all the file dependencies. This is not particularly optimal, and in general I'd like to stop (1) reading all dependencies; (2) keeping the content of all files as their were read. But this is how it is now.

[natebosch]

Another important piece of information is that AnalysisDriver.parseFile() reads all the file dependencies.

This appears to be the root of the problem. We implemented an API assuming that parseFile wouldn't read all dependencies.

I can crawl transitive imports when using parseFile like we do for the other usage, this make things more safe but potentially less performant. @jakemac53 probably has a better intuition for how bad it would be.

I think even outside of the parseFile case we actually have a potential bug around files that we hide from the analyzer if a build step isn't supposed to be able to read it - I haven't proven this for sure yet.

We do have a general bug where we fail to enforce certain async interleavings which we assume to be true. I think what can happen is that while we are crawling transitive imports we might decide to skip files which we expect are already handled (https://github.com/dart-lang/build/blob/88b534c838a71d7dd30eb1ab39154d41cb11ea38/build_resolvers/lib/src/build_asset_uri_resolver.dart#L110-L112), but the matching digest only guarantees that the MemorResourceProvider was updated (https://github.com/dart-lang/build/blob/88b534c838a71d7dd30eb1ab39154d41cb11ea38/build_resolvers/lib/src/build_asset_uri_resolver.dart#L119-L123), but it does not guarantee that the changeFile call was done. The buildStep which updated the cache may still be crawling imports and not have started to call changeFile yet. So in this case if the analyzer had previously read a missing/empty file (from a parseFile call in the likely case and the case we are hitting here) then the buildStep which assumed the cached file was ready would not see that source.

The reason we wait to call changeFile until after the full crawl is complete is because we saw other strange issues around (i think) the changeFile call also causing analyzer to want to read dependencies. I don't remember the specifics but it came up in dart-lang/build#2244

@kevmoo - how urgent is the fix? Should I go forward with a workaround that might impact performance?

@scheglov -

I'd like to stop (1) reading all dependencies;

How realistic is this?

Alternatively, is there some other API we can use to get a CompilationUnit that wouldn't corrupt the AnalysisDriver we are using for other things? Would it be reasonable to have an API that creates an isolated AnalysisDriver and can parse the single file with no deps available? How much overhead might this have compared to our current approach of using an existing AnalysisDriver?