diff --git a/lib/main.dart b/lib/main.dart index a6f55fad..3e1f5401 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -7,6 +7,7 @@ import 'package:solid_lints/src/lints/avoid_global_state/avoid_global_state_rule import 'package:solid_lints/src/lints/avoid_late_keyword/avoid_late_keyword_rule.dart'; import 'package:solid_lints/src/lints/avoid_non_null_assertion/avoid_non_null_assertion_rule.dart'; import 'package:solid_lints/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule.dart'; +import 'package:solid_lints/src/lints/avoid_similar_names/avoid_similar_names_rule.dart'; import 'package:solid_lints/src/lints/avoid_unnecessary_return_variable/avoid_unnecessary_return_variable_rule.dart'; import 'package:solid_lints/src/lints/avoid_unnecessary_setstate/avoid_unnecessary_set_state_rule.dart'; import 'package:solid_lints/src/lints/avoid_unnecessary_type_assertions/avoid_unnecessary_type_assertions_rule.dart'; @@ -57,6 +58,7 @@ class SolidLintsPlugin extends Plugin { AvoidLateKeywordRule(analysisOptionsLoader: analysisLoader), AvoidNonNullAssertionRule(analysisOptionsLoader: analysisLoader), AvoidReturningWidgetsRule(analysisOptionsLoader: analysisLoader), + AvoidSimilarNamesRule(), AvoidUnnecessaryReturnVariableRule(), AvoidUnnecessarySetStateRule(), AvoidUnnecessaryTypeAssertionsRule(), diff --git a/lib/src/lints/avoid_similar_names/avoid_similar_names_rule.dart b/lib/src/lints/avoid_similar_names/avoid_similar_names_rule.dart new file mode 100644 index 00000000..83d4f36d --- /dev/null +++ b/lib/src/lints/avoid_similar_names/avoid_similar_names_rule.dart @@ -0,0 +1,66 @@ +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_similar_names/visitors/avoid_similar_names_visitor.dart'; +import 'package:solid_lints/src/models/solid_lint_rule.dart'; + +/// Avoid similar names +/// +/// Warns about variables or parameters that have confusingly similar names +/// within the same function scope (e.g., using numeric suffixes or +/// single-letter modifiers like `someClass1` and `someClass2`). +/// +/// This encourages using descriptive, distinct names to improve code +/// readability and prevent logical errors caused by mixing up variables. +/// +/// ### Example +/// +/// #### BAD: +/// +/// ```dart +/// void test(SomeClass someClass1, SomeClass someClass2) { // LINT +/// final tempA = 'a'; // LINT +/// final tempB = 'b'; // LINT +/// } +/// ``` +/// +/// #### GOOD: +/// +/// ```dart +/// void test(SomeClass first, SomeClass second) { +/// final that = 'a'; +/// final other = 'b'; +/// } +/// ``` +class AvoidSimilarNamesRule extends SolidLintRule { + /// The name of this lint rule. + static const lintName = 'avoid_similar_names'; + + static const _code = LintCode( + lintName, + 'Avoid using similar names.', + correctionMessage: 'Use more descriptive names.', + ); + + /// Creates an instance of [AvoidSimilarNamesRule]. + AvoidSimilarNamesRule() + : super( + name: lintName, + description: 'Warns about variables or parameters with similar names.', + ); + + @override + LintCode get diagnosticCode => _code; + + @override + void registerNodeProcessors( + RuleVisitorRegistry registry, + RuleContext context, + ) { + final visitor = AvoidSimilarNamesVisitor(this); + + registry.addMethodDeclaration(this, visitor); + registry.addConstructorDeclaration(this, visitor); + registry.addFunctionDeclaration(this, visitor); + } +} diff --git a/lib/src/lints/avoid_similar_names/models/scope_variable.dart b/lib/src/lints/avoid_similar_names/models/scope_variable.dart new file mode 100644 index 00000000..11991556 --- /dev/null +++ b/lib/src/lints/avoid_similar_names/models/scope_variable.dart @@ -0,0 +1,49 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/token.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:solid_lints/src/lints/avoid_similar_names/utils/name_tokenizer.dart'; + +/// Represents a variable or parameter collected from a scope. +class ScopeVariable { + /// The resolved type of the variable, if available. + final DartType? type; + + /// The AST node representing this variable declaration. + final AstNode node; + + /// The token representing the name. + final Token nameToken; + + /// The individual word/digit tokens of the name. + final List tokens; + + /// The minimum length for a variable name to be considered descriptive enough + /// to be analyzed for similarity. + static const minDescriptiveNameLength = 3; + + /// Creates a new [ScopeVariable] if the [nameToken] is descriptive enough + /// to be analyzed for similarity. Returns `null` if the cleaned name + /// is too short. + static ScopeVariable? createOrNull({ + required Token nameToken, + required DartType? type, + required AstNode node, + }) { + final cleaned = NameTokenizer.cleanName(nameToken.lexeme); + if (cleaned.length < minDescriptiveNameLength) return null; + + return ScopeVariable._( + nameToken: nameToken, + type: type, + node: node, + tokens: NameTokenizer.tokenize(cleaned), + ); + } + + ScopeVariable._({ + required this.nameToken, + required this.type, + required this.node, + required this.tokens, + }); +} diff --git a/lib/src/lints/avoid_similar_names/utils/name_tokenizer.dart b/lib/src/lints/avoid_similar_names/utils/name_tokenizer.dart new file mode 100644 index 00000000..58925281 --- /dev/null +++ b/lib/src/lints/avoid_similar_names/utils/name_tokenizer.dart @@ -0,0 +1,70 @@ +/// Utility class for tokenizing identifiers and +/// comparing name similarity. +abstract final class NameTokenizer { + /// Regex Fragment | Meaning + /// ================================================================= + /// [A-Z]?[a-z]+ | Match words (e.g., Class, user): + /// [A-Z]? | ... optional leading uppercase, + /// [a-z]+ | ... followed by lowercase letters. + /// | | OR + /// [A-Z]+(?=[A-Z][a-z]|\d|\b)| Match acronyms (uppercase letters). + /// | | OR + /// \d+ | Match digits (e.g., 1, 10). + static final _tokenPattern = RegExp( + r'[A-Z]?[a-z]+|[A-Z]+(?=[A-Z][a-z]|\d|\b)|\d+', + ); + + /// Regex Fragment | Meaning + /// ================================================================= + /// ^ | Match start of string. + /// _+ | Match one or more leading underscores. + static final _leadingUnderscoresPattern = RegExp('^_+'); + + static const _allowedTokens = {'x', 'y', 'z', 'w', 'i', 'j', 'k'}; + + /// Splits a camelCase or snake_case identifier + /// into lowercase tokens. + /// + /// E.g., `someClass1` returns `['some', 'class', '1']`. + static List tokenize(String name) => [ + for (final match in _tokenPattern.allMatches(name)) + if (match.group(0) case final group?) group.toLowerCase(), + ]; + + /// Strips leading underscores from a name. + /// + /// E.g., `_someName` returns `someName`. + static String cleanName(String name) => + name.replaceFirst(_leadingUnderscoresPattern, ''); + + /// Returns `true` if the string consists only + /// of digit characters. + static bool isDigit(String s) => int.tryParse(s) != null; + + /// Returns `true` if the token is a common + /// loop variable or coordinate name. + static bool isAllowedToken(String s) => _allowedTokens.contains(s); + + /// Returns `true` if the token is considered non-descriptive + /// (either a digit or a disallowed single letter). + static bool isNonDescriptiveToken(String s) => + isDigit(s) || (s.length == 1 && !isAllowedToken(s)); + + /// Returns `true` if [longer] is a superset of + /// [shorter] with exactly one extra non-descriptive token. + static bool isSubsetWithNonDescriptiveToken( + List longer, + List shorter, + ) { + if (longer.length != shorter.length + 1) return false; + var i = 0; + while (i < shorter.length && longer[i] == shorter[i]) { + i++; + } + if (!isNonDescriptiveToken(longer[i])) return false; + while (i < shorter.length && longer[i + 1] == shorter[i]) { + i++; + } + return i == shorter.length; + } +} diff --git a/lib/src/lints/avoid_similar_names/visitors/avoid_similar_names_visitor.dart b/lib/src/lints/avoid_similar_names/visitors/avoid_similar_names_visitor.dart new file mode 100644 index 00000000..9a4726ed --- /dev/null +++ b/lib/src/lints/avoid_similar_names/visitors/avoid_similar_names_visitor.dart @@ -0,0 +1,147 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:solid_lints/src/lints/avoid_similar_names/avoid_similar_names_rule.dart'; +import 'package:solid_lints/src/lints/avoid_similar_names/models/scope_variable.dart'; +import 'package:solid_lints/src/lints/avoid_similar_names/utils/name_tokenizer.dart'; +import 'package:solid_lints/src/lints/avoid_similar_names/visitors/local_variables_visitor.dart'; +import 'package:solid_lints/src/utils/types_utils.dart'; + +/// A visitor that checks for variables with +/// confusingly similar names. +class AvoidSimilarNamesVisitor extends SimpleAstVisitor { + final AvoidSimilarNamesRule _rule; + final _reportedNodes = {}; + final _collector = LocalVariablesVisitor(); + + /// Creates a new instance of + /// [AvoidSimilarNamesVisitor]. + AvoidSimilarNamesVisitor(this._rule); + + @override + void visitMethodDeclaration( + MethodDeclaration node, + ) { + _checkScope(node.parameters, node.body); + } + + @override + void visitConstructorDeclaration( + ConstructorDeclaration node, + ) { + _checkScope(node.parameters, node.body); + } + + @override + void visitFunctionDeclaration( + FunctionDeclaration node, + ) { + _checkScope( + node.functionExpression.parameters, + node.functionExpression.body, + ); + } + + void _checkScope( + FormalParameterList? parameters, + FunctionBody body, + ) { + _reportedNodes.clear(); + _collector.variables.clear(); + body.accept(_collector); + + final variables = [ + ..._extractParameters(parameters), + ..._collector.variables, + ]; + + _compareVariables(variables); + _reportedNodes.clear(); + } + + Iterable _extractParameters( + FormalParameterList? parameters, + ) => [ + for (final parameter in parameters?.parameters ?? const []) + if (parameter.name case final nameToken?) + if (ScopeVariable.createOrNull( + nameToken: nameToken, + type: parameter.declaredFragment?.element.type, + node: parameter, + ) + case final variable?) + variable, + ]; + + void _compareVariables( + List variables, + ) { + for (var i = 0; i < variables.length; i++) { + for (var j = i + 1; j < variables.length; j++) { + _comparePair(variables[i], variables[j]); + } + } + } + + void _comparePair( + ScopeVariable a, + ScopeVariable b, + ) { + if (a.type?.isDifferentIgnoringNullability(b.type) ?? false) { + return; + } + + switch ((a.tokens.length - b.tokens.length).abs()) { + case 0: + _checkSameLengthTokens(a, b); + case 1: + _checkDifferentLengthTokens(a, b); + } + } + + void _checkSameLengthTokens( + ScopeVariable a, + ScopeVariable b, + ) { + var diffCount = 0; + var diffIndex = -1; + for (var k = 0; k < a.tokens.length; k++) { + if (a.tokens[k] != b.tokens[k]) { + diffCount++; + if (diffCount > 1) return; + diffIndex = k; + } + } + + if (diffCount != 1) return; + + if (NameTokenizer.isNonDescriptiveToken(a.tokens[diffIndex]) && + NameTokenizer.isNonDescriptiveToken(b.tokens[diffIndex])) { + _report(a, b); + } + } + + void _checkDifferentLengthTokens( + ScopeVariable a, + ScopeVariable b, + ) { + final (longer, shorter) = a.tokens.length > b.tokens.length + ? (a.tokens, b.tokens) + : (b.tokens, a.tokens); + + if (NameTokenizer.isSubsetWithNonDescriptiveToken( + longer, + shorter, + )) { + _report(a, b); + } + } + + void _report(ScopeVariable a, ScopeVariable b) { + if (_reportedNodes.add(a.node)) { + _rule.reportAtToken(a.nameToken); + } + if (_reportedNodes.add(b.node)) { + _rule.reportAtToken(b.nameToken); + } + } +} diff --git a/lib/src/lints/avoid_similar_names/visitors/local_variables_visitor.dart b/lib/src/lints/avoid_similar_names/visitors/local_variables_visitor.dart new file mode 100644 index 00000000..9fe50538 --- /dev/null +++ b/lib/src/lints/avoid_similar_names/visitors/local_variables_visitor.dart @@ -0,0 +1,51 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/token.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:solid_lints/src/lints/avoid_similar_names/models/scope_variable.dart'; + +/// Collects local variable declarations within a function body, +/// stopping at nested function boundaries. +class LocalVariablesVisitor extends RecursiveAstVisitor { + /// The collected variables. + final List variables = []; + + @override + void visitVariableDeclaration(VariableDeclaration node) { + _collect(node.name, node.declaredFragment?.element.type, node); + super.visitVariableDeclaration(node); + } + + @override + void visitDeclaredIdentifier(DeclaredIdentifier node) { + _collect(node.name, node.declaredFragment?.element.type, node); + super.visitDeclaredIdentifier(node); + } + + @override + void visitDeclaredVariablePattern(DeclaredVariablePattern node) { + _collect(node.name, node.declaredFragment?.element.type, node); + super.visitDeclaredVariablePattern(node); + } + + @override + void visitFunctionDeclaration(FunctionDeclaration node) { + // Stop traversing nested function scopes. + } + + @override + void visitFunctionExpression(FunctionExpression node) { + // Stop traversing nested closures. + } + + void _collect(Token nameToken, DartType? type, AstNode node) { + final variable = ScopeVariable.createOrNull( + nameToken: nameToken, + type: type, + node: node, + ); + if (variable != null) { + variables.add(variable); + } + } +} diff --git a/lib/src/utils/types_utils.dart b/lib/src/utils/types_utils.dart index 64558270..6d60e748 100644 --- a/lib/src/utils/types_utils.dart +++ b/lib/src/utils/types_utils.dart @@ -75,6 +75,13 @@ extension Subtypes on DartType { return false; } + + /// Compares this type with [other] ignoring nullability where applicable. + bool isDifferentIgnoringNullability(DartType? other) { + if (other == null) return false; + + return (element ?? this) != (other.element ?? other); + } } extension TypeString on String { diff --git a/test/src/lints/avoid_similar_names/avoid_similar_names_rule_test.dart b/test/src/lints/avoid_similar_names/avoid_similar_names_rule_test.dart new file mode 100644 index 00000000..8b2cd274 --- /dev/null +++ b/test/src/lints/avoid_similar_names/avoid_similar_names_rule_test.dart @@ -0,0 +1,221 @@ +import 'package:analyzer_testing/analysis_rule/analysis_rule.dart'; +import 'package:analyzer_testing/utilities/utilities.dart'; +import 'package:solid_lints/src/lints/avoid_similar_names/avoid_similar_names_rule.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +import '../../../lints/auto_test_lint_offsets.dart'; + +void main() { + defineReflectiveSuite(() { + defineReflectiveTests(AvoidSimilarNamesRuleTest); + }); +} + +@reflectiveTest +class AvoidSimilarNamesRuleTest extends AnalysisRuleTest + with AutoTestLintOffsets { + @override + void setUp() { + rule = AvoidSimilarNamesRule(); + super.setUp(); + + newAnalysisOptionsYamlFile( + testPackageRootPath, + analysisOptionsContent(rules: [rule.name]), + ); + } + + Future test_reports_on_similar_names_in_function() async { + await assertAutoDiagnostics(''' +void test() { + int ${expectLint('someClass1')} = 1; + int ${expectLint('someClass2')} = 2; +} +'''); + } + + Future test_does_not_report_on_different_types() async { + await assertNoDiagnostics(''' +void test() { + String user1 = 'Alice'; + int user2 = 123; +} +'''); + } + + Future + test_reports_on_similar_names_with_different_nullability() async { + await assertAutoDiagnostics(''' +void test() { + int ${expectLint('user1')} = 1; + int? ${expectLint('user2')} = 2; +} +'''); + } + + Future test_reports_on_borderline_short_names() async { + await assertAutoDiagnostics(''' +void test() { + int ${expectLint('id1')} = 1; + int ${expectLint('id2')} = 2; +} +'''); + } + + Future test_does_not_report_on_different_descriptive_tokens() async { + await assertNoDiagnostics(''' +void test() { + int minHeight = 10; + int maxHeight = 20; +} +'''); + } + + Future test_does_not_report_on_short_names() async { + await assertNoDiagnostics(''' +void test() { + int x1 = 1; + int x2 = 2; + int dx = 5; + int dy = 10; +} +'''); + } + + Future test_does_not_report_on_allowed_coordinates() async { + await assertNoDiagnostics(''' +void test() { + double pointX = 1.0; + double pointY = 2.0; +} +'''); + } + + Future test_reports_on_digits_in_parameters() async { + await assertAutoDiagnostics(''' +bool isEqual( + int ${expectLint('someClass1')}, + int ${expectLint('someClass2')}, +) { + return someClass1 == someClass2; +} +'''); + } + + Future test_reports_on_similar_names_in_method() async { + await assertAutoDiagnostics(''' +class A { + void test() { + String ${expectLint('tempA')} = "a"; + String ${expectLint('tempB')} = "b"; + } +} +'''); + } + + Future test_reports_on_subset_with_extra_digit() async { + await assertAutoDiagnostics(''' +void test() { + String ${expectLint('data')} = "a"; + String ${expectLint('data1')} = "b"; +} +'''); + } + + Future test_reports_on_subset_with_multi_digit_number() async { + await assertAutoDiagnostics(''' +void test() { + String ${expectLint('data')} = "a"; + String ${expectLint('data10')} = "b"; +} +'''); + } + + Future test_reports_on_mixed_non_descriptive_suffixes() async { + await assertAutoDiagnostics(''' +void test() { + String ${expectLint('user1')} = "a"; + String ${expectLint('userA')} = "b"; +} +'''); + } + + Future test_reports_on_subset_with_extra_letter() async { + await assertAutoDiagnostics(''' +void test() { + String ${expectLint('user')} = "a"; + String ${expectLint('userA')} = "b"; +} +'''); + } + + Future test_does_not_report_on_subset_with_descriptive_token() async { + await assertNoDiagnostics(''' +void test() { + String user = "a"; + String userProfile = "b"; + String data = "c"; + String dataFetch = "d"; +} +'''); + } + + Future test_reports_on_three_similar_names() async { + await assertAutoDiagnostics(''' +void test() { + int ${expectLint('id1')} = 1; + int ${expectLint('id2')} = 2; + int ${expectLint('id3')} = 3; +} +'''); + } + + Future test_does_not_report_on_anonymous_lambda() async { + await assertNoDiagnostics(''' +void test() { + void process(int Function(int, int) callback) {} + process((day1, day2) => day1 + day2); +} +'''); + } + + Future test_reports_on_acronym_and_camel_case_suffix() async { + await assertAutoDiagnostics(''' +void test() { + int ${expectLint('APIRequest')} = 1; + int ${expectLint('apiRequest1')} = 2; +} +'''); + } + + Future test_reports_on_similar_names_in_for_in_loops() async { + await assertAutoDiagnostics(''' +void test() { + final users = [1, 2]; + for (final ${expectLint('user1')} in users) { + int ${expectLint('user2')} = user1; + } +} +'''); + } + + Future + test_reports_on_similar_names_in_pattern_variable_declarations() async { + await assertAutoDiagnostics(''' +void test() { + final (${expectLint('user1')}, ${expectLint('user2')}) = (1, 2); +} +'''); + } + + Future + test_reports_on_similar_names_in_pattern_matching_if_case() async { + await assertAutoDiagnostics(''' +void test(Object obj) { + if (obj case [int ${expectLint('user1')}, int ${expectLint('user2')}]) { + // ... + } +} +'''); + } +} diff --git a/test/src/lints/avoid_similar_names/utils/name_tokenizer_test.dart b/test/src/lints/avoid_similar_names/utils/name_tokenizer_test.dart new file mode 100644 index 00000000..55244946 --- /dev/null +++ b/test/src/lints/avoid_similar_names/utils/name_tokenizer_test.dart @@ -0,0 +1,31 @@ +import 'package:solid_lints/src/lints/avoid_similar_names/utils/name_tokenizer.dart'; +import 'package:test/test.dart'; + +void main() { + group('NameTokenizer', () { + test('isSubsetWithNonDescriptiveToken guard clause', () { + expect( + NameTokenizer.isSubsetWithNonDescriptiveToken( + ['a', 'descriptive'], + ['a', 'descriptive'], + ), + isFalse, + ); + expect( + NameTokenizer.isSubsetWithNonDescriptiveToken( + ['a', 'descriptive'], + ['a'], + ), + isFalse, + ); + expect( + NameTokenizer.isSubsetWithNonDescriptiveToken(['a', '1'], ['a']), + isTrue, + ); + expect( + NameTokenizer.isSubsetWithNonDescriptiveToken(['a'], ['a', 'b']), + isFalse, + ); + }); + }); +}