diff --git a/lib/main.dart b/lib/main.dart index 5657b7a1..9174f8d9 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -1,6 +1,8 @@ import 'package:analysis_server_plugin/plugin.dart'; import 'package:analysis_server_plugin/registry.dart'; import 'package:solid_lints/src/lints/avoid_debug_print_in_release/avoid_debug_print_in_release_rule.dart'; +import 'package:solid_lints/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule.dart'; +import 'package:solid_lints/src/lints/avoid_final_with_getter/fixes/avoid_final_with_getter_fix.dart'; import 'package:solid_lints/src/lints/avoid_global_state/avoid_global_state_rule.dart'; import 'package:solid_lints/src/lints/avoid_non_null_assertion/avoid_non_null_assertion_rule.dart'; import 'package:solid_lints/src/lints/proper_super_calls/proper_super_calls_rule.dart'; @@ -21,6 +23,14 @@ class SolidLintsPlugin extends Plugin { @override void register(PluginRegistry registry) { + registry.registerLintRule( + AvoidFinalWithGetterRule(), + ); + registry.registerFixForRule( + AvoidFinalWithGetterRule.code, + AvoidFinalWithGetterFix.new, + ); + registry.registerLintRule( AvoidGlobalStateRule(), ); diff --git a/lib/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule.dart b/lib/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule.dart index d495c5db..727e3dfe 100644 --- a/lib/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule.dart +++ b/lib/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule.dart @@ -1,12 +1,8 @@ -import 'package:analyzer/diagnostic/diagnostic.dart'; -import 'package:analyzer/error/listener.dart'; -import 'package:analyzer/source/source_range.dart'; -import 'package:custom_lint_builder/custom_lint_builder.dart'; +import 'package:analyzer/analysis_rule/analysis_rule.dart'; +import 'package:analyzer/analysis_rule/rule_context.dart'; +import 'package:analyzer/analysis_rule/rule_visitor_registry.dart'; +import 'package:analyzer/error/error.dart'; import 'package:solid_lints/src/lints/avoid_final_with_getter/visitors/avoid_final_with_getter_visitor.dart'; -import 'package:solid_lints/src/models/rule_config.dart'; -import 'package:solid_lints/src/models/solid_lint_rule.dart'; - -part 'fixes/avoid_final_with_getter_fix.dart'; /// Avoid using final private fields with getters. /// @@ -34,45 +30,32 @@ part 'fixes/avoid_final_with_getter_fix.dart'; /// } /// ``` /// -class AvoidFinalWithGetterRule extends SolidLintRule { +class AvoidFinalWithGetterRule extends AnalysisRule { /// This lint rule represents /// the error whether we use final private fields with getters. static const lintName = 'avoid_final_with_getter'; - final _diagnosticsInfoExpando = Expando(); - - AvoidFinalWithGetterRule._(super.config); + /// The code to report for a violation + static const LintCode code = LintCode( + lintName, + 'Avoid final private fields with getters.', + correctionMessage: 'Remove the getter and make the field public.', + ); /// Creates a new instance of [AvoidFinalWithGetterRule] - /// based on the lint configuration. - factory AvoidFinalWithGetterRule.createRule(CustomLintConfigs configs) { - final rule = RuleConfig( - configs: configs, - name: lintName, - problemMessage: (_) => 'Avoid final private fields with getters.', - ); + AvoidFinalWithGetterRule() + : super(name: lintName, description: code.problemMessage); - return AvoidFinalWithGetterRule._(rule); - } + @override + LintCode get diagnosticCode => code; @override - void run( - CustomLintResolver resolver, - DiagnosticReporter reporter, - CustomLintContext context, + void registerNodeProcessors( + RuleVisitorRegistry registry, + RuleContext context, ) { - context.registry.addCompilationUnit((node) { - final visitor = AvoidFinalWithGetterVisitor(); - node.accept(visitor); + final visitor = AvoidFinalWithGetterVisitor(this); - for (final element in visitor.getters) { - final diagnostic = reporter.atNode(element.getter, code); - - _diagnosticsInfoExpando[diagnostic] = element; - } - }); + registry.addCompilationUnit(this, visitor); } - - @override - List getFixes() => [_FinalWithGetterFix(_diagnosticsInfoExpando)]; } diff --git a/lib/src/lints/avoid_final_with_getter/fixes/avoid_final_with_getter_fix.dart b/lib/src/lints/avoid_final_with_getter/fixes/avoid_final_with_getter_fix.dart index 98e08fb4..dd810642 100644 --- a/lib/src/lints/avoid_final_with_getter/fixes/avoid_final_with_getter_fix.dart +++ b/lib/src/lints/avoid_final_with_getter/fixes/avoid_final_with_getter_fix.dart @@ -1,40 +1,84 @@ -part of '../avoid_final_with_getter_rule.dart'; +import 'package:analysis_server_plugin/edit/dart/correction_producer.dart'; +import 'package:analysis_server_plugin/edit/dart/dart_fix_kind_priority.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/element/element.dart'; +import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart'; +import 'package:analyzer_plugin/utilities/fixes/fixes.dart'; +import 'package:solid_lints/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule.dart'; +import 'package:solid_lints/src/lints/avoid_final_with_getter/visitors/getter_variable_visitor.dart'; +import 'package:solid_lints/src/lints/avoid_final_with_getter/visitors/variable_references_visitor.dart'; -class _FinalWithGetterFix extends DartFix { - final Expando _diagnosticsInfoExpando; +/// A Quick fix for [AvoidFinalWithGetterRule] rule +class AvoidFinalWithGetterFix extends ResolvedCorrectionProducer { + static const _avoidFinalWithGetterKind = FixKind( + 'solid_lints.fix.${AvoidFinalWithGetterRule.lintName}', + DartFixKindPriority.standard, + "Remove the getter and make the field public", + ); - _FinalWithGetterFix(this._diagnosticsInfoExpando); + /// Creates a new instance of [AvoidFinalWithGetterFix] + AvoidFinalWithGetterFix({required super.context}); @override - void run( - CustomLintResolver resolver, - ChangeReporter reporter, - CustomLintContext context, - Diagnostic diagnostic, - List others, - ) { - context.registry.addMethodDeclaration((node) { - if (!diagnostic.sourceRange.intersects(node.sourceRange)) return; - - final info = _diagnosticsInfoExpando[diagnostic]; - if (info == null) return; - - _addReplacement(reporter, info); - }); - } + CorrectionApplicability get applicability => + CorrectionApplicability.acrossFiles; + + @override + FixKind get fixKind => _avoidFinalWithGetterKind; + + @override + Future compute(ChangeBuilder builder) async { + final getterNode = node; + if (getterNode + case MethodDeclaration( + isGetter: true, + declaredFragment: ExecutableFragment( + element: GetterElement( + isAbstract: false, + isPublic: true, + ), + ), + )) { + final compilationUnit = node.thisOrAncestorOfType(); + if (compilationUnit == null) return; + + final getterVariableVisitor = GetterVariableVisitor(getterNode); + compilationUnit.accept(getterVariableVisitor); + + final variableDeclaration = getterVariableVisitor.variable; + if (variableDeclaration == null) return; + + final referencesVisitor = VariableReferencesVisitor(variableDeclaration); + compilationUnit.accept(referencesVisitor); + + final variableReferences = referencesVisitor.references; + + final variableName = variableDeclaration.name.lexeme; + final newPublicVariableName = variableName.startsWith('_') + ? variableName.substring(1) + : variableName; + + await builder.addDartFileEdit(file, (builder) { + builder.addDeletion(getterNode.sourceRange); + + builder.addSimpleReplacement( + variableDeclaration.name.sourceRange, + newPublicVariableName, + ); + + for (final reference in variableReferences) { + if (reference.sourceRange.intersects(getterNode.sourceRange)) { + continue; + } + + builder.addSimpleReplacement( + reference.sourceRange, + newPublicVariableName, + ); + } - void _addReplacement( - ChangeReporter reporter, - FinalWithGetterInfo info, - ) { - final changeBuilder = reporter.createChangeBuilder( - message: "Remove getter and make variable public.", - priority: 1, - ); - - changeBuilder.addDartFileEdit((builder) { - builder.addDeletion(info.getter.sourceRange); - builder.addDeletion(SourceRange(info.variable.name.offset, 1)); - }); + builder.format(compilationUnit.sourceRange); + }); + } } } diff --git a/lib/src/lints/avoid_final_with_getter/utils/getter_reference_id.dart b/lib/src/lints/avoid_final_with_getter/utils/getter_reference_id.dart new file mode 100644 index 00000000..0aa47984 --- /dev/null +++ b/lib/src/lints/avoid_final_with_getter/utils/getter_reference_id.dart @@ -0,0 +1,37 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/element/element.dart'; + +/// Extension method to get the reference id of the getter. +extension GetterReferenceId on MethodDeclaration { + /// Get the reference id of the getter. + int? get getterReferenceId { + final returnExpression = switch (body) { + ExpressionFunctionBody(expression: final expr) || + BlockFunctionBody( + block: Block( + statements: [ + ReturnStatement(expression: final expr?), + ], + ) + ) => + expr, + _ => null, + }; + + final identifier = switch (returnExpression) { + SimpleIdentifier() => returnExpression, + PropertyAccess( + target: ThisExpression(), + :final propertyName, + ) => + propertyName, + _ => null, + }; + + return switch (identifier) { + SimpleIdentifier(element: PropertyAccessorElement(:final variable)) => + variable.id, + _ => null, + }; + } +} diff --git a/lib/src/lints/avoid_final_with_getter/visitors/avoid_final_with_getter_visitor.dart b/lib/src/lints/avoid_final_with_getter/visitors/avoid_final_with_getter_visitor.dart index 6e5bfbf1..ee1aa7de 100644 --- a/lib/src/lints/avoid_final_with_getter/visitors/avoid_final_with_getter_visitor.dart +++ b/lib/src/lints/avoid_final_with_getter/visitors/avoid_final_with_getter_visitor.dart @@ -1,18 +1,24 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/element/element.dart'; -import 'package:solid_lints/src/lints/avoid_final_with_getter/visitors/getter_variable_visitor.dart'; +import 'package:solid_lints/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule.dart'; +import 'package:solid_lints/src/lints/avoid_final_with_getter/utils/getter_reference_id.dart'; /// A visitor that checks for final private fields with getters. /// If a final private field has a getter, it is considered as a public field. class AvoidFinalWithGetterVisitor extends RecursiveAstVisitor { - final _getters = {}; + final AvoidFinalWithGetterRule _rule; - /// List of getters - Set get getters => _getters; + final _gettersPairLookup = {}; + final _fieldsPairLookup = {}; + + /// Creates a new instance of [AvoidFinalWithGetterVisitor] + AvoidFinalWithGetterVisitor(this._rule); @override void visitMethodDeclaration(MethodDeclaration node) { + super.visitMethodDeclaration(node); + if (node case MethodDeclaration( isGetter: true, @@ -21,29 +27,36 @@ class AvoidFinalWithGetterVisitor extends RecursiveAstVisitor { isAbstract: false, isPublic: true, ) - ) + ), + getterReferenceId: final getterId?, )) { - final visitor = GetterVariableVisitor(node); - node.parent?.accept(visitor); + _gettersPairLookup[getterId] = node; - final variable = visitor.variable; - - if (variable != null) { - _getters.add(FinalWithGetterInfo(node, variable)); + if (_fieldsPairLookup.containsKey(getterId)) { + _rule.reportAtNode(node); } } - super.visitMethodDeclaration(node); } -} -/// Information about the final private field with a getter. -class FinalWithGetterInfo { - /// The getter method declaration. - final MethodDeclaration getter; + @override + void visitVariableDeclaration(VariableDeclaration node) { + super.visitVariableDeclaration(node); - /// The variable declaration. - final VariableDeclaration variable; + if (node + case VariableDeclaration( + declaredFragment: VariableFragment( + element: VariableElement( + isPrivate: true, + isFinal: true, + id: final variableId, + ) + ) + )) { + _fieldsPairLookup[variableId] = node; - /// Creates a new instance of [FinalWithGetterInfo] - const FinalWithGetterInfo(this.getter, this.variable); + if (_gettersPairLookup[variableId] case final getter?) { + _rule.reportAtNode(getter); + } + } + } } diff --git a/lib/src/lints/avoid_final_with_getter/visitors/getter_variable_visitor.dart b/lib/src/lints/avoid_final_with_getter/visitors/getter_variable_visitor.dart index 978b44d6..0e9b4195 100644 --- a/lib/src/lints/avoid_final_with_getter/visitors/getter_variable_visitor.dart +++ b/lib/src/lints/avoid_final_with_getter/visitors/getter_variable_visitor.dart @@ -1,9 +1,9 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/element/element.dart'; +import 'package:solid_lints/src/lints/avoid_final_with_getter/utils/getter_reference_id.dart'; -/// A visitor that checks the association of the getter with -/// the final private variable +/// A visitor that gets the final private variable associated with the getter. class GetterVariableVisitor extends RecursiveAstVisitor { final int? _getterId; VariableDeclaration? _variable; @@ -17,29 +17,19 @@ class GetterVariableVisitor extends RecursiveAstVisitor { @override void visitVariableDeclaration(VariableDeclaration node) { - final element = node.declaredFragment?.element; - if (element != null && - element.isPrivate && - element.isFinal && - element.id == _getterId) { + if (node + case VariableDeclaration( + declaredFragment: VariableFragment( + element: VariableElement( + isPrivate: true, + isFinal: true, + :final id, + ) + ) + ) when id == _getterId) { _variable = node; } super.visitVariableDeclaration(node); } } - -extension on MethodDeclaration { - int? get getterReferenceId { - if (body - case ExpressionFunctionBody( - expression: SimpleIdentifier( - element: PropertyAccessorElement(:final variable) - ) - )) { - return variable.id; - } - - return null; - } -} diff --git a/lib/src/lints/avoid_final_with_getter/visitors/variable_references_visitor.dart b/lib/src/lints/avoid_final_with_getter/visitors/variable_references_visitor.dart new file mode 100644 index 00000000..9f3fee2c --- /dev/null +++ b/lib/src/lints/avoid_final_with_getter/visitors/variable_references_visitor.dart @@ -0,0 +1,32 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/syntactic_entity.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/element.dart'; + +/// Finds all references to a variable declaration. +class VariableReferencesVisitor extends RecursiveAstVisitor { + final VariableDeclaration _variableDeclaration; + final List _references = []; + + /// List of found references to the variable declaration. + List get references => _references; + + /// Creates a new instance of [VariableReferencesVisitor] + VariableReferencesVisitor(this._variableDeclaration); + + @override + void visitSimpleIdentifier(SimpleIdentifier node) { + super.visitSimpleIdentifier(node); + + final variableId = _variableDeclaration.declaredFragment?.element.id; + final referencedVariableId = switch (node.element) { + PropertyAccessorElement(:final variable) => variable.id, + final element? => element.id, + _ => null, + }; + + if (referencedVariableId == variableId) { + _references.add(node); + } + } +} diff --git a/lint_test/avoid_final_with_getter_test.dart b/lint_test/avoid_final_with_getter_test.dart deleted file mode 100644 index 4f0fa323..00000000 --- a/lint_test/avoid_final_with_getter_test.dart +++ /dev/null @@ -1,35 +0,0 @@ -// ignore_for_file: type_annotate_public_apis, prefer_match_file_name, unused_local_variable - -/// Check final private field with getter fail -/// `avoid_final_with_getter` - -class Fail { - final int _myField = 0; - - // expect_lint: avoid_final_with_getter - int get myField => _myField; -} - -class FailOtherName { - final int _myField = 0; - - // expect_lint: avoid_final_with_getter - int get myFieldInt => _myField; -} - -class FailStatic { - static final int _myField = 0; - - // expect_lint: avoid_final_with_getter - static int get myField => _myField; -} - -class Skip { - final int _myField = 0; - - int get myField => _myField + 1; // it is not a getter for the field -} - -class Good { - final int myField = 0; -} diff --git a/pubspec.yaml b/pubspec.yaml index f85b1309..18c4d1da 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -11,19 +11,17 @@ environment: sdk: ">=3.5.0 <4.0.0" dependencies: + # Needed until required types for fixes are exported by analyzer_server_plugin + # More details: https://github.com/dart-lang/sdk/issues/61821 + analyzer_plugin: ^0.14.2 analyzer: ^10.0.1 collection: ^1.19.0 analysis_server_plugin: ^0.3.3 glob: ^2.1.3 path: ^1.9.1 yaml: ^3.1.3 - # These packages are required for pana analysis to run correctly - flutter: - sdk: flutter - test: ^1.25.14 dev_dependencies: args: ^2.6.0 analyzer_testing: ^0.1.9 test_reflective_loader: ^0.3.0 - diff --git a/test/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule_test.dart b/test/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule_test.dart new file mode 100644 index 00000000..2149a1c0 --- /dev/null +++ b/test/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule_test.dart @@ -0,0 +1,131 @@ +import 'package:analyzer_testing/analysis_rule/analysis_rule.dart'; +import 'package:solid_lints/src/lints/avoid_final_with_getter/avoid_final_with_getter_rule.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +void main() { + defineReflectiveSuite(() { + defineReflectiveTests(AvoidFinalWithGetterRuleTest); + }); +} + +@reflectiveTest +class AvoidFinalWithGetterRuleTest extends AnalysisRuleTest { + @override + void setUp() { + rule = AvoidFinalWithGetterRule(); + super.setUp(); + } + + Future test_reports_on_getter_with_same_name_as_field() async { + await assertDiagnostics( + r''' +class Test { + final int _myField = 0; + + int get myField => _myField; +} +''', + [lint(44, 28)], + ); + } + + Future test_reports_on_getter_with_different_name_from_field() async { + await assertDiagnostics( + r''' +class Test { + final int _myField = 0; + + int get myFieldInt => _myField; +} +''', + [lint(44, 31)], + ); + } + + Future test_reports_on_static_getter_with_private_field() async { + await assertDiagnostics( + r''' +class Test { + static final int _myField = 0; + + static int get myField => _myField; +} +''', + [lint(51, 35)], + ); + } + + Future test_reports_on_getter_with_this_property_access() async { + await assertDiagnostics( + r''' +class Test { + final int _myField = 0; + + int get myField => this._myField; +} +''', + [lint(44, 33)], + ); + } + + Future + test_reports_on_block_body_getter_returning_private_field() async { + await assertDiagnostics( + r''' +class Test { + final int _myField = 0; + + int get myField { + return _myField; + } +} +''', + [lint(44, 42)], + ); + } + + Future test_does_not_report_on_getter_that_contains_logic() async { + await assertNoDiagnostics(r''' +class Test { + final int _myField = 0; + final int _myField2 = 1; + final int _myField3 = 2; + + int get myField => _myField + 1; + + int get myField2 => this._myField2 + 1; + + int get myField3 { + return this._myField3 + 1; + } +} +'''); + } + + Future test_does_not_report_on_public_final_field() async { + await assertNoDiagnostics(r''' +class Test { + final int myField = 0; +} +'''); + } + + Future + test_does_not_report_on_getter_returning_mutable_private_field() async { + await assertNoDiagnostics(r''' +class Test { + int _myField = 0; + int _myField2 = 1; + int _myField3 = 2; + + int get myField => _myField; + + int get myField2 => this._myField2; + + int get myField3 { + return _myField3; + } +} +'''); + } +}