Multiple folders output by "pub list-package-dirs" can lead to analysis loop

[stereotype441]

In circumstances where analysis server's execution of "pub list-package-dirs" yields a mapping from a single package name to multiple source directories (e.g. package "foo" maps to ["/foo1/lib", "/foo2/lib"], there is a risk of creating an analysis loop. The analysis loop occurs as follows:

• A given file exists in more than one source directory (e.g. files "/foo1/lib/bar.dart" and "/foo2/lib/bar.dart" both exist).
• The file in the latter directory appears explicitly in an analysis context (e.g. "/foo2" is set as an analysis context, causing "/foo2/lib/bar.dart" to be analyzed).
• Before analyzing the file, the analysis server converts the explicit file path to a package URI (in accordance with item 1 in issue Change analysis server's handling of package vs file URI's, and add audit rules. #22079). In this example, this yields "package:foo/bar.dart". So it passes a FileBasedSource to the analyzer which points to "package:foo/bar.dart" and "/foo2/lib/bar.dart".
• Later, the analyzer encounters the URI in another context (e.g. an import statement) and resolves it to the file in the former directory. So in this example, this causes it to create a FileBasedSource which points to "package:foo/bar.dart" and "/foo1/lib/bar.dart".
• Since FileBasedSource objects are equality compared based on their URI but hashed based on their file, the two FileBasedSource objects may or may not be treated as equivalent by the analysis cache, depending on whether they happen to land in the same hash bucket.
• If they land in the same hash bucket, then at the end of analysis, when _validateCacheConsistency() runs, it may compare the stored modification timestamp of one file with the actual modification timestamp of the other file, and incorrectly conclude that the file has been changed.
• If this happens, the failed consistency check will trigger another round of analysis, after which the consistency check will run again (and fail again), triggering a further round of analysis, ad infinitum.

I'm working on a fix for this issue.

[bwilkerson]

Added this to the 1.9 milestone.
Removed Priority-Unassigned label.
Added Priority-High, Started labels.

[stereotype441]

Partial fix out for review: https://codereview.chromium.org/935453007/

[stereotype441]

Partially fixed in revision 44003.

Keeping the bug open since it could still arise if files are added/deleted after analysis server starts. The reason is that in general, the mapping from URI to file path depends on which files exist in the filesystem, so even after this CL is applied, if files are added/deleted after analysis begins, it's still possible for two FileBasedSource objects to be created that use the same URI but point to different files.

[stereotype441]

As of revision 44026, two FileBasedSource objects that point to different files will never compare equal, so the analysis loop shouldn't happen anymore.

It's still possible for the analysis server to get confused if adding/removing changes which file is meant by a certain URI, but that is a much lower priority problem, and one which will require a different sort of fix. I'll open a separate ticket for it if necessary.

---

Added Fixed label.