diff --git a/docs/usage.md b/docs/usage.md index a263e6f..5435340 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -119,6 +119,14 @@ was not scanned. `verify` only calls a finding resolved when it looked for it: - a scan with a file that failed to resolve, or truncated by `--limit`, is refused. +A finding can also go away because its subject was deleted instead of fixed. +Those are listed as **REMOVED** ("gone because the code was deleted - not wired +up"), not as confirmed fixes: the finding's file is gone, its event, state, +provider or owner class is no longer declared, or, for a dead handler, the +`on` registration itself was deleted. Removed entries still count as gone +for `--expect-resolved`. Findings labelled by a member (a method or `build`) are +classed by file only. + ## Fix order `pipe suggest` collapses findings to root causes (one action per union, diff --git a/lib/src/analysis.dart b/lib/src/analysis.dart index a1c82ab..91cc728 100644 --- a/lib/src/analysis.dart +++ b/lib/src/analysis.dart @@ -117,6 +117,39 @@ UnresolvedReferences findUnresolvedReferences(List outcomes) { ); } +/// Every type, enum constant and top-level name declared in the resolved units. +/// `verify` uses it to tell a finding that went away because its subject was +/// deleted from one that went away because the code was fixed. +Set declaredNamesOf(List outcomes) { + final names = {}; + for (final outcome in outcomes) { + final unit = outcome.unit; + if (unit == null) continue; + for (final declaration in unit.unit.declarations) { + switch (declaration) { + case ClassDeclaration(): + names.add(declaration.namePart.typeName.lexeme); + case MixinDeclaration(): + names.add(declaration.name.lexeme); + case EnumDeclaration(): + names.add(declaration.namePart.typeName.lexeme); + for (final constant in declaration.body.constants) { + names.add(constant.name.lexeme); + } + case FunctionDeclaration(): + names.add(declaration.name.lexeme); + case TopLevelVariableDeclaration(): + for (final variable in declaration.variables.variables) { + names.add(variable.name.lexeme); + } + default: + break; + } + } + } + return names; +} + class _Names extends RecursiveAstVisitor { final Set names = {}; diff --git a/lib/src/verify.dart b/lib/src/verify.dart index 75ae5b6..e329c78 100644 --- a/lib/src/verify.dart +++ b/lib/src/verify.dart @@ -1,5 +1,6 @@ import 'package:path/path.dart' as p; +import 'analysis.dart'; import 'pipeline.dart'; import 'rules.dart'; @@ -16,6 +17,7 @@ class VerificationReport { required this.newFindings, this.outOfScope = 0, this.notScanned = const [], + this.removed = const [], }); /// Accepted findings of the rules that ran, i.e. the ones this report judges. @@ -25,9 +27,16 @@ class VerificationReport { /// neither resolved nor still present: nothing looked for them. final int outOfScope; - /// Accepted findings that are no longer reported. + /// Accepted findings that are no longer reported while their subject still + /// exists: the code was changed, and the finding went away. final List resolved; + /// Accepted findings that are gone because their subject was deleted - the + /// file, the event or state class, the provider, or the handler. The finding + /// no longer applies, but under the default wire-up policy a deletion is not + /// the fix it asked for, so it is reported apart from [resolved]. + final List removed; + /// Accepted findings that are still reported. final List stillPresent; @@ -43,16 +52,20 @@ class VerificationReport { /// True only when every accepted finding was looked for and is gone: an /// accepted finding in an unscanned file is not a confirmed fix. bool get allResolved => stillPresent.isEmpty && notScanned.isEmpty; + // Removed entries are gone too, so they do not hold `--expect-resolved` back; + // they are only named differently. Map toJson() => { 'baselineTotal': baselineTotal, 'outOfScope': outOfScope, 'resolved': resolved.length, + 'removed': removed.length, 'stillPresent': stillPresent.length, 'notScanned': notScanned.length, 'new': newFindings.length, 'allResolved': allResolved, 'resolvedEntries': [for (final entry in resolved) entry.toJson()], + 'removedEntries': [for (final entry in removed) entry.toJson()], 'stillPresentEntries': [for (final entry in stillPresent) entry.toJson()], 'notScannedEntries': [for (final entry in notScanned) entry.toJson()], 'newFindings': [for (final finding in newFindings) finding.toJson()], @@ -71,6 +84,12 @@ class VerificationReport { 'resolved ${resolved.length} no longer reported' '${resolved.isEmpty ? '' : ' — these fixes are confirmed'}', ) + ..write( + removed.isEmpty + ? '' + : 'removed ${removed.length} gone because the code was deleted - ' + 'not wired up\n', + ) ..writeln('still ${stillPresent.length} still reported') ..write( notScanned.isEmpty @@ -88,6 +107,15 @@ class VerificationReport { ); } } + if (removed.isNotEmpty) { + buffer.writeln('REMOVED'); + for (final entry in removed.take(top)) { + buffer.writeln( + ' x ${entry.location} ${entry.label}' + ' [${entry.rule}]', + ); + } + } if (stillPresent.isNotEmpty) { buffer.writeln('STILL PRESENT'); for (final entry in stillPresent.take(top)) { @@ -134,6 +162,21 @@ String? baselineEntryFile(BaselineEntry entry) { return parts.length > 1 && parts[1].isNotEmpty ? parts[1] : null; } +/// Rules whose baseline label names a declaration - an event or state class, a +/// provider, or the owning class - so its absence from the code means the +/// subject was deleted. The other rules label a member (a method or `build`), +/// which the declaration check cannot see, so they are never classed as +/// removed on that basis. +const Set _declarationLabelledRules = { + ruleEmittedNotRendered, + ruleRenderedNotEmitted, + ruleEventWithoutHandler, + ruleDeadHandler, + ruleRiverpodAsyncGap, + ruleNotifierInert, + ruleNotifierFieldUnread, +}; + /// Compare a fresh analysis against the accepted baseline. /// /// Self-contained: it derives new findings by comparing fingerprints, so it does @@ -146,13 +189,43 @@ String? baselineEntryFile(BaselineEntry entry) { /// An accepted finding in a file that was not analysed - one of /// [unscannedFiles], or outside every one of [scannedDirectories] - is reported /// as not scanned. Its absence is not a fix. +/// +/// A gone finding whose subject no longer exists - its file is not among +/// [existingFiles], its declaration is not among [declaredNames], or, for a +/// dead handler, its event is not among [handledEvents] - is reported as +/// removed rather than resolved. Each check runs only when its set is given. VerificationReport verifyAgainstBaseline({ required FindingReport report, required List baseline, Set? rules, Set unscannedFiles = const {}, List? scannedDirectories, + Set? existingFiles, + Set? declaredNames, + Set? handledEvents, }) { + bool subjectRemoved(BaselineEntry entry) { + final file = baselineEntryFile(entry); + if (existingFiles != null && + file != null && + !existingFiles.contains(file)) { + return true; + } + final rule = baselineEntryRule(entry); + // A grouped label reads "Union [group]"; the declaration is the union. + final bracket = entry.label.indexOf(' ['); + final subject = bracket < 0 + ? entry.label + : entry.label.substring(0, bracket); + if (rule == ruleDeadHandler && handledEvents != null) { + return !handledEvents.contains(subject); + } + if (declaredNames != null && _declarationLabelledRules.contains(rule)) { + return !declaredNames.contains(subject); + } + return false; + } + final directories = scannedDirectories; bool scanned(BaselineEntry entry) { final file = baselineEntryFile(entry); @@ -182,12 +255,15 @@ VerificationReport verifyAgainstBaseline({ final accepted = {for (final entry in baseline) entry.fingerprint}; final notScanned = []; final resolved = []; + final removed = []; final stillPresent = []; for (final entry in inScope) { if (present.contains(entry.fingerprint)) { stillPresent.add(entry); } else if (!scanned(entry)) { notScanned.add(entry); + } else if (subjectRemoved(entry)) { + removed.add(entry); } else { resolved.add(entry); } @@ -196,6 +272,7 @@ VerificationReport verifyAgainstBaseline({ baselineTotal: inScope.length, outOfScope: baseline.length - inScope.length, resolved: resolved, + removed: removed, stillPresent: stillPresent, notScanned: notScanned, newFindings: [ @@ -230,5 +307,12 @@ VerificationReport verifyAnalysis({ rules: rules, unscannedFiles: unscanned, scannedDirectories: project.directories, + existingFiles: { + for (final path in project.dartFiles) project.relative(path), + }, + declaredNames: declaredNamesOf(analysis.run.outcomes), + handledEvents: { + for (final handler in analysis.flow.handlers) handler.event, + }, ); } diff --git a/test/verify_removed_test.dart b/test/verify_removed_test.dart new file mode 100644 index 0000000..a03ede2 --- /dev/null +++ b/test/verify_removed_test.dart @@ -0,0 +1,69 @@ +import 'dart:convert'; +import 'dart:io'; + +import 'package:pipe/pipe.dart'; +import 'package:test/test.dart'; + +/// A copy of the fixture, so deleting code there cannot touch the real one. +String _fixtureCopy() { + final from = Directory('test/fixtures/stuck_loading'); + final to = Directory.systemTemp.createTempSync('pipe-verify-removed'); + for (final entity in from.listSync(recursive: true)) { + final relative = entity.path.substring(from.path.length + 1); + final target = '${to.path}/$relative'; + if (entity is Directory) { + Directory(target).createSync(recursive: true); + } else if (entity is File) { + File(target) + ..createSync(recursive: true) + ..writeAsBytesSync(entity.readAsBytesSync()); + } + } + return to.path; +} + +void main() { + test('verify reports a deleted dead handler as removed', () async { + final root = _fixtureCopy(); + final before = await analyzeProject( + await loadProject(root), + concurrency: 1, + ); + final baseline = decodeBaselineEntries( + encodeBaseline(root, before.report.findings), + ); + await before.dispose(); + expect( + baseline.where( + (e) => e.rule == ruleDeadHandler && e.label == 'NeverDispatched', + ), + hasLength(1), + ); + + // Delete the dead handler instead of wiring it up. + final gaps = File('$root/lib/gaps.dart'); + gaps.writeAsStringSync( + gaps.readAsStringSync().replaceFirst( + 'on((event, emit) => emit(const GapIdle()));', + '', + ), + ); + + final after = await analyzeProject(await loadProject(root), concurrency: 1); + try { + final verification = verifyAnalysis(analysis: after, baseline: baseline); + final json = jsonDecode(jsonEncode(verification.toJson())) as Map; + expect( + verification.removed.map((e) => e.label), + contains('NeverDispatched'), + ); + expect( + verification.resolved.map((e) => e.label), + isNot(contains('NeverDispatched')), + ); + expect(json['removed'], greaterThanOrEqualTo(1)); + } finally { + await after.dispose(); + } + }); +} diff --git a/test/verify_test.dart b/test/verify_test.dart index d5876a4..4b490a2 100644 --- a/test/verify_test.dart +++ b/test/verify_test.dart @@ -219,4 +219,59 @@ void main() { expect(verification.toJson(), containsPair('notScanned', 2)); expect(verification.describe(), contains('NOT SCANNED')); }); + + test('a finding whose subject was deleted is removed, not resolved', () { + // nuwayze deleted a dead handler's event ("delete dead firestore query + // code") and verify printed it under "these fixes are confirmed". Under the + // default wire-up policy a deletion is not the fix the finding asked for. + Finding dead(String event, String file) => _finding( + rule: 'dead-handler', + file: file, + line: 5, + data: {'union': 'E', 'owner': 'B', 'event': event}, + ); + final wired = dead('Wired', 'lib/bloc.dart'); + final deletedHandler = dead('HandlerGone', 'lib/bloc.dart'); + final deletedFile = _finding( + rule: 'loading-flag-not-reset', + file: 'lib/gone.dart', + line: 3, + data: const {'owner': 'C', 'member': 'load'}, + ); + final memberFixed = _finding( + rule: 'loading-flag-not-reset', + file: 'lib/bloc.dart', + line: 9, + data: const {'owner': 'C', 'member': 'refresh'}, + ); + final baseline = decodeBaselineEntries( + encodeBaseline('/root', [ + wired, + deletedHandler, + deletedFile, + memberFixed, + ]), + ); + + final verification = verifyAgainstBaseline( + report: FindingReport( + root: '/root', + findings: const [], + suppressed: const [], + ), + baseline: baseline, + existingFiles: const {'lib/bloc.dart'}, + declaredNames: const {'Wired', 'HandlerGone', 'B', 'C'}, + handledEvents: const {'Wired'}, + ); + + // Wired still has its handler and is simply no longer reported: a fix. + // refresh is a member label, which the declaration check cannot judge, so + // it stays resolved rather than being guessed either way. + expect(verification.resolved.map((e) => e.label), {'Wired', 'refresh'}); + expect(verification.removed.map((e) => e.label), {'HandlerGone', 'load'}); + expect(verification.allResolved, isTrue, reason: 'removed is still gone'); + expect(verification.describe(), contains('REMOVED')); + expect(verification.toJson()['removed'], 2); + }); }