feat(verify): report a deleted subject as removed, not resolved #10
5 changed files with 250 additions and 1 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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 = {};
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
},
|
||||
);
|
||||
}
|
||||
|
|
|
|||
69
test/verify_removed_test.dart
Normal file
69
test/verify_removed_test.dart
Normal 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();
|
||||
}
|
||||
});
|
||||
}
|
||||
|
|
@ -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);
|
||||
});
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue