From 5fc5f3678ac03b28ea462101eab76305caee0eb4 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Thu, 19 Jun 2025 11:13:27 -0700 Subject: [PATCH] [analyzer] Simplify and clean up duplicate declaration checking. 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 Commit-Queue: Paul Berry --- .../error/duplicate_definition_verifier.dart | 255 +++++++++--------- ...ing_constructor_and_static_field_test.dart | 19 ++ 2 files changed, 145 insertions(+), 129 deletions(-) diff --git a/pkg/analyzer/lib/src/error/duplicate_definition_verifier.dart b/pkg/analyzer/lib/src/error/duplicate_definition_verifier.dart index 6a98bdaf1dd..f3fff5ba46c 100644 --- a/pkg/analyzer/lib/src/error/duplicate_definition_verifier.dart +++ b/pkg/analyzer/lib/src/error/duplicate_definition_verifier.dart @@ -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 staticGetters, - required Map staticSetters, + required Map 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 getterScope, + Map scope, Token identifier, { - required FragmentImpl element, - Map? 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 constructorNames = {}; - final Map instanceGetters = {}; - final Map instanceSetters = {}; - final Map staticGetters = {}; - final Map staticSetters = {}; + final Map instanceScope = {}; + final Map 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}); } diff --git a/pkg/analyzer/test/src/diagnostics/conflicting_constructor_and_static_field_test.dart b/pkg/analyzer/test/src/diagnostics/conflicting_constructor_and_static_field_test.dart index 0241c16fa48..32a9c3d8526 100644 --- a/pkg/analyzer/test/src/diagnostics/conflicting_constructor_and_static_field_test.dart +++ b/pkg/analyzer/test/src/diagnostics/conflicting_constructor_and_static_field_test.dart @@ -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 {