From b56190503725de2125b626ff1b3423192fa1f7ae Mon Sep 17 00:00:00 2001 From: Phil Quitslund Date: Mon, 3 Apr 2023 13:07:50 -0700 Subject: [PATCH] Revert "unreachable_from_main: Report unreachable constructors (#4162)" (#4253) This reverts commit 644755218f410b4a68e801d0ba0be9788b42c89a. --- lib/src/rules/unreachable_from_main.dart | 166 ++---------- test/rules/unreachable_from_main_test.dart | 286 +-------------------- 2 files changed, 19 insertions(+), 433 deletions(-) diff --git a/lib/src/rules/unreachable_from_main.dart b/lib/src/rules/unreachable_from_main.dart index 1de5135e8..8acb59188 100644 --- a/lib/src/rules/unreachable_from_main.dart +++ b/lib/src/rules/unreachable_from_main.dart @@ -8,16 +8,15 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/element/element.dart'; import 'package:analyzer/dart/element/type.dart'; -import 'package:collection/collection.dart'; import '../analyzer.dart'; const _desc = 'Unreachable top-level members in executable libraries.'; const _details = r''' -Top-level members and static members in an executable library should be used -directly inside this library. An executable library is a library that contains -a `main` top-level function or that contains a top-level function annotated with +Top-level members in an executable library should be used directly inside this +library. An executable library is a library that contains a `main` top-level +function or that contains a top-level function annotated with `@pragma('vm:entry-point')`). Executable libraries are not usually imported and it's better to avoid defining unused members. @@ -44,7 +43,7 @@ void f() {} class UnreachableFromMain extends LintRule { static const LintCode code = LintCode('unreachable_from_main', - "Unreachable member '{0}' in an executable library.", + 'Unreachable top-level member in an executable library.', correctionMessage: 'Try referencing the member or removing it.'); UnreachableFromMain() @@ -100,12 +99,7 @@ class _DeclarationGatherer { } void _addStaticMember(ClassMember member) { - if (member is ConstructorDeclaration) { - var e = member.declaredElement; - if (e != null && e.isPublic && member.parent is! EnumDeclaration) { - declarations.add(member); - } - } else if (member is FieldDeclaration && member.isStatic) { + if (member is FieldDeclaration && member.isStatic) { for (var field in member.fields.variables) { var e = field.declaredElement; if (e != null && e.isPublic) { @@ -121,26 +115,13 @@ class _DeclarationGatherer { } } -/// A visitor which gathers the declarations of the "references" it visits. -/// -/// "References" are most often [SimpleIdentifier]s, but can also be other -/// nodes which refer to a declaration. -// TODO(srawlins): Add support for patterns. -class _ReferenceVisitor extends RecursiveAstVisitor { +/// A visitor which gathers the declarations of the identifiers it visits. +class _IdentifierVisitor extends RecursiveAstVisitor { Map declarationMap; Set declarations = {}; - _ReferenceVisitor(this.declarationMap); - - @override - void visitAnnotation(Annotation node) { - var e = node.element; - if (e != null) { - _addDeclaration(e); - } - super.visitAnnotation(node); - } + _IdentifierVisitor(this.declarationMap); @override void visitAssignmentExpression(AssignmentExpression node) { @@ -148,45 +129,6 @@ class _ReferenceVisitor extends RecursiveAstVisitor { super.visitAssignmentExpression(node); } - @override - void visitClassDeclaration(ClassDeclaration node) { - var element = node.declaredElement; - if (element != null) { - var hasConstructors = - node.members.any((e) => e is ConstructorDeclaration); - if (!hasConstructors) { - // The default constructor will have an implicit super-initializer to - // the super-type's unnamed constructor. - _addDefaultSuperConstructorDeclaration(node); - } - } - super.visitClassDeclaration(node); - } - - @override - void visitConstructorDeclaration(ConstructorDeclaration node) { - // If a constructor does not have an explicit super-initializer (or redirection?) - // then it has an implicit super-initializer to the super-type's unnamed constructor. - var hasSuperInitializer = - node.initializers.any((e) => e is SuperConstructorInvocation); - if (!hasSuperInitializer) { - var enclosingClass = node.parent; - if (enclosingClass is ClassDeclaration) { - _addDefaultSuperConstructorDeclaration(enclosingClass); - } - } - super.visitConstructorDeclaration(node); - } - - @override - void visitConstructorName(ConstructorName node) { - var e = node.staticElement; - if (e != null) { - _addDeclaration(e); - } - super.visitConstructorName(node); - } - @override void visitPostfixExpression(PostfixExpression node) { _visitCompoundAssignmentExpression(node); @@ -199,16 +141,6 @@ class _ReferenceVisitor extends RecursiveAstVisitor { super.visitPrefixExpression(node); } - @override - void visitRedirectingConstructorInvocation( - RedirectingConstructorInvocation node) { - var element = node.staticElement; - if (element != null) { - _addDeclaration(element); - } - super.visitRedirectingConstructorInvocation(node); - } - @override void visitSimpleIdentifier(SimpleIdentifier node) { if (!node.inDeclarationContext()) { @@ -220,15 +152,6 @@ class _ReferenceVisitor extends RecursiveAstVisitor { super.visitSimpleIdentifier(node); } - @override - void visitSuperConstructorInvocation(SuperConstructorInvocation node) { - var e = node.staticElement; - if (e != null) { - _addDeclaration(e); - } - super.visitSuperConstructorInvocation(node); - } - /// Adds the declaration of the top-level element which contains [element] to /// [declarations], if it is found in [declarationMap]. /// @@ -244,8 +167,8 @@ class _ReferenceVisitor extends RecursiveAstVisitor { declarations.add(enclosingTopLevelDeclaration); } - // Also add [element]'s declaration if it is a constructor, static accessor, - // or static method. + // Also add [element]'s declaration if it is a static accessor or static + // method. if (element.isPrivate) { return; } @@ -255,7 +178,7 @@ class _ReferenceVisitor extends RecursiveAstVisitor { } if (enclosingElement is InterfaceElement || enclosingElement is ExtensionElement) { - if (element is ConstructorElement) { + if (element is PropertyAccessorElement && element.isStatic) { var declaration = declarationMap[element]; if (declaration != null) { declarations.add(declaration); @@ -265,23 +188,6 @@ class _ReferenceVisitor extends RecursiveAstVisitor { if (declaration != null) { declarations.add(declaration); } - } else if (element is PropertyAccessorElement && element.isStatic) { - var declaration = declarationMap[element]; - if (declaration != null) { - declarations.add(declaration); - } - } - } - } - - void _addDefaultSuperConstructorDeclaration(ClassDeclaration class_) { - var classElement = class_.declaredElement; - var supertype = classElement?.supertype; - if (supertype != null) { - var unnamedConstructor = - supertype.constructors.firstWhereOrNull((e) => e.name.isEmpty); - if (unnamedConstructor != null) { - _addDeclaration(unnamedConstructor); } } } @@ -335,14 +241,13 @@ class _Visitor extends SimpleAstVisitor { } } - // The set of the declarations which each top-level and static declaration - // references. + // The set of the declarations which each top-level declaration references. var dependencies = >{}; // Map each declaration to the collection of declarations which are // referenced within its body. for (var declaration in declarations) { - var visitor = _ReferenceVisitor(declarationByElement); + var visitor = _IdentifierVisitor(declarationByElement); declaration.accept(visitor); dependencies[declaration] = visitor.declarations; } @@ -371,25 +276,15 @@ class _Visitor extends SimpleAstVisitor { }); for (var member in unusedMembers) { - if (member is ConstructorDeclaration) { - if (member.name == null) { - rule.reportLint(member.returnType, arguments: [member.nameForError]); - } else { - rule.reportLintForToken(member.name, - arguments: [member.nameForError]); - } - } else if (member is NamedCompilationUnitMember) { - rule.reportLintForToken(member.name, arguments: [member.nameForError]); + if (member is NamedCompilationUnitMember) { + rule.reportLintForToken(member.name); } else if (member is VariableDeclaration) { - rule.reportLintForToken(member.name, arguments: [member.nameForError]); + rule.reportLintForToken(member.name); } else if (member is ExtensionDeclaration) { - var memberName = member.name; rule.reportLintForToken( - memberName ?? member.firstTokenAfterCommentAndMetadata, - arguments: [member.nameForError]); + member.name ?? member.firstTokenAfterCommentAndMetadata); } else { - rule.reportLintForToken(member.firstTokenAfterCommentAndMetadata, - arguments: [member.nameForError]); + rule.reportLintForToken(member.firstTokenAfterCommentAndMetadata); } } } @@ -428,28 +323,3 @@ extension on Annotation { return type is InterfaceType && type.element.isPragma; } } - -extension on Declaration { - String get nameForError { - // TODO(srawlins): Move this to analyzer when other uses are found. - // TODO(srawlins): Convert to switch-expression, hopefully. - var self = this; - if (self is ConstructorDeclaration) { - return self.name?.lexeme ?? self.returnType.name; - } else if (self is EnumConstantDeclaration) { - return self.name.lexeme; - } else if (self is ExtensionDeclaration) { - var name = self.name; - return name?.lexeme ?? 'the unnamed extension'; - } else if (self is MethodDeclaration) { - return self.name.lexeme; - } else if (self is NamedCompilationUnitMember) { - return self.name.lexeme; - } else if (self is VariableDeclaration) { - return self.name.lexeme; - } - - assert(false, 'Uncovered Declaration subtype: ${self.runtimeType}'); - return ''; - } -} diff --git a/test/rules/unreachable_from_main_test.dart b/test/rules/unreachable_from_main_test.dart index 0ffcab822..9e47bd21d 100644 --- a/test/rules/unreachable_from_main_test.dart +++ b/test/rules/unreachable_from_main_test.dart @@ -9,6 +9,7 @@ import '../rule_test_support.dart'; main() { defineReflectiveSuite(() { // TODO(srawlins): Add tests for unreachable public constructors. + // TODO(srawlins): Add tests for errors that should be reported in parts. defineReflectiveTests(UnreachableFromMainTest); }); } @@ -116,10 +117,6 @@ class C { } ''', [ lint(22, 1), - // TODO(srawlins): See if we can skip reporting a declaration if it's - //enclosing declaration is being reported. - lint(28, 1), - lint(37, 5), ]); } @@ -194,287 +191,6 @@ part 'part.dart'; ]); } - test_constructor_named_onEnum() async { - await assertDiagnostics(r''' -void main() { - E.one; - E.two; -} - -enum E { - one(), two(); - - const E(); - const E.named(); -} -''', [ - // No lint. - error(HintCode.UNUSED_ELEMENT, 84, 5), - ]); - } - - test_constructor_named_reachableViaDirectCall() async { - await assertNoDiagnostics(r''' -void main() { - C.named(); -} - -class C { - C.named(); -} -'''); - } - - test_constructor_named_reachableViaExplicitSuperCall() async { - await assertNoDiagnostics(r''' -void main() { - D(); -} - -class C { - C.named(); -} - -class D extends C { - D() : super.named(); -} -'''); - } - - test_constructor_named_reachableViaRedirectedConstructor() async { - await assertNoDiagnostics(r''' -void main() { - C.two(); -} - -class C { - C.named(); - factory C.two() = C.named; -} -'''); - } - - test_constructor_named_reachableViaRedirection() async { - await assertNoDiagnostics(r''' -void main() { - C.two(); -} - -class C { - C.named(); - - C.two() : this.named(); -} -'''); - } - - test_constructor_named_reachableViaTearoff() async { - await assertNoDiagnostics(r''' -void main() { - C.named; -} - -class C { - C.named(); -} -'''); - } - - test_constructor_named_unreachable() async { - await assertDiagnostics(r''' -void main() { - C; -} - -class C { - C.named(); -} -''', [ - lint(36, 5), - ]); - } - - test_constructor_named_unreachable_otherHasRedirectedConstructor() async { - await assertDiagnostics(r''' -void main() { - C.two(); -} - -class C { - C.named(); - C.one(); - factory C.two() = C.one; -} -''', [ - lint(42, 5), - ]); - } - - test_constructor_unnamed_reachableViaDefaultImplicitSuperCall() async { - await assertNoDiagnostics(r''' -void main() { - D(); -} - -class C { - C(); -} - -class D extends C { - // Just a default constructor. -} -'''); - } - - test_constructor_unnamed_reachableViaDirectCall() async { - await assertNoDiagnostics(r''' -void main() { - C(); -} - -class C { - C(); -} -'''); - } - - test_constructor_unnamed_reachableViaExplicitSuperCall() async { - await assertNoDiagnostics(r''' -void main() { - D(); -} - -class C { - C(); -} - -class D extends C { - D() : super(); -} -'''); - } - - test_constructor_unnamed_reachableViaImplicitSuperCall() async { - await assertNoDiagnostics(r''' -void main() { - D(); -} - -class C { - C(); -} - -class D extends C { - D(); -} -'''); - } - - test_constructor_unnamed_reachableViaImplicitSuperCall_indirectly() async { - await assertNoDiagnostics(r''' -void main() { - E(); -} - -class C { - C(); -} - -class D extends C { - // Just a default constructor. -} - -class E extends D { - E(); -} -'''); - } - - test_constructor_unnamed_reachableViaImplicitSuperCall_superParameters() async { - await assertNoDiagnostics(r''' -void main() { - D(1); -} - -class C { - C(int p); -} - -class D extends C { - D(super.p); -} -'''); - } - - test_constructor_unnamed_reachableViaRedirectedConstructor() async { - await assertNoDiagnostics(r''' -void main() { - C.two(); -} - -class C { - C(); - factory C.two() = C; -} -'''); - } - - test_constructor_unnamed_reachableViaRedirection() async { - await assertNoDiagnostics(r''' -void main() { - C.two(); -} - -class C { - C(); - - C.two() : this(); -} -'''); - } - - test_constructor_unnamed_reachableViaTearoff() async { - await assertNoDiagnostics(r''' -void main() { - C.new; -} - -class C { - C(); -} -'''); - } - - test_constructor_unnamed_unreachable() async { - await assertDiagnostics(r''' -void main() { - C; -} - -class C { - C(); -} -''', [ - lint(34, 1), - ]); - } - - test_constructor_unnamed_unreachable_otherHasRedirection() async { - await assertDiagnostics(r''' -void main() { - C.two(); -} - -class C { - C(); - C.one(); - C.two() : this.one(); -} -''', [ - lint(40, 1), - ]); - } - test_enum_reachableViaValue() async { await assertNoDiagnostics(r''' void main() {