dart-lang/sdk
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

[analyzer] Simplify and clean up duplicate declaration checking.

Browse Source 
This change reworks
`MemberDuplicateDefinitionVerifier._checkConflictingConstructorAndStatic`
and `MemberDuplicateDefinitionVerifier._checkDuplicateIdentifier` into
a form that is easier to reason about and has slightly faster
performance (measured by instruction count).

In the previous design, two maps were maintained for each scope in
which getters and setters might appear:

- one called `getterScope`, which (confusingly) held getters, setters,
  and method declarations,

- and one called `setterScope` which only held setters.

This was difficult to reason about. In particular, it had a
longstanding bug that was only recently fixed (see
https://dart-review.googlesource.com/c/sdk/+/434061): if a setter was
encountered first, it was stored in `getterScope`, but if a getter was
encountered next, it was necessary to move the setter to `setterScope`
in order to store the getter in `getterScope`.

It was also inefficient, since in many cases, two map lookups were
needed in order to check for both getter and setter conflicts.

In the new design, there is a single map, whose values point to either
a `_ScopeEntryFragment` (in the case where just one declaration of the
given name has been seen) or a `_ScopeEntryGetterSetterPair` (in the
case where both a getter and a setter have been seen).

The new design has modestly better performance when measured by
front_end/tool/benchmarker.dart:

     Comparing snapshot #1 (before.aot) with snapshot #2 (after.aot)
     instructions:u: -0.0192% +/- 0.0131% (-6291379.67 +/- 4292101.91) (32692292436.33 -> 32686001056.67)

Change-Id: Ia6675fa4d3579779e6066db3760d2cc38a2c859e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/435602
Reviewed-by: Jens Johansen <jensj@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
This commit is contained in:
Paul Berry
2025-06-19 11:13:27 -07:00
committed by Commit Queue
parent 2ae193dfde
commit 5fc5f3678a
2 changed files with 145 additions and 129 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/analyzer/lib/src/error/duplicate_definition_verifier.dart
+126 -129
View File
@@ -393,10 +393,8 @@ class MemberDuplicateDefinitionVerifier {
var elementContext = _getElementContext(firstFragment);
var constructorNames = elementContext.constructorNames;
var instanceGetters = elementContext.instanceGetters;
var instanceSetters = elementContext.instanceSetters;
var staticGetters = elementContext.staticGetters;
var staticSetters = elementContext.staticSetters;
var instanceScope = elementContext.instanceScope;
var staticScope = elementContext.staticScope;
for (var member in members) {
switch (member) {
@@ -427,10 +425,9 @@ class MemberDuplicateDefinitionVerifier {
case FieldDeclarationImpl():
for (var field in member.fields.variables) {
_checkDuplicateIdentifier(
member.isStatic ? staticGetters : instanceGetters,
member.isStatic ? staticScope : instanceScope,
field.name,
element: field.declaredFragment!,
setterScope: member.isStatic ? staticSetters : instanceSetters,
fragment: field.declaredFragment!,
);
if (fragment is EnumFragmentImpl) {
_checkValuesDeclarationInEnum(field.name);
@@ -438,10 +435,9 @@ class MemberDuplicateDefinitionVerifier {
}
case MethodDeclarationImpl():
_checkDuplicateIdentifier(
member.isStatic ? staticGetters : instanceGetters,
member.isStatic ? staticScope : instanceScope,
member.name,
element: member.declaredFragment!,
setterScope: member.isStatic ? staticSetters : instanceSetters,
fragment: member.declaredFragment!,
);
if (fragment is EnumFragmentImpl) {
if (!(member.isStatic && member.isSetter)) {
@@ -454,8 +450,7 @@ class MemberDuplicateDefinitionVerifier {
if (firstFragment is InterfaceFragmentImpl) {
_checkConflictingConstructorAndStatic(
interfaceElement: firstFragment,
staticGetters: staticGetters,
staticSetters: staticSetters,
staticScope: staticScope,
);
}
}
@@ -467,8 +462,7 @@ class MemberDuplicateDefinitionVerifier {
var firstFragment = fragment.element.firstFragment;
var elementContext = _getElementContext(firstFragment);
var instanceGetters = elementContext.instanceGetters;
var instanceSetters = elementContext.instanceSetters;
var instanceScope = elementContext.instanceScope;
// Check for local static members conflicting with local instance members.
// TODO(scheglov): This code is duplicated for enums. But for classes it is
@@ -479,8 +473,7 @@ class MemberDuplicateDefinitionVerifier {
for (VariableDeclaration field in member.fields.variables) {
var identifier = field.name;
String name = identifier.lexeme;
if (instanceGetters.containsKey(name) ||
instanceSetters.containsKey(name)) {
if (instanceScope.containsKey(name)) {
if (firstFragment is InterfaceFragmentImpl) {
String className = firstFragment.name2 ?? '';
_diagnosticReporter.atToken(
@@ -496,8 +489,7 @@ class MemberDuplicateDefinitionVerifier {
if (member.isStatic) {
var identifier = member.name;
String name = identifier.lexeme;
if (instanceGetters.containsKey(name) ||
instanceSetters.containsKey(name)) {
if (instanceScope.containsKey(name)) {
if (firstFragment is InterfaceFragmentImpl) {
String className = firstFragment.name2 ?? '';
_diagnosticReporter.atToken(
@@ -514,8 +506,7 @@ class MemberDuplicateDefinitionVerifier {
void _checkConflictingConstructorAndStatic({
required InterfaceFragmentImpl interfaceElement,
required Map<String, FragmentImpl> staticGetters,
required Map<String, FragmentImpl> staticSetters,
required Map<String, _ScopeEntry> staticScope,
}) {
for (var constructor in interfaceElement.constructors) {
var name = constructor.name2;
@@ -525,122 +516,124 @@ class MemberDuplicateDefinitionVerifier {
continue;
}
var staticMember = staticGetters[name] ?? staticSetters[name];
if (staticMember is PropertyAccessorFragmentImpl) {
CompileTimeErrorCode errorCode;
if (staticMember.isSynthetic) {
errorCode =
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_FIELD;
} else if (staticMember.isGetter) {
errorCode =
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_GETTER;
} else {
errorCode =
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_SETTER;
}
_diagnosticReporter.atElement2(
constructor.asElement2,
errorCode,
arguments: [name],
);
} else if (staticMember is MethodFragmentImpl) {
_diagnosticReporter.atElement2(
constructor.asElement2,
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_METHOD,
arguments: [name],
);
var state = staticScope[name];
switch (state) {
case null:
// ok
break;
case _ScopeEntryFragment(
fragment: PropertyAccessorFragmentImpl staticMember,
):
CompileTimeErrorCode errorCode;
if (staticMember.isSynthetic) {
errorCode =
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_FIELD;
} else if (staticMember.isGetter) {
errorCode =
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_GETTER;
} else {
errorCode =
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_SETTER;
}
_diagnosticReporter.atElement2(
constructor.asElement2,
errorCode,
arguments: [name],
);
case _ScopeEntryFragment(fragment: FieldFragmentImpl()):
_diagnosticReporter.atElement2(
constructor.asElement2,
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_FIELD,
arguments: [name],
);
case _ScopeEntryFragment(fragment: MethodFragmentImpl()):
_diagnosticReporter.atElement2(
constructor.asElement2,
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_METHOD,
arguments: [name],
);
case _ScopeEntryGetterSetterPair():
_diagnosticReporter.atElement2(
constructor.asElement2,
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_GETTER,
arguments: [name],
);
case _ScopeEntryFragment(:var fragment):
throw StateError(
'Unexpected type in duplicate map: ${fragment.runtimeType}',
);
}
}
}
/// Check whether the given [element] defined by the [identifier] is already
/// in one of the scopes - [getterScope] or [setterScope], and produce an
/// error if it is.
/// Checks whether the given [fragment] defined by the [identifier] conflicts
/// with a [fragment] already in [scope], and produces an error if it is.
void _checkDuplicateIdentifier(
Map<String, FragmentImpl> getterScope,
Map<String, _ScopeEntry> scope,
Token identifier, {
required FragmentImpl element,
Map<String, FragmentImpl>? setterScope,
required FragmentImpl fragment,
}) {
if (identifier.isSynthetic || element.asElement2.isWildcardVariable) {
if (identifier.isSynthetic || fragment.asElement2.isWildcardVariable) {
return;
}
switch (element) {
switch (fragment) {
case ExecutableFragmentImpl _:
if (element.isAugmentation) return;
if (fragment.isAugmentation) return;
case FieldFragmentImpl _:
if (element.isAugmentation) return;
if (fragment.isAugmentation) return;
case InstanceFragmentImpl _:
if (element.isAugmentation) return;
if (fragment.isAugmentation) return;
case TypeAliasFragmentImpl _:
if (element.isAugmentation) return;
if (fragment.isAugmentation) return;
case TopLevelVariableFragmentImpl _:
if (element.isAugmentation) return;
if (fragment.isAugmentation) return;
}
// Fields define getters and setters, so check them separately.
if (element is PropertyInducingFragmentImpl) {
_checkDuplicateIdentifier(
getterScope,
identifier,
element: element.element.getter2!.firstFragment,
setterScope: setterScope,
);
var setter = element.element.setter2?.firstFragment;
if (setter != null && setter.isSynthetic) {
_checkDuplicateIdentifier(
getterScope,
identifier,
element: setter,
setterScope: setterScope,
);
if (fragment is PropertyInducingFragmentImpl) {
var definesSetter =
(!fragment.isFinal && !fragment.isConst) ||
(fragment.isLate && !fragment.hasInitializer);
if (!definesSetter) {
// The field just defines a getter, so treat it as a getter for
// duplicate checking purposes.
fragment = fragment.getter!;
}
return;
}
var name = switch (element) {
MethodFragmentImpl() => element.element.lookupName ?? '',
var name = switch (fragment) {
MethodFragmentImpl() => fragment.element.lookupName ?? '',
_ => identifier.lexeme,
};
var previous = getterScope[name];
if (previous != null) {
if (!_isGetterSetterPair(element, previous)) {
var scopeEntry = scope[name];
switch (scopeEntry) {
case null:
scope[name] = _ScopeEntryFragment(fragment);
case _ScopeEntryFragment(fragment: GetterFragmentImpl previous)
when fragment is SetterFragmentImpl:
scope[name] = _ScopeEntryGetterSetterPair(
getter: previous,
setter: fragment,
);
case _ScopeEntryFragment(fragment: SetterFragmentImpl previous)
when fragment is GetterFragmentImpl:
scope[name] = _ScopeEntryGetterSetterPair(
getter: fragment,
setter: previous,
);
case _ScopeEntryGetterSetterPair(setter: FragmentImpl duplicateFragment)
when fragment is SetterFragment:
case _ScopeEntryGetterSetterPair(getter: FragmentImpl duplicateFragment):
case _ScopeEntryFragment(fragment: FragmentImpl duplicateFragment):
_diagnosticReporter.reportError(
_diagnosticFactory.duplicateDefinition(
CompileTimeErrorCode.DUPLICATE_DEFINITION,
element.asElement2!,
previous.asElement2!,
fragment.asElement2!,
duplicateFragment.asElement2!,
[name],
),
);
} else {
// Getter setter pair. Make sure the *getter* is in the getter map.
if (element is PropertyAccessorFragmentImpl && element.isGetter) {
getterScope[name] = element;
}
}
} else {
getterScope[name] = element;
}
if (setterScope != null) {
if (element is PropertyAccessorFragmentImpl && element.isSetter) {
previous = setterScope[name];
if (previous != null) {
_diagnosticReporter.reportError(
_diagnosticFactory.duplicateDefinition(
CompileTimeErrorCode.DUPLICATE_DEFINITION,
element.asElement2,
previous.asElement2!,
[name],
),
);
} else {
setterScope[name] = element;
}
}
}
}
@@ -651,7 +644,7 @@ class MemberDuplicateDefinitionVerifier {
var declarationName = firstFragment.name2;
var elementContext = _getElementContext(firstFragment);
var staticGetters = elementContext.staticGetters;
var staticScope = elementContext.staticScope;
for (var constant in node.constants) {
if (constant.name.lexeme == declarationName) {
@@ -661,9 +654,9 @@ class MemberDuplicateDefinitionVerifier {
);
}
_checkDuplicateIdentifier(
staticGetters,
staticScope,
constant.name,
element: constant.declaredFragment!,
fragment: constant.declaredFragment!,
);
_checkValuesDeclarationInEnum(constant.name);
}
@@ -776,8 +769,7 @@ class MemberDuplicateDefinitionVerifier {
var firstFragment = fragment.element.firstFragment;
var elementContext = _getElementContext(firstFragment);
var instanceGetters = elementContext.instanceGetters;
var instanceSetters = elementContext.instanceSetters;
var instanceScope = elementContext.instanceScope;
for (var member in node.members) {
if (member is FieldDeclarationImpl) {
@@ -785,8 +777,7 @@ class MemberDuplicateDefinitionVerifier {
for (var field in member.fields.variables) {
var identifier = field.name;
var name = identifier.lexeme;
if (instanceGetters.containsKey(name) ||
instanceSetters.containsKey(name)) {
if (instanceScope.containsKey(name)) {
_diagnosticReporter.atToken(
identifier,
CompileTimeErrorCode.EXTENSION_CONFLICTING_STATIC_AND_INSTANCE,
@@ -799,8 +790,7 @@ class MemberDuplicateDefinitionVerifier {
if (member.isStatic) {
var identifier = member.name;
var name = identifier.lexeme;
if (instanceGetters.containsKey(name) ||
instanceSetters.containsKey(name)) {
if (instanceScope.containsKey(name)) {
_diagnosticReporter.atToken(
identifier,
CompileTimeErrorCode.EXTENSION_CONFLICTING_STATIC_AND_INSTANCE,
@@ -821,7 +811,9 @@ class MemberDuplicateDefinitionVerifier {
var elementContext = _getElementContext(firstFragment);
elementContext.constructorNames.add(primaryConstructorName);
if (representationGetter.name2 case var getterName?) {
elementContext.instanceGetters[getterName] = representationGetter;
elementContext.instanceScope[getterName] = _ScopeEntryFragment(
representationGetter,
);
}
_checkClassMembers(firstFragment, node.members);
@@ -952,21 +944,26 @@ class MemberDuplicateDefinitionVerifier {
forUnit(fileAnalysis)._checkUnitStatic(fileAnalysis.unit);
}
}
static bool _isGetterSetterPair(FragmentImpl a, FragmentImpl b) {
if (a is PropertyAccessorFragmentImpl &&
b is PropertyAccessorFragmentImpl) {
return a.isGetter && b.isSetter || a.isSetter && b.isGetter;
}
return false;
}
}
/// Information accumulated for a single declaration and its augmentations.
class _InstanceElementContext {
final Set<String> constructorNames = {};
final Map<String, FragmentImpl> instanceGetters = {};
final Map<String, FragmentImpl> instanceSetters = {};
final Map<String, FragmentImpl> staticGetters = {};
final Map<String, FragmentImpl> staticSetters = {};
final Map<String, _ScopeEntry> instanceScope = {};
final Map<String, _ScopeEntry> staticScope = {};
}
sealed class _ScopeEntry {}
class _ScopeEntryFragment extends _ScopeEntry {
final FragmentImpl fragment;
_ScopeEntryFragment(this.fragment);
}
class _ScopeEntryGetterSetterPair extends _ScopeEntry {
final GetterFragmentImpl getter;
final SetterFragmentImpl setter;
_ScopeEntryGetterSetterPair({required this.getter, required this.setter});
}
pkg/analyzer/test/src/diagnostics/conflicting_constructor_and_static_field_test.dart
+19
View File
@@ -61,6 +61,25 @@ class C {
);
}
test_class_static_getter_setter_pair() async {
await assertErrorsInCode(
r'''
class C {
C.foo();
static int get foo => 0;
static set foo(_) {}
}
''',
[
error(
CompileTimeErrorCode.CONFLICTING_CONSTRUCTOR_AND_STATIC_GETTER,
14,
3,
),
],
);
}
test_class_static_notSameClass() async {
await assertNoErrorsInCode(r'''
class A {