[cfe] Cleanup DillLibrary/ExportNameSpace implementation

This simplifies the computation of DillExportNameSpace and removes the
need for LazyNameSpace.

Change-Id: I6f29e9a6def47ed8751437b57c6ad7d29fcf494a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/429940
Reviewed-by: Chloe Stefantsova <[email protected]>

Author: johnniwinther

pkg/front_end/lib/src/base/incremental_compiler.dart

@@ -66,13 +66,12 @@ import '../api_prototype/incremental_kernel_generator.dart'
 import '../api_prototype/lowering_predicates.dart'
     show isExtensionThisName, syntheticThisName;
 import '../api_prototype/memory_file_system.dart' show MemoryFileSystem;
-import '../builder/builder.dart' show Builder, NamedBuilder;
+import '../builder/builder.dart' show Builder;
 import '../builder/declaration_builders.dart'
     show ClassBuilder, ExtensionBuilder, ExtensionTypeDeclarationBuilder;
 import '../builder/library_builder.dart'
     show CompilationUnit, LibraryBuilder, SourceCompilationUnit;
 import '../builder/member_builder.dart' show MemberBuilder;
-import '../builder/property_builder.dart';
 import '../codes/cfe_codes.dart';
 import '../dill/dill_class_builder.dart' show DillClassBuilder;
 import '../dill/dill_library_builder.dart' show DillLibraryBuilder;
@@ -82,7 +81,6 @@ import '../kernel/benchmarker.dart' show BenchmarkPhases, Benchmarker;
 import '../kernel/hierarchy/hierarchy_builder.dart' show ClassHierarchyBuilder;
 import '../kernel/internal_ast.dart' show VariableDeclarationImpl;
 import '../kernel/kernel_target.dart' show BuildResult, KernelTarget;
-import '../source/source_extension_builder.dart';
 import '../source/source_library_builder.dart'
     show
         ImplicitLanguageVersion,
@@ -592,23 +590,16 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
         /// dill library builders might have links (via export scopes) to the
         /// source builders. Patch that up.
 
-        // Maps from old library builder to map of new content.
-        Map<LibraryBuilder, Map<String, NamedBuilder>>? replacementMap = {};
+        // Map from old library builder to name space of new content.
+        Map<LibraryBuilder, NameSpace>? replacementNameSpaceMap = {};
 
-        // Maps from old library builder to map of new content.
-        Map<LibraryBuilder, Map<String, NamedBuilder>>? replacementSettersMap =
-            {};
-
-        Map<LibraryBuilder, NameSpace> replacementLookupMap = {};
-
-        _experimentalInvalidationFillReplacementMaps(convertedLibraries!,
-            replacementMap, replacementSettersMap, replacementLookupMap);
+        _experimentalInvalidationFillReplacementMaps(
+            convertedLibraries!, replacementNameSpaceMap);
 
         for (DillLibraryBuilder builder
             in experimentalInvalidation.originalNotReusedLibraries) {
           if (builder.isBuilt) {
-            builder.patchUpExportScope(
-                replacementMap, replacementSettersMap, replacementLookupMap);
+            builder.patchUpExportScope(replacementNameSpaceMap);
 
             // Clear cached calculations that points (potential) to now replaced
             // things.
@@ -619,8 +610,7 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
             }
           }
         }
-        replacementMap = null;
-        replacementSettersMap = null;
+        replacementNameSpaceMap = null;
       }
     }
     nextGoodKernelTarget.loader.buildersCreatedWithReferences.clear();
@@ -770,42 +760,12 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
   /// happen because of experimental invalidation.
   void _experimentalInvalidationFillReplacementMaps(
       Map<LibraryBuilder, CompilationUnit> rebuildBodiesMap,
-      Map<LibraryBuilder, Map<String, NamedBuilder>> replacementMap,
-      Map<LibraryBuilder, Map<String, NamedBuilder>> replacementSettersMap,
-      Map<LibraryBuilder, NameSpace> replacementLookupMap) {
+      Map<LibraryBuilder, NameSpace> replacementNameSpaceMap) {
     for (MapEntry<LibraryBuilder, CompilationUnit> entry
         in rebuildBodiesMap.entries) {
-      Map<String, NamedBuilder> childReplacementMap = {};
-      Map<String, NamedBuilder> childReplacementSettersMap = {};
       CompilationUnit mainCompilationUnit = rebuildBodiesMap[entry.key]!;
-      replacementMap[entry.key] = childReplacementMap;
-      replacementSettersMap[entry.key] = childReplacementSettersMap;
-      replacementLookupMap[entry.key] =
+      replacementNameSpaceMap[entry.key] =
           mainCompilationUnit.libraryBuilder.libraryNameSpace;
-
-      Iterator<NamedBuilder> iterator =
-          mainCompilationUnit.libraryBuilder.unfilteredMembersIterator;
-      while (iterator.moveNext()) {
-        NamedBuilder childBuilder = iterator.current;
-        if (childBuilder is SourceExtensionBuilder &&
-            childBuilder.isUnnamedExtension) {
-          continue;
-        }
-        String name = childBuilder.name;
-        Map<String, Builder> map;
-        if (isMappedAsSetter(childBuilder)) {
-          map = childReplacementSettersMap;
-        } else {
-          map = childReplacementMap;
-        }
-        assert(
-            !map.containsKey(name),
-            "Unexpected double-entry for $name in "
-            "${mainCompilationUnit.importUri} "
-            "(org from ${entry.key.importUri}): "
-            "$childBuilder and ${map[name]}");
-        map[name] = childBuilder;
-      }
     }
   }
 
@@ -843,23 +803,17 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
       ExperimentalInvalidation? experimentalInvalidation,
       Map<DillLibraryBuilder, CompilationUnit> rebuildBodiesMap) {
     if (experimentalInvalidation != null) {
-      // Maps from old library builder to map of new content.
-      Map<LibraryBuilder, Map<String, NamedBuilder>> replacementMap = {};
-
-      // Maps from old library builder to map of new content.
-      Map<LibraryBuilder, Map<String, NamedBuilder>> replacementSettersMap = {};
-
-      Map<LibraryBuilder, NameSpace> replacementLookupMap = {};
+      // Map from old library builder to name space of new content.
+      Map<LibraryBuilder, NameSpace> replacementNameSpaceMap = {};
 
-      _experimentalInvalidationFillReplacementMaps(rebuildBodiesMap,
-          replacementMap, replacementSettersMap, replacementLookupMap);
+      _experimentalInvalidationFillReplacementMaps(
+          rebuildBodiesMap, replacementNameSpaceMap);
 
       for (DillLibraryBuilder builder
           in experimentalInvalidation.originalNotReusedLibraries) {
         // There's only something to patch up if it was build already.
         if (builder.isBuilt) {
-          builder.patchUpExportScope(
-              replacementMap, replacementSettersMap, replacementLookupMap);
+          builder.patchUpExportScope(replacementNameSpaceMap);
         }
       }
     }

pkg/front_end/lib/src/base/name_space.dart

@@ -7,7 +7,6 @@ import '../builder/declaration_builders.dart';
 import '../builder/library_builder.dart';
 import '../builder/member_builder.dart';
 import '../builder/property_builder.dart';
-import '../dill/dill_library_builder.dart';
 import 'lookup_result.dart';
 import 'scope.dart';
 import 'uris.dart';
@@ -375,53 +374,17 @@ final class DillDeclarationNameSpace extends DeclarationNameSpaceBase {
   DillDeclarationNameSpace() : super._();
 }
 
-abstract base class LazyNameSpace extends ComputedMutableNameSpaceImpl {
-  LazyNameSpace() : super._();
-
-  /// Override this method to lazily populate the scope before access.
-  void ensureNameSpace();
-
-  @override
-  Map<String, LookupResult>? get _content {
-    ensureNameSpace();
-    return super._content;
-  }
-
-  @override
-  Set<ExtensionBuilder>? get _extensions {
-    ensureNameSpace();
-    return super._extensions;
-  }
-}
-
-final class DillLibraryNameSpace extends LazyNameSpace {
-  final DillLibraryBuilder _libraryBuilder;
-
-  DillLibraryNameSpace(this._libraryBuilder);
-
-  @override
-  void ensureNameSpace() {
-    _libraryBuilder.ensureLoaded();
-  }
+final class DillLibraryNameSpace extends ComputedMutableNameSpaceImpl {
+  DillLibraryNameSpace() : super._();
 }
 
-final class DillExportNameSpace extends LazyNameSpace {
-  final DillLibraryBuilder _libraryBuilder;
-
-  DillExportNameSpace(this._libraryBuilder);
-
-  @override
-  void ensureNameSpace() {
-    _libraryBuilder.ensureLoaded();
-  }
+final class DillExportNameSpace extends ComputedMutableNameSpaceImpl {
+  DillExportNameSpace() : super._();
 
   /// Patch up the scope, using the two replacement maps to replace builders in
   /// scope. The replacement maps from old LibraryBuilder to map, mapping
   /// from name to new (replacement) builder.
-  void patchUpScope(
-      Map<LibraryBuilder, Map<String, NamedBuilder>> replacementMap,
-      Map<LibraryBuilder, Map<String, NamedBuilder>> replacementMapSetters,
-      Map<LibraryBuilder, NameSpace> replacementLookupMap) {
+  void patchUpScope(Map<LibraryBuilder, NameSpace> replacementNameSpaceMap) {
     // In the following we refer to non-setters as 'getters' for brevity.
     //
     // We have to replace all getters and setters in [_locals] and [_setters]
@@ -435,70 +398,71 @@ final class DillExportNameSpace extends LazyNameSpace {
     // For this reason we start by collecting the names of all getters/setters
     // that need (some) replacement. Afterwards we go through these names
     // handling both getters and setters at the same time.
-    {
-      Set<String> replacedNames = {};
-      _content?.forEach((String name, LookupResult result) {
-        if (replacementMap.containsKey(result.getable?.parent)) {
-          replacedNames.add(name);
-        }
-        if (replacementMapSetters.containsKey(result.setable?.parent)) {
-          replacedNames.add(name);
-        }
-      });
-      if (replacedNames.isNotEmpty) {
-        for (String name in replacedNames) {
-          // We start be collecting the relation between an existing getter/setter
-          // and the getter/setter that will replace it. This information is used
-          // below to handle all the different cases that can occur.
-          LookupResult existingResult = _content![name]!;
-          NamedBuilder? existingGetter = existingResult.getable;
-          NamedBuilder? existingSetter = existingResult.setable;
-          LookupResult? replacementResult;
-          if (existingGetter != null && existingSetter != null) {
-            if (existingGetter == existingSetter) {
-              replacementResult = replacementLookupMap[existingGetter.parent]!
-                  .lookupLocalMember(name);
-            } else {
-              NamedBuilder? replacementGetter =
-                  replacementLookupMap[existingGetter.parent]
-                      ?.lookupLocalMember(name)
-                      ?.getable;
-              NamedBuilder? replacementSetter =
-                  replacementLookupMap[existingSetter.parent]
-                      ?.lookupLocalMember(name)
-                      ?.setable;
-              replacementResult = LookupResult.createResult(
-                  replacementGetter ?? existingGetter,
-                  replacementSetter ?? existingSetter);
-            }
-          } else if (existingGetter != null) {
-            replacementResult = LookupResult.createResult(
-                replacementLookupMap[existingGetter.parent]
+
+    Set<String> replacedNames = {};
+    _content?.forEach((String name, LookupResult result) {
+      if (replacementNameSpaceMap.containsKey(result.getable?.parent)) {
+        replacedNames.add(name);
+      }
+      if (replacementNameSpaceMap.containsKey(result.setable?.parent)) {
+        replacedNames.add(name);
+      }
+    });
+    if (replacedNames.isNotEmpty) {
+      for (String name in replacedNames) {
+        // We start be collecting the relation between an existing getter/setter
+        // and the getter/setter that will replace it. This information is used
+        // below to handle all the different cases that can occur.
+        LookupResult existingResult = _content![name]!;
+        NamedBuilder? existingGetter = existingResult.getable;
+        NamedBuilder? existingSetter = existingResult.setable;
+        LookupResult? replacementResult;
+        if (existingGetter != null && existingSetter != null) {
+          if (existingGetter == existingSetter) {
+            replacementResult = replacementNameSpaceMap[existingGetter.parent]!
+                .lookupLocalMember(name);
+          } else {
+            NamedBuilder? replacementGetter =
+                replacementNameSpaceMap[existingGetter.parent]
                     ?.lookupLocalMember(name)
-                    ?.getable,
-                null);
-          } else if (existingSetter != null) {
-            replacementResult = LookupResult.createResult(
-                null,
-                replacementLookupMap[existingSetter.parent]
+                    ?.getable;
+            NamedBuilder? replacementSetter =
+                replacementNameSpaceMap[existingSetter.parent]
                     ?.lookupLocalMember(name)
-                    ?.setable);
-          }
-          if (replacementResult != null) {
-            (_content ??= // Coverage-ignore(suite): Not run.
-                {})[name] = replacementResult;
-          } else {
-            // Coverage-ignore-block(suite): Not run.
-            _content?.remove(name);
+                    ?.setable;
+            replacementResult = LookupResult.createResult(
+                replacementGetter ?? existingGetter,
+                replacementSetter ?? existingSetter);
           }
+        } else if (existingGetter != null) {
+          replacementResult = LookupResult.createResult(
+              replacementNameSpaceMap[existingGetter.parent]
+                  ?.lookupLocalMember(name)
+                  ?.getable,
+              null);
+        } else if (existingSetter != null) {
+          replacementResult = LookupResult.createResult(
+              null,
+              replacementNameSpaceMap[existingSetter.parent]
+                  ?.lookupLocalMember(name)
+                  ?.setable);
+        }
+        if (replacementResult != null) {
+          (_content ??= // Coverage-ignore(suite): Not run.
+              {})[name] = replacementResult;
+        } else {
+          // Coverage-ignore-block(suite): Not run.
+          _content?.remove(name);
         }
       }
     }
+
     if (_extensions != null) {
       // Coverage-ignore-block(suite): Not run.
       bool needsPatching = false;
       for (ExtensionBuilder extensionBuilder in _extensions!) {
-        if (replacementMap.containsKey(extensionBuilder.libraryBuilder)) {
+        if (replacementNameSpaceMap
+            .containsKey(extensionBuilder.libraryBuilder)) {
           needsPatching = true;
           break;
         }
@@ -507,12 +471,16 @@ final class DillExportNameSpace extends LazyNameSpace {
         Set<ExtensionBuilder> extensionsReplacement =
             new Set<ExtensionBuilder>();
         for (ExtensionBuilder extensionBuilder in _extensions!) {
-          if (replacementMap.containsKey(extensionBuilder.libraryBuilder)) {
-            assert(replacementMap[extensionBuilder.libraryBuilder]![
-                    extensionBuilder.name] !=
+          if (replacementNameSpaceMap
+              .containsKey(extensionBuilder.libraryBuilder)) {
+            assert(replacementNameSpaceMap[extensionBuilder.libraryBuilder]!
+                    .lookupLocalMember(extensionBuilder.name)!
+                    .getable !=
                 null);
-            extensionsReplacement.add(replacementMap[extensionBuilder
-                .libraryBuilder]![extensionBuilder.name] as ExtensionBuilder);
+            extensionsReplacement.add(
+                replacementNameSpaceMap[extensionBuilder.libraryBuilder]!
+                    .lookupLocalMember(extensionBuilder.name)!
+                    .getable as ExtensionBuilder);
             break;
           } else {
             extensionsReplacement.add(extensionBuilder);

pkg/front_end/lib/src/builder/library_builder.dart

@@ -355,11 +355,6 @@ abstract class LibraryBuilder implements Builder, ProblemReporting {
   /// used in conditional imports and `bool.fromEnvironment` constants.
   bool get isUnsupported;
 
-  /// Returns an iterator of all members (typedefs, classes and members)
-  /// declared in this library, including duplicate declarations.
-  // TODO(johnniwinther): Should the only exist on [SourceLibraryBuilder]?
-  Iterator<NamedBuilder> get unfilteredMembersIterator;
-
   /// [Iterator] for all declarations declared in this library of type [T].
   ///
   /// If [includeDuplicates] is `true`, duplicate declarations are included.