feat(verify): report a deleted subject as removed, not resolved #10

Merged
trevon merged 1 commit from feat/verify-removed into main 2026-09-25 13:34:02 +00:00
5 changed files with 250 additions and 1 deletions
Showing only changes of commit 4f3f22acad - Show all commits

feat(verify): report a deleted subject as removed, not resolved
Some checks failed
Lint Commits / commitlint (pull_request) Successful in 15s
Tests / Static Analysis (pull_request) Has been cancelled
Tests / Tests (pull_request) Has been cancelled

Found by dogfooding on nuwayze main: `pipe verify` against the ratchet
baseline printed

  resolved  1 no longer reported - these fixes are confirmed
    + service_browse_bloc.dart:20  LoadServicesByArtist  [dead-handler]

but nothing was fixed. nuwayze commit ccca9bd1e ("delete dead firestore
query code") deleted the event and its handler. Under pipe's default
wire-up policy, deleting an unreachable handler is the exception that
needs approval, so calling it a confirmed fix overstated what happened.

verify now separates the two. A gone finding is REMOVED when its
subject no longer exists:
- its file is not among the project's files;
- for rules labelled by a declaration (the event/state/union/provider or
  owner class), that name is no longer declared anywhere;
- for dead-handler, the on<Event> registration itself is gone.
Everything else stays RESOLVED. Member-labelled rules (a method or
build) are classed by file only, never guessed. Removed entries still
count as gone for --expect-resolved; they are only named honestly, in
text ("removed   N gone because the code was deleted - not wired up",
a REMOVED section) and JSON (removed, removedEntries).

On nuwayze main the entry now reads REMOVED. Tests: a unit test over
hand-built entries (a wired handler stays resolved, a deleted handler
and a deleted file are removed, a member label is not guessed), and an
end-to-end test that deletes a fixture's dead handler and runs
verifyAnalysis. 216 tests pass.
trvon 2026-09-24 20:04:52 -06:00
Signed by: trevon
GPG key ID: 38686EA797D7A8A7

View file

@ -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<Event>` 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,

View file

@ -117,6 +117,39 @@ UnresolvedReferences findUnresolvedReferences(List<FileOutcome> 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<String> declaredNamesOf(List<FileOutcome> outcomes) {
final names = <String>{};
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<void> {
final Set<String> names = {};

View file

@ -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<BaselineEntry> 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<BaselineEntry> removed;
/// Accepted findings that are still reported.
final List<BaselineEntry> 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<String, Object?> 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<String> _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<BaselineEntry> baseline,
Set<String>? rules,
Set<String> unscannedFiles = const {},
List<String>? scannedDirectories,
Set<String>? existingFiles,
Set<String>? declaredNames,
Set<String>? 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 = <BaselineEntry>[];
final resolved = <BaselineEntry>[];
final removed = <BaselineEntry>[];
final stillPresent = <BaselineEntry>[];
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,
},
);
}

View file

@ -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<NeverDispatched>((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();
}
});
}

View file

@ -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);
});
}