diff --git a/lib/src/rules/prefer_final_fields.dart b/lib/src/rules/prefer_final_fields.dart index 9f18f9fcc..1001fdd05 100644 --- a/lib/src/rules/prefer_final_fields.dart +++ b/lib/src/rules/prefer_final_fields.dart @@ -2,8 +2,6 @@ // 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 'dart:collection'; - import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/token.dart'; import 'package:analyzer/dart/ast/visitor.dart'; @@ -83,17 +81,6 @@ class NotAssignedInAllConstructors { ``` '''; -bool _containedInFormal(Element element, FormalParameter formal) { - var formalField = formal.declaredElement; - return formalField is FieldFormalParameterElement && - formalField.field == element; -} - -bool _containedInInitializer( - Element element, ConstructorInitializer initializer) => - initializer is ConstructorFieldInitializer && - initializer.fieldName.canonicalElement == element; - class PreferFinalFields extends LintRule { static const LintCode code = LintCode( 'prefer_final_fields', "The private field {0} could be 'final'.", @@ -112,16 +99,39 @@ class PreferFinalFields extends LintRule { @override void registerNodeProcessors( NodeLintRegistry registry, LinterContext context) { - var visitor = _Visitor(this); + var visitor = _Visitor(this, context); registry.addCompilationUnit(this, visitor); - registry.addFieldDeclaration(this, visitor); } } -class _MutatedFieldsCollector extends RecursiveAstVisitor { - final Set _mutatedFields; +class _DeclarationsCollector extends RecursiveAstVisitor { + final fields = {}; + + @override + void visitFieldDeclaration(FieldDeclaration node) { + if (node.parent is EnumDeclaration) return; + if (node.fields.isFinal || node.fields.isConst) { + return; + } + + for (var variable in node.fields.variables) { + var element = variable.declaredElement; + if (element is FieldElement && + element.isPrivate && + !element.overridesField) { + fields[element] = variable; + } + } + } +} + +class _FieldMutationFinder extends RecursiveAstVisitor { + /// The collection of fields declared in this library. + /// + /// This visitor removes a field when it finds that it is assigned anywhere. + final Map _fields; - _MutatedFieldsCollector(this._mutatedFields); + _FieldMutationFinder(this._fields); @override void visitAssignmentExpression(AssignmentExpression node) { @@ -148,7 +158,7 @@ class _MutatedFieldsCollector extends RecursiveAstVisitor { void _addMutatedFieldElement(CompoundAssignmentExpression assignment) { var element = assignment.writeElement?.canonicalElement; if (element is FieldElement) { - _mutatedFields.add(element); + _fields.remove(element); } } } @@ -156,59 +166,49 @@ class _MutatedFieldsCollector extends RecursiveAstVisitor { class _Visitor extends SimpleAstVisitor { final LintRule rule; - final Set _mutatedFields = HashSet(); + final LinterContext context; - _Visitor(this.rule); + _Visitor(this.rule, this.context); @override void visitCompilationUnit(CompilationUnit node) { - node.accept(_MutatedFieldsCollector(_mutatedFields)); - } - - @override - void visitFieldDeclaration(FieldDeclaration node) { - if (node.parent is EnumDeclaration) return; + var declarationsCollector = _DeclarationsCollector(); + node.accept(declarationsCollector); + var fields = declarationsCollector.fields; - var fields = node.fields; - if (fields.isFinal || fields.isConst) { - return; + var fieldMutationFinder = _FieldMutationFinder(fields); + for (var unit in context.allUnits) { + unit.unit.accept(fieldMutationFinder); } - for (var variable in fields.variables) { - var element = variable.declaredElement; + for (var MapEntry(key: field, value: variable) in fields.entries) { + // TODO(srawlins): We could look at the constructors once and store a set + // of which fields are initialized by any, and a set of which fields are + // initialized by all. This would concievably improve performance. + var classDeclaration = variable.parent?.parent?.parent; + var constructors = classDeclaration is ClassDeclaration + ? classDeclaration.members.whereType() + : []; - if (element is PropertyInducingElement && - element.isPrivate && - !_mutatedFields.contains(element)) { - bool fieldInConstructor(ConstructorDeclaration constructor) => - constructor.initializers.any((ConstructorInitializer initializer) => - _containedInInitializer(element, initializer)) || - constructor.parameters.parameters.any((FormalParameter formal) => - _containedInFormal(element, formal)); - - var classDeclaration = node.parent; - var constructors = classDeclaration is ClassDeclaration - ? classDeclaration.members.whereType() - : []; - var isFieldInConstructors = constructors.any(fieldInConstructor); - var isFieldInAllConstructors = constructors.every(fieldInConstructor); - - if (element.overridesField()) continue; - - if (isFieldInConstructors) { - if (isFieldInAllConstructors) { - rule.reportLint(variable, arguments: [variable.name.lexeme]); - } - } else if (element.hasInitializer) { + var isSetInAnyConstructor = constructors + .any((constructor) => field.isSetInConstructor(constructor)); + + if (isSetInAnyConstructor) { + var isSetInEveryConstructor = constructors + .every((constructor) => field.isSetInConstructor(constructor)); + + if (isSetInEveryConstructor) { rule.reportLint(variable, arguments: [variable.name.lexeme]); } + } else if (field.hasInitializer) { + rule.reportLint(variable, arguments: [variable.name.lexeme]); } } } } extension on VariableElement { - bool overridesField() { + bool get overridesField { var enclosingElement = this.enclosingElement; if (enclosingElement is! InterfaceElement) return false; @@ -219,4 +219,20 @@ extension on VariableElement { .lookUpSetter2(name, inherited: true, library) != null; } + + bool isSetInConstructor(ConstructorDeclaration constructor) => + constructor.initializers.any(isSetInInitializer) || + constructor.parameters.parameters.any(isSetInParameter); + + /// Whether `this` is initialized in [initializer]. + bool isSetInInitializer(ConstructorInitializer initializer) => + initializer is ConstructorFieldInitializer && + initializer.fieldName.canonicalElement == this; + + /// Whether `this` is initialized with [parameter]. + bool isSetInParameter(FormalParameter parameter) { + var formalField = parameter.declaredElement; + return formalField is FieldFormalParameterElement && + formalField.field == this; + } } diff --git a/test/rules/prefer_final_fields_test.dart b/test/rules/prefer_final_fields_test.dart index a1a78b803..22d6a680a 100644 --- a/test/rules/prefer_final_fields_test.dart +++ b/test/rules/prefer_final_fields_test.dart @@ -17,6 +17,38 @@ class PreferFinalFieldsTest extends LintRuleTest { @override String get lintRule => 'prefer_final_fields'; + test_assignedInPart() async { + newFile('$testPackageLibPath/part.dart', r''' +part of 'test.dart'; +int f(C c) { + c._x = 1; + return c._x; +} +'''); + await assertNoDiagnostics(r''' +part 'part.dart'; +class C { + int _x = 0; +} +'''); + } + + test_declaredInPart() async { + newFile('$testPackageLibPath/part.dart', r''' +part of 'test.dart'; +class C { + int _x = 0; +} +'''); + await assertNoDiagnostics(r''' +part 'part.dart'; +int f(C c) { + c._x = 1; + return c._x; +} +'''); + } + test_enum() async { await assertDiagnostics(r''' enum A {