Skip to content

Commit 5087b62

Browse files
johnniwintherCommit Queue
johnniwinther
authored and
Commit Queue
committed
[cfe] Check NameSpace.addLocalMember
This adds assertions to NameSpaceImpl.addLocalMember to ensure that the name space entries are not overwritten unexpectedly. The computations of library and declaration name spaces from dill are changed to exclude private members from other libraries which might otherwise replace exiting members. To support private class members inherited from other libraries as of the member hierarchy computation, the member builders in a dill class builder are kept separately from the name space. Change-Id: Ib6b24e539a838093a3a5a2abfc056dfc3883903c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/427701 Commit-Queue: Johnni Winther <[email protected]> Reviewed-by: Chloe Stefantsova <[email protected]>
1 parent d04a3ed commit 5087b62

26 files changed

+504
-190
lines changed

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -609,8 +609,8 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
609609

610610
// Clear cached calculations that points (potential) to now replaced
611611
// things.
612-
Iterator<DillClassBuilder> iterator = builder.libraryNameSpace
613-
.filteredIterator(includeDuplicates: true);
612+
Iterator<DillClassBuilder> iterator =
613+
builder.filteredMembersIterator(includeDuplicates: true);
614614
while (iterator.moveNext()) {
615615
iterator.current.clearCachedValues();
616616
}
@@ -777,7 +777,7 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
777777
replacementMap[entry.key] = childReplacementMap;
778778
replacementSettersMap[entry.key] = childReplacementSettersMap;
779779
Iterator<NamedBuilder> iterator =
780-
mainCompilationUnit.libraryBuilder.localMembersIterator;
780+
mainCompilationUnit.libraryBuilder.unfilteredMembersIterator;
781781
while (iterator.moveNext()) {
782782
NamedBuilder childBuilder = iterator.current;
783783
if (childBuilder is SourceExtensionBuilder &&
@@ -909,8 +909,8 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
909909
if (builder.isBuiltAndMarked) {
910910
// Clear cached calculations in classes which upon calculation can
911911
// mark things as needed.
912-
Iterator<DillClassBuilder> iterator = builder.libraryNameSpace
913-
.filteredIterator(includeDuplicates: true);
912+
Iterator<DillClassBuilder> iterator =
913+
builder.filteredMembersIterator(includeDuplicates: true);
914914
while (iterator.moveNext()) {
915915
iterator.current.clearCachedValues();
916916
}

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

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ abstract class NameSpace {
5252
abstract class MutableNameSpace implements NameSpace {
5353
factory MutableNameSpace() = NameSpaceImpl._;
5454

55-
void addLocalMember(String name, NamedBuilder member, {required bool setter});
55+
void addLocalMember(String name, NamedBuilder member,
56+
{required bool setter, bool allowReplace = false});
5657

5758
/// Adds [builder] to the extensions in this name space.
5859
void addExtension(ExtensionBuilder builder);
@@ -95,10 +96,24 @@ base class NameSpaceImpl implements NameSpace, MutableNameSpace {
9596

9697
@override
9798
void addLocalMember(String name, NamedBuilder member,
98-
{required bool setter}) {
99+
{required bool setter, bool allowReplace = false}) {
99100
if (setter) {
101+
assert(
102+
_setables == null ||
103+
!_setables!.containsKey(name) ||
104+
_setables![name] == member ||
105+
allowReplace,
106+
"Trying to map setable $member to $name "
107+
"replacing the existing value ${_setables?[name]}");
100108
(_setables ??= {})[name] = member;
101109
} else {
110+
assert(
111+
_getables == null ||
112+
!_getables!.containsKey(name) ||
113+
_getables![name] == member ||
114+
allowReplace,
115+
"Trying to map getable $member to $name "
116+
"replacing the existing value ${_getables?[name]}");
102117
(_getables ??= {})[name] = member;
103118
}
104119
}

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

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -358,18 +358,13 @@ abstract class LibraryBuilder implements Builder, ProblemReporting {
358358
/// Returns an iterator of all members (typedefs, classes and members)
359359
/// declared in this library, including duplicate declarations.
360360
// TODO(johnniwinther): Should the only exist on [SourceLibraryBuilder]?
361-
Iterator<NamedBuilder> get localMembersIterator;
361+
Iterator<NamedBuilder> get unfilteredMembersIterator;
362362

363-
/// Returns an iterator of all members of specified type
364-
/// declared in this library, including duplicate declarations.
365-
// TODO(johnniwinther): Should the only exist on [SourceLibraryBuilder]?
366-
Iterator<T> localMembersIteratorOfType<T extends NamedBuilder>();
367-
368-
/// [Iterator] for all declarations declared in this library or any of its
369-
/// augmentations.
363+
/// [Iterator] for all declarations declared in this library of type [T].
370364
///
371-
/// Duplicates and augmenting members are _not_ included.
372-
Iterator<T> fullMemberIterator<T extends NamedBuilder>();
365+
/// If [includeDuplicates] is `true`, duplicate declarations are included.
366+
Iterator<T> filteredMembersIterator<T extends NamedBuilder>(
367+
{required bool includeDuplicates});
373368

374369
/// Looks up [constructorName] in the class named [className].
375370
///
@@ -443,16 +438,6 @@ abstract class LibraryBuilderImpl extends BuilderImpl
443438
@override
444439
Uri get importUri;
445440

446-
@override
447-
Iterator<NamedBuilder> get localMembersIterator {
448-
return libraryNameSpace.filteredIterator(includeDuplicates: true);
449-
}
450-
451-
@override
452-
Iterator<T> localMembersIteratorOfType<T extends NamedBuilder>() {
453-
return libraryNameSpace.filteredIterator<T>(includeDuplicates: true);
454-
}
455-
456441
@override
457442
FormattedMessage? addProblem(
458443
Message message, int charOffset, int length, Uri? fileUri,

‎pkg/front_end/lib/src/builder/prefix_builder.dart‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -81,15 +81,15 @@ class PrefixBuilder extends NamedBuilderImpl implements LookupResult {
8181
LookupResult? existingResult = _prefixNameSpace.lookupLocalMember(name);
8282
NamedBuilder? existing =
8383
isSetter ? existingResult?.setable : existingResult?.getable;
84-
NamedBuilder result;
8584
if (existing != null) {
86-
result = computeAmbiguousDeclarationForImport(
85+
NamedBuilder result = computeAmbiguousDeclarationForImport(
8786
parent, name, existing, member,
8887
uriOffset: new UriOffset(fileUri, prefixOffset));
88+
_prefixNameSpace.addLocalMember(name, result,
89+
setter: isSetter, allowReplace: true);
8990
} else {
90-
result = member;
91+
_prefixNameSpace.addLocalMember(name, member, setter: isSetter);
9192
}
92-
_prefixNameSpace.addLocalMember(name, result, setter: isSetter);
9393
if (member is ExtensionBuilder) {
9494
_prefixNameSpace.addExtension(member);
9595
}

‎pkg/front_end/lib/src/dill/dill_class_builder.dart‎

Lines changed: 59 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import 'package:kernel/class_hierarchy.dart';
77

88
import '../base/loader.dart';
99
import '../base/name_space.dart';
10+
import '../base/scope.dart';
1011
import '../builder/builder.dart';
1112
import '../builder/declaration_builders.dart';
1213
import '../builder/library_builder.dart';
@@ -16,20 +17,7 @@ import 'dill_library_builder.dart' show DillLibraryBuilder;
1617
import 'dill_member_builder.dart';
1718
import 'dill_type_parameter_builder.dart';
1819

19-
mixin DillClassMemberAccessMixin implements ClassMemberAccess {
20-
DeclarationNameSpace get nameSpace;
21-
22-
@override
23-
Iterator<T> fullConstructorIterator<T extends MemberBuilder>() =>
24-
nameSpace.filteredConstructorIterator<T>(includeDuplicates: false);
25-
26-
@override
27-
Iterator<T> fullMemberIterator<T extends NamedBuilder>() =>
28-
nameSpace.filteredIterator<T>(includeDuplicates: false);
29-
}
30-
31-
class DillClassBuilder extends ClassBuilderImpl
32-
with DillClassMemberAccessMixin {
20+
class DillClassBuilder extends ClassBuilderImpl {
3321
@override
3422
final DillLibraryBuilder parent;
3523

@@ -38,6 +26,9 @@ class DillClassBuilder extends ClassBuilderImpl
3826

3927
final MutableDeclarationNameSpace _nameSpace;
4028

29+
final List<MemberBuilder> _constructorBuilders = [];
30+
final List<MemberBuilder> _memberBuilders = [];
31+
4132
List<NominalParameterBuilder>? _typeParameters;
4233

4334
TypeBuilder? _supertypeBuilder;
@@ -112,24 +103,47 @@ class DillClassBuilder extends ClassBuilderImpl
112103
return supertype;
113104
}
114105

106+
@override
107+
Iterator<T> fullConstructorIterator<T extends MemberBuilder>() =>
108+
new FilteredIterator<T>(_constructorBuilders.iterator,
109+
includeDuplicates: false);
110+
111+
@override
112+
Iterator<T> fullMemberIterator<T extends NamedBuilder>() =>
113+
new FilteredIterator<T>(_memberBuilders.iterator,
114+
includeDuplicates: false);
115+
116+
bool _isPrivateFromOtherLibrary(Member member) {
117+
Name name = member.name;
118+
return name.isPrivate &&
119+
name.libraryReference != cls.enclosingLibrary.reference;
120+
}
121+
115122
void addField(Field field) {
116123
DillFieldBuilder builder =
117124
new DillFieldBuilder(field, libraryBuilder, this);
118-
String name = field.name.text;
119-
_nameSpace.addLocalMember(name, builder, setter: false);
125+
if (!_isPrivateFromOtherLibrary(field)) {
126+
_nameSpace.addLocalMember(field.name.text, builder, setter: false);
127+
}
128+
_memberBuilders.add(builder);
120129
}
121130

122131
void addConstructor(Constructor constructor, Procedure? constructorTearOff) {
123132
DillConstructorBuilder builder = new DillConstructorBuilder(
124133
constructor, constructorTearOff, libraryBuilder, this);
125-
String name = constructor.name.text;
126-
_nameSpace.addConstructor(name, builder);
134+
if (!_isPrivateFromOtherLibrary(constructor)) {
135+
_nameSpace.addConstructor(constructor.name.text, builder);
136+
}
137+
_constructorBuilders.add(builder);
127138
}
128139

129140
void addFactory(Procedure factory, Procedure? factoryTearOff) {
130-
String name = factory.name.text;
131-
_nameSpace.addConstructor(name,
132-
new DillFactoryBuilder(factory, factoryTearOff, libraryBuilder, this));
141+
DillFactoryBuilder builder =
142+
new DillFactoryBuilder(factory, factoryTearOff, libraryBuilder, this);
143+
if (!_isPrivateFromOtherLibrary(factory)) {
144+
_nameSpace.addConstructor(factory.name.text, builder);
145+
}
146+
_constructorBuilders.add(builder);
133147
}
134148

135149
void addProcedure(Procedure procedure) {
@@ -139,24 +153,36 @@ class DillClassBuilder extends ClassBuilderImpl
139153
// Coverage-ignore(suite): Not run.
140154
throw new UnsupportedError("Use addFactory for adding factories");
141155
case ProcedureKind.Setter:
142-
_nameSpace.addLocalMember(
143-
name, new DillSetterBuilder(procedure, libraryBuilder, this),
144-
setter: true);
156+
DillSetterBuilder builder =
157+
new DillSetterBuilder(procedure, libraryBuilder, this);
158+
if (!_isPrivateFromOtherLibrary(procedure)) {
159+
_nameSpace.addLocalMember(name, builder, setter: true);
160+
}
161+
_memberBuilders.add(builder);
145162
break;
146163
case ProcedureKind.Getter:
147-
_nameSpace.addLocalMember(
148-
name, new DillGetterBuilder(procedure, libraryBuilder, this),
149-
setter: false);
164+
DillGetterBuilder builder =
165+
new DillGetterBuilder(procedure, libraryBuilder, this);
166+
if (!_isPrivateFromOtherLibrary(procedure)) {
167+
_nameSpace.addLocalMember(name, builder, setter: false);
168+
}
169+
_memberBuilders.add(builder);
150170
break;
151171
case ProcedureKind.Operator:
152-
_nameSpace.addLocalMember(
153-
name, new DillOperatorBuilder(procedure, libraryBuilder, this),
154-
setter: false);
172+
DillOperatorBuilder builder =
173+
new DillOperatorBuilder(procedure, libraryBuilder, this);
174+
if (!_isPrivateFromOtherLibrary(procedure)) {
175+
_nameSpace.addLocalMember(name, builder, setter: false);
176+
}
177+
_memberBuilders.add(builder);
155178
break;
156179
case ProcedureKind.Method:
157-
_nameSpace.addLocalMember(
158-
name, new DillMethodBuilder(procedure, libraryBuilder, this),
159-
setter: false);
180+
DillMethodBuilder builder =
181+
new DillMethodBuilder(procedure, libraryBuilder, this);
182+
if (!_isPrivateFromOtherLibrary(procedure)) {
183+
_nameSpace.addLocalMember(name, builder, setter: false);
184+
}
185+
_memberBuilders.add(builder);
160186
break;
161187
}
162188
}

‎pkg/front_end/lib/src/dill/dill_extension_builder.dart‎

Lines changed: 51 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@ class DillExtensionBuilder extends ExtensionBuilderImpl
2727

2828
DillExtensionBuilder(this.extension, this.libraryBuilder)
2929
: _nameSpace = new DillDeclarationNameSpace() {
30+
bool isPrivateFromOtherLibrary(Member member) {
31+
Name name = member.name;
32+
return name.isPrivate &&
33+
name.libraryReference != extension.enclosingLibrary.reference;
34+
}
35+
3036
for (ExtensionMemberDescriptor descriptor in extension.memberDescriptors) {
3137
if (descriptor.isInternalImplementation) continue;
3238

@@ -35,53 +41,66 @@ class DillExtensionBuilder extends ExtensionBuilderImpl
3541
case ExtensionMemberKind.Method:
3642
if (descriptor.isStatic) {
3743
Procedure procedure = descriptor.memberReference!.asProcedure;
38-
_nameSpace.addLocalMember(
39-
name.text,
40-
new DillExtensionStaticMethodBuilder(
41-
procedure, descriptor, libraryBuilder, this),
42-
setter: false);
44+
if (!isPrivateFromOtherLibrary(procedure)) {
45+
_nameSpace.addLocalMember(
46+
name.text,
47+
new DillExtensionStaticMethodBuilder(
48+
procedure, descriptor, libraryBuilder, this),
49+
setter: false);
50+
}
4351
} else {
4452
Procedure procedure = descriptor.memberReference!.asProcedure;
45-
Procedure? tearOff = descriptor.tearOffReference?.asProcedure;
46-
assert(tearOff != null, "No tear found for ${descriptor}");
47-
_nameSpace.addLocalMember(
48-
name.text,
49-
new DillExtensionInstanceMethodBuilder(
50-
procedure, descriptor, libraryBuilder, this, tearOff!),
51-
setter: false);
53+
if (!isPrivateFromOtherLibrary(procedure)) {
54+
Procedure? tearOff = descriptor.tearOffReference?.asProcedure;
55+
assert(tearOff != null, "No tear found for ${descriptor}");
56+
57+
_nameSpace.addLocalMember(
58+
name.text,
59+
new DillExtensionInstanceMethodBuilder(
60+
procedure, descriptor, libraryBuilder, this, tearOff!),
61+
setter: false);
62+
}
5263
}
5364
break;
5465
case ExtensionMemberKind.Getter:
5566
Procedure procedure = descriptor.memberReference!.asProcedure;
56-
_nameSpace.addLocalMember(
57-
name.text,
58-
new DillExtensionGetterBuilder(
59-
procedure, descriptor, libraryBuilder, this),
60-
setter: false);
67+
if (!isPrivateFromOtherLibrary(procedure)) {
68+
_nameSpace.addLocalMember(
69+
name.text,
70+
new DillExtensionGetterBuilder(
71+
procedure, descriptor, libraryBuilder, this),
72+
setter: false);
73+
}
6174
break;
6275
case ExtensionMemberKind.Field:
6376
Field field = descriptor.memberReference!.asField;
64-
_nameSpace.addLocalMember(
65-
name.text,
66-
new DillExtensionFieldBuilder(
67-
field, descriptor, libraryBuilder, this),
68-
setter: false);
77+
if (!isPrivateFromOtherLibrary(field)) {
78+
_nameSpace.addLocalMember(
79+
name.text,
80+
new DillExtensionFieldBuilder(
81+
field, descriptor, libraryBuilder, this),
82+
setter: false);
83+
}
6984
break;
7085
case ExtensionMemberKind.Setter:
7186
Procedure procedure = descriptor.memberReference!.asProcedure;
72-
_nameSpace.addLocalMember(
73-
name.text,
74-
new DillExtensionSetterBuilder(
75-
procedure, descriptor, libraryBuilder, this),
76-
setter: true);
87+
if (!isPrivateFromOtherLibrary(procedure)) {
88+
_nameSpace.addLocalMember(
89+
name.text,
90+
new DillExtensionSetterBuilder(
91+
procedure, descriptor, libraryBuilder, this),
92+
setter: true);
93+
}
7794
break;
7895
case ExtensionMemberKind.Operator:
7996
Procedure procedure = descriptor.memberReference!.asProcedure;
80-
_nameSpace.addLocalMember(
81-
name.text,
82-
new DillExtensionOperatorBuilder(
83-
procedure, descriptor, libraryBuilder, this),
84-
setter: false);
97+
if (!isPrivateFromOtherLibrary(procedure)) {
98+
_nameSpace.addLocalMember(
99+
name.text,
100+
new DillExtensionOperatorBuilder(
101+
procedure, descriptor, libraryBuilder, this),
102+
setter: false);
103+
}
85104
break;
86105
}
87106
}

0 commit comments

Comments
 (0)