diff --git a/example/all.yaml b/example/all.yaml index 6d2a3f698..f3415b745 100644 --- a/example/all.yaml +++ b/example/all.yaml @@ -34,6 +34,7 @@ linter: - avoid_types_as_parameter_names - avoid_types_on_closure_parameters - avoid_unused_constructor_parameters + - avoid_unused_functions - await_only_futures - camel_case_types - cancel_subscriptions diff --git a/lib/src/rules.dart b/lib/src/rules.dart index 706e246ed..89ef5536f 100644 --- a/lib/src/rules.dart +++ b/lib/src/rules.dart @@ -35,6 +35,7 @@ import 'package:linter/src/rules/avoid_slow_async_io.dart'; import 'package:linter/src/rules/avoid_types_as_parameter_names.dart'; import 'package:linter/src/rules/avoid_types_on_closure_parameters.dart'; import 'package:linter/src/rules/avoid_unused_constructor_parameters.dart'; +import 'package:linter/src/rules/avoid_unused_functions.dart'; import 'package:linter/src/rules/await_only_futures.dart'; import 'package:linter/src/rules/camel_case_types.dart'; import 'package:linter/src/rules/cancel_subscriptions.dart'; @@ -154,6 +155,7 @@ void registerLintRules() { ..register(new AvoidSlowAsyncIo()) ..register(new AvoidTypesAsParameterNames()) ..register(new AvoidUnusedConstructorParameters()) + ..register(new AvoidUnusedFunctions()) ..register(new AwaitOnlyFutures()) ..registerDefault(new CamelCaseTypes()) ..register(new CancelSubscriptions()) diff --git a/lib/src/rules/avoid_unused_functions.dart b/lib/src/rules/avoid_unused_functions.dart new file mode 100644 index 000000000..e2e1f66ad --- /dev/null +++ b/lib/src/rules/avoid_unused_functions.dart @@ -0,0 +1,176 @@ +// Copyright (c) 2018, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:linter/src/analyzer.dart'; + +const _desc = r'Avoid unused functions in expressions.'; + +const _details = r''' + +**AVOID** unused functions in expressions. + +Expressions that have no side effects but include a function that is not called +indicate missing parentheses. + +For example, + +**BAD:** +``` +list.clear; +flag ? list.clear() : list.sort; +``` + +Since an unused tearoff has no effect, this was almost certainly the intent: + +**GOOD:** +``` +list.clear(); +flag ? list.clear() : list.sort(); +``` + +'''; + +class AvoidUnusedFunctions extends LintRule implements NodeLintRule { + AvoidUnusedFunctions() + : super( + name: 'avoid_unused_functions', + description: _desc, + details: _details, + group: Group.errors); + + @override + void registerNodeProcessors(NodeLintRegistry registry) { + final visitor = new _Visitor(new _ReportNoClearEffectVisitor(this)); + registry.addExpressionStatement(this, visitor); + registry.addForStatement(this, visitor); + registry.addCascadeExpression(this, visitor); + } +} + +class _Visitor extends SimpleAstVisitor { + final _ReportNoClearEffectVisitor reportNoClearEffect; + + _Visitor(this.reportNoClearEffect); + @override + void visitCascadeExpression(CascadeExpression node) { + for (var section in node.cascadeSections) { + if (section is PropertyAccess && section.bestType is FunctionType) { + reportNoClearEffect.rule.reportLint(section); + } + } + } + + @override + void visitExpressionStatement(ExpressionStatement node) { + if (node.parent is FunctionBody) { + return; + } + node.expression.accept(reportNoClearEffect); + } + + @override + void visitForStatement(ForStatement node) { + node.initialization?.accept(reportNoClearEffect); + node.updaters?.forEach((u) { + u.accept(reportNoClearEffect); + }); + } +} + +class _ReportNoClearEffectVisitor extends UnifyingAstVisitor { + final LintRule rule; + + _ReportNoClearEffectVisitor(this.rule); + + @override + visitAssignmentExpression(AssignmentExpression node) { + // Has a clear effect + } + + @override + visitAwaitExpression(AwaitExpression node) { + // Has a clear effect + } + + @override + visitBinaryExpression(BinaryExpression node) { + switch (node.operator.lexeme) { + case '??': + case '||': + case '&&': + // these are OK when used for control flow + node.rightOperand.accept(this); + return; + } + + super.visitBinaryExpression(node); + } + + @override + visitCascadeExpression(CascadeExpression node) { + // Has a clear effect + } + + @override + visitConditionalExpression(ConditionalExpression node) { + node.thenExpression.accept(this); + node.elseExpression.accept(this); + } + + @override + visitFunctionExpressionInvocation(FunctionExpressionInvocation node) { + // Has a clear effect + } + + @override + visitInstanceCreationExpression(InstanceCreationExpression node) { + // A few APIs use this for side effects, like Timer. Also, for constructors + // that have side effects, they should have tests. Those tests will often + // include an instantiation expression statement with nothing else. + } + + @override + visitMethodInvocation(MethodInvocation node) { + // Has a clear effect + } + + @override + visitNode(AstNode expression) { + if (expression is Expression && expression.bestType is FunctionType) { + rule.reportLint(expression); + } + } + + @override + visitPostfixExpression(PostfixExpression node) { + // Has a clear effect + } + + @override + visitPrefixExpression(PrefixExpression node) { + if (node.operator.lexeme == '--' || node.operator.lexeme == '++') { + // Has a clear effect + return; + } + super.visitPrefixExpression(node); + } + + @override + visitRethrowExpression(RethrowExpression node) { + // Has a clear effect + } + + @override + visitSuperConstructorInvocation(SuperConstructorInvocation node) { + // Has a clear effect + } + + @override + visitThrowExpression(ThrowExpression node) { + // Has a clear effect + } +} diff --git a/test/rules/avoid_unused_functions.dart b/test/rules/avoid_unused_functions.dart new file mode 100644 index 000000000..e9c112de7 --- /dev/null +++ b/test/rules/avoid_unused_functions.dart @@ -0,0 +1,161 @@ +// Copyright (c) 2018, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// test w/ `pub run test -N avoid_unused_tearoffs` + +notReturned() { + 1; // OK + 1 + 1; // OK + foo; // LINT + new MyClass().foo; // LINT + new MyClass()..foo; // LINT + new MyClass() + ..getter // OK + ..foo() // OK + ..foo; // LINT + []; // OK + {}; // OK + "blah"; // OK + ~1; // OK + + new MyClass(); // OK + foo(); // OK + new MyClass().foo(); // OK + var x = 2; // OK + x++; // OK + x--; // OK + ++x; // OK + --x; // OK + try { + throw new Exception(); // OK + } catch (x) { + rethrow; // OK + } +} + +asConditionAndReturnOk() { + if (true == someBool) // OK + { + return 1 + 1; // OK + } else if (false == someBool) { + return foo; // OK + } + while (new MyClass() != null) // OK + { + return new MyClass().foo; // OK + } + while (null == someBool) // OK + { + return new MyClass()..foo; // LINT + } + for (; someBool ?? someBool;) // OK + { + return {}; // OK + } + do {} while ("blah".isEmpty); // OK + for (var i in []) {} // OK + switch (~1) // OK + { + } + + () => new MyClass().foo; // LINT + myfun() => new MyClass().foo; // OK + myfun2() => new MyClass()..foo; // LINT +} + +myfun() => new MyClass().foo; // OK +myfun2() => new MyClass()..foo; // LINT + +expressionBranching() { + null ?? 1 + 1; // OK + null ?? foo; // LINT + null ?? new MyClass().foo; // LINT + false || 1 + 1 == 2; // OK + false || foo == true; // OK + false || new MyClass() as bool; // OK + false || new MyClass().foo == true; // OK + true && 1 + 1 == 2; // OK + true && foo == true; // OK + true && new MyClass() as bool; // OK + true && new MyClass().foo == true; // OK + + // ternaries can detect either/both sides + someBool // OK + ? 1 + 1 // OK + : foo(); // OK + someBool // OK + ? foo() // OK + : foo; // LINT + someBool // OK + ? new MyClass() // OK + : foo(); // OK + someBool // OK + ? foo() // OK + : new MyClass().foo; // LINT + someBool // OK + ? [] // OK + : {}; // OK + + // not unnecessary condition, but unnecessary branching + foo() ?? 1 + 1; // OK + foo() || new MyClass() as bool; // OK + foo() && foo == true; // OK + foo() ? 1 + 1 : foo(); // OK + foo() ? foo() : foo; // LINT + foo() ? foo() : new MyClass().foo; // LINT + + null ?? new MyClass(); // OK + null ?? foo(); // OK + null ?? new MyClass().foo(); // OK + false || foo(); // OK + false || new MyClass().foo(); // OK + true && foo(); // OK + true && new MyClass().foo(); // OK + someBool ? foo() : new MyClass().foo(); // OK + foo() ? foo() : new MyClass().foo(); // OK + foo() ? new MyClass() : foo(); // OK +} + +inOtherStatements() { + if (foo()) { + 1; // OK + } + while (someBool) { + 1 + 1; // OK + } + for (foo; foo();) {} // LINT + for (; foo(); 1 + 1) {} // OK + for (; + foo(); + foo(), // OK + 1 + 1, // OK + new MyClass().foo) {} // LINT + do { + new MyClass().foo; // LINT + } while (foo()); + + switch (foo()) { + case true: + []; // OK + break; // OK + case false: + {}; // OK + break; // OK + default: + "blah"; // OK + } + + for (var i in [1, 2, 3]) { + ~1; // OK + } +} + +bool someBool = true; +bool foo() => true; + +class MyClass { + bool foo() => true; + + get getter => true; +}