diff --git a/lib/analysis_options.yaml b/lib/analysis_options.yaml index b9203a45..056e1227 100644 --- a/lib/analysis_options.yaml +++ b/lib/analysis_options.yaml @@ -38,7 +38,6 @@ solid_lints: avoid_global_state: true avoid_duplicate_code: min_tokens: 30 - check_blocks: true exclude: - method_name: initState - method_name: dispose diff --git a/lib/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule.dart b/lib/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule.dart index 28f48542..70515334 100644 --- a/lib/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule.dart +++ b/lib/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule.dart @@ -3,7 +3,7 @@ import 'package:analyzer/analysis_rule/rule_visitor_registry.dart'; import 'package:analyzer/error/error.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/models/avoid_duplicate_code_parameters.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/visitors/avoid_duplicate_code_visitor.dart'; -import 'package:solid_lints/src/models/solid_lint_rule.dart'; +import 'package:solid_lints/src/models/solid_multi_lint_rule.dart'; import 'package:solid_lints/src/utils/ignore_matcher.dart'; /// A lint rule that detects duplicated code blocks (clones) across the project. @@ -18,13 +18,13 @@ import 'package:solid_lints/src/utils/ignore_matcher.dart'; /// The rule is built upon the fundamental code clone classification by /// **Roy & Cordy (2007)** (*"A Survey on Software Clone Detection Research"*): /// -/// :::note Type 2 Clones -/// The rule focuses on **Type 2 clones** (syntactic clones): structurally -/// identical AST subtrees where names of local variables, formal parameters, -/// or literal values may differ (if configured via `ignore_identifiers` or -/// `ignore_literals`). While plain text diff tools only catch exact -/// copies (Type 1), this rule operates on the AST level to detect copy-pasted -/// logic even after variable renaming or code formatting changes. +/// :::note Type 2 & Type 3 Clones +/// The rule focuses on **Type 2 clones** (syntactic clones with renamed +/// variables) and **Type 3 clones with differing literals** (structurally +/// identical AST subtrees where literal values differ). While plain text diff +/// tools only catch exact copies (Type 1), this rule operates on the AST level +/// to detect copy-pasted logic even after variable renaming, code formatting +/// changes, or literal constant tweaks. /// ::: /// :::info Sequential Variable Indexing /// Local variable and parameter names in the AST subtree are replaced with @@ -34,9 +34,13 @@ import 'package:solid_lints/src/utils/ignore_matcher.dart'; /// renamed (e.g., `x` to `item`). /// ::: /// -/// :::info Structural Hashing -/// Builds an AST subtree fingerprint using **Bob Jenkins' One-at-a-time** -/// **hash** algorithm (structural hashing). +/// :::info Dual Structural & Exact Hashing +/// Computes both a **structural hash** (ignoring literal values) and an +/// **exact hash** (including literal values) in a single pass using **Bob +/// Jenkins' One-at-a-time hash** algorithm. When duplicate candidates have +/// identical structural hashes but differing exact hashes, the rule provides +/// detailed context messages showing which literal slots differ (e.g., `[1, 2]` +/// or `['hello', 'world']`). /// ::: /// /// :::info Nested Clone Suppression @@ -104,15 +108,12 @@ import 'package:solid_lints/src/utils/ignore_matcher.dart'; /// diagnostics: /// avoid_duplicate_code: /// min_tokens: 30 -/// ignore_literals: false -/// ignore_identifiers: true -/// check_blocks: true /// exclude: /// - method_name: initState /// - method_name: dispose /// ``` class AvoidDuplicateCodeRule - extends SolidLintRule { + extends SolidMultiLintRule { /// Name of the lint. static const lintName = 'avoid_duplicate_code'; @@ -122,13 +123,30 @@ class AvoidDuplicateCodeRule 'Consider extracting the shared logic into a common function.', ); + static const _differentLiteralsCode = LintCode( + lintName, + 'This code has identical structure but differs in literal values{0}.\n' + 'Extracting it directly will alter behavior — consider extracting a ' + 'shared function with parameters for the differing values.', + uniqueName: 'avoid_duplicate_code_different_literals', + ); + + /// Diagnostic code for exact duplicates. + DiagnosticCode get exactCode => _code; + + /// Diagnostic code for structural duplicates with differing literal values. + DiagnosticCode get differentLiteralsCode => _differentLiteralsCode; + @override - DiagnosticCode get diagnosticCode => _code; + List get diagnosticCodes => [ + _code, + _differentLiteralsCode, + ]; /// Creates a new instance of [AvoidDuplicateCodeRule]. AvoidDuplicateCodeRule({ required super.analysisOptionsLoader, - }) : super.withParameters( + }) : super( name: lintName, description: 'Detects structurally identical function/method bodies ' diff --git a/lib/src/lints/avoid_duplicate_code/models/analyzed_candidate.dart b/lib/src/lints/avoid_duplicate_code/models/analyzed_candidate.dart new file mode 100644 index 00000000..c76ba63e --- /dev/null +++ b/lib/src/lints/avoid_duplicate_code/models/analyzed_candidate.dart @@ -0,0 +1,8 @@ +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/body_candidate.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/hash_entry.dart'; + +/// A record pairing a [BodyCandidate] with its computed [HashEntry]. +typedef AnalyzedCandidate = ({ + BodyCandidate candidate, + HashEntry entry, +}); diff --git a/lib/src/lints/avoid_duplicate_code/models/avoid_duplicate_code_parameters.dart b/lib/src/lints/avoid_duplicate_code/models/avoid_duplicate_code_parameters.dart index 963dccc0..f804bbe9 100644 --- a/lib/src/lints/avoid_duplicate_code/models/avoid_duplicate_code_parameters.dart +++ b/lib/src/lints/avoid_duplicate_code/models/avoid_duplicate_code_parameters.dart @@ -39,90 +39,6 @@ class AvoidDuplicateCodeParameters { /// ``` final int minTokens; - /// When `true`, literal values (strings, numbers, booleans) are excluded - /// from the structural hash, ignoring literal differences during duplicate - /// search. - /// - /// ##### Example: - /// ```dart - /// // Function A - /// double calculateTax(double amount) { - /// final tax = amount * 0.20; - /// return amount + tax; - /// } - /// - /// // Function B (differs only by literal 0.15 vs 0.20) - /// double calculateDiscount(double amount) { - /// final tax = amount * 0.15; - /// return amount + tax; - /// } - /// ``` - /// * **When `ignore_literals: false` (default):** **NOT reported** - /// because numbers `0.20` and `0.15` differ. - /// * **When `ignore_literals: true`:** **Reported as duplicate** - /// because literal values are ignored. - final bool ignoreLiterals; - - /// When `true`, local variable and parameter names are excluded from the - /// structural hash (using Sequential Variable Indexing). This enables - /// detection of renamed variable clones (Type 2). Note that method, class, - /// and field names are NOT ignored to prevent excessive false positives. - /// - /// ##### Example: - /// ```dart - /// // Function A - /// double calcTotal(double price, int count) { - /// final subtotal = price * count; - /// return subtotal > 100 ? subtotal * 0.9 : subtotal; - /// } - /// - /// // Function B (renamed: price->amount, count->qty, subtotal->total) - /// double calcTotal(double amount, int qty) { - /// final total = amount * qty; - /// return total > 100 ? total * 0.9 : total; - /// } - /// ``` - /// * **When `ignore_identifiers: true` (default):** **Reported as** - /// **duplicate** (Type 2 Clone). - /// * **When `ignore_identifiers: false`:** **NOT reported as duplicate** - /// because local names differ. - final bool ignoreIdentifiers; - - /// When `true`, statement blocks (such as `if` blocks or loops) inside - /// functions are also checked for duplication. - /// - /// ##### Example: - /// ```dart - /// // Function A - /// void processUser(User user) { - /// print('Starting user process...'); - /// if (user.isActive) { - /// logger.log('Processing user'); - /// user.lastActive = DateTime.now(); - /// user.status = UserStatus.active; - /// repository.save(user); - /// analytics.track('user_processed', user.id); - /// } - /// } - /// - /// // Function B (different function, same inner if block) - /// void processAdmin(User user) { - /// validateAdmin(user); - /// if (user.isActive) { - /// logger.log('Processing user'); - /// user.lastActive = DateTime.now(); - /// user.status = UserStatus.active; - /// repository.save(user); - /// analytics.track('user_processed', user.id); - /// } - /// } - /// ``` - /// * **When `check_blocks: true` (default):** **Reported as duplicate** - /// for the inner `if` block. - /// * **When `check_blocks: false`:** **NOT reported as duplicate** - /// because nested `{ ... }` block nodes are skipped. - final bool checkBlocks; - /// A list of methods/functions that should be excluded from clone detection. final ExcludedIdentifiersListParameter exclude; @@ -135,18 +51,12 @@ class AvoidDuplicateCodeParameters { /// Constructor for [AvoidDuplicateCodeParameters] model. const AvoidDuplicateCodeParameters({ required this.minTokens, - required this.ignoreLiterals, - required this.ignoreIdentifiers, - required this.checkBlocks, required this.exclude, }); /// Empty [AvoidDuplicateCodeParameters] model with default values. factory AvoidDuplicateCodeParameters.empty() => AvoidDuplicateCodeParameters( minTokens: _defaultMinTokens, - ignoreLiterals: false, - ignoreIdentifiers: true, - checkBlocks: true, exclude: _defaultExclude, ); @@ -154,18 +64,12 @@ class AvoidDuplicateCodeParameters { factory AvoidDuplicateCodeParameters.fromJson(Map json) => AvoidDuplicateCodeParameters( minTokens: json['min_tokens'] as int? ?? _defaultMinTokens, - ignoreLiterals: json['ignore_literals'] as bool? ?? false, - ignoreIdentifiers: json['ignore_identifiers'] as bool? ?? true, - checkBlocks: json['check_blocks'] as bool? ?? true, exclude: ExcludedIdentifiersListParameter.defaultFromJson(json), ); /// Converts the parameters to a JSON-compatible Map. Map toJson() => { 'min_tokens': minTokens, - 'ignore_literals': ignoreLiterals, - 'ignore_identifiers': ignoreIdentifiers, - 'check_blocks': checkBlocks, 'exclude': exclude.exclude.map((e) => e.toJson()).toList(), }; @@ -174,17 +78,11 @@ class AvoidDuplicateCodeParameters { identical(this, other) || other is AvoidDuplicateCodeParameters && other.minTokens == minTokens && - other.ignoreLiterals == ignoreLiterals && - other.ignoreIdentifiers == ignoreIdentifiers && - other.checkBlocks == checkBlocks && other.exclude == exclude; @override int get hashCode => Object.hash( minTokens, - ignoreLiterals, - ignoreIdentifiers, - checkBlocks, exclude, ); } diff --git a/lib/src/lints/avoid_duplicate_code/models/cross_file_match.dart b/lib/src/lints/avoid_duplicate_code/models/cross_file_match.dart index a8889b3c..71eae154 100644 --- a/lib/src/lints/avoid_duplicate_code/models/cross_file_match.dart +++ b/lib/src/lints/avoid_duplicate_code/models/cross_file_match.dart @@ -1,3 +1,4 @@ +import 'package:collection/collection.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/models/duplicate_location.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/models/hash_entry.dart'; @@ -21,7 +22,11 @@ class CrossFileMatch { extension CrossFileMatchIterableExtension on Iterable { /// Converts this iterable of cross-file matches to a map of duplicates /// grouped by hash. - Map> toDuplicatesByHash() => { - for (final match in this) match.currentEntry.hash: match.duplicates, - }; + Map> toDuplicatesByHash() => + groupBy(this, (m) => m.currentEntry.hash).map( + (hash, matches) => MapEntry( + hash, + matches.expand((m) => m.duplicates).toSet().toList(), + ), + ); } diff --git a/lib/src/lints/avoid_duplicate_code/models/hash_entry.dart b/lib/src/lints/avoid_duplicate_code/models/hash_entry.dart index f4fbdb2f..3b81947c 100644 --- a/lib/src/lints/avoid_duplicate_code/models/hash_entry.dart +++ b/lib/src/lints/avoid_duplicate_code/models/hash_entry.dart @@ -4,6 +4,9 @@ class HashEntry { /// The structural hash of the AST subtree. final int hash; + /// The exact hash of the AST subtree (including literal values). + final int exactHash; + /// The line number where this candidate starts. final int lineNumber; @@ -22,15 +25,17 @@ class HashEntry { /// Creates a new [HashEntry]. const HashEntry({ required this.hash, + required this.exactHash, required this.lineNumber, + required this.offset, + required this.length, required this.tokenCount, - this.offset = 0, - this.length = 0, }); /// Converts this [HashEntry] to a JSON-compatible map using shortened keys. Map toJson() => { 'h': hash, + 'e': exactHash, 'n': lineNumber, 'o': offset, 'l': length, @@ -40,8 +45,9 @@ class HashEntry { /// Creates a [HashEntry] from a JSON map. HashEntry.fromJson(Map json) : hash = json['h']! as int, + exactHash = json['e']! as int, lineNumber = json['n']! as int, offset = (json['o'] ?? 0) as int, length = (json['l'] ?? 0) as int, - tokenCount = (json['t'] ?? json['s'] ?? 0) as int; + tokenCount = (json['t'] ?? 0) as int; } diff --git a/lib/src/lints/avoid_duplicate_code/models/literal_info.dart b/lib/src/lints/avoid_duplicate_code/models/literal_info.dart new file mode 100644 index 00000000..4dcaa477 --- /dev/null +++ b/lib/src/lints/avoid_duplicate_code/models/literal_info.dart @@ -0,0 +1,18 @@ +/// Represents information about a literal found within a code block. +class LiteralInfo { + /// The string representation of the literal value. + final String text; + + /// The character offset where the literal begins. + final int offset; + + /// The character length of the literal. + final int length; + + /// Creates a new [LiteralInfo]. + const LiteralInfo({ + required this.text, + required this.offset, + required this.length, + }); +} diff --git a/lib/src/lints/avoid_duplicate_code/reporters/avoid_duplicate_code_reporter.dart b/lib/src/lints/avoid_duplicate_code/reporters/avoid_duplicate_code_reporter.dart new file mode 100644 index 00000000..8c4f483d --- /dev/null +++ b/lib/src/lints/avoid_duplicate_code/reporters/avoid_duplicate_code_reporter.dart @@ -0,0 +1,177 @@ +import 'package:analyzer/diagnostic/diagnostic.dart'; +import 'package:collection/collection.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/duplicate_location.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/hash_entry.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/literal_info.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/reporters/duplicate_report_context.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/services/differing_literals_analyzer.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/utils/range_extension.dart'; +import 'package:solid_lints/src/models/solid_diagnostic_message.dart'; + +/// Handles diagnostic reporting and context message formatting for duplicate +/// code candidates. +class AvoidDuplicateCodeReporter { + static const _exactDuplicateContextMessage = 'Exact duplicate'; + static const _differentLiteralsDuplicateContextMessage = + 'Duplicate with different literals'; + + final AvoidDuplicateCodeRule _rule; + final DifferingLiteralsAnalyzer _literalsAnalyzer; + + /// Creates a new [AvoidDuplicateCodeReporter]. + const AvoidDuplicateCodeReporter({ + required AvoidDuplicateCodeRule rule, + required DifferingLiteralsAnalyzer literalsAnalyzer, + }) : _rule = rule, + _literalsAnalyzer = literalsAnalyzer; + + /// Reports duplicate lint diagnostics for duplicate contexts of a file. + void report({ + required String filePath, + required List contexts, + required Map> crossFileDuplicatesByHash, + }) { + final hashGroups = groupBy(contexts, (c) => c.entry.hash); + final suppressedRanges = <(int, int)>[]; + final reportedOffsets = {}; + + for (final context in contexts) { + final entry = context.entry; + final group = hashGroups[entry.hash]; + if (group == null || + reportedOffsets.contains(entry.offset) || + suppressedRanges.anyContainsStrictly(entry.range)) { + continue; + } + + final internalPartners = group + .where( + (c) => + c.entry.offset != entry.offset && + !suppressedRanges.anyContainsStrictly(c.entry.range), + ) + .toList(); + final externalPartners = + crossFileDuplicatesByHash[entry.hash] ?? const []; + + if (internalPartners.isEmpty && externalPartners.isEmpty) continue; + + _report( + filePath: filePath, + target: context, + internalPartners: internalPartners, + externalPartners: externalPartners, + ); + + reportedOffsets.add(entry.offset); + suppressedRanges.add(entry.range); + } + } + + void _report({ + required String filePath, + required DuplicateReportContext target, + required List internalPartners, + required List externalPartners, + }) { + final currentEntry = target.entry; + final internalEntries = internalPartners.map((p) => p.entry).toList(); + + final hasExactPartner = _hasExactPartner( + currentExactHash: currentEntry.exactHash, + internalPartners: internalEntries, + externalPartners: externalPartners, + ); + + final diagnosticCode = hasExactPartner + ? _rule.exactCode + : _rule.differentLiteralsCode; + final isDifferentLiterals = diagnosticCode == _rule.differentLiteralsCode; + + final arguments = isDifferentLiterals + ? [ + _computeLiteralsSummary( + currentLiterals: target.collectLiterals(_literalsAnalyzer), + partnerLiterals: [ + ...internalPartners.map( + (p) => p.collectLiterals(_literalsAnalyzer), + ), + ..._loadExternalLiterals(externalPartners), + ], + ), + ] + : const []; + + final contextMessages = _buildContextMessages( + currentFilePath: filePath, + currentExactHash: currentEntry.exactHash, + externalPartners: externalPartners, + internalPartners: internalEntries, + ); + + target.report( + _rule, + diagnosticCode: diagnosticCode, + arguments: arguments, + contextMessages: contextMessages, + ); + } + + bool _hasExactPartner({ + required int currentExactHash, + required Iterable internalPartners, + required List externalPartners, + }) => + internalPartners.any((p) => p.exactHash == currentExactHash) || + externalPartners.any((p) => p.entry.exactHash == currentExactHash); + + Iterable> _loadExternalLiterals( + List locations, + ) => locations + .map(_literalsAnalyzer.loadExternalLiterals) + .nonNulls + .where((lits) => lits.isNotEmpty); + + String _computeLiteralsSummary({ + required List currentLiterals, + required List> partnerLiterals, + }) { + if (currentLiterals.isEmpty) return ''; + + return _literalsAnalyzer.computeLiteralsSummary( + currentLiterals: currentLiterals, + partnerLiteralsList: partnerLiterals, + ); + } + + List _buildContextMessages({ + required String currentFilePath, + required int currentExactHash, + required List externalPartners, + required Iterable internalPartners, + }) { + String messageFor(int exactHash) => exactHash == currentExactHash + ? _exactDuplicateContextMessage + : _differentLiteralsDuplicateContextMessage; + + return [ + ...externalPartners.map( + (location) => SolidDiagnosticMessage( + filePath: location.filePath, + offset: location.entry.offset, + length: location.entry.length, + message: messageFor(location.entry.exactHash), + ), + ), + ...internalPartners.map( + (entry) => SolidDiagnosticMessage( + filePath: currentFilePath, + offset: entry.offset, + length: entry.length, + message: messageFor(entry.exactHash), + ), + ), + ]; + } +} diff --git a/lib/src/lints/avoid_duplicate_code/reporters/duplicate_report_context.dart b/lib/src/lints/avoid_duplicate_code/reporters/duplicate_report_context.dart new file mode 100644 index 00000000..556d40ac --- /dev/null +++ b/lib/src/lints/avoid_duplicate_code/reporters/duplicate_report_context.dart @@ -0,0 +1,76 @@ +import 'package:analyzer/diagnostic/diagnostic.dart'; +import 'package:analyzer/error/error.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/analyzed_candidate.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/duplicate_location.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/hash_entry.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/literal_info.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/services/differing_literals_analyzer.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/visitors/literal_collector_visitor.dart'; + +/// Encapsulates reporting and literal extraction for duplicate code candidates. +class DuplicateReportContext { + /// The [HashEntry] associated with this duplicate candidate. + final HashEntry entry; + + final List Function(DifferingLiteralsAnalyzer) _literalsProvider; + + DuplicateReportContext._({ + required this.entry, + required List Function(DifferingLiteralsAnalyzer) + literalsProvider, + }) : _literalsProvider = literalsProvider; + + /// Creates a list of contexts from analyzed AST [candidates]. + static List fromAstCandidates( + List candidates, + ) => candidates + .map( + (candidate) => DuplicateReportContext._( + entry: candidate.entry, + literalsProvider: (_) => LiteralCollectorVisitor.collect( + candidate.candidate.node, + ), + ), + ) + .toList(); + + /// Creates a list of contexts from [cachedEntries]. + static List fromCachedEntries( + List cachedEntries, { + required String filePath, + }) => cachedEntries + .map( + (entry) => DuplicateReportContext._( + entry: entry, + literalsProvider: (literalsAnalyzer) => + literalsAnalyzer.loadExternalLiterals( + DuplicateLocation(filePath: filePath, entry: entry), + ) ?? + const [], + ), + ) + .toList(); + + /// Collects literal values present in this duplicate candidate using the + /// provided [literalsAnalyzer]. + List collectLiterals( + DifferingLiteralsAnalyzer literalsAnalyzer, + ) => _literalsProvider(literalsAnalyzer); + + /// Reports a diagnostic lint on this candidate using the provided [rule]. + void report( + AvoidDuplicateCodeRule rule, { + required DiagnosticCode diagnosticCode, + required List arguments, + required List contextMessages, + }) { + rule.reportAtOffset( + entry.offset, + entry.length, + diagnosticCode: diagnosticCode, + arguments: arguments, + contextMessages: contextMessages, + ); + } +} diff --git a/lib/src/lints/avoid_duplicate_code/services/differing_literals_analyzer.dart b/lib/src/lints/avoid_duplicate_code/services/differing_literals_analyzer.dart new file mode 100644 index 00000000..16b6d045 --- /dev/null +++ b/lib/src/lints/avoid_duplicate_code/services/differing_literals_analyzer.dart @@ -0,0 +1,117 @@ +import 'package:analyzer/dart/analysis/utilities.dart'; +import 'package:analyzer/file_system/file_system.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/duplicate_location.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/hash_entry.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/literal_info.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/visitors/literal_collector_visitor.dart'; +import 'package:solid_lints/src/utils/function_utils.dart'; +import 'package:solid_lints/src/utils/resource_provider_utils.dart'; + +/// Analyzes differences in literal values across duplicate code candidates. +class DifferingLiteralsAnalyzer { + static const _maxDisplayedLiterals = 3; + static const _bodyPrefixes = [ + '{', + 'async {', + 'async* {', + 'sync* {', + '=>', + 'async =>', + 'async* =>', + 'sync* =>', + ]; + + final ResourceProvider _resourceProvider; + final _fileContentCache = {}; + + /// Creates a new [DifferingLiteralsAnalyzer]. + DifferingLiteralsAnalyzer({ + required ResourceProvider resourceProvider, + }) : _resourceProvider = resourceProvider; + + /// Loads literals for an external code clone location from the file system. + List? loadExternalLiterals(DuplicateLocation dup) { + final content = _fileContentCache.putIfAbsent( + dup.filePath, + () => _resourceProvider.readFileContent(dup.filePath), + ); + if (content.isEmpty) return null; + + final HashEntry(:offset, :length) = dup.entry; + final end = offset + length; + if (offset < 0 || length <= 0 || end > content.length) return null; + + final snippet = content.substring(offset, end); + final wrapped = _wrapSnippet(snippet); + + return FunctionUtils.tryOrNull(() { + final parsed = parseString(content: wrapped, throwIfDiagnostics: false); + + return LiteralCollectorVisitor.collect(parsed.unit); + }); + } + + String _wrapSnippet(String snippet) { + final trimmed = snippet.trimLeft(); + final isBody = _bodyPrefixes.any(trimmed.startsWith); + + return isBody ? 'void _f() $snippet' : 'void _f() {\n$snippet\n}'; + } + + /// Computes a human-readable summary of differing literal values between + /// the [currentLiterals] and all [partnerLiteralsList]. + String computeLiteralsSummary({ + required List currentLiterals, + required List> partnerLiteralsList, + }) { + if (partnerLiteralsList.isEmpty || currentLiterals.isEmpty) return ''; + + final slotsFormatted = currentLiterals.indexed + .map((entry) => _extractSlotValues(entry, partnerLiteralsList)) + .where((slots) => slots.length > 1) + .map(_formatSlot) + .toList(); + + if (slotsFormatted.isEmpty) return ''; + + final displayed = slotsFormatted.take(_maxDisplayedLiterals).join(', '); + final remaining = slotsFormatted.length - _maxDisplayedLiterals; + final extra = remaining > 0 ? ' (+$remaining more)' : ''; + + return ': $displayed$extra'; + } + + /// Extracts all unique literal text values at the given slot [entry] index + /// across the current clone and all [partnerLiteralsList]. + /// + /// For example, if the current literal is `'foo'` at index `0` and partner + /// clones have `['bar']` and `['foo']` at index `0`, this returns + /// `{'foo', 'bar'}`. + Set _extractSlotValues( + (int, LiteralInfo) entry, + List> partnerLiteralsList, + ) => switch (entry) { + (final index, final literal) => { + literal.text, + ...partnerLiteralsList + .map((pLits) => pLits.elementAtOrNull(index)?.text) + .nonNulls, + }, + }; + + /// Formats literal slot [values] into a bracketed string, truncating with + /// `+N more` when exceeding [_maxDisplayedLiterals]. + /// + /// Examples: + /// - `['a', 'b']` -> `[a, b]` + /// - `['a', 'b', 'c', 'd']` with limit `3` -> `[a, b, c, +1 more]` + String _formatSlot(Set values) { + final remaining = values.length - _maxDisplayedLiterals; + final items = [ + ...values.take(_maxDisplayedLiterals), + if (remaining > 0) '+$remaining more', + ].join(', '); + + return '[$items]'; + } +} diff --git a/lib/src/lints/avoid_duplicate_code/services/global_hash_registry.dart b/lib/src/lints/avoid_duplicate_code/services/global_hash_registry.dart index b77ac50d..2fd0b217 100644 --- a/lib/src/lints/avoid_duplicate_code/services/global_hash_registry.dart +++ b/lib/src/lints/avoid_duplicate_code/services/global_hash_registry.dart @@ -1,7 +1,7 @@ import 'dart:io' as io; import 'package:analyzer/file_system/file_system.dart'; -import 'package:analyzer/file_system/physical_file_system.dart'; +import 'package:collection/collection.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/models/avoid_duplicate_code_parameters.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/models/cross_file_match.dart'; @@ -12,7 +12,6 @@ import 'package:solid_lints/src/lints/avoid_duplicate_code/services/hash_cache_s import 'package:solid_lints/src/lints/avoid_duplicate_code/utils/debouncer.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/utils/hash_entry_list_extension.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/utils/path_utils.dart'; -import 'package:solid_lints/src/lints/avoid_duplicate_code/visitors/avoid_duplicate_code_visitor.dart'; import 'package:solid_lints/src/utils/iterable_utils.dart'; import 'package:solid_lints/src/utils/set_dictionary.dart'; @@ -37,9 +36,6 @@ class GlobalHashRegistry { /// Set to `false` in tests using a virtual resource provider. bool enablePhysicalFileCleanup = true; - /// The resource provider used for file system operations. - ResourceProvider resourceProvider = PhysicalResourceProvider.INSTANCE; - /// Internal index: filePath → FileCacheEntry. final _index = {}; @@ -48,6 +44,7 @@ class GlobalHashRegistry { final _loadedRoots = {}; final _saveDebouncers = {}; + final _packageRootCache = {}; GlobalHashRegistry._(); @@ -68,9 +65,14 @@ class GlobalHashRegistry { ) => _hashToLocations.removeAll(entries.asIndexEntries(absoluteFilePath)); void _ensureLoaded( - String packageRoot, [ + String packageRoot, + ResourceProvider resourceProvider, [ AvoidDuplicateCodeParameters? currentParams, ]) { + if (currentParams == null && _loadedRoots.containsKey(packageRoot)) { + return; + } + final params = currentParams ?? AvoidDuplicateCodeParameters.empty(); if (_loadedRoots[packageRoot] case final previousParams?) { if (previousParams == params) return; @@ -98,7 +100,9 @@ class GlobalHashRegistry { _addToInvertedIndex(k, cached[k]!.entries); } - if (deletedFiles.isNotEmpty) _scheduleSave(packageRoot, params); + if (deletedFiles.isNotEmpty) { + _scheduleSave(packageRoot, resourceProvider); + } } void _clearEntriesForRoot(String packageRoot) { @@ -114,44 +118,80 @@ class GlobalHashRegistry { String _resolveAndLoad( String filePath, String? packageRoot, + ResourceProvider resourceProvider, AvoidDuplicateCodeParameters? parameters, ) { final root = _getRoot(packageRoot); - _ensureLoaded(root, parameters); + _ensureLoaded(root, resourceProvider, parameters); return PathUtils.normalizePath(filePath, root); } - /// Returns the cached modification stamp for [filePath], or `null` if no + /// Returns the cached modification stamp for [filePath], or `null` if not /// indexed. int? getModificationStamp( String filePath, { + required ResourceProvider resourceProvider, AvoidDuplicateCodeParameters? parameters, String? packageRoot, - }) => _getCache(filePath, packageRoot, parameters)?.modificationStamp; + }) => _getCache( + filePath, + packageRoot, + parameters, + resourceProvider, + )?.modificationStamp; /// Returns the cached entries for [filePath], or `null` if not indexed. List? getFileEntries( String filePath, { + required ResourceProvider resourceProvider, AvoidDuplicateCodeParameters? parameters, String? packageRoot, - }) => _getCache(filePath, packageRoot, parameters)?.entries; + }) => _getCache( + filePath, + packageRoot, + parameters, + resourceProvider, + )?.entries; FileCacheEntry? _getCache( String filePath, String? packageRoot, AvoidDuplicateCodeParameters? parameters, - ) => _index[_resolveAndLoad(filePath, packageRoot, parameters)]; + ResourceProvider resourceProvider, + ) => + _index[_resolveAndLoad( + filePath, + packageRoot, + resourceProvider, + parameters, + )]; /// Updates the hash entries for [filePath], replacing any previous entries. void updateFile( String filePath, List entries, { required int modificationStamp, + required ResourceProvider resourceProvider, AvoidDuplicateCodeParameters? parameters, String? packageRoot, }) { + if (entries.isEmpty) { + removeFile( + filePath, + resourceProvider: resourceProvider, + parameters: parameters, + packageRoot: packageRoot, + ); + return; + } + final root = _getRoot(packageRoot); - final absoluteFilePath = _resolveAndLoad(filePath, packageRoot, parameters); + final absoluteFilePath = _resolveAndLoad( + filePath, + packageRoot, + resourceProvider, + parameters, + ); if (_index[absoluteFilePath] case final oldEntry?) { _removeFromInvertedIndex(absoluteFilePath, oldEntry.entries); @@ -162,7 +202,7 @@ class GlobalHashRegistry { entries: entries, ); _addToInvertedIndex(absoluteFilePath, entries); - _scheduleSave(root, parameters); + _scheduleSave(root, resourceProvider); } /// Finds cross-file duplicates for [currentEntries] against all other @@ -172,6 +212,7 @@ class GlobalHashRegistry { List findCrossFileMatches( String currentFilePath, List currentEntries, { + required ResourceProvider resourceProvider, AvoidDuplicateCodeParameters? parameters, bool Function(String filePath)? isFileExcluded, String? packageRoot, @@ -180,6 +221,7 @@ class GlobalHashRegistry { final absoluteCurrentFilePath = _resolveAndLoad( currentFilePath, packageRoot, + resourceProvider, parameters, ); @@ -232,7 +274,7 @@ class GlobalHashRegistry { } } - _scheduleSave(root, parameters); + _scheduleSave(root, resourceProvider); return matches; } @@ -240,39 +282,43 @@ class GlobalHashRegistry { /// Removes [filePath] from the index. void removeFile( String filePath, { + required ResourceProvider resourceProvider, AvoidDuplicateCodeParameters? parameters, String? packageRoot, }) { final root = _getRoot(packageRoot); - final absoluteFilePath = _resolveAndLoad(filePath, packageRoot, parameters); + final absoluteFilePath = _resolveAndLoad( + filePath, + packageRoot, + resourceProvider, + parameters, + ); if (_index.remove(absoluteFilePath) case final oldEntry?) { _removeFromInvertedIndex(absoluteFilePath, oldEntry.entries); } - _scheduleSave(root, parameters); + _scheduleSave(root, resourceProvider); } void _scheduleSave( - String packageRoot, [ - AvoidDuplicateCodeParameters? parameters, - ]) { + String packageRoot, + ResourceProvider resourceProvider, + ) { _saveDebouncers .putIfAbsent(packageRoot, () => Debouncer(_saveDebounceDuration)) .run(() { - _performSave(packageRoot, parameters); + _performSave(packageRoot, resourceProvider); }); } void _performSave( - String packageRoot, [ - AvoidDuplicateCodeParameters? parameters, - ]) => + String packageRoot, + ResourceProvider resourceProvider, + ) => HashCacheStorage( packageRoot: packageRoot, resourceProvider: resourceProvider, currentParams: - parameters ?? - _loadedRoots[packageRoot] ?? - AvoidDuplicateCodeParameters.empty(), + _loadedRoots[packageRoot] ?? AvoidDuplicateCodeParameters.empty(), ).save( // Filter _index for files belonging to this packageRoot _index.entries.whereKey( @@ -280,21 +326,44 @@ class GlobalHashRegistry { ), ); + /// Finds the package root directory containing `pubspec.yaml` for [filePath]. + String? findPackageRoot( + String filePath, { + required ResourceProvider resourceProvider, + }) { + if (filePath.isEmpty) return null; + final dirPath = resourceProvider.pathContext.dirname(filePath); + return _packageRootCache.putIfAbsent( + dirPath, + () => resourceProvider + .getFolder(dirPath) + .withAncestors + .firstWhereOrNull( + (dir) => dir.getChildAssumingFile('pubspec.yaml').exists, + ) + ?.path, + ); + } + /// Clears the entire index and deletes the persistent cache file. /// /// Primarily used in tests to ensure test isolation. - void clear() { + void clear({required ResourceProvider resourceProvider}) { for (final debouncer in _saveDebouncers.values) { debouncer.cancel(); } _saveDebouncers.clear(); + + for (final root in {..._loadedRoots.keys, io.Directory.current.path}) { + HashCacheStorage( + packageRoot: root, + resourceProvider: resourceProvider, + ).delete(); + } + _loadedRoots.clear(); _index.clear(); _hashToLocations.clear(); - HashCacheStorage( - packageRoot: io.Directory.current.path, - resourceProvider: resourceProvider, - ).delete(); - AvoidDuplicateCodeVisitor.clearPackageRootCache(); + _packageRootCache.clear(); } } diff --git a/lib/src/lints/avoid_duplicate_code/visitors/ast_structural_hash_visitor.dart b/lib/src/lints/avoid_duplicate_code/visitors/ast_structural_hash_visitor.dart index 49ea8816..c81a6cfa 100644 --- a/lib/src/lints/avoid_duplicate_code/visitors/ast_structural_hash_visitor.dart +++ b/lib/src/lints/avoid_duplicate_code/visitors/ast_structural_hash_visitor.dart @@ -4,49 +4,72 @@ import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/element/element.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/utils/jenkins_hasher.dart'; -/// A [UnifyingAstVisitor] that builds a structural fingerprint of an AST -/// subtree for clone detection. +/// A [UnifyingAstVisitor] that builds structural and exact fingerprints of an +/// AST subtree for clone detection. /// -/// The fingerprint captures the structure of the code (node types, operators, -/// and optionally literal values) while ignoring identifier names (like local -/// variables), whitespace, and comments. This enables Type 2 clone detection -/// where two code blocks with identical structure but different variable names -/// are considered clones. +/// The structural fingerprint captures the structure of the code (node types, +/// operators, and type annotations) while ignoring identifier names (like +/// local variables), literal values, whitespace, and comments. The exact +/// fingerprint also incorporates literal values. This enables Type 2 and Type 3 +/// clone detection where code blocks with identical structure are identified +/// as clones and compared for differing literals. class AstStructuralHashVisitor extends UnifyingAstVisitor { static final _typeNameCache = {}; static const _pipeAscii = 0x7C; // '|' - final _hasher = JenkinsHasher(); + final _structuralHasher = JenkinsHasher(); + final _exactHasher = JenkinsHasher(); final _localVariableIds = {}; - final bool _ignoreLiterals; - final bool _ignoreIdentifiers; /// Creates a new [AstStructuralHashVisitor]. - AstStructuralHashVisitor({ - required bool ignoreLiterals, - required bool ignoreIdentifiers, - }) : _ignoreLiterals = ignoreLiterals, - _ignoreIdentifiers = ignoreIdentifiers; + AstStructuralHashVisitor(); - /// Computes the structural hash for the given [node]. - /// - /// Visits the entire subtree of [node] and returns an integer hash - /// of the accumulated structural fingerprint. - int computeHash(AstNode node) { - _hasher.reset(); + /// Computes both the structural hash (ignoring literal values) and the exact + /// hash (including literal values) for the given [node]. + ({int structuralHash, int exactHash}) computeHashes(AstNode node) { + _structuralHasher.reset(); + _exactHasher.reset(); _localVariableIds.clear(); node.accept(this); - return _hasher.hash; + return ( + structuralHash: _structuralHasher.hash, + exactHash: _exactHasher.hash, + ); + } + + /// Computes the structural hash for the given [node]. + int computeHash(AstNode node) => computeHashes(node).structuralHash; + + void _append(String value) { + _appendStructural(value); + _appendExact(value); + } + + void _appendHash(int hashCode) { + _appendStructuralHash(hashCode); + _appendExactHash(hashCode); } - void _append(String value) => _hasher + void _appendBool(bool value) => _appendHash(value ? 1 : 0); + + void _appendStructural(String value) => _structuralHasher + ..addString(value) + ..add(_pipeAscii); + + void _appendStructuralHash(int hashCode) => _structuralHasher + ..add(hashCode) + ..add(_pipeAscii); + + void _appendExact(String value) => _exactHasher ..addString(value) ..add(_pipeAscii); - void _appendHash(int hashCode) => _hasher + void _appendExactHash(int hashCode) => _exactHasher ..add(hashCode) ..add(_pipeAscii); + void _appendExactBool(bool value) => _appendExactHash(value ? 1 : 0); + @override void visitNode(AstNode node) { // Use the type name string instead of runtimeType.hashCode, because @@ -70,19 +93,19 @@ class AstStructuralHashVisitor extends UnifyingAstVisitor { @override void visitIfStatement(IfStatement node) { - _appendHash(node.elseKeyword != null ? 1 : 0); + _appendBool(node.elseKeyword != null); super.visitIfStatement(node); } @override void visitTryStatement(TryStatement node) { - _appendHash(node.finallyBlock != null ? 1 : 0); + _appendBool(node.finallyBlock != null); super.visitTryStatement(node); } @override void visitYieldStatement(YieldStatement node) { - _appendHash(node.star != null ? 1 : 0); + _appendBool(node.star != null); super.visitYieldStatement(node); } @@ -95,14 +118,17 @@ class AstStructuralHashVisitor extends UnifyingAstVisitor { @override void visitPrefixExpression(PrefixExpression node) { if (node case PrefixExpression( - operator: Token(type: TokenType.MINUS || TokenType.PLUS), + operator: Token(type: TokenType.MINUS || TokenType.PLUS, :final lexeme), operand: IntegerLiteral() || DoubleLiteral(), - ) when _ignoreLiterals) { + )) { + _appendExact(lexeme); node.operand.accept(this); - } else { - _append(node.operator.lexeme); - super.visitPrefixExpression(node); + + return; } + + _append(node.operator.lexeme); + super.visitPrefixExpression(node); } @override @@ -119,7 +145,7 @@ class AstStructuralHashVisitor extends UnifyingAstVisitor { @override void visitIsExpression(IsExpression node) { - _appendHash(node.notOperator != null ? 1 : 0); + _appendBool(node.notOperator != null); super.visitIsExpression(node); } @@ -133,55 +159,57 @@ class AstStructuralHashVisitor extends UnifyingAstVisitor { @override void visitIntegerLiteral(IntegerLiteral node) { - if (!_ignoreLiterals) { - _append(node.literal.lexeme); - } + _appendExact(node.literal.lexeme); super.visitIntegerLiteral(node); } @override void visitDoubleLiteral(DoubleLiteral node) { - if (!_ignoreLiterals) { - _append(node.literal.lexeme); - } + _appendExact(node.literal.lexeme); super.visitDoubleLiteral(node); } @override void visitSimpleStringLiteral(SimpleStringLiteral node) { - if (!_ignoreLiterals) { - _append(node.value); - } + _appendExact(node.value); super.visitSimpleStringLiteral(node); } @override void visitInterpolationString(InterpolationString node) { - if (!_ignoreLiterals) { - _append(node.value); - } + _appendExact(node.value); super.visitInterpolationString(node); } @override void visitBooleanLiteral(BooleanLiteral node) { - if (!_ignoreLiterals) { - _appendHash(node.value ? 1 : 0); - } + _appendExactBool(node.value); super.visitBooleanLiteral(node); } + @override + void visitSymbolLiteral(SymbolLiteral node) { + _appendExact(node.components.map((t) => t.lexeme).join('.')); + super.visitSymbolLiteral(node); + } + // --- Identifiers --- @override void visitSimpleIdentifier(SimpleIdentifier node) { + // If this identifier is the label of a named argument in a call expression + // (e.g. `foo(param: value)`), it is a reference to the callee's parameter + // rather than a local variable of the method being analyzed. + if (node.parent case Label(parent: NamedExpression())) return; + final element = node.element; switch (element) { // Smart ignore: ignore ONLY local variables and parameters. // This preserves field names, getters, methods, class names, etc. - case LocalVariableElement() || FormalParameterElement() - when _ignoreIdentifiers: + case LocalVariableElement() || + FormalParameterElement() || + PatternVariableElement(): // It is a local variable or parameter. // Assign it a local ID (De Bruijn Indexing) to distinguish clones // that wire their variables differently. diff --git a/lib/src/lints/avoid_duplicate_code/visitors/avoid_duplicate_code_visitor.dart b/lib/src/lints/avoid_duplicate_code/visitors/avoid_duplicate_code_visitor.dart index 3ca1ca67..c5b0330b 100644 --- a/lib/src/lints/avoid_duplicate_code/visitors/avoid_duplicate_code_visitor.dart +++ b/lib/src/lints/avoid_duplicate_code/visitors/avoid_duplicate_code_visitor.dart @@ -1,56 +1,87 @@ import 'package:analyzer/dart/analysis/context_root.dart'; import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/visitor.dart'; -import 'package:analyzer/diagnostic/diagnostic.dart'; import 'package:analyzer/file_system/file_system.dart'; import 'package:analyzer/file_system/physical_file_system.dart'; import 'package:collection/collection.dart'; import 'package:solid_lints/src/common/parameter_parser/analysis_options_loader.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/analyzed_candidate.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/models/avoid_duplicate_code_parameters.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/models/body_candidate.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/models/cross_file_match.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/models/duplicate_location.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/models/hash_entry.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/reporters/avoid_duplicate_code_reporter.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/reporters/duplicate_report_context.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/services/differing_literals_analyzer.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/services/global_hash_registry.dart'; -import 'package:solid_lints/src/lints/avoid_duplicate_code/utils/range_extension.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/utils/token_utils.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/visitors/ast_structural_hash_visitor.dart'; import 'package:solid_lints/src/lints/avoid_duplicate_code/visitors/candidate_visitor.dart'; -import 'package:solid_lints/src/lints/avoid_duplicate_code/visitors/descendant_visitor.dart'; -import 'package:solid_lints/src/models/solid_diagnostic_message.dart'; import 'package:solid_lints/src/utils/ignore_matcher.dart'; /// A visitor that detects duplicate code blocks (at the function level and/or -/// statement block level) within a single compilation unit and across files. +/// statement block level) within a single compilation unit and across files, +/// differentiating between exact duplicates and duplicates with differing +/// literal values. class AvoidDuplicateCodeVisitor extends RecursiveAstVisitor { - static const _duplicateContextMessage = 'Duplicate'; - static final _packageRootCache = {}; - - final AvoidDuplicateCodeRule _rule; final AvoidDuplicateCodeParameters _parameters; final String _filePath; final int _modificationStamp; final ContextRoot? _contextRoot; final ResourceProvider _resourceProvider; + final AvoidDuplicateCodeReporter _reporter; final AnalysisOptionsLoader? _analysisOptionsLoader; final IgnoreMatcher _ignoreMatcher; /// Creates a new instance of [AvoidDuplicateCodeVisitor]. - AvoidDuplicateCodeVisitor( - this._rule, - this._parameters, { + factory AvoidDuplicateCodeVisitor( + AvoidDuplicateCodeRule rule, + AvoidDuplicateCodeParameters parameters, { required String filePath, required int modificationStamp, required IgnoreMatcher ignoreMatcher, ContextRoot? contextRoot, ResourceProvider? resourceProvider, AnalysisOptionsLoader? analysisOptionsLoader, - }) : _filePath = filePath, + }) { + final effectiveResourceProvider = + resourceProvider ?? PhysicalResourceProvider.INSTANCE; + final reporter = AvoidDuplicateCodeReporter( + rule: rule, + literalsAnalyzer: DifferingLiteralsAnalyzer( + resourceProvider: effectiveResourceProvider, + ), + ); + + return AvoidDuplicateCodeVisitor._( + parameters: parameters, + filePath: filePath, + modificationStamp: modificationStamp, + contextRoot: contextRoot, + resourceProvider: effectiveResourceProvider, + reporter: reporter, + analysisOptionsLoader: analysisOptionsLoader, + ignoreMatcher: ignoreMatcher, + ); + } + + AvoidDuplicateCodeVisitor._({ + required AvoidDuplicateCodeParameters parameters, + required String filePath, + required int modificationStamp, + required ContextRoot? contextRoot, + required ResourceProvider resourceProvider, + required AvoidDuplicateCodeReporter reporter, + required AnalysisOptionsLoader? analysisOptionsLoader, + required IgnoreMatcher ignoreMatcher, + }) : _parameters = parameters, + _filePath = filePath, _modificationStamp = modificationStamp, _contextRoot = contextRoot, - _resourceProvider = - resourceProvider ?? PhysicalResourceProvider.INSTANCE, + _resourceProvider = resourceProvider, + _reporter = reporter, _analysisOptionsLoader = analysisOptionsLoader, _ignoreMatcher = ignoreMatcher; @@ -59,9 +90,13 @@ class AvoidDuplicateCodeVisitor extends RecursiveAstVisitor { if (_filePath.isEmpty) return; final filePath = _filePath; - GlobalHashRegistry.instance.resourceProvider = _resourceProvider; final packageRoot = - _contextRoot?.root.path ?? _findPackageRoot(filePath) ?? ''; + _contextRoot?.root.path ?? + GlobalHashRegistry.instance.findPackageRoot( + filePath, + resourceProvider: _resourceProvider, + ) ?? + ''; final isExcluded = _analysisOptionsLoader?.isFileExcludedForFile(filePath) ?? false; @@ -69,6 +104,7 @@ class AvoidDuplicateCodeVisitor extends RecursiveAstVisitor { if (isExcluded || _ignoreMatcher.isFileIgnored(node)) { GlobalHashRegistry.instance.removeFile( filePath, + resourceProvider: _resourceProvider, parameters: _parameters, packageRoot: packageRoot, ); @@ -76,39 +112,50 @@ class AvoidDuplicateCodeVisitor extends RecursiveAstVisitor { } if (_tryReportFromCache(filePath, packageRoot)) return; - final hasher = AstStructuralHashVisitor( - ignoreLiterals: _parameters.ignoreLiterals, - ignoreIdentifiers: _parameters.ignoreIdentifiers, - ); - final candidates = _collectCandidates(node); - final candidateHashes = { - for (final BodyCandidate(:node) in candidates) - node: hasher.computeHash(node), - }; + + final rawCandidates = _collectCandidates(node); + final candidates = _analyzeCandidates(node, rawCandidates); + final crossFileDuplicatesByHash = _findAndSaveCrossFileMatches( filePath, - candidates - .map( - (c) => HashEntry( - hash: candidateHashes[c.node]!, - lineNumber: node.lineInfo.getLocation(c.node.offset).lineNumber, - offset: c.node.offset, - length: c.node.length, - tokenCount: c.node.tokenCount, - ), - ) - .toList(), + candidates.map((c) => c.entry).toList(), packageRoot, ); - _groupAndReportDuplicates( - filePath, - candidates, - candidateHashes, - hasher, - crossFileDuplicatesByHash, + + if (candidates.isEmpty) return; + + _reporter.report( + filePath: filePath, + contexts: DuplicateReportContext.fromAstCandidates(candidates), + crossFileDuplicatesByHash: crossFileDuplicatesByHash, ); } + List _analyzeCandidates( + CompilationUnit unit, + List candidates, + ) { + final hasher = AstStructuralHashVisitor(); + + return candidates.map((candidate) { + final candidateNode = candidate.node; + final hashes = hasher.computeHashes(candidateNode); + final lineInfo = unit.lineInfo.getLocation(candidateNode.offset); + + return ( + candidate: candidate, + entry: HashEntry( + hash: hashes.structuralHash, + exactHash: hashes.exactHash, + lineNumber: lineInfo.lineNumber, + offset: candidateNode.offset, + length: candidateNode.length, + tokenCount: candidateNode.tokenCount, + ), + ); + }).toList(); + } + List _collectCandidates(CompilationUnit node) { final collector = CandidateVisitor(_parameters); node.accept(collector); @@ -128,78 +175,41 @@ class AvoidDuplicateCodeVisitor extends RecursiveAstVisitor { final registry = GlobalHashRegistry.instance; final cachedStamp = registry.getModificationStamp( filePath, + resourceProvider: _resourceProvider, parameters: _parameters, packageRoot: packageRoot, ); - if (cachedStamp != _modificationStamp) return false; + if (cachedStamp == null || cachedStamp != _modificationStamp) return false; final cachedEntries = registry.getFileEntries( filePath, + resourceProvider: _resourceProvider, parameters: _parameters, packageRoot: packageRoot, ); + if (cachedEntries == null) return false; final crossMatches = registry.findCrossFileMatches( filePath, cachedEntries, + resourceProvider: _resourceProvider, parameters: _parameters, packageRoot: packageRoot, isFileExcluded: _isFileExcluded, ); - final hashGroups = groupBy(cachedEntries, (entry) => entry.hash); - - final hasIntraDuplicates = hashGroups.values.any( - (group) => group.length > 1, + _reporter.report( + filePath: filePath, + contexts: DuplicateReportContext.fromCachedEntries( + cachedEntries, + filePath: filePath, + ), + crossFileDuplicatesByHash: crossMatches.toDuplicatesByHash(), ); - final hasCrossDuplicates = crossMatches.isNotEmpty; - - if (!hasIntraDuplicates && !hasCrossDuplicates) { - return true; // We checked the cache, and there are no duplicates. - } - - final suppressedRanges = <(int, int)>[]; - final reportedOffsets = {}; - - final crossFileDuplicatesByHash = crossMatches.toDuplicatesByHash(); - - for (final entry in cachedEntries) { - final group = hashGroups[entry.hash]; - if (suppressedRanges.anyContainsStrictly(entry.range) || - reportedOffsets.contains(entry.offset) || - group == null) { - continue; - } - - final activeGroup = group - .where((e) => !suppressedRanges.anyContainsStrictly(e.range)) - .toList(); - - final externalPartners = - crossFileDuplicatesByHash[entry.hash] ?? const []; - final internalPartners = activeGroup.where((e) => e != entry).toList(); - - if (internalPartners.isNotEmpty || externalPartners.isNotEmpty) { - final contextMessages = _buildContextMessages( - currentFilePath: filePath, - externalPartners: externalPartners, - internalPartners: internalPartners.map((e) => (e.offset, e.length)), - ); - _rule.reportAtOffset( - entry.offset, - entry.length, - contextMessages: contextMessages, - ); - - reportedOffsets.add(entry.offset); - suppressedRanges.add((entry.offset, entry.length)); - } - } - - return true; // Cache hit and handled + return true; } bool _isFileExcluded(String path) => @@ -210,12 +220,11 @@ class AvoidDuplicateCodeVisitor extends RecursiveAstVisitor { List hashEntries, String packageRoot, ) { - if (packageRoot.isEmpty) return const {}; - final registry = GlobalHashRegistry.instance; final crossMatches = registry.findCrossFileMatches( filePath, hashEntries, + resourceProvider: _resourceProvider, parameters: _parameters, packageRoot: packageRoot, isFileExcluded: _isFileExcluded, @@ -225,109 +234,11 @@ class AvoidDuplicateCodeVisitor extends RecursiveAstVisitor { filePath, hashEntries, modificationStamp: _modificationStamp, + resourceProvider: _resourceProvider, parameters: _parameters, packageRoot: packageRoot, ); return crossMatches.toDuplicatesByHash(); } - - void _groupAndReportDuplicates( - String filePath, - List candidates, - Map candidateHashes, - AstStructuralHashVisitor hasher, - Map> crossFileDuplicatesByHash, - ) { - final groups = groupBy( - candidates, - (c) => candidateHashes[c.node] ?? hasher.computeHash(c.node), - ); - - final suppressed = {}; - - for (final candidate in candidates.toSet()) { - // Skip candidates that are descendants of an already reported duplicate - // block to prevent nested warnings. - if (suppressed.contains(candidate.node)) continue; - - final hash = - candidateHashes[candidate.node] ?? hasher.computeHash(candidate.node); - final internalPartners = groups[hash]?.toList(); - if (internalPartners == null) continue; - - final externalPartners = crossFileDuplicatesByHash[hash] ?? const []; - internalPartners - ..removeWhere((c) => suppressed.contains(c.node)) - ..remove(candidate); - - if (internalPartners.isEmpty && externalPartners.isEmpty) continue; - - _rule.reportAtNode( - candidate.node, - contextMessages: _buildContextMessages( - currentFilePath: filePath, - externalPartners: externalPartners, - internalPartners: internalPartners.map( - (c) => (c.node.offset, c.node.length), - ), - ), - ); - - _suppressDescendants(candidate.node, suppressed); - } - } - - void _suppressDescendants(AstNode root, Set suppressed) { - final descendantCollector = DescendantVisitor(suppressed, root); - root.accept(descendantCollector); - } - - List _buildContextMessages({ - required String currentFilePath, - required List externalPartners, - required Iterable<(int offset, int length)> internalPartners, - }) { - return [ - for (final dup in externalPartners) - SolidDiagnosticMessage( - filePath: dup.filePath, - offset: dup.entry.offset, - length: dup.entry.length, - message: _duplicateContextMessage, - ), - for (final (offset, length) in internalPartners) - SolidDiagnosticMessage( - filePath: currentFilePath, - offset: offset, - length: length, - message: _duplicateContextMessage, - ), - ]; - } - - /// Clears the cached package root lookups. Should be called when - /// the registry is cleared to avoid stale project path references. - static void clearPackageRootCache() => _packageRootCache.clear(); - - String? _findPackageRoot(String filePath) { - if (filePath.isEmpty) return null; - final pathContext = _resourceProvider.pathContext; - final dirPath = pathContext.dirname(filePath); - return _packageRootCache.putIfAbsent(dirPath, () { - var dir = _resourceProvider.getFolder(dirPath); - while (true) { - final pubspec = dir.getChildAssumingFile('pubspec.yaml'); - if (pubspec.exists) { - return dir.path; - } - final parent = dir.parent; - if (parent.path == dir.path) { - break; - } - dir = parent; - } - return null; - }); - } } diff --git a/lib/src/lints/avoid_duplicate_code/visitors/candidate_visitor.dart b/lib/src/lints/avoid_duplicate_code/visitors/candidate_visitor.dart index 6b840a4d..fba2dcc4 100644 --- a/lib/src/lints/avoid_duplicate_code/visitors/candidate_visitor.dart +++ b/lib/src/lints/avoid_duplicate_code/visitors/candidate_visitor.dart @@ -16,12 +16,7 @@ class CandidateVisitor extends RecursiveAstVisitor { CandidateVisitor(this.parameters); @override - void visitBlock(Block node) => - // If checkBlocks is false, only consider blocks that represent function - // bodies. - !parameters.checkBlocks && node.parent is! BlockFunctionBody - ? super.visitBlock(node) - : _visit(node, super.visitBlock); + void visitBlock(Block node) => _visit(node, super.visitBlock); @override void visitExpressionFunctionBody(ExpressionFunctionBody node) => diff --git a/lib/src/lints/avoid_duplicate_code/visitors/descendant_visitor.dart b/lib/src/lints/avoid_duplicate_code/visitors/descendant_visitor.dart deleted file mode 100644 index 3c22b9dc..00000000 --- a/lib/src/lints/avoid_duplicate_code/visitors/descendant_visitor.dart +++ /dev/null @@ -1,26 +0,0 @@ -import 'package:analyzer/dart/ast/ast.dart'; -import 'package:analyzer/dart/ast/visitor.dart'; - -/// A visitor that collects descendant candidate blocks to suppress them. -class DescendantVisitor extends RecursiveAstVisitor { - /// The set of suppressed nodes. - final Set suppressed; - - /// The root node from which to start the descent. - final AstNode root; - - /// Creates a new instance of [DescendantVisitor]. - DescendantVisitor(this.suppressed, this.root); - - @override - void visitBlock(Block node) => _visit(node, super.visitBlock); - - @override - void visitExpressionFunctionBody(ExpressionFunctionBody node) => - _visit(node, super.visitExpressionFunctionBody); - - void _visit(T node, void Function(T) visitSuper) { - if (node != root) suppressed.add(node); - visitSuper(node); - } -} diff --git a/lib/src/lints/avoid_duplicate_code/visitors/literal_collector_visitor.dart b/lib/src/lints/avoid_duplicate_code/visitors/literal_collector_visitor.dart new file mode 100644 index 00000000..a4a32d3e --- /dev/null +++ b/lib/src/lints/avoid_duplicate_code/visitors/literal_collector_visitor.dart @@ -0,0 +1,75 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/token.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/literal_info.dart'; + +/// A visitor that traverses an AST node and collects literal values and their +/// source spans in deterministic traversal order. +class LiteralCollectorVisitor extends RecursiveAstVisitor { + final List _literals = []; + + /// Collects all literals from the given [node]. + static List collect(AstNode node) { + final visitor = LiteralCollectorVisitor(); + node.accept(visitor); + return visitor._literals; + } + + void _addLiteral(AstNode node, String text) { + _literals.add( + LiteralInfo( + text: text, + offset: node.offset, + length: node.length, + ), + ); + } + + @override + void visitPrefixExpression(PrefixExpression node) { + if (node case PrefixExpression( + operator: Token(type: TokenType.MINUS || TokenType.PLUS), + operand: IntegerLiteral() || DoubleLiteral(), + )) { + _addLiteral(node, node.toSource()); + return; + } + super.visitPrefixExpression(node); + } + + @override + void visitIntegerLiteral(IntegerLiteral node) { + _addLiteral(node, node.literal.lexeme); + super.visitIntegerLiteral(node); + } + + @override + void visitDoubleLiteral(DoubleLiteral node) { + _addLiteral(node, node.literal.lexeme); + super.visitDoubleLiteral(node); + } + + @override + void visitSimpleStringLiteral(SimpleStringLiteral node) { + _addLiteral(node, node.toSource()); + super.visitSimpleStringLiteral(node); + } + + @override + void visitInterpolationString(InterpolationString node) { + _addLiteral(node, "'${node.value}'"); + super.visitInterpolationString(node); + } + + @override + void visitBooleanLiteral(BooleanLiteral node) { + _addLiteral(node, node.value ? 'true' : 'false'); + super.visitBooleanLiteral(node); + } + + @override + void visitSymbolLiteral(SymbolLiteral node) { + _addLiteral(node, node.toSource()); + super.visitSymbolLiteral(node); + } +} diff --git a/lib/src/utils/ignore_matcher.dart b/lib/src/utils/ignore_matcher.dart index 123f26b6..00e6164d 100644 --- a/lib/src/utils/ignore_matcher.dart +++ b/lib/src/utils/ignore_matcher.dart @@ -28,9 +28,15 @@ final class IgnoreMatcher { ].commentLexemes.any(_fileIgnoreRegex.hasMatch); /// Checks if a candidate [node] or its enclosing [declaration] is ignored. - bool isCandidateIgnored(AstNode node, [Declaration? declaration]) => [ - ?declaration?.beginToken, - ?declaration?.firstTokenAfterCommentAndMetadata, - node.beginToken, - ].commentLexemes.any(_lineIgnoreRegex.hasMatch); + bool isCandidateIgnored(AstNode node, [Declaration? declaration]) { + final tokens = [ + if (declaration case final decl?) ...[ + decl.beginToken, + ...decl.firstTokenAfterCommentAndMetadata.upTo(node.beginToken), + ], + node.beginToken, + ]; + + return tokens.commentLexemes.any(_lineIgnoreRegex.hasMatch); + } } diff --git a/lib/src/utils/resource_provider_utils.dart b/lib/src/utils/resource_provider_utils.dart index 6541026e..cdda147e 100644 --- a/lib/src/utils/resource_provider_utils.dart +++ b/lib/src/utils/resource_provider_utils.dart @@ -1,9 +1,27 @@ import 'package:analyzer/file_system/file_system.dart'; -/// Extension on [ResourceProvider] to provide folder helpers. +/// Extension on [ResourceProvider] to provide file and folder helpers. extension ResourceProviderUtils on ResourceProvider { /// Ensures that a folder at the path joined from [root] and [dir] exists, /// creating it if it doesn't. void ensureFolderExists(String root, String dir) => getFolder(pathContext.join(root, dir)).create(); + + /// Reads file content at [path] if it exists, otherwise returns an empty + /// string. + /// + /// Returns an empty string if the file does not exist or cannot be read. + /// Guards against a TOCTOU race where the file is deleted or becomes + /// unreadable between the existence check and the actual read (e.g. during + /// an active editor session). + String readFileContent(String path) { + final file = getFile(path); + if (!file.exists) return ''; + + try { + return file.readAsStringSync(); + } on FileSystemException { + return ''; + } + } } diff --git a/lib/src/utils/token_utils.dart b/lib/src/utils/token_utils.dart index 2bdd3494..78478f2c 100644 --- a/lib/src/utils/token_utils.dart +++ b/lib/src/utils/token_utils.dart @@ -9,6 +9,19 @@ extension TokenUtils on Token { yield c; } } + + /// Returns an iterable sequence of tokens starting from this token up to + /// (and including) [end]. + Iterable upTo(Token end) sync* { + var current = this; + while (true) { + yield current; + if (current == end) break; + final next = current.next; + if (next == null || next == current) break; + current = next; + } + } } /// Extension methods for [Iterable] manipulation. diff --git a/test/src/common/utils/ignore_matcher_test.dart b/test/src/common/utils/ignore_matcher_test.dart index 40e03df3..2881b498 100644 --- a/test/src/common/utils/ignore_matcher_test.dart +++ b/test/src/common/utils/ignore_matcher_test.dart @@ -252,6 +252,85 @@ void foo() { expect(matcher.isCandidateIgnored(innerBlock, null), isTrue); }); + test( + 'returns true when inline ignore is placed inside parameter list', + () { + final (body, decl) = _parseFunction(''' +void foo( + int a, + // ignore: $ruleName + int b, +) { + final x = 1; +} +'''); + + expect(matcher.isCandidateIgnored(body, decl), isTrue); + }, + ); + + test( + 'returns true when inline ignore is placed before closing parenthesis', + () { + final (body, decl) = _parseFunction(''' +void foo( + int a, + int b, + // ignore: $ruleName +) { + final x = 1; +} +'''); + + expect(matcher.isCandidateIgnored(body, decl), isTrue); + }, + ); + + test( + 'returns true when package-prefixed inline ignore is placed before closing parenthesis', + () { + final (body, decl) = _parseFunction(''' +void foo( + int a, + int b, + // ignore: solid_lints/$ruleName +) { + final x = 1; +} +'''); + + expect(matcher.isCandidateIgnored(body, decl), isTrue); + }, + ); + + test( + 'returns true for method inside class with ignore in parameters', + () { + final result = parseString( + content: + ''' +abstract class Foo { + static void bar( + int a, + int b, + // ignore: solid_lints/$ruleName + ) { + final x = 1; + } +} +''', + ); + final classDecl = result.unit.declarations.first as ClassDeclaration; + final methodDecl = switch (classDecl.body) { + BlockClassBody(:final members) => + members.first as MethodDeclaration, + _ => throw StateError('Expected BlockClassBody'), + }; + final body = (methodDecl.body as BlockFunctionBody).block; + expect(matcher.isCandidateIgnored(body, methodDecl), isTrue); + }, + ); + test('returns false when declaration is not ignored', () { final (body, decl) = _parseFunction(''' void foo() { diff --git a/test/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule_test.dart b/test/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule_test.dart index 68c9c850..060aab1e 100644 --- a/test/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule_test.dart +++ b/test/src/lints/avoid_duplicate_code/avoid_duplicate_code_rule_test.dart @@ -29,14 +29,13 @@ plugins: diagnostics: avoid_duplicate_code: min_tokens: 15 - check_blocks: true exclude: - method_name: excluded '''; @override void setUp() { - GlobalHashRegistry.instance.clear(); + GlobalHashRegistry.instance.clear(resourceProvider: resourceProvider); GlobalHashRegistry.instance.enablePhysicalFileCleanup = false; rule = AvoidDuplicateCodeRule( analysisOptionsLoader: AnalysisOptionsLoader( @@ -54,11 +53,11 @@ $_mockAnalysisOptionsContent''', @override Future tearDown() async { - GlobalHashRegistry.instance.clear(); + GlobalHashRegistry.instance.clear(resourceProvider: resourceProvider); await super.tearDown(); } - // --- Base Tests (min_tokens: 15, check_blocks: true, default) --- + // --- Base Tests (min_tokens: 15, default) --- Future test_reports_when_two_functions_have_identical_bodies() async { await assertAutoDiagnostics(''' @@ -186,20 +185,9 @@ void third() ${expectLint(r'''{ '''); } - // --- Ignore Literals Tests --- + // --- Literals Tests --- Future test_reports_when_only_literal_values_differ() async { - newAnalysisOptionsYamlFile( - testPackageRootPath, - '''${analysisOptionsContent(rules: [rule.name])} -plugins: - solid_lints: - diagnostics: - avoid_duplicate_code: - min_tokens: 15 - ignore_literals: true -''', - ); await assertAutoDiagnostics(''' void first() ${expectLint(r'''{ final x = 1; @@ -207,7 +195,7 @@ void first() ${expectLint(r'''{ print('hello'); } print('world'); -}''')} +}''', messageContainsAll: ['differs in literal values', '[1, 2]', "['hello', 'foo']", "['world', 'bar']"])} void second() ${expectLint(r'''{ final y = 2; @@ -215,62 +203,124 @@ void second() ${expectLint(r'''{ print('foo'); } print('bar'); -}''')} +}''', messageContainsAll: ['differs in literal values', '[2, 1]', "['foo', 'hello']", "['bar', 'world']"])} +'''); + } + + Future test_reports_differing_literals_across_three_clones() async { + await assertAutoDiagnostics(''' +void first() ${expectLint(r'''{ + final x = 1; + if (x > 0) { + print('hello'); + } + print('world'); +}''', messageContainsAll: ['differs in literal values', '[1, 2, 3]', "['hello', 'foo', 'baz']", "['world', 'bar', 'qux']"])} + +void second() ${expectLint(r'''{ + final y = 2; + if (y > 0) { + print('foo'); + } + print('bar'); +}''', messageContainsAll: ['differs in literal values', '[2, 1, 3]', "['foo', 'hello', 'baz']", "['bar', 'world', 'qux']"])} + +void third() ${expectLint(r'''{ + final z = 3; + if (z > 0) { + print('baz'); + } + print('qux'); +}''', messageContainsAll: ['differs in literal values', '[3, 1, 2]', "['baz', 'hello', 'foo']", "['qux', 'world', 'bar']"])} '''); } Future - test_does_not_report_when_literal_values_differ_without_flag() async { - await assertNoDiagnostics(r''' -void first() { + test_reports_exact_and_different_literals_in_mixed_scenario() async { + await assertAutoDiagnostics(''' +void first() ${expectLint(r'''{ final x = 1; if (x > 0) { - print('a'); + print('hello'); } - print('b'); -} + print('world'); +}''', messageContainsAll: ['Perhaps this code is a duplicate'])} -void second() { - final y = 99; +void second() ${expectLint(r'''{ + final y = 1; if (y > 0) { - print('c'); + print('hello'); } - print('d'); -} + print('world'); +}''', messageContainsAll: ['Perhaps this code is a duplicate'])} + +void third() ${expectLint(r'''{ + final z = 2; + if (z > 0) { + print('foo'); + } + print('bar'); +}''', messageContainsAll: ['differs in literal values'])} '''); } - // --- Ignore Identifiers Tests --- + Future + test_reports_differing_literals_truncates_slots_when_more_than_limit() async { + await assertAutoDiagnostics(''' +void first() ${expectLint(r'''{ + final a = 1; + final b = 10; + if (a > 0) { + print('hello'); + } + print('world'); +}''', messageContainsAll: ['differs in literal values', '[1, 2]', '[10, 20]', "['hello', 'foo']", '(+1 more)'])} + +void second() ${expectLint(r'''{ + final a = 2; + final b = 20; + if (a > 0) { + print('foo'); + } + print('bar'); +}''', messageContainsAll: ['differs in literal values', '[2, 1]', '[20, 10]', "['foo', 'hello']", '(+1 more)'])} +'''); + } Future - test_does_not_report_when_identifiers_differ_without_flag() async { - newAnalysisOptionsYamlFile( - testPackageRootPath, - '''${analysisOptionsContent(rules: [rule.name])} -plugins: - solid_lints: - diagnostics: - avoid_duplicate_code: - min_tokens: 15 - ignore_identifiers: false -''', - ); - await assertNoDiagnostics(r''' -void first() { + test_reports_differing_literals_truncates_values_when_more_than_limit() async { + await assertAutoDiagnostics(''' +void first() ${expectLint(r'''{ final x = 1; if (x > 0) { - print(x); + print('hello'); } - print('done'); -} + print('world'); +}''', messageContainsAll: ['differs in literal values', '[1, 2, 3, +1 more]'])} -void second() { - final y = 1; +void second() ${expectLint(r'''{ + final y = 2; if (y > 0) { - print(y); + print('foo'); } - print('done'); -} + print('bar'); +}''', messageContainsAll: ['differs in literal values', '[2, 1, 3, +1 more]'])} + +void third() ${expectLint(r'''{ + final z = 3; + if (z > 0) { + print('baz'); + } + print('qux'); +}''', messageContainsAll: ['differs in literal values', '[3, 1, 2, +1 more]'])} + +void fourth() ${expectLint(r'''{ + final w = 4; + if (w > 0) { + print('quux'); + } + print('corge'); +}''', messageContainsAll: ['differs in literal values', '[4, 1, 2, +1 more]'])} '''); } @@ -407,45 +457,34 @@ void second() ${expectLint(r'''{ '''); } - Future test_ignores_nested_blocks_when_check_blocks_false() async { - newAnalysisOptionsYamlFile( - testPackageRootPath, - '''${analysisOptionsContent(rules: [rule.name])} -plugins: - solid_lints: - diagnostics: - avoid_duplicate_code: - min_tokens: 15 - check_blocks: false -''', - ); - - // The nested blocks are identical (>15 tokens), but check_blocks is false, - // and the outer function bodies differ significantly. - await assertNoDiagnostics(r''' -void one() { + Future + test_reports_cross_file_duplicate_when_literals_differ_in_registry() async { + final otherFile = newFile('$testPackageLibPath/other.dart', ''' +void otherMethod() { final x = 1; if (x > 0) { print('hello'); - print('world'); - print('done'); } + print('world'); } +'''); -void two() { - print('completely different start'); - if (true) { - print('hello'); - print('world'); - print('done'); + await _indexFile(otherFile); + + await assertAutoDiagnostics(''' +void mainMethod() ${expectLint(r'''{ + final y = 2; + if (y > 0) { + print('foo'); } -} + print('bar'); +}''', messageContainsAll: ['differs in literal values', '[2, 1]', "['foo', 'hello']", "['bar', 'world']"])} '''); } Future test_does_not_report_when_identifiers_are_different_method_calls() async { - // Tests that ignore_identifiers=true still differentiates method calls. + // Tests that different method calls are differentiated. // Local variables are ignored, but external method names are preserved. await assertNoDiagnostics(''' void doSomething(int x) {} @@ -469,6 +508,85 @@ void second() { '''); } + Future test_reports_when_only_symbol_literals_differ() async { + await assertAutoDiagnostics(''' +void first() ${expectLint(r'''{ + final action = #foo; + if (action == #foo) { + print('hello'); + } + print('done'); +}''', messageContainsAll: ['differs in literal values', '[#foo, #bar]'])} + +void second() ${expectLint(r'''{ + final action = #bar; + if (action == #bar) { + print('hello'); + } + print('done'); +}''', messageContainsAll: ['differs in literal values', '[#bar, #foo]'])} +'''); + } + + Future + test_reports_exact_duplicate_when_symbol_literals_are_identical() async { + await assertAutoDiagnostics(''' +void first() ${expectLint(r'''{ + final action = #foo; + if (action == #foo) { + print('hello'); + } + print('done'); +}''', messageContainsAll: ['Perhaps this code is a duplicate'])} + +void second() ${expectLint(r'''{ + final action = #foo; + if (action == #foo) { + print('hello'); + } + print('done'); +}''', messageContainsAll: ['Perhaps this code is a duplicate'])} +'''); + } + + Future test_does_not_report_when_named_argument_labels_differ() async { + await assertNoDiagnostics(''' +void callMe({int? width, int? height, int? count}) {} + +void first() { + final x = 1; + callMe(width: x, count: 10); + print('done'); +} + +void second() { + final y = 1; + callMe(height: y, count: 10); + print('done'); +} +'''); + } + + Future test_reports_when_numeric_literal_signs_differ() async { + await assertAutoDiagnostics(''' +void moveLeft() ${expectLint(r'''{ + final dx = -10; + if (dx < 0) { + print('moving'); + } + print('done'); +}''', messageContainsAll: ['differs in literal values', '[-10, 10]'])} + +void moveRight() ${expectLint(r'''{ + final dx = 10; + if (dx < 0) { + print('moving'); + } + print('done'); +}''', messageContainsAll: ['differs in literal values', '[10, -10]'])} +'''); + } + Future test_duplicate_code_with_three_files_and_excluded_part_file() async { newAnalysisOptionsYamlFile(testPackageRootPath, ''' @@ -559,6 +677,58 @@ void mainMethod() { '''); } + Future + test_ignored_method_with_comment_inside_parameters_suppresses_warning() async { + await assertNoDiagnostics(''' +void first( + int a, + int b, + // ignore: avoid_duplicate_code +) { + final x = 1; + if (x > 0) { + print(x); + } + print('done'); +} + +void second(int a, int b) { + final x = 1; + if (x > 0) { + print(x); + } + print('done'); +} +'''); + } + + Future + test_ignored_method_with_package_prefixed_comment_inside_parameters_suppresses_warning() async { + await assertNoDiagnostics(''' +abstract final class Foo { + static void first( + int a, + int b, + // ignore: solid_lints/avoid_duplicate_code + ) { + final x = 1; + if (x > 0) { + print(x); + } + print('done'); + } + + static void second(int a, int b) { + final x = 1; + if (x > 0) { + print(x); + } + print('done'); + } +} +'''); + } + Future test_ignored_file_with_ignore_for_file_does_not_trigger_cross_file_duplicates() async { // 1. Create other.dart with ignore_for_file comment @@ -630,9 +800,56 @@ void otherMethod() { expect(GlobalHashRegistry.instance.fileCount, 0); } + Future + test_visiting_file_with_inline_ignore_removes_it_from_registry() async { + final otherFile = newFile('$testPackageLibPath/other.dart', ''' +void otherMethod() { + final x = 1; + if (x > 0) { + print(x); + } + print('done'); +} +'''); + await _indexFile(otherFile); + expect(GlobalHashRegistry.instance.fileCount, 1); + + final result = parseString( + content: ''' +// ignore: avoid_duplicate_code +void otherMethod() { + final x = 1; + if (x > 0) { + print(x); + } + print('done'); +} +''', + ); + final avoidRule = rule as AvoidDuplicateCodeRule; + final visitor = AvoidDuplicateCodeVisitor( + avoidRule, + AvoidDuplicateCodeParameters( + minTokens: 15, + exclude: ExcludedIdentifiersListParameter( + exclude: [ExcludedIdentifierParameter(methodName: 'excluded')], + ), + ), + filePath: otherFile.path, + modificationStamp: 2, + ignoreMatcher: avoidRule.ignoreMatcher, + resourceProvider: resourceProvider, + analysisOptionsLoader: avoidRule.analysisOptionsLoader, + ); + result.unit.accept(visitor); + + expect(GlobalHashRegistry.instance.fileCount, 0); + } + Future _indexFile( File file, { AvoidDuplicateCodeParameters? parameters, + int? modificationStamp, }) async { final resolved = await resolveFile(file.path); final avoidRule = rule as AvoidDuplicateCodeRule; @@ -641,15 +858,12 @@ void otherMethod() { parameters ?? AvoidDuplicateCodeParameters( minTokens: 15, - ignoreLiterals: false, - ignoreIdentifiers: true, - checkBlocks: true, exclude: ExcludedIdentifiersListParameter( exclude: [ExcludedIdentifierParameter(methodName: 'excluded')], ), ), filePath: file.path, - modificationStamp: 1, + modificationStamp: modificationStamp ?? file.modificationStamp, ignoreMatcher: avoidRule.ignoreMatcher, contextRoot: resolved.session.analysisContext.contextRoot, resourceProvider: resourceProvider, diff --git a/test/src/lints/avoid_duplicate_code/global_hash_registry_test.dart b/test/src/lints/avoid_duplicate_code/global_hash_registry_test.dart index 93c8de1e..78a6deb4 100644 --- a/test/src/lints/avoid_duplicate_code/global_hash_registry_test.dart +++ b/test/src/lints/avoid_duplicate_code/global_hash_registry_test.dart @@ -2,7 +2,6 @@ import 'dart:convert'; import 'dart:io' as io; import 'package:analyzer/file_system/memory_file_system.dart'; -import 'package:analyzer/file_system/physical_file_system.dart'; import 'package:path/path.dart' as p; import 'package:solid_lints/src/common/parameters/excluded_identifier_parameter.dart'; import 'package:solid_lints/src/common/parameters/excluded_identifiers_list_parameter.dart'; @@ -21,528 +20,776 @@ void main() { setUp(() { memoryResourceProvider = MemoryResourceProvider(); registry = GlobalHashRegistry.instance - ..resourceProvider = memoryResourceProvider - ..clear() + ..clear(resourceProvider: memoryResourceProvider) ..enablePhysicalFileCleanup = false; }); tearDown( () => registry - ..clear() - ..resourceProvider = PhysicalResourceProvider.INSTANCE + ..clear(resourceProvider: memoryResourceProvider) ..enablePhysicalFileCleanup = true, ); - test('updateFile stores entries', () { - final entries = [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - const HashEntry(hash: 456, lineNumber: 20, tokenCount: 3), - ]; + group('in-memory file indexing and lookup', () { + test('updateFile stores entries and increments fileCount', () { + final entries = [ + _TestFactory.entry(hash: 123), + _TestFactory.entry(hash: 456, lineNumber: 20, tokenCount: 3), + ]; - registry.updateFile('file_a.dart', entries, modificationStamp: 1); - - expect(registry.fileCount, equals(1)); - }); + registry.updateFile( + 'file_a.dart', + entries, + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); - test('findCrossFileMatches finds duplicate in other files', () { - final fileAEntries = [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ]; - final fileBEntries = [ - const HashEntry(hash: 123, lineNumber: 15, tokenCount: 5), - ]; + expect(registry.fileCount, equals(1)); + }); - registry.updateFile('file_a.dart', fileAEntries, modificationStamp: 1); + test('getFileEntries returns stored entries for a file', () { + final entries = [ + _TestFactory.entry(hash: 123), + _TestFactory.entry(hash: 456, lineNumber: 20), + ]; - final matches = registry.findCrossFileMatches( - 'file_b.dart', - fileBEntries, - ); + registry.updateFile( + 'file_a.dart', + entries, + modificationStamp: 42, + resourceProvider: memoryResourceProvider, + ); - expect(matches, hasLength(1)); - expect(matches.first.duplicates, hasLength(1)); - final expectedPath = p.normalize( - p.join(io.Directory.current.path, 'file_a.dart'), - ); - expect(matches.first.duplicates.first.filePath, equals(expectedPath)); - expect(matches.first.duplicates.first.entry.hash, equals(123)); - expect(matches.first.duplicates.first.entry.lineNumber, equals(10)); - }); + final stored = registry.getFileEntries( + 'file_a.dart', + resourceProvider: memoryResourceProvider, + ); + expect(stored, isNotNull); + expect(stored, hasLength(2)); + expect(stored!.first.hash, equals(123)); + expect(stored.last.hash, equals(456)); - test('findCrossFileMatches ignores same file', () { - final entries = [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ]; + expect( + registry.getFileEntries( + 'unknown.dart', + resourceProvider: memoryResourceProvider, + ), + isNull, + ); + }); - registry.updateFile('file_a.dart', entries, modificationStamp: 1); + test('getModificationStamp returns stored stamp or null', () { + registry.updateFile( + 'file_a.dart', + [_TestFactory.entry(hash: 123)], + modificationStamp: 12345, + resourceProvider: memoryResourceProvider, + ); - final matches = registry.findCrossFileMatches('file_a.dart', entries); + expect( + registry.getModificationStamp( + 'file_a.dart', + resourceProvider: memoryResourceProvider, + ), + equals(12345), + ); + expect( + registry.getModificationStamp( + 'unknown.dart', + resourceProvider: memoryResourceProvider, + ), + isNull, + ); + }); - expect(matches, isEmpty); - }); + test('updateFile replaces previous entries', () { + final oldEntries = [_TestFactory.entry(hash: 123)]; + final newEntries = [ + _TestFactory.entry(hash: 456, lineNumber: 20, tokenCount: 3), + ]; - test('updateFile replaces previous entries', () { - final oldEntries = [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ]; - final newEntries = [ - const HashEntry(hash: 456, lineNumber: 20, tokenCount: 3), - ]; - - registry.updateFile('file_a.dart', oldEntries, modificationStamp: 1); - registry.updateFile('file_a.dart', newEntries, modificationStamp: 2); - - expect(registry.fileCount, equals(1)); - - // File B tries to match against the old hash 123, should find nothing - final matches1 = registry.findCrossFileMatches('file_b.dart', [ - const HashEntry(hash: 123, lineNumber: 15, tokenCount: 5), - ]); - expect(matches1, isEmpty); - - // File B tries to match against the new hash 456, should match - final matches2 = registry.findCrossFileMatches('file_b.dart', [ - const HashEntry(hash: 456, lineNumber: 25, tokenCount: 3), - ]); - expect(matches2, hasLength(1)); - }); + registry.updateFile( + 'file_a.dart', + oldEntries, + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); + registry.updateFile( + 'file_a.dart', + newEntries, + modificationStamp: 2, + resourceProvider: memoryResourceProvider, + ); - test('removeFile clears entries for specific file', () { - registry.updateFile('file_a.dart', [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ], modificationStamp: 1); - registry.updateFile('file_b.dart', [ - const HashEntry(hash: 456, lineNumber: 20, tokenCount: 5), - ], modificationStamp: 1); + expect(registry.fileCount, equals(1)); - expect(registry.fileCount, equals(2)); + // File B tries to match against the old hash 123, should find nothing + final matches1 = registry.findCrossFileMatches('file_b.dart', [ + _TestFactory.entry(hash: 123, lineNumber: 15), + ], resourceProvider: memoryResourceProvider); + expect(matches1, isEmpty); - registry.removeFile('file_a.dart'); + // File B tries to match against the new hash 456, should match + final matches2 = registry.findCrossFileMatches('file_b.dart', [ + _TestFactory.entry(hash: 456, lineNumber: 25, tokenCount: 3), + ], resourceProvider: memoryResourceProvider); + expect(matches2, hasLength(1)); + }); - expect(registry.fileCount, equals(1)); + test('removeFile clears entries for specific file', () { + registry.updateFile( + 'file_a.dart', + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); + registry.updateFile( + 'file_b.dart', + [_TestFactory.entry(hash: 456, lineNumber: 20)], + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); - final matches = registry.findCrossFileMatches('file_c.dart', [ - const HashEntry(hash: 123, lineNumber: 30, tokenCount: 5), - ]); - expect(matches, isEmpty); - }); + expect(registry.fileCount, equals(2)); - test('clear empties the registry', () { - registry.updateFile('file_a.dart', [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ], modificationStamp: 1); - expect(registry.fileCount, equals(1)); + registry.removeFile( + 'file_a.dart', + resourceProvider: memoryResourceProvider, + ); - registry.clear(); + expect(registry.fileCount, equals(1)); - expect(registry.fileCount, equals(0)); - }); + final matches = registry.findCrossFileMatches('file_c.dart', [ + _TestFactory.entry(hash: 123, lineNumber: 30), + ], resourceProvider: memoryResourceProvider); + expect(matches, isEmpty); + }); - test('findCrossFileMatches groups multiple duplicate locations', () { - registry.updateFile('file_a.dart', [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ], modificationStamp: 1); - registry.updateFile('file_b.dart', [ - const HashEntry(hash: 123, lineNumber: 20, tokenCount: 5), - ], modificationStamp: 1); - - final matches = registry.findCrossFileMatches('file_c.dart', [ - const HashEntry(hash: 123, lineNumber: 30, tokenCount: 5), - ]); - - expect(matches, hasLength(1)); - expect(matches.first.duplicates, hasLength(2)); - final expectedPathA = p.normalize( - p.join(io.Directory.current.path, 'file_a.dart'), - ); - final expectedPathB = p.normalize( - p.join(io.Directory.current.path, 'file_b.dart'), - ); - expect(matches.first.duplicates[0].filePath, equals(expectedPathA)); - expect(matches.first.duplicates[1].filePath, equals(expectedPathB)); - }); + test('clear empties the entire registry', () { + registry.updateFile( + 'file_a.dart', + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); + expect(registry.fileCount, equals(1)); - test('HashCacheStorage saves and loads index', () { - final absoluteFilePath = p.normalize( - p.join(io.Directory.current.path, 'file_a.dart'), - ); - final index = { - absoluteFilePath: const FileCacheEntry( - modificationStamp: 123456, - entries: [HashEntry(hash: 123, lineNumber: 10, tokenCount: 5)], - ), - }; - - final storage = HashCacheStorage( - packageRoot: io.Directory.current.path, - resourceProvider: memoryResourceProvider, - ); + registry.clear(resourceProvider: memoryResourceProvider); - storage.save(index); + expect(registry.fileCount, equals(0)); + }); - final loaded = storage.load(); - expect(loaded, isNotNull); - expect(loaded!.length, equals(1)); - expect(loaded[absoluteFilePath]!.entries, hasLength(1)); - expect(loaded[absoluteFilePath]!.modificationStamp, equals(123456)); + test('clear deletes cache files for all loaded roots', () { + final pkgRoot1 = '/workspace/pkg1'; + final pkgRoot2 = '/workspace/pkg2'; - final entry = loaded[absoluteFilePath]!.entries.first; - expect(entry.hash, equals(123)); - expect(entry.lineNumber, equals(10)); - expect(entry.tokenCount, equals(5)); + final file1 = p.normalize(p.join(pkgRoot1, 'lib/file.dart')); + final file2 = p.normalize(p.join(pkgRoot2, 'lib/file.dart')); - storage.delete(); - }); + registry.updateFile( + file1, + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + packageRoot: pkgRoot1, + resourceProvider: memoryResourceProvider, + ); - test('findCrossFileMatches cleans up absolute paths of deleted files', () { - registry.enablePhysicalFileCleanup = true; - final tempPath = p.normalize( - p.join(io.Directory.systemTemp.path, 'temp_test_file.dart'), - ); - memoryResourceProvider.newFile(tempPath, 'void main() {}'); + registry.updateFile( + file2, + [_TestFactory.entry(hash: 456)], + modificationStamp: 1, + packageRoot: pkgRoot2, + resourceProvider: memoryResourceProvider, + ); - registry.updateFile(tempPath, [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ], modificationStamp: 1); - expect(registry.fileCount, equals(1)); + HashCacheStorage( + packageRoot: pkgRoot1, + resourceProvider: memoryResourceProvider, + ).save({ + file1: FileCacheEntry( + modificationStamp: 1, + entries: [_TestFactory.entry(hash: 123)], + ), + }); + + HashCacheStorage( + packageRoot: pkgRoot2, + resourceProvider: memoryResourceProvider, + ).save({ + file2: FileCacheEntry( + modificationStamp: 1, + entries: [_TestFactory.entry(hash: 456)], + ), + }); + + final cacheFile1 = memoryResourceProvider.getFile( + p.normalize( + p.join(pkgRoot1, '.dart_tool/solid_lints/duplicate_index.json'), + ), + ); + final cacheFile2 = memoryResourceProvider.getFile( + p.normalize( + p.join(pkgRoot2, '.dart_tool/solid_lints/duplicate_index.json'), + ), + ); - // Delete the file from the memory resource provider - memoryResourceProvider.deleteFile(tempPath); + expect(cacheFile1.exists, isTrue); + expect(cacheFile2.exists, isTrue); - // Trigger matching, which should clean up the deleted tempPath - // from registry - final matches = registry.findCrossFileMatches('other_file.dart', [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ]); + registry.clear(resourceProvider: memoryResourceProvider); - expect(matches, isEmpty); - expect(registry.fileCount, equals(0)); - }); + expect(registry.fileCount, equals(0)); + expect(cacheFile1.exists, isFalse); + expect(cacheFile2.exists, isFalse); + }); - test('findCrossFileMatches cleans up absolute paths of excluded files', () { - final absoluteExcludedPath = p.normalize( - '/workspace/project/lib/excluded.dart', - ); + test('findPackageRoot discovers package root from pubspec.yaml', () { + const pkgRoot = '/workspace/my_package'; + const filePath = '$pkgRoot/lib/src/feature/file.dart'; - registry.updateFile(absoluteExcludedPath, [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ], modificationStamp: 1); - expect(registry.fileCount, equals(1)); + memoryResourceProvider.newFile('$pkgRoot/pubspec.yaml', 'name: my_pkg'); - // Trigger matching with a callback that considers absoluteExcludedPath - // as excluded - final matches = registry.findCrossFileMatches('other_file.dart', [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ], isFileExcluded: (path) => path == absoluteExcludedPath); + final discovered = registry.findPackageRoot( + filePath, + resourceProvider: memoryResourceProvider, + ); + expect(discovered, equals(p.normalize(pkgRoot))); - expect(matches, isEmpty); - expect(registry.fileCount, equals(0)); + // Returns null when no pubspec exists + expect( + registry.findPackageRoot( + '/other/dir/file.dart', + resourceProvider: memoryResourceProvider, + ), + isNull, + ); + expect( + registry.findPackageRoot( + '', + resourceProvider: memoryResourceProvider, + ), + isNull, + ); + }); }); - test('HashCacheStorage invalidates cache on config change', () { - final absoluteFilePath = p.normalize( - p.join(io.Directory.current.path, 'file.dart'), - ); - final index = { - absoluteFilePath: const FileCacheEntry( - modificationStamp: 123456, - entries: [HashEntry(hash: 123, lineNumber: 10, tokenCount: 5)], - ), - }; - - final params1 = AvoidDuplicateCodeParameters.empty(); - final params2 = AvoidDuplicateCodeParameters( - minTokens: 5, - ignoreLiterals: true, - ignoreIdentifiers: false, - checkBlocks: true, - exclude: params1.exclude, - ); - - final storage1 = HashCacheStorage( - packageRoot: io.Directory.current.path, - resourceProvider: memoryResourceProvider, - currentParams: params1, - ); - - // Save with params1 - storage1.save(index); - - // Loading with params1 should succeed - final loaded1 = storage1.load(); - expect(loaded1, isNotNull); + group('cross-file duplicate matching', () { + test('findCrossFileMatches finds duplicate in other files', () { + final fileAEntries = [_TestFactory.entry(hash: 123)]; + final fileBEntries = [_TestFactory.entry(hash: 123, lineNumber: 15)]; - // Loading with params2 (different config) should return null - // (invalidated) - final storage2 = HashCacheStorage( - packageRoot: io.Directory.current.path, - resourceProvider: memoryResourceProvider, - currentParams: params2, - ); - final loaded2 = storage2.load(); - expect(loaded2, isNull); - - storage1.delete(); - }); + registry.updateFile( + 'file_a.dart', + fileAEntries, + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); - test('findCrossFileMatches processes multiple candidates correctly', () { - registry.updateFile('file_a.dart', [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ], modificationStamp: 1); + final matches = registry.findCrossFileMatches( + 'file_b.dart', + fileBEntries, + resourceProvider: memoryResourceProvider, + ); - final matches = registry.findCrossFileMatches('file_b.dart', [ - const HashEntry(hash: 123, lineNumber: 20, tokenCount: 5), // Match - const HashEntry(hash: 999, lineNumber: 30, tokenCount: 10), // No match - ]); + expect(matches, hasLength(1)); + expect(matches.first.duplicates, hasLength(1)); + final expectedPath = p.normalize( + p.join(io.Directory.current.path, 'file_a.dart'), + ); + expect(matches.first.duplicates.first.filePath, equals(expectedPath)); + expect(matches.first.duplicates.first.entry.hash, equals(123)); + expect(matches.first.duplicates.first.entry.lineNumber, equals(10)); + }); - expect(matches, hasLength(1)); - expect(matches.first.duplicates.first.entry.hash, equals(123)); - }); + test('findCrossFileMatches ignores same file', () { + final entries = [_TestFactory.entry(hash: 123)]; - test('HashCacheStorage.load returns null when cache file is missing', () { - final storage = HashCacheStorage( - packageRoot: io.Directory.current.path, - resourceProvider: memoryResourceProvider, - ); - - // Ensure any existing cache is deleted - storage.delete(); + registry.updateFile( + 'file_a.dart', + entries, + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); - final loaded = storage.load(); + final matches = registry.findCrossFileMatches( + 'file_a.dart', + entries, + resourceProvider: memoryResourceProvider, + ); - expect(loaded, isNull); - }); + expect(matches, isEmpty); + }); - test('HashCacheStorage.load returns null and does not throw when cache ' - 'file is corrupted', () { - final storage = HashCacheStorage( - packageRoot: io.Directory.current.path, - resourceProvider: memoryResourceProvider, - ); + test('findCrossFileMatches groups multiple duplicate locations', () { + registry.updateFile( + 'file_a.dart', + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); + registry.updateFile( + 'file_b.dart', + [_TestFactory.entry(hash: 123, lineNumber: 20)], + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); - final cachePath = p.normalize( - p.join( - io.Directory.current.path, - '.dart_tool', - 'solid_lints', - 'duplicate_index.json', - ), - ); - memoryResourceProvider.newFile( - cachePath, - '["invalid", "json", "structure", "not", "a", "map"]', - ); + final matches = registry.findCrossFileMatches('file_c.dart', [ + _TestFactory.entry(hash: 123, lineNumber: 30), + ], resourceProvider: memoryResourceProvider); - final loaded = storage.load(); - expect(loaded, isNull); + expect(matches, hasLength(1)); + expect(matches.first.duplicates, hasLength(2)); + final expectedPathA = p.normalize( + p.join(io.Directory.current.path, 'file_a.dart'), + ); + final expectedPathB = p.normalize( + p.join(io.Directory.current.path, 'file_b.dart'), + ); + expect(matches.first.duplicates[0].filePath, equals(expectedPathA)); + expect(matches.first.duplicates[1].filePath, equals(expectedPathB)); + }); - storage.delete(); - }); + test('findCrossFileMatches processes multiple candidates correctly', () { + registry.updateFile( + 'file_a.dart', + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); - test('AvoidDuplicateCodeParameters value equality', () { - final params1 = AvoidDuplicateCodeParameters( - minTokens: 30, - ignoreLiterals: false, - ignoreIdentifiers: true, - checkBlocks: true, - exclude: ExcludedIdentifiersListParameter( - exclude: [ - const ExcludedIdentifierParameter( - methodName: 'foo', - className: 'Bar', + final matches = registry.findCrossFileMatches('file_b.dart', [ + _TestFactory.entry(hash: 123, lineNumber: 20), // Match + _TestFactory.entry(hash: 999, lineNumber: 30, tokenCount: 10), + ], resourceProvider: memoryResourceProvider); + + expect(matches, hasLength(1)); + expect(matches.first.duplicates.first.entry.hash, equals(123)); + }); + + test( + 'does not match or clear files from sibling directories with prefixing names', + () { + final currentRoot = io.Directory.current.path; + final siblingRoot = '${currentRoot}_sibling'; + final siblingFilePath = p.normalize(p.join(siblingRoot, 'file.dart')); + final projectFilePath = p.normalize(p.join(currentRoot, 'file.dart')); + + registry.updateFile( + projectFilePath, + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); + + registry.updateFile( + siblingFilePath, + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); + + expect(registry.fileCount, equals(2)); + + // 1. findCrossFileMatches should not find duplicate in siblingFilePath + // if limited to currentRoot. + final matches = registry.findCrossFileMatches( + projectFilePath, + [_TestFactory.entry(hash: 123)], + packageRoot: currentRoot, + resourceProvider: memoryResourceProvider, + ); + expect(matches, isEmpty); + + // 2. clearEntriesForRoot should not clear siblingFilePath when + // clearing currentRoot. + final newParams = AvoidDuplicateCodeParameters( + minTokens: 40, + exclude: AvoidDuplicateCodeParameters.empty().exclude, + ); + + registry.updateFile( + projectFilePath, + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + parameters: newParams, + packageRoot: currentRoot, + resourceProvider: memoryResourceProvider, + ); + + expect( + registry.getFileEntries( + siblingFilePath, + packageRoot: siblingRoot, + resourceProvider: memoryResourceProvider, ), - ], - ), + isNotNull, + ); + }, ); + }); - final params2 = AvoidDuplicateCodeParameters( - minTokens: 30, - ignoreLiterals: false, - ignoreIdentifiers: true, - checkBlocks: true, - exclude: ExcludedIdentifiersListParameter( - exclude: [ - const ExcludedIdentifierParameter( - methodName: 'foo', - className: 'Bar', - ), - ], - ), + group('stale entry cleanup', () { + test( + 'findCrossFileMatches cleans up absolute paths of deleted files', + () { + registry.enablePhysicalFileCleanup = true; + final tempPath = p.normalize( + p.join(io.Directory.systemTemp.path, 'temp_test_file.dart'), + ); + memoryResourceProvider.newFile(tempPath, 'void main() {}'); + + registry.updateFile( + tempPath, + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); + expect(registry.fileCount, equals(1)); + + // Delete the file from the memory resource provider + memoryResourceProvider.deleteFile(tempPath); + + // Trigger matching, which should clean up the deleted tempPath + // from registry + final matches = registry.findCrossFileMatches('other_file.dart', [ + _TestFactory.entry(hash: 123), + ], resourceProvider: memoryResourceProvider); + + expect(matches, isEmpty); + expect(registry.fileCount, equals(0)); + }, ); - final paramsDifferentExclude = AvoidDuplicateCodeParameters( - minTokens: 30, - ignoreLiterals: false, - ignoreIdentifiers: true, - checkBlocks: true, - exclude: ExcludedIdentifiersListParameter( - exclude: [const ExcludedIdentifierParameter(methodName: 'different')], - ), + test( + 'findCrossFileMatches cleans up absolute paths of excluded files', + () { + final absoluteExcludedPath = p.normalize( + '/workspace/project/lib/excluded.dart', + ); + + registry.updateFile( + absoluteExcludedPath, + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + resourceProvider: memoryResourceProvider, + ); + expect(registry.fileCount, equals(1)); + + // Trigger matching with a callback that considers absoluteExcludedPath + // as excluded + final matches = registry.findCrossFileMatches( + 'other_file.dart', + [_TestFactory.entry(hash: 123)], + isFileExcluded: (path) => path == absoluteExcludedPath, + resourceProvider: memoryResourceProvider, + ); + + expect(matches, isEmpty); + expect(registry.fileCount, equals(0)); + }, ); - - expect(params1, equals(params2)); - expect(params1.hashCode, equals(params2.hashCode)); - - expect(params1, isNot(equals(paramsDifferentExclude))); }); - test( - 'does not match or clear files from sibling directories with prefixing names', - () { - final currentRoot = io.Directory.current.path; - final siblingRoot = '${currentRoot}_sibling'; - final siblingFilePath = p.normalize(p.join(siblingRoot, 'file.dart')); - final projectFilePath = p.normalize(p.join(currentRoot, 'file.dart')); - - registry.updateFile(projectFilePath, [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ], modificationStamp: 1); - - registry.updateFile(siblingFilePath, [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ], modificationStamp: 1); + group('multi-package workspaces and debounced persistence', () { + test( + 'debounces save operations independently for different package roots', + () async { + final tempDir1 = '/temp/package1'; + final tempDir2 = '/temp/package2'; + + final file1 = p.normalize(p.join(tempDir1, 'file.dart')); + final file2 = p.normalize(p.join(tempDir2, 'file.dart')); + + registry.updateFile( + file1, + [_TestFactory.entry(hash: 123)], + modificationStamp: 1, + packageRoot: tempDir1, + resourceProvider: memoryResourceProvider, + ); + + registry.updateFile( + file2, + [_TestFactory.entry(hash: 456)], + modificationStamp: 1, + packageRoot: tempDir2, + resourceProvider: memoryResourceProvider, + ); + + // Wait for debounce duration (500ms + some buffer) + await Future.delayed(const Duration(milliseconds: 600)); + + // Both caches should be saved on disk + final loaded1 = HashCacheStorage( + packageRoot: tempDir1, + resourceProvider: memoryResourceProvider, + ).load(); + final loaded2 = HashCacheStorage( + packageRoot: tempDir2, + resourceProvider: memoryResourceProvider, + ).load(); + + expect(loaded1, isNotNull); + expect(loaded1!.keys.first, equals(file1)); + + expect(loaded2, isNotNull); + expect(loaded2!.keys.first, equals(file2)); + }, + ); - expect(registry.fileCount, equals(2)); + test('uses correct package-specific parameters during debounced save ' + 'in multi-package workspace', () async { + final tempDir1 = '/temp/package1'; + final tempDir2 = '/temp/package2'; - // 1. findCrossFileMatches should not find duplicate in siblingFilePath - // if limited to currentRoot. - final matches = registry.findCrossFileMatches(projectFilePath, [ - const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5), - ], packageRoot: currentRoot); - expect(matches, isEmpty); + final file1 = p.normalize(p.join(tempDir1, 'file.dart')); + final file2 = p.normalize(p.join(tempDir2, 'file.dart')); - // 2. clearEntriesForRoot should not clear siblingFilePath when - // clearing currentRoot. - final newParams = AvoidDuplicateCodeParameters( - minTokens: 40, - ignoreLiterals: false, - ignoreIdentifiers: false, - checkBlocks: true, + final params1 = AvoidDuplicateCodeParameters( + minTokens: 30, exclude: AvoidDuplicateCodeParameters.empty().exclude, ); - registry.updateFile( - projectFilePath, - [const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5)], - modificationStamp: 1, - parameters: newParams, - packageRoot: currentRoot, - ); - - expect( - registry.getFileEntries(siblingFilePath, packageRoot: siblingRoot), - isNotNull, + final params2 = AvoidDuplicateCodeParameters( + minTokens: 40, + exclude: AvoidDuplicateCodeParameters.empty().exclude, ); - }, - ); - - test( - 'debounces save operations independently for different package roots', - () async { - final tempDir1 = '/temp/package1'; - final tempDir2 = '/temp/package2'; - - final file1 = p.normalize(p.join(tempDir1, 'file.dart')); - final file2 = p.normalize(p.join(tempDir2, 'file.dart')); registry.updateFile( file1, - [const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5)], + [_TestFactory.entry(hash: 123)], modificationStamp: 1, + parameters: params1, packageRoot: tempDir1, + resourceProvider: memoryResourceProvider, ); registry.updateFile( file2, - [const HashEntry(hash: 456, lineNumber: 10, tokenCount: 5)], + [_TestFactory.entry(hash: 456)], modificationStamp: 1, + parameters: params2, packageRoot: tempDir2, + resourceProvider: memoryResourceProvider, ); // Wait for debounce duration (500ms + some buffer) await Future.delayed(const Duration(milliseconds: 600)); - // Both caches should be saved on disk - final loaded1 = HashCacheStorage( - packageRoot: tempDir1, - resourceProvider: memoryResourceProvider, - ).load(); - final loaded2 = HashCacheStorage( - packageRoot: tempDir2, - resourceProvider: memoryResourceProvider, - ).load(); + final cachePath1 = p.normalize( + p.join(tempDir1, '.dart_tool/solid_lints/duplicate_index.json'), + ); + final cachePath2 = p.normalize( + p.join(tempDir2, '.dart_tool/solid_lints/duplicate_index.json'), + ); - expect(loaded1, isNotNull); - expect(loaded1!.keys.first, equals(file1)); + final cacheFile1 = memoryResourceProvider.getFile(cachePath1); + final cacheFile2 = memoryResourceProvider.getFile(cachePath2); - expect(loaded2, isNotNull); - expect(loaded2!.keys.first, equals(file2)); - }, - ); + expect(cacheFile1.exists, isTrue); + expect(cacheFile2.exists, isTrue); - test('uses correct package-specific parameters during debounced save ' - 'in multi-package workspace', () async { - final tempDir1 = '/temp/package1'; - final tempDir2 = '/temp/package2'; + final content1 = + jsonDecode(cacheFile1.readAsStringSync()) as Map; + final content2 = + jsonDecode(cacheFile2.readAsStringSync()) as Map; - final file1 = p.normalize(p.join(tempDir1, 'file.dart')); - final file2 = p.normalize(p.join(tempDir2, 'file.dart')); + expect(content1['config']?['min_tokens'], equals(30)); + expect(content2['config']?['min_tokens'], equals(40)); + }); + }); - final params1 = AvoidDuplicateCodeParameters( - minTokens: 30, - ignoreLiterals: false, - ignoreIdentifiers: false, - checkBlocks: true, - exclude: AvoidDuplicateCodeParameters.empty().exclude, - ); + group('HashCacheStorage', () { + test('saves and loads index correctly', () { + final absoluteFilePath = p.normalize( + p.join(io.Directory.current.path, 'file_a.dart'), + ); + final index = { + absoluteFilePath: FileCacheEntry( + modificationStamp: 123456, + entries: [_TestFactory.entry(hash: 123)], + ), + }; + + final storage = HashCacheStorage( + packageRoot: io.Directory.current.path, + resourceProvider: memoryResourceProvider, + ); - final params2 = AvoidDuplicateCodeParameters( - minTokens: 40, - ignoreLiterals: false, - ignoreIdentifiers: false, - checkBlocks: true, - exclude: AvoidDuplicateCodeParameters.empty().exclude, - ); + storage.save(index); + + final loaded = storage.load(); + expect(loaded, isNotNull); + expect(loaded!.length, equals(1)); + expect(loaded[absoluteFilePath]!.entries, hasLength(1)); + expect(loaded[absoluteFilePath]!.modificationStamp, equals(123456)); + + final entry = loaded[absoluteFilePath]!.entries.first; + expect(entry.hash, equals(123)); + expect(entry.exactHash, equals(123)); + expect(entry.lineNumber, equals(10)); + expect(entry.offset, equals(0)); + expect(entry.length, equals(0)); + expect(entry.tokenCount, equals(5)); + }); + + test('invalidates cache on config change', () { + final absoluteFilePath = p.normalize( + p.join(io.Directory.current.path, 'file.dart'), + ); + final index = { + absoluteFilePath: FileCacheEntry( + modificationStamp: 123456, + entries: [_TestFactory.entry(hash: 123)], + ), + }; + + final params1 = AvoidDuplicateCodeParameters.empty(); + final params2 = AvoidDuplicateCodeParameters( + minTokens: 5, + exclude: params1.exclude, + ); - registry.updateFile( - file1, - [const HashEntry(hash: 123, lineNumber: 10, tokenCount: 5)], - modificationStamp: 1, - parameters: params1, - packageRoot: tempDir1, - ); + final storage1 = HashCacheStorage( + packageRoot: io.Directory.current.path, + resourceProvider: memoryResourceProvider, + currentParams: params1, + ); - registry.updateFile( - file2, - [const HashEntry(hash: 456, lineNumber: 10, tokenCount: 5)], - modificationStamp: 1, - parameters: params2, - packageRoot: tempDir2, - ); + storage1.save(index); + + // Loading with params1 should succeed + final loaded1 = storage1.load(); + expect(loaded1, isNotNull); - // Wait for debounce duration (500ms + some buffer) - await Future.delayed(const Duration(milliseconds: 600)); + // Loading with params2 (different config) should return null (invalidated) + final storage2 = HashCacheStorage( + packageRoot: io.Directory.current.path, + resourceProvider: memoryResourceProvider, + currentParams: params2, + ); + final loaded2 = storage2.load(); + expect(loaded2, isNull); + }); - final cachePath1 = p.normalize( - p.join(tempDir1, '.dart_tool/solid_lints/duplicate_index.json'), - ); - final cachePath2 = p.normalize( - p.join(tempDir2, '.dart_tool/solid_lints/duplicate_index.json'), + test('load returns null when cache file is missing', () { + final storage = HashCacheStorage( + packageRoot: io.Directory.current.path, + resourceProvider: memoryResourceProvider, + ); + + expect(storage.load(), isNull); + }); + + test( + 'load returns null and does not throw when cache file is corrupted', + () { + final storage = HashCacheStorage( + packageRoot: io.Directory.current.path, + resourceProvider: memoryResourceProvider, + ); + + final cachePath = p.normalize( + p.join( + io.Directory.current.path, + '.dart_tool', + 'solid_lints', + 'duplicate_index.json', + ), + ); + memoryResourceProvider.newFile( + cachePath, + '["invalid", "json", "structure", "not", "a", "map"]', + ); + + expect(storage.load(), isNull); + }, ); + }); + + group('HashEntry', () { + test('serialization preserves exactHash', () { + final entry = _TestFactory.entry( + hash: 100, + exactHash: 200, + lineNumber: 15, + offset: 50, + length: 80, + tokenCount: 25, + ); + + final json = entry.toJson(); + expect(json['h'], equals(100)); + expect(json['e'], equals(200)); + + final restored = HashEntry.fromJson(json); + expect(restored.hash, equals(100)); + expect(restored.exactHash, equals(200)); + expect(restored.lineNumber, equals(15)); + expect(restored.offset, equals(50)); + expect(restored.length, equals(80)); + expect(restored.tokenCount, equals(25)); + }); + }); - final cacheFile1 = memoryResourceProvider.getFile(cachePath1); - final cacheFile2 = memoryResourceProvider.getFile(cachePath2); + group('AvoidDuplicateCodeParameters', () { + test('supports value equality and hashCode', () { + final params1 = AvoidDuplicateCodeParameters( + minTokens: 30, + exclude: ExcludedIdentifiersListParameter( + exclude: [ + const ExcludedIdentifierParameter( + methodName: 'foo', + className: 'Bar', + ), + ], + ), + ); - expect(cacheFile1.exists, isTrue); - expect(cacheFile2.exists, isTrue); + final params2 = AvoidDuplicateCodeParameters( + minTokens: 30, + exclude: ExcludedIdentifiersListParameter( + exclude: [ + const ExcludedIdentifierParameter( + methodName: 'foo', + className: 'Bar', + ), + ], + ), + ); - final content1 = - jsonDecode(cacheFile1.readAsStringSync()) as Map; - final content2 = - jsonDecode(cacheFile2.readAsStringSync()) as Map; + final paramsDifferent = AvoidDuplicateCodeParameters( + minTokens: 30, + exclude: ExcludedIdentifiersListParameter( + exclude: [ + const ExcludedIdentifierParameter(methodName: 'different'), + ], + ), + ); - expect(content1['config']?['min_tokens'], equals(30)); - expect(content2['config']?['min_tokens'], equals(40)); + expect(params1, equals(params2)); + expect(params1.hashCode, equals(params2.hashCode)); + expect(params1, isNot(equals(paramsDifferent))); + }); }); }); } + +abstract final class _TestFactory { + static HashEntry entry({ + required int hash, + int? exactHash, + int lineNumber = 10, + int offset = 0, + int length = 0, + int tokenCount = 5, + }) => HashEntry( + hash: hash, + exactHash: exactHash ?? hash, + lineNumber: lineNumber, + offset: offset, + length: length, + tokenCount: tokenCount, + ); +} diff --git a/test/src/lints/avoid_duplicate_code/models/cross_file_match_test.dart b/test/src/lints/avoid_duplicate_code/models/cross_file_match_test.dart new file mode 100644 index 00000000..7177e339 --- /dev/null +++ b/test/src/lints/avoid_duplicate_code/models/cross_file_match_test.dart @@ -0,0 +1,109 @@ +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/cross_file_match.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/duplicate_location.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/hash_entry.dart'; +import 'package:test/test.dart'; + +void main() { + group('CrossFileMatchIterableExtension', () { + test('toDuplicatesByHash groups matches by entry hash', () { + final match1 = CrossFileMatch( + currentEntry: _TestFactory.entry(hash: 100, offset: 0), + duplicates: [ + DuplicateLocation( + filePath: 'lib/a.dart', + entry: _TestFactory.entry(hash: 100, offset: 10), + ), + ], + ); + final match2 = CrossFileMatch( + currentEntry: _TestFactory.entry(hash: 200, offset: 50), + duplicates: [ + DuplicateLocation( + filePath: 'lib/b.dart', + entry: _TestFactory.entry(hash: 200, offset: 20), + ), + ], + ); + + final result = [match1, match2].toDuplicatesByHash(); + + expect(result.keys, containsAll([100, 200])); + expect(result[100], hasLength(1)); + expect(result[100]!.first.filePath, 'lib/a.dart'); + expect(result[200], hasLength(1)); + expect(result[200]!.first.filePath, 'lib/b.dart'); + }); + + test( + 'toDuplicatesByHash aggregates duplicates when matches share same hash', + () { + final match1 = CrossFileMatch( + currentEntry: _TestFactory.entry(hash: 100, offset: 0), + duplicates: [ + DuplicateLocation( + filePath: 'lib/a.dart', + entry: _TestFactory.entry(hash: 100, offset: 10), + ), + ], + ); + final match2 = CrossFileMatch( + currentEntry: _TestFactory.entry(hash: 100, offset: 100), + duplicates: [ + DuplicateLocation( + filePath: 'lib/b.dart', + entry: _TestFactory.entry(hash: 100, offset: 20), + ), + ], + ); + + final result = [match1, match2].toDuplicatesByHash(); + + expect(result.keys, equals([100])); + expect(result[100], hasLength(2)); + expect( + result[100]!.map((d) => d.filePath), + containsAll(['lib/a.dart', 'lib/b.dart']), + ); + }, + ); + + test('toDuplicatesByHash deduplicates identical duplicate locations', () { + final loc = DuplicateLocation( + filePath: 'lib/a.dart', + entry: _TestFactory.entry(hash: 100, offset: 10), + ); + final match1 = CrossFileMatch( + currentEntry: _TestFactory.entry(hash: 100, offset: 0), + duplicates: [loc], + ); + final match2 = CrossFileMatch( + currentEntry: _TestFactory.entry(hash: 100, offset: 50), + duplicates: [loc], + ); + + final result = [match1, match2].toDuplicatesByHash(); + + expect(result.keys, equals([100])); + expect(result[100], hasLength(1)); + expect(result[100]!.first.filePath, 'lib/a.dart'); + }); + }); +} + +abstract final class _TestFactory { + static HashEntry entry({ + required int hash, + int? exactHash, + int lineNumber = 10, + int offset = 0, + int length = 0, + int tokenCount = 5, + }) => HashEntry( + hash: hash, + exactHash: exactHash ?? hash, + lineNumber: lineNumber, + offset: offset, + length: length, + tokenCount: tokenCount, + ); +} diff --git a/test/src/lints/avoid_duplicate_code/services/differing_literals_analyzer_test.dart b/test/src/lints/avoid_duplicate_code/services/differing_literals_analyzer_test.dart new file mode 100644 index 00000000..a635945b --- /dev/null +++ b/test/src/lints/avoid_duplicate_code/services/differing_literals_analyzer_test.dart @@ -0,0 +1,350 @@ +import 'package:analyzer/file_system/memory_file_system.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/duplicate_location.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/hash_entry.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/literal_info.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/services/differing_literals_analyzer.dart'; +import 'package:test/test.dart'; + +void main() { + group('DifferingLiteralsAnalyzer', () { + late MemoryResourceProvider resourceProvider; + late DifferingLiteralsAnalyzer analyzer; + + setUp(() { + resourceProvider = MemoryResourceProvider(); + analyzer = DifferingLiteralsAnalyzer(resourceProvider: resourceProvider); + }); + + group('computeLiteralsSummary', () { + group('validation and empty inputs', () { + test('returns empty string when currentLiterals is empty', () { + expect( + analyzer.computeLiteralsSummary( + currentLiterals: [], + partnerLiteralsList: [ + _TestFactory.literals(['1', '2']), + ], + ), + isEmpty, + ); + }); + + test('returns empty string when partnerLiteralsList is empty', () { + expect( + analyzer.computeLiteralsSummary( + currentLiterals: _TestFactory.literals(['1', '2']), + partnerLiteralsList: [], + ), + isEmpty, + ); + }); + }); + + group('formatting and truncation', () { + test('returns empty string when literals are identical', () { + final current = _TestFactory.literals(['1', "'a'"]); + final partner = _TestFactory.literals(['1', "'a'"]); + + expect( + analyzer.computeLiteralsSummary( + currentLiterals: current, + partnerLiteralsList: [partner], + ), + isEmpty, + ); + }); + + test('formats single differing literal slot', () { + final current = _TestFactory.literals(['1']); + final partner = _TestFactory.literals(['2']); + + expect( + analyzer.computeLiteralsSummary( + currentLiterals: current, + partnerLiteralsList: [partner], + ), + equals(': [1, 2]'), + ); + }); + + test('deduplicates identical values from partners in slot', () { + final current = _TestFactory.literals(["'foo'"]); + final partner1 = _TestFactory.literals(["'bar'"]); + final partner2 = _TestFactory.literals(["'bar'"]); + final partner3 = _TestFactory.literals(["'baz'"]); + + expect( + analyzer.computeLiteralsSummary( + currentLiterals: current, + partnerLiteralsList: [partner1, partner2, partner3], + ), + equals(": ['foo', 'bar', 'baz']"), + ); + }); + + test('filters out identical slots and formats only differing ones', () { + final current = _TestFactory.literals(['1', "'same'", '10']); + final partner = _TestFactory.literals(['2', "'same'", '20']); + + expect( + analyzer.computeLiteralsSummary( + currentLiterals: current, + partnerLiteralsList: [partner], + ), + equals(': [1, 2], [10, 20]'), + ); + }); + + test('formats multiple differing literal slots up to limit', () { + final current = _TestFactory.literals(['1', '10', "'hello'"]); + final partner = _TestFactory.literals(['2', '20', "'foo'"]); + + expect( + analyzer.computeLiteralsSummary( + currentLiterals: current, + partnerLiteralsList: [partner], + ), + equals(": [1, 2], [10, 20], ['hello', 'foo']"), + ); + }); + + test('handles partners with fewer literals gracefully', () { + final current = _TestFactory.literals(['1', '10']); + final partnerWithFewer = _TestFactory.literals(['2']); + + expect( + analyzer.computeLiteralsSummary( + currentLiterals: current, + partnerLiteralsList: [partnerWithFewer], + ), + equals(': [1, 2]'), + ); + }); + + test('truncates slots when exceeding max displayed slots limit', () { + final current = _TestFactory.literals([ + '1', + '10', + "'hello'", + "'world'", + ]); + final partner = _TestFactory.literals(['2', '20', "'foo'", "'bar'"]); + + expect( + analyzer.computeLiteralsSummary( + currentLiterals: current, + partnerLiteralsList: [partner], + ), + equals(": [1, 2], [10, 20], ['hello', 'foo'] (+1 more)"), + ); + }); + + test('truncates values within slot when exceeding limit', () { + final current = _TestFactory.literals(['1']); + final partner1 = _TestFactory.literals(['2']); + final partner2 = _TestFactory.literals(['3']); + final partner3 = _TestFactory.literals(['4']); + + expect( + analyzer.computeLiteralsSummary( + currentLiterals: current, + partnerLiteralsList: [partner1, partner2, partner3], + ), + equals(': [1, 2, 3, +1 more]'), + ); + }); + }); + }); + + group('loadExternalLiterals', () { + group('extraction', () { + test('extracts literals when snippet is at non-zero offset', () { + const filePath = '/test/lib/sample_offset.dart'; + const fileContent = '// comment\nvoid foo() => 123;'; + resourceProvider.newFile(filePath, fileContent); + + final location = _TestFactory.location( + filePath, + offset: 22, + length: 6, + ); + + final lits = analyzer.loadExternalLiterals(location); + + expect(lits, isNotNull); + expect(lits!.map((l) => l.text), equals(['123'])); + }); + }); + + group('validation and error handling', () { + test('returns null if file does not exist', () { + final location = _TestFactory.location( + '/non/existent.dart', + length: 10, + ); + + final lits = analyzer.loadExternalLiterals(location); + + expect(lits, isNull); + }); + + test('returns null if file content is empty', () { + const filePath = '/test/lib/sample_empty.dart'; + resourceProvider.newFile(filePath, ''); + + final location = _TestFactory.location(filePath, length: 0); + + final lits = analyzer.loadExternalLiterals(location); + + expect(lits, isNull); + }); + + test('returns null for zero or negative length', () { + const filePath = '/test/lib/sample_zero.dart'; + resourceProvider.newFile(filePath, 'void foo() {}'); + + final location = _TestFactory.location(filePath); + + final lits = analyzer.loadExternalLiterals(location); + + expect(lits, isNull); + }); + + test('returns null when offset is negative', () { + const filePath = '/test/lib/sample_neg.dart'; + resourceProvider.newFile(filePath, 'void foo() => 1;'); + + final location = _TestFactory.location( + filePath, + offset: -1, + length: 5, + ); + + final lits = analyzer.loadExternalLiterals(location); + + expect(lits, isNull); + }); + + test( + 'returns null when offset + length exceeds file content length', + () { + const filePath = '/test/lib/sample_overflow.dart'; + resourceProvider.newFile(filePath, 'void foo() => 1;'); + + final location = _TestFactory.location( + filePath, + offset: 10, + length: 100, + ); + + final lits = analyzer.loadExternalLiterals(location); + + expect(lits, isNull); + }, + ); + }); + + group('caching', () { + test('caches file content across multiple calls for same file', () { + const filePath = '/test/lib/sample_cached.dart'; + const fileContent = 'void foo() => 42;'; + resourceProvider.newFile(filePath, fileContent); + + final location = _TestFactory.location( + filePath, + offset: 0, + length: fileContent.length, + ); + + final first = analyzer.loadExternalLiterals(location); + expect(first, isNotNull); + + // Delete file from resource provider; cache should serve the content. + resourceProvider.deleteFile(filePath); + + final second = analyzer.loadExternalLiterals(location); + expect(second, isNotNull); + expect(second!.map((l) => l.text), equals(['42'])); + }); + }); + + group('body prefixes and snippet wrapping', () { + for (final (label, snippet, expected) in [ + ( + 'block body with {', + '{\n final x = 42;\n print("hi");\n}', + ['42', '"hi"'], + ), + ( + 'async block body with async {', + 'async {\n final x = 42;\n print("hi");\n}', + ['42', '"hi"'], + ), + ( + 'async* block body with async* {', + 'async* {\n final x = 42;\n yield "hi";\n}', + ['42', '"hi"'], + ), + ( + 'sync* block body with sync* {', + 'sync* {\n final x = 42;\n yield "hi";\n}', + ['42', '"hi"'], + ), + ('arrow body with =>', '=> 42;', ['42']), + ('async arrow body with async =>', 'async => 42;', ['42']), + ('async* arrow body with async* =>', 'async* => 42;', ['42']), + ('sync* arrow body with sync* =>', 'sync* => 42;', ['42']), + ('raw statement block without body prefix', 'final x = 42;', ['42']), + ]) { + test('loads literals from snippet with $label', () { + final filePath = '/test/lib/sample_$label.dart'; + resourceProvider.newFile(filePath, snippet); + + final location = _TestFactory.location( + filePath, + length: snippet.length, + ); + + final lits = analyzer.loadExternalLiterals(location); + + expect(lits, isNotNull); + expect(lits!.map((l) => l.text), equals(expected)); + }); + } + }); + }); + }); +} + +abstract final class _TestFactory { + static LiteralInfo literal(String text, {int offset = 0, int length = 0}) => + LiteralInfo(offset: offset, length: length, text: text); + + static List literals(List texts) => + texts.map(literal).toList(); + + static HashEntry entry({ + int hash = 123, + int exactHash = 456, + int lineNumber = 1, + int offset = 0, + int length = 0, + int tokenCount = 4, + }) => HashEntry( + hash: hash, + exactHash: exactHash, + lineNumber: lineNumber, + offset: offset, + length: length, + tokenCount: tokenCount, + ); + + static DuplicateLocation location( + String filePath, { + int offset = 0, + int length = 0, + }) => DuplicateLocation( + filePath: filePath, + entry: entry(offset: offset, length: length), + ); +} diff --git a/test/src/lints/avoid_duplicate_code/visitors/ast_structural_hash_visitor_test.dart b/test/src/lints/avoid_duplicate_code/visitors/ast_structural_hash_visitor_test.dart new file mode 100644 index 00000000..f60b49d2 --- /dev/null +++ b/test/src/lints/avoid_duplicate_code/visitors/ast_structural_hash_visitor_test.dart @@ -0,0 +1,317 @@ +import 'package:analyzer/dart/analysis/results.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer_testing/src/analysis_rule/pub_package_resolution.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/visitors/ast_structural_hash_visitor.dart'; +import 'package:test/test.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +void main() { + defineReflectiveSuite(() { + defineReflectiveTests(AstStructuralHashVisitorTest); + }); +} + +typedef _Hashes = ({int structuralHash, int exactHash}); + +@reflectiveTest +class AstStructuralHashVisitorTest extends PubPackageResolutionTest { + Future + test_named_arguments_do_not_pollute_local_variable_indexing() async { + final (:fn1, :fn2) = await _computeHashes(''' +void foo({required int count, required int delay}) {} + +void fn1() { + final a = 1; + foo(count: 10, delay: 20); + final b = 2; + print(a + b); +} + +void fn2() { + final x = 1; + foo(count: 10, delay: 20); + final y = 2; + print(x + y); +} +'''); + + expect(fn1.structuralHash, fn2.structuralHash); + expect(fn1.exactHash, fn2.exactHash); + } + + Future test_local_variable_renaming_has_same_structural_hash() async { + final (:fn1, :fn2) = await _computeHashes(''' +int fn1() { + final a = 10; + final b = 20; + return a + b; +} + +int fn2() { + final x = 10; + final y = 20; + return x + y; +} +'''); + + expect(fn1.structuralHash, fn2.structuralHash); + expect(fn1.exactHash, fn2.exactHash); + } + + Future + test_formal_parameters_renaming_has_same_structural_hash() async { + final (:fn1, :fn2) = await _computeHashes(''' +int fn1(int a, int b) => a + b; +int fn2(int x, int y) => x + y; +'''); + + expect(fn1.structuralHash, fn2.structuralHash); + expect(fn1.exactHash, fn2.exactHash); + } + + Future test_different_variable_wiring_has_different_hashes() async { + final (:fn1, :fn2) = await _computeHashes(''' +int fn1() { + final a = 10; + final b = 20; + print(a); + print(b); + return a - b; +} + +int fn2() { + final a = 10; + final b = 20; + print(a); + print(b); + return b - a; +} +'''); + + expect(fn1.structuralHash, isNot(fn2.structuralHash)); + } + + Future + test_pattern_variables_renaming_has_same_structural_hash() async { + final (:fn1, :fn2) = await _computeHashes(''' +void fn1((int, int) pair) { + if (pair case (final a, final b)) { + print(a + b); + } +} + +void fn2((int, int) pair) { + if (pair case (final x, final y)) { + print(x + y); + } +} +'''); + + expect(fn1.structuralHash, fn2.structuralHash); + expect(fn1.exactHash, fn2.exactHash); + } + + Future + test_different_field_or_method_names_have_different_hashes() async { + final (:fn1, :fn2) = await _computeHashes(''' +class User { + String name = ''; + int age = 0; + void save() {} + void delete() {} +} + +void fn1(User user) { + user.save(); + print(user.name); +} + +void fn2(User user) { + user.delete(); + print(user.age); +} +'''); + + expect(fn1.structuralHash, isNot(fn2.structuralHash)); + } + + Future + test_string_literals_have_same_structural_but_different_exact_hash() async { + final (:fn1, :fn2) = await _computeHashes(''' +void fn1() { + final a = 'hello'; + print(a); +} + +void fn2() { + final a = 'world'; + print(a); +} +'''); + + expect(fn1.structuralHash, fn2.structuralHash); + expect(fn1.exactHash, isNot(fn2.exactHash)); + } + + Future + test_boolean_literals_have_same_structural_but_different_exact_hash() async { + final (:fn1, :fn2) = await _computeHashes(''' +void fn1() { + final a = true; + print(a); +} + +void fn2() { + final a = false; + print(a); +} +'''); + + expect(fn1.structuralHash, fn2.structuralHash); + expect(fn1.exactHash, isNot(fn2.exactHash)); + } + + Future + test_symbol_literals_have_same_structural_but_different_exact_hash() async { + final (:fn1, :fn2) = await _computeHashes(''' +void fn1() { + final a = #foo; + print(a); +} + +void fn2() { + final a = #bar; + print(a); +} +'''); + + expect(fn1.structuralHash, fn2.structuralHash); + expect(fn1.exactHash, isNot(fn2.exactHash)); + } + + Future + test_negative_and_positive_numbers_have_same_structural_hash() async { + final (:fn1, :fn2) = await _computeHashes(''' +void fn1() { + final a = -5; + print(a); +} + +void fn2() { + final a = 5; + print(a); +} +'''); + + expect(fn1.structuralHash, fn2.structuralHash); + expect(fn1.exactHash, isNot(fn2.exactHash)); + } + + Future + test_different_negative_numbers_have_same_structural_but_different_exact_hash() async { + final (:fn1, :fn2) = await _computeHashes(''' +void fn1() { + final a = -10.5; + print(a); +} + +void fn2() { + final a = -20.5; + print(a); +} +'''); + + expect(fn1.structuralHash, fn2.structuralHash); + expect(fn1.exactHash, isNot(fn2.exactHash)); + } + + Future test_variable_keywords_have_different_structural_hashes() async { + final (:fn1, :fn2) = await _computeHashes(''' +void fn1() { + final a = 10; + print(a); +} + +void fn2() { + var a = 10; + print(a); +} +'''); + + expect(fn1.structuralHash, isNot(fn2.structuralHash)); + } + + Future + test_if_with_else_and_without_else_have_different_structural_hashes() async { + final (:fn1, :fn2) = await _computeHashes(''' +void fn1(bool condition) { + if (condition) { + print(1); + } else { + print(2); + } +} + +void fn2(bool condition) { + if (condition) { + print(1); + } + print(2); +} +'''); + + expect(fn1.structuralHash, isNot(fn2.structuralHash)); + } + + Future + test_is_and_is_not_expressions_have_different_structural_hashes() async { + final (:fn1, :fn2) = await _computeHashes(''' +void fn1(Object x) { + if (x is int) { + print(x); + } +} + +void fn2(Object x) { + if (x is! int) { + print(x); + } +} +'''); + + expect(fn1.structuralHash, isNot(fn2.structuralHash)); + } + + Future + test_different_binary_operators_have_different_structural_hashes() async { + final (:fn1, :fn2) = await _computeHashes(''' +int fn1(int a, int b) => a + b; +int fn2(int a, int b) => a * b; +'''); + + expect(fn1.structuralHash, isNot(fn2.structuralHash)); + } + + Future<({_Hashes fn1, _Hashes fn2})> _computeHashes( + String source, { + String fn1Name = 'fn1', + String fn2Name = 'fn2', + }) async { + final file = await _resolveSource(source); + final declarations = file.unit.declarations + .whereType(); + final fn1 = declarations.firstWhere((d) => d.name.lexeme == fn1Name); + final fn2 = declarations.firstWhere((d) => d.name.lexeme == fn2Name); + + final hasher = AstStructuralHashVisitor(); + return ( + fn1: hasher.computeHashes(fn1.functionExpression.body), + fn2: hasher.computeHashes(fn2.functionExpression.body), + ); + } + + Future _resolveSource(String source) async { + newFile(testFile.path, source); + return resolveFile(testFile.path); + } +} diff --git a/test/src/lints/avoid_duplicate_code/visitors/literal_collector_visitor_test.dart b/test/src/lints/avoid_duplicate_code/visitors/literal_collector_visitor_test.dart new file mode 100644 index 00000000..a79aa221 --- /dev/null +++ b/test/src/lints/avoid_duplicate_code/visitors/literal_collector_visitor_test.dart @@ -0,0 +1,116 @@ +import 'package:analyzer/dart/analysis/utilities.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/models/literal_info.dart'; +import 'package:solid_lints/src/lints/avoid_duplicate_code/visitors/literal_collector_visitor.dart'; +import 'package:test/test.dart'; + +void main() { + group('LiteralCollectorVisitor', () { + test( + 'collects signed and unsigned number, string, boolean, symbol literals', + () { + final literals = _collect(''' +void test() { + final a = 10; + final b = -20; + final c = 3.14; + final d = -1.5; + final e = 'hello'; + final f = true; + final g = false; + final h = #mySymbol; +} +'''); + + expect(literals.map((l) => l.text).toList(), [ + '10', + '-20', + '3.14', + '-1.5', + "'hello'", + 'true', + 'false', + '#mySymbol', + ]); + }, + ); + + test('collects string parts of string interpolation', () { + final literals = _collect(r''' +void test(String name, String category) { + final msg = 'Hello $name, welcome to $category!'; +} +'''); + + expect(literals.map((l) => l.text).toList(), [ + "'Hello '", + "', welcome to '", + "'!'", + ]); + }); + + test( + 'collects literals inside collections and function call arguments', + () { + final literals = _collect(''' +void test() { + final list = [1, 'two']; + final map = {'key': 3}; + print(4, true); +} +'''); + + expect(literals.map((l) => l.text).toList(), [ + '1', + "'two'", + "'key'", + '3', + '4', + 'true', + ]); + }, + ); + + test('collects boolean literal inside not expression', () { + final literals = _collect(''' +void test(bool flag) { + final a = !true; + final b = !flag; +} +'''); + + expect(literals.map((l) => l.text).toList(), ['true']); + }); + + test('records accurate source offset and length for each literal', () { + const content = ''' +void test() { + final count = 42; + final name = 'solid'; +} +'''; + final literals = _collect(content); + + expect(literals, hasLength(2)); + for (final literal in literals) { + final snippet = content.substring( + literal.offset, + literal.offset + literal.length, + ); + expect(snippet, literal.text); + } + }); + + test('returns empty list when no literals are present', () { + final literals = _collect(''' +void test(int a, int b) { + final sum = a + b; +} +'''); + + expect(literals, isEmpty); + }); + }); +} + +List _collect(String content) => + LiteralCollectorVisitor.collect(parseString(content: content).unit);