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
|
- a scan with a file that failed to resolve, or truncated by `--limit`, is
|
||||||
refused.
|
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
|
## Fix order
|
||||||
|
|
||||||
`pipe suggest` collapses findings to root causes (one action per union,
|
`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> {
|
class _Names extends RecursiveAstVisitor<void> {
|
||||||
final Set<String> names = {};
|
final Set<String> names = {};
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,6 @@
|
||||||
import 'package:path/path.dart' as p;
|
import 'package:path/path.dart' as p;
|
||||||
|
|
||||||
|
import 'analysis.dart';
|
||||||
import 'pipeline.dart';
|
import 'pipeline.dart';
|
||||||
import 'rules.dart';
|
import 'rules.dart';
|
||||||
|
|
||||||
|
|
@ -16,6 +17,7 @@ class VerificationReport {
|
||||||
required this.newFindings,
|
required this.newFindings,
|
||||||
this.outOfScope = 0,
|
this.outOfScope = 0,
|
||||||
this.notScanned = const [],
|
this.notScanned = const [],
|
||||||
|
this.removed = const [],
|
||||||
});
|
});
|
||||||
|
|
||||||
/// Accepted findings of the rules that ran, i.e. the ones this report judges.
|
/// 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.
|
/// neither resolved nor still present: nothing looked for them.
|
||||||
final int outOfScope;
|
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;
|
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.
|
/// Accepted findings that are still reported.
|
||||||
final List<BaselineEntry> stillPresent;
|
final List<BaselineEntry> stillPresent;
|
||||||
|
|
||||||
|
|
@ -43,16 +52,20 @@ class VerificationReport {
|
||||||
/// True only when every accepted finding was looked for and is gone: an
|
/// True only when every accepted finding was looked for and is gone: an
|
||||||
/// accepted finding in an unscanned file is not a confirmed fix.
|
/// accepted finding in an unscanned file is not a confirmed fix.
|
||||||
bool get allResolved => stillPresent.isEmpty && notScanned.isEmpty;
|
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() => {
|
Map<String, Object?> toJson() => {
|
||||||
'baselineTotal': baselineTotal,
|
'baselineTotal': baselineTotal,
|
||||||
'outOfScope': outOfScope,
|
'outOfScope': outOfScope,
|
||||||
'resolved': resolved.length,
|
'resolved': resolved.length,
|
||||||
|
'removed': removed.length,
|
||||||
'stillPresent': stillPresent.length,
|
'stillPresent': stillPresent.length,
|
||||||
'notScanned': notScanned.length,
|
'notScanned': notScanned.length,
|
||||||
'new': newFindings.length,
|
'new': newFindings.length,
|
||||||
'allResolved': allResolved,
|
'allResolved': allResolved,
|
||||||
'resolvedEntries': [for (final entry in resolved) entry.toJson()],
|
'resolvedEntries': [for (final entry in resolved) entry.toJson()],
|
||||||
|
'removedEntries': [for (final entry in removed) entry.toJson()],
|
||||||
'stillPresentEntries': [for (final entry in stillPresent) entry.toJson()],
|
'stillPresentEntries': [for (final entry in stillPresent) entry.toJson()],
|
||||||
'notScannedEntries': [for (final entry in notScanned) entry.toJson()],
|
'notScannedEntries': [for (final entry in notScanned) entry.toJson()],
|
||||||
'newFindings': [for (final finding in newFindings) finding.toJson()],
|
'newFindings': [for (final finding in newFindings) finding.toJson()],
|
||||||
|
|
@ -71,6 +84,12 @@ class VerificationReport {
|
||||||
'resolved ${resolved.length} no longer reported'
|
'resolved ${resolved.length} no longer reported'
|
||||||
'${resolved.isEmpty ? '' : ' — these fixes are confirmed'}',
|
'${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')
|
..writeln('still ${stillPresent.length} still reported')
|
||||||
..write(
|
..write(
|
||||||
notScanned.isEmpty
|
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) {
|
if (stillPresent.isNotEmpty) {
|
||||||
buffer.writeln('STILL PRESENT');
|
buffer.writeln('STILL PRESENT');
|
||||||
for (final entry in stillPresent.take(top)) {
|
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;
|
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.
|
/// Compare a fresh analysis against the accepted baseline.
|
||||||
///
|
///
|
||||||
/// Self-contained: it derives new findings by comparing fingerprints, so it does
|
/// 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
|
/// An accepted finding in a file that was not analysed - one of
|
||||||
/// [unscannedFiles], or outside every one of [scannedDirectories] - is reported
|
/// [unscannedFiles], or outside every one of [scannedDirectories] - is reported
|
||||||
/// as not scanned. Its absence is not a fix.
|
/// 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({
|
VerificationReport verifyAgainstBaseline({
|
||||||
required FindingReport report,
|
required FindingReport report,
|
||||||
required List<BaselineEntry> baseline,
|
required List<BaselineEntry> baseline,
|
||||||
Set<String>? rules,
|
Set<String>? rules,
|
||||||
Set<String> unscannedFiles = const {},
|
Set<String> unscannedFiles = const {},
|
||||||
List<String>? scannedDirectories,
|
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;
|
final directories = scannedDirectories;
|
||||||
bool scanned(BaselineEntry entry) {
|
bool scanned(BaselineEntry entry) {
|
||||||
final file = baselineEntryFile(entry);
|
final file = baselineEntryFile(entry);
|
||||||
|
|
@ -182,12 +255,15 @@ VerificationReport verifyAgainstBaseline({
|
||||||
final accepted = {for (final entry in baseline) entry.fingerprint};
|
final accepted = {for (final entry in baseline) entry.fingerprint};
|
||||||
final notScanned = <BaselineEntry>[];
|
final notScanned = <BaselineEntry>[];
|
||||||
final resolved = <BaselineEntry>[];
|
final resolved = <BaselineEntry>[];
|
||||||
|
final removed = <BaselineEntry>[];
|
||||||
final stillPresent = <BaselineEntry>[];
|
final stillPresent = <BaselineEntry>[];
|
||||||
for (final entry in inScope) {
|
for (final entry in inScope) {
|
||||||
if (present.contains(entry.fingerprint)) {
|
if (present.contains(entry.fingerprint)) {
|
||||||
stillPresent.add(entry);
|
stillPresent.add(entry);
|
||||||
} else if (!scanned(entry)) {
|
} else if (!scanned(entry)) {
|
||||||
notScanned.add(entry);
|
notScanned.add(entry);
|
||||||
|
} else if (subjectRemoved(entry)) {
|
||||||
|
removed.add(entry);
|
||||||
} else {
|
} else {
|
||||||
resolved.add(entry);
|
resolved.add(entry);
|
||||||
}
|
}
|
||||||
|
|
@ -196,6 +272,7 @@ VerificationReport verifyAgainstBaseline({
|
||||||
baselineTotal: inScope.length,
|
baselineTotal: inScope.length,
|
||||||
outOfScope: baseline.length - inScope.length,
|
outOfScope: baseline.length - inScope.length,
|
||||||
resolved: resolved,
|
resolved: resolved,
|
||||||
|
removed: removed,
|
||||||
stillPresent: stillPresent,
|
stillPresent: stillPresent,
|
||||||
notScanned: notScanned,
|
notScanned: notScanned,
|
||||||
newFindings: [
|
newFindings: [
|
||||||
|
|
@ -230,5 +307,12 @@ VerificationReport verifyAnalysis({
|
||||||
rules: rules,
|
rules: rules,
|
||||||
unscannedFiles: unscanned,
|
unscannedFiles: unscanned,
|
||||||
scannedDirectories: project.directories,
|
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.toJson(), containsPair('notScanned', 2));
|
||||||
expect(verification.describe(), contains('NOT SCANNED'));
|
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