From dceefd02ca1affcf1a3d44509da658464eb7ceaf Mon Sep 17 00:00:00 2001 From: Johnni Winther Date: Wed, 21 May 2025 07:03:53 -0700 Subject: [PATCH] [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 --- .../lib/src/base/incremental_compiler.dart | 74 ++------ pkg/front_end/lib/src/base/name_space.dart | 172 +++++++----------- .../lib/src/builder/library_builder.dart | 5 - .../lib/src/dill/dill_library_builder.dart | 33 ++-- .../src/source/source_library_builder.dart | 3 +- .../test/coverage_suite_expected.dart | 6 +- 6 files changed, 102 insertions(+), 191 deletions(-) diff --git a/pkg/front_end/lib/src/base/incremental_compiler.dart b/pkg/front_end/lib/src/base/incremental_compiler.dart index f450f110cd6..751170ad675 100644 --- a/pkg/front_end/lib/src/base/incremental_compiler.dart +++ b/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>? replacementMap = {}; + // Map from old library builder to name space of new content. + Map? replacementNameSpaceMap = {}; - // Maps from old library builder to map of new content. - Map>? replacementSettersMap = - {}; - - Map 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 rebuildBodiesMap, - Map> replacementMap, - Map> replacementSettersMap, - Map replacementLookupMap) { + Map replacementNameSpaceMap) { for (MapEntry entry in rebuildBodiesMap.entries) { - Map childReplacementMap = {}; - Map childReplacementSettersMap = {}; CompilationUnit mainCompilationUnit = rebuildBodiesMap[entry.key]!; - replacementMap[entry.key] = childReplacementMap; - replacementSettersMap[entry.key] = childReplacementSettersMap; - replacementLookupMap[entry.key] = + replacementNameSpaceMap[entry.key] = mainCompilationUnit.libraryBuilder.libraryNameSpace; - - Iterator iterator = - mainCompilationUnit.libraryBuilder.unfilteredMembersIterator; - while (iterator.moveNext()) { - NamedBuilder childBuilder = iterator.current; - if (childBuilder is SourceExtensionBuilder && - childBuilder.isUnnamedExtension) { - continue; - } - String name = childBuilder.name; - Map 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 rebuildBodiesMap) { if (experimentalInvalidation != null) { - // Maps from old library builder to map of new content. - Map> replacementMap = {}; + // Map from old library builder to name space of new content. + Map replacementNameSpaceMap = {}; - // Maps from old library builder to map of new content. - Map> replacementSettersMap = {}; - - Map replacementLookupMap = {}; - - _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); } } } diff --git a/pkg/front_end/lib/src/base/name_space.dart b/pkg/front_end/lib/src/base/name_space.dart index 47a613dd0df..9f8b6c4d4e6 100644 --- a/pkg/front_end/lib/src/base/name_space.dart +++ b/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? get _content { - ensureNameSpace(); - return super._content; - } - - @override - Set? get _extensions { - ensureNameSpace(); - return super._extensions; - } +final class DillLibraryNameSpace extends ComputedMutableNameSpaceImpl { + DillLibraryNameSpace() : super._(); } -final class DillLibraryNameSpace extends LazyNameSpace { - final DillLibraryBuilder _libraryBuilder; - - DillLibraryNameSpace(this._libraryBuilder); - - @override - void ensureNameSpace() { - _libraryBuilder.ensureLoaded(); - } -} - -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> replacementMap, - Map> replacementMapSetters, - Map replacementLookupMap) { + void patchUpScope(Map 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 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] - ?.lookupLocalMember(name) - ?.getable, - null); - } else if (existingSetter != null) { - replacementResult = LookupResult.createResult( - null, - replacementLookupMap[existingSetter.parent] - ?.lookupLocalMember(name) - ?.setable); - } - if (replacementResult != null) { - (_content ??= // Coverage-ignore(suite): Not run. - {})[name] = replacementResult; + + Set 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 { - // Coverage-ignore-block(suite): Not run. - _content?.remove(name); + NamedBuilder? replacementGetter = + replacementNameSpaceMap[existingGetter.parent] + ?.lookupLocalMember(name) + ?.getable; + NamedBuilder? replacementSetter = + replacementNameSpaceMap[existingSetter.parent] + ?.lookupLocalMember(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 extensionsReplacement = new Set(); 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); diff --git a/pkg/front_end/lib/src/builder/library_builder.dart b/pkg/front_end/lib/src/builder/library_builder.dart index abaadf3368d..bdb36b92d60 100644 --- a/pkg/front_end/lib/src/builder/library_builder.dart +++ b/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 get unfilteredMembersIterator; - /// [Iterator] for all declarations declared in this library of type [T]. /// /// If [includeDuplicates] is `true`, duplicate declarations are included. diff --git a/pkg/front_end/lib/src/dill/dill_library_builder.dart b/pkg/front_end/lib/src/dill/dill_library_builder.dart index 9cf234e9caf..85db427ddea 100644 --- a/pkg/front_end/lib/src/dill/dill_library_builder.dart +++ b/pkg/front_end/lib/src/dill/dill_library_builder.dart @@ -97,9 +97,9 @@ class DillCompilationUnitImpl extends DillCompilationUnit { } class DillLibraryBuilder extends LibraryBuilderImpl { - late final DillLibraryNameSpace _nameSpace; + final DillLibraryNameSpace _nameSpace = new DillLibraryNameSpace(); - late final DillExportNameSpace _exportNameSpace; + final DillExportNameSpace _exportNameSpace = new DillExportNameSpace(); @override final Library library; @@ -129,16 +129,18 @@ class DillLibraryBuilder extends LibraryBuilderImpl { final List _memberBuilders = []; - DillLibraryBuilder(this.library, this.loader) : super(library.fileUri) { - _nameSpace = new DillLibraryNameSpace(this); - _exportNameSpace = new DillExportNameSpace(this); + DillLibraryBuilder(this.library, this.loader) : super(library.fileUri); + @override + NameSpace get libraryNameSpace { + ensureLoaded(); + return _nameSpace; } @override - NameSpace get libraryNameSpace => _nameSpace; - - @override - ComputedNameSpace get exportNameSpace => _exportNameSpace; + ComputedNameSpace get exportNameSpace { + ensureLoaded(); + return _exportNameSpace; + } @override List get exporters => mainCompilationUnit.exporters; @@ -481,12 +483,6 @@ class DillLibraryBuilder extends LibraryBuilderImpl { } } - @override - Iterator get unfilteredMembersIterator { - ensureLoaded(); - return _memberBuilders.iterator; - } - @override Iterator filteredMembersIterator( {required bool includeDuplicates}) { @@ -502,10 +498,7 @@ class DillLibraryBuilder extends LibraryBuilderImpl { /// builders in the export scope. The replacement maps from old LibraryBuilder /// to map, mapping from name to new (replacement) builder. void patchUpExportScope( - Map> replacementMap, - Map> replacementMapSetters, - Map replacementLookupMap) { - _exportNameSpace.patchUpScope( - replacementMap, replacementMapSetters, replacementLookupMap); + Map replacementNameSpaceMap) { + _exportNameSpace.patchUpScope(replacementNameSpaceMap); } } diff --git a/pkg/front_end/lib/src/source/source_library_builder.dart b/pkg/front_end/lib/src/source/source_library_builder.dart index d3a86e73fdf..ac90ac5403a 100644 --- a/pkg/front_end/lib/src/source/source_library_builder.dart +++ b/pkg/front_end/lib/src/source/source_library_builder.dart @@ -473,7 +473,8 @@ class SourceLibraryBuilder extends LibraryBuilderImpl { new FilteredIterator(_memberBuilders.iterator, includeDuplicates: includeDuplicates); - @override + /// Returns an iterator of all members (typedefs, classes and members) + /// declared in this library, including duplicate declarations. Iterator get unfilteredMembersIterator { return _memberBuilders.iterator; } diff --git a/pkg/front_end/test/coverage_suite_expected.dart b/pkg/front_end/test/coverage_suite_expected.dart index 475661a6529..8c186d3de1b 100644 --- a/pkg/front_end/test/coverage_suite_expected.dart +++ b/pkg/front_end/test/coverage_suite_expected.dart @@ -150,7 +150,7 @@ const Map _expect = { ), // 100.0%. "package:front_end/src/base/incremental_compiler.dart": ( - hitCount: 834, + hitCount: 813, missCount: 0, ), // 100.0%. @@ -195,7 +195,7 @@ const Map _expect = { ), // 100.0%. "package:front_end/src/base/name_space.dart": ( - hitCount: 175, + hitCount: 163, missCount: 0, ), // 100.0%. @@ -455,7 +455,7 @@ const Map _expect = { ), // 100.0%. "package:front_end/src/dill/dill_library_builder.dart": ( - hitCount: 353, + hitCount: 347, missCount: 0, ), // 100.0%.