Skip to content

Commit

Permalink
[cfe] Add _PreBuilder
Browse files Browse the repository at this point in the history
This moves the checking of duplicates, member/setter, and static/instance conflicts to a _PreBuilder.

Change-Id: I85f33750c1579676696223d1b19019887ae41e50
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/397163
Reviewed-by: Erik Ernst <[email protected]>
Commit-Queue: Johnni Winther <[email protected]>
  • Loading branch information
johnniwinther authored and Commit Queue committed Nov 27, 2024
1 parent b77865d commit e88020b
Show file tree
Hide file tree
Showing 134 changed files with 5,720 additions and 5,116 deletions.
542 changes: 483 additions & 59 deletions pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart

Large diffs are not rendered by default.

4 changes: 1 addition & 3 deletions pkg/front_end/lib/src/base/uri_translator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,7 @@ class UriTranslator {
: _packageUriNotFoundNoReport)(uri);
}
return translated;
}
// Coverage-ignore(suite): Not run.
on ArgumentError catch (e) {
} on ArgumentError catch (e) {
// TODO(sigmund): catch a more precise error when
// https://github.com/dart-lang/package_config/issues/40 is fixed.
if (reportMessage) {
Expand Down
5 changes: 3 additions & 2 deletions pkg/front_end/lib/src/fragment/field.dart
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ class FieldFragment implements Fragment {
: _initializerToken = initializerToken,
_constInitializerToken = constInitializerToken;

// Coverage-ignore(suite): Not run.
bool get hasSetter {
if (modifiers.isFinal) {
if (modifiers.isConst) {
return false;
} else if (modifiers.isFinal) {
if (modifiers.isLate) {
return !modifiers.hasInitializer;
} else {
Expand Down
48 changes: 0 additions & 48 deletions pkg/front_end/lib/src/kernel/hierarchy/members_node.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ import '../../base/messages.dart'
templateCantInferTypesDueToNoCombinedSignature,
templateCantInferReturnTypeDueToNoCombinedSignature,
templateCantInferTypeDueToNoCombinedSignature,
templateDuplicatedDeclaration,
templateDuplicatedDeclarationCause,
templateInstanceAndSynthesizedStaticConflict,
templateMissingImplementationCause,
templateMissingImplementationNotAbstract;
Expand Down Expand Up @@ -135,52 +133,6 @@ abstract class MembersNodeBuilder {
name.length,
instanceMember.fileUri);
}
} else {
// This message can be reported twice (when merging localMembers with
// classSetters, or localSetters with classMembers). By ensuring that
// we always report the one with higher charOffset as the duplicate,
// the message duplication logic ensures that we only report this
// problem once.
ClassMember existing;
ClassMember duplicate;
assert(a.fileUri == b.fileUri ||
// Coverage-ignore(suite): Not run.
a.name.text == "toString" &&
(a.fileUri.isScheme("org-dartlang-sdk") &&
a.fileUri.pathSegments.isNotEmpty &&
a.fileUri.pathSegments.last == "enum.dart" ||
b.fileUri.isScheme("org-dartlang-sdk") &&
b.fileUri.pathSegments.isNotEmpty &&
b.fileUri.pathSegments.last == "enum.dart"));

if (a.fileUri != b.fileUri) {
// Coverage-ignore-block(suite): Not run.
if (a.fileUri.isScheme("org-dartlang-sdk")) {
existing = a;
duplicate = b;
} else {
assert(b.fileUri.isScheme("org-dartlang-sdk"));
existing = b;
duplicate = a;
}
} else {
if (a.charOffset < b.charOffset) {
existing = a;
duplicate = b;
} else {
existing = b;
duplicate = a;
}
}
declarationBuilder.libraryBuilder.addProblem(
templateDuplicatedDeclaration.withArguments(name),
duplicate.charOffset,
name.length,
duplicate.fileUri,
context: <LocatedMessage>[
templateDuplicatedDeclarationCause.withArguments(name).withLocation(
existing.fileUri, existing.charOffset, name.length)
]);
}
}
}
Expand Down
1 change: 0 additions & 1 deletion pkg/front_end/lib/src/source/outline_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2888,7 +2888,6 @@ class OutlineBuilder extends StackListenerImpl {
if (identifier is Identifier) {
if (enumConstantInfos == null) {
if (!leftBrace.isSynthetic) {
// Coverage-ignore-block(suite): Not run.
addProblem(messageEnumDeclarationEmpty, identifier.token.offset,
identifier.token.length);
}
Expand Down
48 changes: 0 additions & 48 deletions pkg/front_end/lib/src/source/source_builder_mixins.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import '../builder/builder_mixins.dart';
import '../builder/declaration_builders.dart';
import '../builder/library_builder.dart';
import '../builder/member_builder.dart';
import '../builder/name_iterator.dart';
import '../builder/procedure_builder.dart';
import '../builder/type_builder.dart';
import '../kernel/body_builder_context.dart';
Expand Down Expand Up @@ -55,10 +54,6 @@ mixin SourceDeclarationBuilderMixin
/// library.
void buildInternal(LibraryBuilder coreLibrary,
{required bool addMembersToLibrary}) {
SourceLibraryBuilder.checkMemberConflicts(libraryBuilder, nameSpace,
checkForInstanceVsStaticConflict: true,
checkForMethodVsSetterConflict: true);

ClassBuilder objectClassBuilder =
coreLibrary.lookupLocalMember('Object', required: true) as ClassBuilder;

Expand Down Expand Up @@ -306,46 +301,3 @@ mixin SourceDeclarationBuilderMixin
@override
int get typeParametersCount => typeParameters?.length ?? 0;
}

mixin SourceTypedDeclarationBuilderMixin implements IDeclarationBuilder {
/// Checks for conflicts between constructors and static members declared
/// in this type declaration.
void checkConstructorStaticConflict() {
NameIterator<MemberBuilder> iterator =
nameSpace.filteredConstructorNameIterator(
includeDuplicates: false, includeAugmentations: true);
while (iterator.moveNext()) {
String name = iterator.name;
MemberBuilder constructor = iterator.current;
Builder? member = nameSpace.lookupLocalMember(name, setter: false);
if (member == null) continue;
if (!member.isStatic) continue;
// TODO(ahe): Revisit these messages. It seems like the last two should
// be `context` parameter to this message.
addProblem(templateConflictsWithMember.withArguments(name),
constructor.fileOffset, noLength);
if (constructor.isFactory) {
addProblem(
templateConflictsWithFactory.withArguments("${this.name}.${name}"),
member.fileOffset,
noLength);
} else {
addProblem(
templateConflictsWithConstructor
.withArguments("${this.name}.${name}"),
member.fileOffset,
noLength);
}
}

nameSpace.forEachLocalSetter((String name, Builder setter) {
Builder? constructor = nameSpace.lookupConstructor(name);
if (constructor == null || !setter.isStatic) return;
// Coverage-ignore-block(suite): Not run.
addProblem(templateConflictsWithConstructor.withArguments(name),
setter.fileOffset, noLength);
addProblem(templateConflictsWithSetter.withArguments(name),
constructor.fileOffset, noLength);
});
}
}
10 changes: 1 addition & 9 deletions pkg/front_end/lib/src/source/source_class_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ Class initializeClass(
}

class SourceClassBuilder extends ClassBuilderImpl
with ClassDeclarationMixin, SourceTypedDeclarationBuilderMixin
with ClassDeclarationMixin
implements
Comparable<SourceClassBuilder>,
ClassDeclaration,
Expand Down Expand Up @@ -280,12 +280,6 @@ class SourceClassBuilder extends ClassBuilderImpl
SourceLibraryBuilder get parent => libraryBuilder;

Class build(LibraryBuilder coreLibrary) {
SourceLibraryBuilder.checkMemberConflicts(libraryBuilder, nameSpace,
// These checks are performed as part of the class hierarchy
// computation.
checkForInstanceVsStaticConflict: false,
checkForMethodVsSetterConflict: false);

void buildBuilders(Builder declaration) {
if (declaration.parent != this) {
if (declaration.parent?.origin != origin) {
Expand Down Expand Up @@ -403,8 +397,6 @@ class SourceClassBuilder extends ClassBuilderImpl
}
}

checkConstructorStaticConflict();

cls.procedures.sort(compareProcedures);
return cls;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,7 @@ import 'type_parameter_scope_builder.dart';

class SourceExtensionTypeDeclarationBuilder
extends ExtensionTypeDeclarationBuilderImpl
with
SourceDeclarationBuilderMixin,
ClassDeclarationMixin,
SourceTypedDeclarationBuilderMixin
with SourceDeclarationBuilderMixin, ClassDeclarationMixin
implements
Comparable<SourceExtensionTypeDeclarationBuilder>,
ClassDeclaration {
Expand Down Expand Up @@ -393,7 +390,6 @@ class SourceExtensionTypeDeclarationBuilder
_extensionTypeDeclaration.declaredRepresentationType = representationType;
_extensionTypeDeclaration.representationName = representationName;
buildInternal(coreLibrary, addMembersToLibrary: addMembersToLibrary);
checkConstructorStaticConflict();

return _extensionTypeDeclaration;
}
Expand Down
85 changes: 0 additions & 85 deletions pkg/front_end/lib/src/source/source_library_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import '../base/uris.dart';
import '../builder/builder.dart';
import '../builder/declaration_builders.dart';
import '../builder/dynamic_type_declaration_builder.dart';
import '../builder/field_builder.dart';
import '../builder/formal_parameter_builder.dart';
import '../builder/library_builder.dart';
import '../builder/member_builder.dart';
Expand Down Expand Up @@ -553,86 +552,6 @@ class SourceLibraryBuilder extends LibraryBuilderImpl {
}
}

/// Checks [nameSpace] for conflicts between setters and non-setters and
/// reports them in [sourceLibraryBuilder].
///
/// If [checkForInstanceVsStaticConflict] is `true`, conflicts between
/// instance and static members of the same name are reported.
///
/// If [checkForMethodVsSetterConflict] is `true`, conflicts between
/// methods and setters of the same name are reported.
static void checkMemberConflicts(
SourceLibraryBuilder sourceLibraryBuilder, NameSpace nameSpace,
{required bool checkForInstanceVsStaticConflict,
required bool checkForMethodVsSetterConflict}) {
nameSpace.forEachLocalSetter((String name, MemberBuilder setter) {
Builder? getable = nameSpace.lookupLocalMember(name, setter: false);
if (getable == null) {
// Setter without getter.
return;
}

bool isConflictingSetter = false;
Set<Builder> conflictingGetables = {};
for (Builder? currentGetable = getable;
currentGetable != null;
currentGetable = currentGetable.next) {
if (currentGetable is FieldBuilder) {
if (currentGetable.isAssignable) {
// Setter with writable field.
isConflictingSetter = true;
conflictingGetables.add(currentGetable);
}
} else if (checkForMethodVsSetterConflict && !currentGetable.isGetter) {
// Setter with method.
conflictingGetables.add(currentGetable);
}
}
for (SourceMemberBuilderImpl? currentSetter =
setter as SourceMemberBuilderImpl?;
currentSetter != null;
currentSetter = currentSetter.next as SourceMemberBuilderImpl?) {
bool conflict = conflictingGetables.isNotEmpty;
for (Builder? currentGetable = getable;
currentGetable != null;
currentGetable = currentGetable.next) {
if (checkForInstanceVsStaticConflict &&
currentGetable.isDeclarationInstanceMember !=
currentSetter.isDeclarationInstanceMember) {
conflict = true;
conflictingGetables.add(currentGetable);
}
}
if (isConflictingSetter) {
currentSetter.isConflictingSetter = true;
}
if (conflict) {
if (currentSetter.isConflictingSetter) {
sourceLibraryBuilder.addProblem(
templateConflictsWithImplicitSetter.withArguments(name),
currentSetter.fileOffset,
noLength,
currentSetter.fileUri);
} else {
sourceLibraryBuilder.addProblem(
templateConflictsWithMember.withArguments(name),
currentSetter.fileOffset,
noLength,
currentSetter.fileUri);
}
}
}
for (Builder conflictingGetable in conflictingGetables) {
// TODO(ahe): Context argument to previous message?
sourceLibraryBuilder.addProblem(
templateConflictsWithSetter.withArguments(name),
conflictingGetable.fileOffset,
noLength,
conflictingGetable.fileUri!);
}
});
}

/// Builds the core AST structure of this library as needed for the outline.
Library buildOutlineNodes(LibraryBuilder coreLibrary) {
assert(checkState(
Expand All @@ -658,10 +577,6 @@ class SourceLibraryBuilder extends LibraryBuilderImpl {
part.buildOutlineNode(library);
}*/

checkMemberConflicts(this, libraryNameSpace,
checkForInstanceVsStaticConflict: false,
checkForMethodVsSetterConflict: true);

Iterator<Builder> iterator = localMembersIterator;
while (iterator.moveNext()) {
_buildOutlineNodes(iterator.current, coreLibrary);
Expand Down
Loading

0 comments on commit e88020b

Please sign in to comment.