diff --git a/lib/main.dart b/lib/main.dart index ed4503b5..f68bdf9d 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -9,6 +9,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'; @@ -62,6 +63,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..58140dab --- /dev/null +++ b/lib/src/lints/avoid_similar_names/avoid_similar_names_rule.dart @@ -0,0 +1,67 @@ +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) + ..addConstructorDeclaration(this, visitor) + ..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..e753d87f --- /dev/null +++ b/lib/src/lints/avoid_similar_names/visitors/avoid_similar_names_visitor.dart @@ -0,0 +1,109 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:collection/collection.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/iterable_utils.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); + + _compareVariables([ + ..._extractParameters(parameters), + ..._collector.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) { + final lengthDiff = (a.tokens.length - b.tokens.length).abs(); + final shouldReport = + a.type?.isDifferentIgnoringNullability(b.type) != true && + ((lengthDiff == 0 && _checkSameLengthTokens(a, b)) || + (lengthDiff == 1 && _checkSingleTokenLengthDifference(a, b))); + + if (shouldReport) { + _report(a, b); + } + } + + bool _checkSameLengthTokens( + ScopeVariable a, + ScopeVariable b, + ) { + final diff = a.tokens + .zipWithIndexed(b.tokens) + .where((e) => e.$2 != e.$3) + .singleOrNull + ?.$1; + + return diff != null && + NameTokenizer.isNonDescriptiveToken(a.tokens[diff]) && + NameTokenizer.isNonDescriptiveToken(b.tokens[diff]); + } + + bool _checkSingleTokenLengthDifference(ScopeVariable a, ScopeVariable b) { + final [shorter, longer] = [a.tokens, b.tokens].sortedBy((e) => e.length); + + return NameTokenizer.isSubsetWithNonDescriptiveToken(longer, shorter); + } + + void _report(ScopeVariable a, ScopeVariable b) => [a, b] + .where((e) => _reportedNodes.add(e.node)) + .forEach((e) => _rule.reportAtToken(e.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/iterable_utils.dart b/lib/src/utils/iterable_utils.dart index f7037447..3dcf538b 100644 --- a/lib/src/utils/iterable_utils.dart +++ b/lib/src/utils/iterable_utils.dart @@ -25,3 +25,20 @@ extension IterablePairwise on Iterable { } } } + +/// Extension on [Iterable] to zip elements with another iterable. +extension IterableZip on Iterable { + /// Zips this iterable with [other]. + Iterable<(T, U)> zipWith(Iterable other) sync* { + for (var i = 0; i < length && i < other.length; i++) { + yield (elementAt(i), other.elementAt(i)); + } + } + + /// Zips this iterable with [other] and includes the index. + Iterable<(int, T, U)> zipWithIndexed(Iterable other) sync* { + for (var i = 0; i < length && i < other.length; i++) { + yield (i, elementAt(i), other.elementAt(i)); + } + } +} diff --git a/lib/src/utils/types_utils.dart b/lib/src/utils/types_utils.dart index 64558270..d3ebac5e 100644 --- a/lib/src/utils/types_utils.dart +++ b/lib/src/utils/types_utils.dart @@ -75,6 +75,10 @@ extension Subtypes on DartType { return false; } + + /// Compares this type with [other] ignoring nullability where applicable. + bool isDifferentIgnoringNullability(DartType? other) => + other != null && (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..2082a301 --- /dev/null +++ b/test/src/lints/avoid_similar_names/avoid_similar_names_rule_test.dart @@ -0,0 +1,158 @@ +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 '../../utils/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() => _assert(''' + int ${expectLint('someClass1')} = 1; + int ${expectLint('someClass2')} = 2; +'''); + + Future test_does_not_report_on_different_types() => _assertNo(''' + String user1 = 'Alice'; + int user2 = 123; +'''); + + Future test_reports_on_similar_names_with_different_nullability() => + _assert(''' + int ${expectLint('user1')} = 1; + int? ${expectLint('user2')} = 2; +'''); + + Future test_reports_on_borderline_short_names() => _assert(''' + int ${expectLint('id1')} = 1; + int ${expectLint('id2')} = 2; +'''); + + Future test_does_not_report_on_different_descriptive_tokens() => + _assertNo(''' + int minHeight = 10; + int maxHeight = 20; +'''); + + Future test_does_not_report_on_short_names() => _assertNo(''' + int x1 = 1; + int x2 = 2; + int dx = 5; + int dy = 10; +'''); + + Future test_does_not_report_on_allowed_coordinates() => _assertNo(''' + double pointX = 1.0; + double pointY = 2.0; +'''); + + Future test_reports_on_digits_in_parameters() => + assertAutoDiagnostics(''' +bool isEqual( + int ${expectLint('someClass1')}, + int ${expectLint('someClass2')}, +) { + return someClass1 == someClass2; +} +'''); + + Future test_reports_on_similar_names_in_method() => + assertAutoDiagnostics(''' +class A { + void test() { + String ${expectLint('tempA')} = "a"; + String ${expectLint('tempB')} = "b"; + } +} +'''); + + Future test_reports_on_subset_with_extra_digit() => _assert(''' + String ${expectLint('data')} = "a"; + String ${expectLint('data1')} = "b"; +'''); + + Future test_reports_on_subset_with_multi_digit_number() => _assert(''' + String ${expectLint('data')} = "a"; + String ${expectLint('data10')} = "b"; +'''); + + Future test_reports_on_mixed_non_descriptive_suffixes() => _assert(''' + String ${expectLint('user1')} = "a"; + String ${expectLint('userA')} = "b"; +'''); + + Future test_reports_on_subset_with_extra_letter() => _assert(''' + String ${expectLint('user')} = "a"; + String ${expectLint('userA')} = "b"; +'''); + + Future test_does_not_report_on_subset_with_descriptive_token() => + _assertNo(''' + String user = "a"; + String userProfile = "b"; + String data = "c"; + String dataFetch = "d"; +'''); + + Future test_reports_on_three_similar_names() => _assert(''' + int ${expectLint('id1')} = 1; + int ${expectLint('id2')} = 2; + int ${expectLint('id3')} = 3; +'''); + + Future test_does_not_report_on_anonymous_lambda() => _assertNo(''' + void process(int Function(int, int) callback) {} + process((day1, day2) => day1 + day2); +'''); + + Future test_reports_on_acronym_and_camel_case_suffix() => _assert(''' + int ${expectLint('APIRequest')} = 1; + int ${expectLint('apiRequest1')} = 2; +'''); + + Future test_reports_on_similar_names_in_for_in_loops() => _assert(''' + final users = [1, 2]; + for (final ${expectLint('user1')} in users) { + int ${expectLint('user2')} = user1; + } +'''); + + Future + test_reports_on_similar_names_in_pattern_variable_declarations() => + _assert(''' + final (${expectLint('user1')}, ${expectLint('user2')}) = (1, 2); +'''); + + Future test_reports_on_similar_names_in_pattern_matching_if_case() => + assertAutoDiagnostics(''' +void test(Object obj) { + if (obj case [int ${expectLint('user1')}, int ${expectLint('user2')}]) { + // ... + } +} +'''); + + Future _assert(String body) => + assertAutoDiagnostics('''void test() {$body}'''); + + Future _assertNo(String body) => + assertNoDiagnostics('''void test() {$body}'''); +} 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, + ); + }); + }); +}