Use single FileState for a path.

For some time now we decided to use canonical URIs for files in lib/.

Change-Id: I416ef87e7c4e6b27be9e1bc274deee67286a9d05
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/220223
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

pkg/analyzer/lib/src/context/source.dart

@@ -11,6 +11,7 @@ import 'package:analyzer/src/generated/sdk.dart';
 import 'package:analyzer/src/generated/source.dart';
 import 'package:analyzer/src/generated/utilities_dart.dart' as utils;
 import 'package:analyzer/src/source/package_map_resolver.dart';
+import 'package:analyzer/src/summary/package_bundle_reader.dart';
 import 'package:analyzer/src/util/file_paths.dart' as file_paths;
 import 'package:analyzer/src/workspace/package_build.dart';
 
@@ -137,6 +138,9 @@ class SourceFactoryImpl implements SourceFactory {
 
   @override
   Uri? restoreUri(Source source) {
+    if (source is InSummarySource) {
+      return source.uri;
+    }
     for (UriResolver resolver in resolvers) {
       // First see if a resolver can restore the URI.
       Uri? uri = resolver.restoreAbsolute(source);

pkg/analyzer/lib/src/dart/analysis/file_state.dart

@@ -35,6 +35,7 @@ import 'package:analyzer/src/summary/api_signature.dart';
 import 'package:analyzer/src/summary/package_bundle_reader.dart';
 import 'package:analyzer/src/summary2/informative_data.dart';
 import 'package:analyzer/src/util/either.dart';
+import 'package:analyzer/src/util/uri.dart';
 import 'package:analyzer/src/workspace/workspace.dart';
 import 'package:collection/collection.dart';
 import 'package:convert/convert.dart';
@@ -716,11 +717,8 @@ class FileSystemState {
   /// Mapping from a path to the flag whether there is a URI for the path.
   final Map<String, bool> _hasUriForPath = {};
 
-  /// Mapping from a path to the corresponding [FileState]s, canonical or not.
-  final Map<String, List<FileState>> _pathToFiles = {};
-
-  /// Mapping from a path to the corresponding canonical [FileState].
-  final Map<String, FileState> _pathToCanonicalFile = {};
+  /// Mapping from a path to the corresponding [FileState].
+  final Map<String, FileState> _pathToFile = {};
 
   /// We don't read parts until requested, but if we need to know the
   /// library for a file, we need to read parts of every file to know
@@ -775,7 +773,8 @@ class FileSystemState {
       }
     }
 
-    for (var file in _pathToFiles[path] ?? <FileState>[]) {
+    var file = _pathToFile[path];
+    if (file != null) {
       collectAffected(file);
     }
   }
@@ -808,25 +807,16 @@ class FileSystemState {
     return featureSetProvider.getLanguageVersion(path, uri);
   }
 
-  /// Return the canonical [FileState] for the given absolute [path]. The
-  /// returned file has the last known state since if was last refreshed.
-  ///
-  /// Here "canonical" means that if the [path] is in a package `lib` then the
-  /// returned file will have the `package:` style URI.
+  /// Return the [FileState] for the given absolute [path]. The returned file
+  /// has the last known state since if was last refreshed.
   FileState getFileForPath(String path) {
-    FileState? file = _pathToCanonicalFile[path];
+    var file = _pathToFile[path];
     if (file == null) {
       File resource = _resourceProvider.getFile(path);
       Source fileSource = resource.createSource();
       Uri? uri = _sourceFactory.restoreUri(fileSource);
-      // Try to get the existing instance.
-      file = _uriToFile[uri];
-      // If we have a file, call it the canonical one and return it.
-      if (file != null) {
-        _pathToCanonicalFile[path] = file;
-        return file;
-      }
       // Create a new file.
+      // TODO(scheglov) this is duplicate
       FileSource uriSource = FileSource(resource, uri!);
       WorkspacePackage? workspacePackage = _workspace?.findPackageFor(path);
       FeatureSet featureSet = contextFeatureSet(path, uri, workspacePackage);
@@ -834,9 +824,9 @@ class FileSystemState {
           contextLanguageVersion(path, uri, workspacePackage);
       file = FileState._(this, path, uri, uriSource, workspacePackage,
           featureSet, packageLanguageVersion);
+      _pathToFile[path] = file;
       _uriToFile[uri] = file;
       _addFileWithPath(path, file);
-      _pathToCanonicalFile[path] = file;
       file.refresh();
     }
     return file;
@@ -874,6 +864,13 @@ class FileSystemState {
 
       String path = uriSource.fullName;
       File resource = _resourceProvider.getFile(path);
+
+      var rewrittenUri = rewriteFileToPackageUri(_sourceFactory, uri);
+      if (rewrittenUri == null) {
+        return Either2.t1(null);
+      }
+      uri = rewrittenUri;
+
       FileSource source = FileSource(resource, uri);
       WorkspacePackage? workspacePackage = _workspace?.findPackageFor(path);
       FeatureSet featureSet = contextFeatureSet(path, uri, workspacePackage);
@@ -881,6 +878,7 @@ class FileSystemState {
           contextLanguageVersion(path, uri, workspacePackage);
       file = FileState._(this, path, uri, source, workspacePackage, featureSet,
           packageLanguageVersion);
+      _pathToFile[path] = file;
       _uriToFile[uri] = file;
       _addFileWithPath(path, file);
       file.refresh();
@@ -888,19 +886,6 @@ class FileSystemState {
     return Either2.t1(file);
   }
 
-  /// Return the list of all [FileState]s corresponding to the given [path]. The
-  /// list has at least one item, and the first item is the canonical file.
-  List<FileState> getFilesForPath(String path) {
-    FileState canonicalFile = getFileForPath(path);
-    List<FileState> allFiles = _pathToFiles[path]!.toList();
-    if (allFiles.length == 1) {
-      return allFiles;
-    }
-    return allFiles
-      ..remove(canonicalFile)
-      ..insert(0, canonicalFile);
-  }
-
   /// Return files where the given [name] is subtyped, i.e. used in `extends`,
   /// `with` or `implements` clauses.
   Set<FileState>? getFilesSubtypingName(String name) {
@@ -951,15 +936,9 @@ class FileSystemState {
   }
 
   void _addFileWithPath(String path, FileState file) {
-    var files = _pathToFiles[path];
-    if (files == null) {
-      knownFilePaths.add(path);
-      knownFiles.add(file);
-      files = <FileState>[];
-      _pathToFiles[path] = files;
-      fileStamp++;
-    }
-    files.add(file);
+    knownFilePaths.add(path);
+    knownFiles.add(file);
+    fileStamp++;
   }
 
   /// Clear all [FileState] data - all maps from path or URI, etc.
@@ -968,8 +947,7 @@ class FileSystemState {
     knownFilePaths.clear();
     knownFiles.clear();
     _hasUriForPath.clear();
-    _pathToFiles.clear();
-    _pathToCanonicalFile.clear();
+    _pathToFile.clear();
     _librariesWithoutPartsRead.clear();
     _partToLibraries.clear();
     _subtypedNameToFiles.clear();

pkg/analyzer/lib/src/dart/analysis/file_tracker.dart

@@ -166,15 +166,9 @@ class FileTracker {
     return _logger.run('Verify API signature of $path', () {
       _logger.writeln('Work in ${_fsState.contextName}');
 
-      bool anyApiChanged = false;
-      List<FileState> files = _fsState.getFilesForPath(path);
-      for (FileState file in files) {
-        bool apiChanged = file.refresh();
-        if (apiChanged) {
-          anyApiChanged = true;
-        }
-      }
-      if (anyApiChanged) {
+      var file = _fsState.getFileForPath(path);
+      var apiChanged = file.refresh();
+      if (apiChanged) {
         _logger.writeln('API signatures mismatch found.');
         // TODO(scheglov) schedule analysis of only affected files
         var pendingChangedFiles = <String>{};
@@ -190,10 +184,8 @@ class FileTracker {
         // Add files that directly import the changed file.
         for (String addedPath in addedFiles) {
           FileState addedFile = _fsState.getFileForPath(addedPath);
-          for (FileState changedFile in files) {
-            if (addedFile.importedFiles.contains(changedFile)) {
-              pendingImportFiles.add(addedPath);
-            }
+          if (addedFile.importedFiles.contains(file)) {
+            pendingImportFiles.add(addedPath);
           }
         }
 
@@ -220,7 +212,7 @@ class FileTracker {
         _pendingErrorFiles = pendingErrorFiles;
         _pendingFiles = pendingFiles;
       }
-      return files[0];
+      return file;
     });
   }
 

pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart

@@ -41,6 +41,7 @@ import 'package:analyzer/src/generated/error_verifier.dart';
 import 'package:analyzer/src/generated/ffi_verifier.dart';
 import 'package:analyzer/src/generated/resolver.dart';
 import 'package:analyzer/src/generated/source.dart';
+import 'package:analyzer/src/generated/utilities_dart.dart';
 import 'package:analyzer/src/hint/sdk_constraint_verifier.dart';
 import 'package:analyzer/src/ignore_comments/ignore_info.dart';
 import 'package:analyzer/src/lint/linter.dart';
@@ -49,6 +50,7 @@ import 'package:analyzer/src/services/lint.dart';
 import 'package:analyzer/src/summary/package_bundle_reader.dart';
 import 'package:analyzer/src/task/strong/checker.dart';
 import 'package:analyzer/src/util/performance/operation_performance.dart';
+import 'package:analyzer/src/util/uri.dart';
 import 'package:pub_semver/pub_semver.dart';
 
 var timerLibraryAnalyzer = Stopwatch();
@@ -772,6 +774,18 @@ class LibraryAnalyzer {
         featureSet: unit.featureSet, flowAnalysisHelper: flowAnalysisHelper));
   }
 
+  Uri? _resolveRelativeUri(String relativeUriStr) {
+    Uri relativeUri;
+    try {
+      relativeUri = Uri.parse(relativeUriStr);
+    } on FormatException {
+      return null;
+    }
+
+    var absoluteUri = resolveRelativeUri(_library.uri, relativeUri);
+    return rewriteFileToPackageUri(_sourceFactory, absoluteUri);
+  }
+
   /// Return the result of resolve the given [uriContent], reporting errors
   /// against the [uriLiteral].
   Source? _resolveUri(FileState file, bool isImport, StringLiteral uriLiteral,
@@ -813,12 +827,12 @@ class LibraryAnalyzer {
         directive.uriSource = defaultSource;
       }
       if (directive is NamespaceDirectiveImpl) {
-        var relativeUri = _selectRelativeUri(directive);
-        directive.selectedUriContent = relativeUri;
-        directive.selectedSource = _sourceFactory.resolveUri(
-          _library.source,
-          relativeUri,
-        );
+        var relativeUriStr = _selectRelativeUri(directive);
+        directive.selectedUriContent = relativeUriStr;
+        var absoluteUri = _resolveRelativeUri(relativeUriStr);
+        if (absoluteUri != null) {
+          directive.selectedSource = _sourceFactory.forUri2(absoluteUri);
+        }
         for (var configuration in directive.configurations) {
           configuration as ConfigurationImpl;
           var uriLiteral = configuration.uri;

pkg/analyzer/lib/src/summary2/element_builder.dart

@@ -13,6 +13,7 @@ import 'package:analyzer/src/summary2/library_builder.dart';
 import 'package:analyzer/src/summary2/link.dart';
 import 'package:analyzer/src/summary2/reference.dart';
 import 'package:analyzer/src/util/comment.dart';
+import 'package:analyzer/src/util/uri.dart';
 import 'package:collection/collection.dart';
 
 class ElementBuilder extends ThrowingAstVisitor<void> {
@@ -997,16 +998,24 @@ class ElementBuilder extends ThrowingAstVisitor<void> {
     if (relativeUriStr == null) {
       return null;
     }
-    var relativeUri = Uri.parse(relativeUriStr);
-    return resolveRelativeUri(_libraryBuilder.uri, relativeUri);
+
+    Uri relativeUri;
+    try {
+      relativeUri = Uri.parse(relativeUriStr);
+    } on FormatException {
+      return null;
+    }
+
+    var absoluteUri = resolveRelativeUri(_libraryBuilder.uri, relativeUri);
+
+    var sourceFactory = _linker.analysisContext.sourceFactory;
+    return rewriteFileToPackageUri(sourceFactory, absoluteUri);
   }
 
   LibraryElement? _selectLibrary(NamespaceDirective node) {
-    try {
-      var uri = _selectAbsoluteUri(node);
+    var uri = _selectAbsoluteUri(node);
+    if (uri != null) {
       return _linker.elementFactory.libraryOfUri('$uri');
-    } on FormatException {
-      return null;
     }
   }
 

pkg/analyzer/lib/src/util/uri.dart

@@ -2,6 +2,7 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'package:analyzer/src/generated/source.dart';
 import 'package:path/path.dart';
 
 String fileUriToNormalizedPath(Context context, Uri fileUri) {
@@ -10,3 +11,28 @@ String fileUriToNormalizedPath(Context context, Uri fileUri) {
   path = context.normalize(path);
   return path;
 }
+
+/// If the [absoluteUri] is a `file` URI that has corresponding `package` URI,
+/// return it. If the URI is not valid, e.g. has empty path segments, so
+/// does not represent a valid file path, return `null`.
+Uri? rewriteFileToPackageUri(SourceFactory sourceFactory, Uri absoluteUri) {
+  // Only file URIs get rewritten into package URIs.
+  if (!absoluteUri.isScheme('file')) {
+    return absoluteUri;
+  }
+
+  // It must be a valid URI, e.g. `file:///home/` is not.
+  var pathSegments = absoluteUri.pathSegments;
+  if (pathSegments.isEmpty || pathSegments.last.isEmpty) {
+    return null;
+  }
+
+  // We ask for Source only because `restoreUri` needs it.
+  // TODO(scheglov) Add more direct way to convert a path to URI.
+  var source = sourceFactory.forUri2(absoluteUri);
+  if (source == null) {
+    return null;
+  }
+
+  return sourceFactory.restoreUri(source);
+}

pkg/analyzer/test/src/dart/analysis/driver_test.dart

@@ -1952,16 +1952,13 @@ String z = "string";
       expect(result.errors, isEmpty);
     }
 
-    // Analysis of my_pkg/bin/b.dart produces the error "A value of type
-    // 'String' can't be assigned to a variable of type 'int'", because
-    // file:///my_pkg/bin/b.dart imports file:///my_pkg/lib/c.dart, which
-    // successfully imports file:///my_pkg/test/d.dart, causing y to have an
-    // inferred type of String.
+    // Analysis of my_pkg/bin/a.dart produces no error because
+    // the import `../lib/c.dart` is resolved to package:my_pkg/c.dart, and
+    // package:my_pkg/c.dart's import is erroneous, causing y's reference to z
+    // to be unresolved (and therefore have type dynamic).
     {
       ResolvedUnitResult result = await driver.getResultValid(b);
-      List<AnalysisError> errors = result.errors;
-      expect(errors, hasLength(1));
-      expect(errors[0].errorCode, CompileTimeErrorCode.INVALID_ASSIGNMENT);
+      expect(result.errors, isEmpty);
     }
   }
 
@@ -2028,8 +2025,10 @@ var VC = new A<double>();
 
     {
       ResolvedUnitResult result = await driver.getResultValid(b);
-      expect(_getImportSource(result.unit, 0).uri.toString(),
-          'package:test/a.dart');
+      expect(
+        _getImportSource(result.unit, 0).uri,
+        Uri.parse('package:test/a.dart'),
+      );
       _assertTopLevelVarType(result.unit, 'VB', 'A<int>');
     }
 
@@ -2037,7 +2036,7 @@ var VC = new A<double>();
       ResolvedUnitResult result = await driver.getResultValid(c);
       expect(
         _getImportSource(result.unit, 0).uri,
-        toUri('/test/lib/a.dart'),
+        Uri.parse('package:test/a.dart'),
       );
       _assertTopLevelVarType(result.unit, 'VC', 'A<double>');
     }

pkg/analyzer/test/src/dart/analysis/file_state_test.dart

@@ -23,7 +23,6 @@ import 'package:analyzer/src/generated/source.dart';
 import 'package:analyzer/src/source/package_map_resolver.dart';
 import 'package:analyzer/src/test_utilities/mock_sdk.dart';
 import 'package:analyzer/src/test_utilities/resource_provider_mixin.dart';
-import 'package:analyzer/src/util/either.dart';
 import 'package:analyzer/src/workspace/basic.dart';
 import 'package:convert/convert.dart';
 import 'package:crypto/crypto.dart';
@@ -268,8 +267,6 @@ class A1 {}
     expect(file.libraryFiles, [file, file.partedFiles[0]]);
 
     expect(_excludeSdk(file.directReferencedFiles), hasLength(5));
-
-    expect(fileSystemState.getFilesForPath(a1), [file]);
   }
 
   test_getFileForPath_onlyDartFiles() {
@@ -363,27 +360,6 @@ part 'not-a2.dart';
     );
   }
 
-  test_getFileForUri_packageVsFileUri() {
-    String path = convertPath('/aaa/lib/a.dart');
-    var packageUri = Uri.parse('package:aaa/a.dart');
-    var fileUri = toUri(path);
-
-    // The files with `package:` and `file:` URIs are different.
-    var filePackageUri = fileSystemState.getFileForUri(packageUri).asFileState;
-    var fileFileUri = fileSystemState.getFileForUri(fileUri).asFileState;
-    expect(filePackageUri, isNot(same(fileFileUri)));
-
-    expect(filePackageUri.path, path);
-    expect(filePackageUri.uri, packageUri);
-
-    expect(fileFileUri.path, path);
-    expect(fileFileUri.uri, fileUri);
-
-    // The file with the `package:` style URI is canonical, and is the first.
-    var files = fileSystemState.getFilesForPath(path);
-    expect(files, [filePackageUri, fileFileUri]);
-  }
-
   test_getFilesSubtypingName() {
     String a = convertPath('/a.dart');
     String b = convertPath('/b.dart');
@@ -783,12 +759,3 @@ class _SourceMock implements Source {
     throw StateError('Unexpected invocation of ${invocation.memberName}');
   }
 }
-
-extension on Either2<FileState?, ExternalLibrary> {
-  FileState get asFileState {
-    return map(
-      (file) => file!,
-      (_) => fail('Expected a file'),
-    );
-  }
-}