fix: stop verify, reports and briefs from claiming more than pipe checked #1
3 changed files with 239 additions and 32 deletions
fix(baseline): key every fingerprint on identity, not on its line
A finding with no discriminating key fell back to `line=N`, and three rules
had none: a dormant emitted-not-rendered union carried only {union, owner},
and notifier-field-unread and notifier-inert-consumer only {owner}.
loading-not-reset embedded the catch's absolute line as `catchLine`. Any edit
above one of those findings changed its fingerprint, so `check --baseline`
re-reported an accepted finding as new and `verify` reported its entry as
RESOLVED.
union and owner now count as discriminating (each of those rules emits at most
one finding per union or notifier), and loading-not-reset carries `catchSite`,
the catch's index among the catch clauses of its enclosing member
(`twoSwallowedAwaits#1`), which does not move when code outside the member
does.
The test that guarded this shifted only findings.first. It now shifts every
finding, and a second test really inserts lines at the top of every fixture
file, re-analyses, and requires the same fingerprint set.
Migration: fingerprints change for emitted-not-rendered (dormant unions),
notifier-field-unread, notifier-inert-consumer and loading-not-reset. Old
baselines stay readable and keep working - both `check --baseline` and
`verify` also match the version-1 fingerprint (`legacyFingerprint`) - but
those entries stay line-sensitive until the baseline is rewritten with
--write-baseline, which now writes `"version": 2`.
commit
a0844bb6f0
|
|
@ -60,7 +60,69 @@ class Finding {
|
||||||
final bool baselined;
|
final bool baselined;
|
||||||
|
|
||||||
/// Stable identity that survives line shifts, used to ratchet a baseline.
|
/// Stable identity that survives line shifts, used to ratchet a baseline.
|
||||||
|
///
|
||||||
|
/// Every rule now carries a semantic key, so `line=` is a last resort for a
|
||||||
|
/// finding with no identity at all. It used to be the norm for three rules -
|
||||||
|
/// a dormant emitted-not-rendered union and both notifier-level rules carried
|
||||||
|
/// only {union, owner} or {owner}, which did not count as discriminating - and
|
||||||
|
/// loading-not-reset embedded the catch's absolute line. Any edit above such a
|
||||||
|
/// finding changed its identity, so a baseline re-reported it as new and
|
||||||
|
/// `verify` reported the old entry as resolved.
|
||||||
String get fingerprint {
|
String get fingerprint {
|
||||||
|
final parts = <String>[rule, file ?? ''];
|
||||||
|
final seen = <String>{};
|
||||||
|
for (final key in const [
|
||||||
|
'union',
|
||||||
|
'variant',
|
||||||
|
'group',
|
||||||
|
'event',
|
||||||
|
'provider',
|
||||||
|
'owner',
|
||||||
|
'member',
|
||||||
|
'stateType',
|
||||||
|
'valueType',
|
||||||
|
'call',
|
||||||
|
'catchSite',
|
||||||
|
]) {
|
||||||
|
final value = data[key];
|
||||||
|
if (value != null) {
|
||||||
|
parts.add('$key=$value');
|
||||||
|
seen.add(key);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
// Findings with no discriminating key at all would collide, so fall back to
|
||||||
|
// the position.
|
||||||
|
const discriminating = {
|
||||||
|
// A union or owner is one finding per file for the rules that carry only
|
||||||
|
// that: one dormant union, one unread-member list or one inert notifier
|
||||||
|
// per class.
|
||||||
|
'union',
|
||||||
|
'owner',
|
||||||
|
'variant',
|
||||||
|
'group',
|
||||||
|
'event',
|
||||||
|
'provider',
|
||||||
|
'stateType',
|
||||||
|
'valueType',
|
||||||
|
'call',
|
||||||
|
// A member name is semantic identity, not position: adding it keeps
|
||||||
|
// fingerprints stable when code moves.
|
||||||
|
'member',
|
||||||
|
// The one rule that can emit two findings with the same owner and member
|
||||||
|
// (two swallowed catches in one method) carries the catch's index within
|
||||||
|
// its enclosing member, which does not move when code above it does.
|
||||||
|
'catchSite',
|
||||||
|
};
|
||||||
|
if (seen.intersection(discriminating).isEmpty && line != null) {
|
||||||
|
parts.add('line=$line');
|
||||||
|
}
|
||||||
|
return parts.join('|');
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The identity baselines written before [fingerprint] stopped depending on
|
||||||
|
/// line numbers used. Matching it keeps an older baseline working: its
|
||||||
|
/// entries still suppress and verify, until it is rewritten.
|
||||||
|
String get legacyFingerprint {
|
||||||
final parts = <String>[rule, file ?? ''];
|
final parts = <String>[rule, file ?? ''];
|
||||||
final seen = <String>{};
|
final seen = <String>{};
|
||||||
for (final key in const [
|
for (final key in const [
|
||||||
|
|
@ -82,8 +144,6 @@ class Finding {
|
||||||
seen.add(key);
|
seen.add(key);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// Findings with no discriminating key (two methods of the same rule in the
|
|
||||||
// same file, say) would collide, so fall back to the position.
|
|
||||||
const discriminating = {
|
const discriminating = {
|
||||||
'variant',
|
'variant',
|
||||||
'group',
|
'group',
|
||||||
|
|
@ -92,10 +152,6 @@ class Finding {
|
||||||
'stateType',
|
'stateType',
|
||||||
'valueType',
|
'valueType',
|
||||||
'call',
|
'call',
|
||||||
// A member name is semantic identity, not position: adding it keeps
|
|
||||||
// fingerprints stable when code moves. The one rule that can emit two
|
|
||||||
// findings with the same owner and member (two swallowed catches in one
|
|
||||||
// method) carries an explicit `catchLine` key instead.
|
|
||||||
'member',
|
'member',
|
||||||
'catchLine',
|
'catchLine',
|
||||||
};
|
};
|
||||||
|
|
@ -105,6 +161,10 @@ class Finding {
|
||||||
return parts.join('|');
|
return parts.join('|');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// True when [accepted] holds this finding under either identity.
|
||||||
|
bool isAcceptedBy(Set<String> accepted) =>
|
||||||
|
accepted.contains(fingerprint) || accepted.contains(legacyFingerprint);
|
||||||
|
|
||||||
/// Human-readable identity used in baseline entries and verify output.
|
/// Human-readable identity used in baseline entries and verify output.
|
||||||
String get baselineLabel {
|
String get baselineLabel {
|
||||||
final data = this.data;
|
final data = this.data;
|
||||||
|
|
@ -776,9 +836,7 @@ FindingReport runChecks({
|
||||||
? findings
|
? findings
|
||||||
: [
|
: [
|
||||||
for (final finding in findings)
|
for (final finding in findings)
|
||||||
baseline.contains(finding.fingerprint)
|
finding.isAcceptedBy(baseline) ? finding.asBaselined() : finding,
|
||||||
? finding.asBaselined()
|
|
||||||
: finding,
|
|
||||||
];
|
];
|
||||||
resolved.sort((a, b) {
|
resolved.sort((a, b) {
|
||||||
final bySeverity = _severityRank(
|
final bySeverity = _severityRank(
|
||||||
|
|
@ -811,8 +869,10 @@ String encodeBaseline(String root, List<Finding> findings) {
|
||||||
byFingerprint.putIfAbsent(finding.fingerprint, () => finding);
|
byFingerprint.putIfAbsent(finding.fingerprint, () => finding);
|
||||||
}
|
}
|
||||||
final fingerprints = byFingerprint.keys.toList()..sort();
|
final fingerprints = byFingerprint.keys.toList()..sort();
|
||||||
|
// Version 2: fingerprints no longer embed line numbers for rules with a
|
||||||
|
// semantic identity. Version-1 fingerprints are still matched on read.
|
||||||
return const JsonEncoder.withIndent(' ').convert(<String, Object?>{
|
return const JsonEncoder.withIndent(' ').convert(<String, Object?>{
|
||||||
'version': 1,
|
'version': 2,
|
||||||
'root': root,
|
'root': root,
|
||||||
'fingerprints': fingerprints,
|
'fingerprints': fingerprints,
|
||||||
'entries': [
|
'entries': [
|
||||||
|
|
@ -1176,6 +1236,16 @@ class _BodyScan extends RecursiveAstVisitor<void> {
|
||||||
void visitFunctionDeclaration(FunctionDeclaration node) {}
|
void visitFunctionDeclaration(FunctionDeclaration node) {}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
class _CatchCollector extends RecursiveAstVisitor<void> {
|
||||||
|
final List<CatchClause> clauses = [];
|
||||||
|
|
||||||
|
@override
|
||||||
|
void visitCatchClause(CatchClause node) {
|
||||||
|
clauses.add(node);
|
||||||
|
super.visitCatchClause(node);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
class _HasAwait extends RecursiveAstVisitor<void> {
|
class _HasAwait extends RecursiveAstVisitor<void> {
|
||||||
bool found = false;
|
bool found = false;
|
||||||
|
|
||||||
|
|
@ -1202,6 +1272,10 @@ class _LoadingResetCollector extends RecursiveAstVisitor<void> {
|
||||||
final List<Finding> findings;
|
final List<Finding> findings;
|
||||||
final List<String> _classStack = [];
|
final List<String> _classStack = [];
|
||||||
|
|
||||||
|
/// Enclosing named members (method, function, constructor), innermost last,
|
||||||
|
/// so a catch in a closure is placed relative to the member that holds it.
|
||||||
|
final List<({String name, AstNode node})> _members = [];
|
||||||
|
|
||||||
static final RegExp _stateAssign = RegExp(r'(^|[^.\w])state\s*=(?!=)');
|
static final RegExp _stateAssign = RegExp(r'(^|[^.\w])state\s*=(?!=)');
|
||||||
static final RegExp _throwing = RegExp(r'\b(rethrow|throw)\b');
|
static final RegExp _throwing = RegExp(r'\b(rethrow|throw)\b');
|
||||||
static final RegExp _errorish = RegExp(
|
static final RegExp _errorish = RegExp(
|
||||||
|
|
@ -1225,8 +1299,28 @@ class _LoadingResetCollector extends RecursiveAstVisitor<void> {
|
||||||
|
|
||||||
@override
|
@override
|
||||||
void visitMethodDeclaration(MethodDeclaration node) {
|
void visitMethodDeclaration(MethodDeclaration node) {
|
||||||
|
_members.add((name: node.name.lexeme, node: node));
|
||||||
_analyze(node.body, node.name.lexeme);
|
_analyze(node.body, node.name.lexeme);
|
||||||
super.visitMethodDeclaration(node);
|
super.visitMethodDeclaration(node);
|
||||||
|
_members.removeLast();
|
||||||
|
}
|
||||||
|
|
||||||
|
@override
|
||||||
|
void visitFunctionDeclaration(FunctionDeclaration node) {
|
||||||
|
_members.add((name: node.name.lexeme, node: node));
|
||||||
|
super.visitFunctionDeclaration(node);
|
||||||
|
_members.removeLast();
|
||||||
|
}
|
||||||
|
|
||||||
|
@override
|
||||||
|
void visitConstructorDeclaration(ConstructorDeclaration node) {
|
||||||
|
final name = node.name;
|
||||||
|
_members.add((
|
||||||
|
name: name == null ? 'new' : 'new ${name.lexeme}',
|
||||||
|
node: node,
|
||||||
|
));
|
||||||
|
super.visitConstructorDeclaration(node);
|
||||||
|
_members.removeLast();
|
||||||
}
|
}
|
||||||
|
|
||||||
@override
|
@override
|
||||||
|
|
@ -1235,6 +1329,19 @@ class _LoadingResetCollector extends RecursiveAstVisitor<void> {
|
||||||
super.visitFunctionExpression(node);
|
super.visitFunctionExpression(node);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// `member#index`: the catch's position among every catch clause of its
|
||||||
|
/// enclosing member, in source order. Unlike its line, this does not change
|
||||||
|
/// when code outside that member moves.
|
||||||
|
String _catchSite(CatchClause clause, FunctionBody body) {
|
||||||
|
final enclosing = _members.isEmpty ? null : _members.last;
|
||||||
|
final collector = _CatchCollector();
|
||||||
|
(enclosing?.node ?? body).accept(collector);
|
||||||
|
final index = collector.clauses.indexWhere(
|
||||||
|
(candidate) => identical(candidate, clause),
|
||||||
|
);
|
||||||
|
return '${enclosing?.name ?? '<closure>'}#$index';
|
||||||
|
}
|
||||||
|
|
||||||
void _analyze(FunctionBody? body, String member) {
|
void _analyze(FunctionBody? body, String member) {
|
||||||
if (body == null) return;
|
if (body == null) return;
|
||||||
final owner = _classStack.isEmpty ? '<top>' : _classStack.last;
|
final owner = _classStack.isEmpty ? '<top>' : _classStack.last;
|
||||||
|
|
@ -1282,8 +1389,11 @@ class _LoadingResetCollector extends RecursiveAstVisitor<void> {
|
||||||
data: {
|
data: {
|
||||||
'owner': owner,
|
'owner': owner,
|
||||||
'member': member,
|
'member': member,
|
||||||
// A method can hold more than one swallowed catch, so position is
|
// A method can hold more than one swallowed catch, so the catch's
|
||||||
// part of this rule's identity.
|
// place in its member is part of this rule's identity.
|
||||||
|
'catchSite': _catchSite(catchClause, body),
|
||||||
|
// Kept for reading version-1 baselines, whose fingerprints
|
||||||
|
// embedded it; no longer part of the fingerprint.
|
||||||
'catchLine': lineInfo.getLocation(catchClause.offset).lineNumber,
|
'catchLine': lineInfo.getLocation(catchClause.offset).lineNumber,
|
||||||
},
|
},
|
||||||
),
|
),
|
||||||
|
|
|
||||||
|
|
@ -170,7 +170,15 @@ VerificationReport verifyAgainstBaseline({
|
||||||
for (final entry in baseline)
|
for (final entry in baseline)
|
||||||
if (rules == null || rules.contains(baselineEntryRule(entry))) entry,
|
if (rules == null || rules.contains(baselineEntryRule(entry))) entry,
|
||||||
];
|
];
|
||||||
final present = {for (final finding in report.findings) finding.fingerprint};
|
// A finding is present under its current identity or the one a version-1
|
||||||
|
// baseline recorded, so an older baseline does not read every changed
|
||||||
|
// fingerprint as a fix.
|
||||||
|
final present = {
|
||||||
|
for (final finding in report.findings) ...[
|
||||||
|
finding.fingerprint,
|
||||||
|
finding.legacyFingerprint,
|
||||||
|
],
|
||||||
|
};
|
||||||
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>[];
|
||||||
|
|
@ -192,7 +200,7 @@ VerificationReport verifyAgainstBaseline({
|
||||||
notScanned: notScanned,
|
notScanned: notScanned,
|
||||||
newFindings: [
|
newFindings: [
|
||||||
for (final finding in report.findings)
|
for (final finding in report.findings)
|
||||||
if (!accepted.contains(finding.fingerprint)) finding,
|
if (!finding.isAcceptedBy(accepted)) finding,
|
||||||
],
|
],
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1,3 +1,5 @@
|
||||||
|
import 'dart:io';
|
||||||
|
|
||||||
import 'package:pipe/pipe.dart';
|
import 'package:pipe/pipe.dart';
|
||||||
import 'package:test/test.dart';
|
import 'package:test/test.dart';
|
||||||
|
|
||||||
|
|
@ -33,7 +35,7 @@ void main() {
|
||||||
hasLength(analysis.report.findings.length),
|
hasLength(analysis.report.findings.length),
|
||||||
reason: 'fingerprints should be unique per finding',
|
reason: 'fingerprints should be unique per finding',
|
||||||
);
|
);
|
||||||
expect(encoded, contains('"version": 1'));
|
expect(encoded, contains('"version": 2'));
|
||||||
});
|
});
|
||||||
|
|
||||||
test('a baseline turns every existing finding into an accepted one', () {
|
test('a baseline turns every existing finding into an accepted one', () {
|
||||||
|
|
@ -65,9 +67,13 @@ void main() {
|
||||||
expect(fresh.newFindings, hasLength(analysis.report.findings.length));
|
expect(fresh.newFindings, hasLength(analysis.report.findings.length));
|
||||||
});
|
});
|
||||||
|
|
||||||
test('fingerprints are stable across rule identity, not line numbers', () {
|
test('no fingerprint depends on the line its finding sits on', () {
|
||||||
final finding = analysis.report.findings.first;
|
// Shifting only findings.first proved nothing about the other rules: three
|
||||||
// Moving the finding down the file must not change its identity.
|
// of them fell back to `line=` and loading-not-reset embedded the catch's
|
||||||
|
// absolute line, so moving code broke their baseline entries.
|
||||||
|
expect(analysis.report.findings, isNotEmpty);
|
||||||
|
for (final finding in analysis.report.findings) {
|
||||||
|
final catchLine = finding.data['catchLine'];
|
||||||
final moved = Finding(
|
final moved = Finding(
|
||||||
rule: finding.rule,
|
rule: finding.rule,
|
||||||
severity: finding.severity,
|
severity: finding.severity,
|
||||||
|
|
@ -77,9 +83,79 @@ void main() {
|
||||||
line: (finding.line ?? 1) + 500,
|
line: (finding.line ?? 1) + 500,
|
||||||
evidence: finding.evidence,
|
evidence: finding.evidence,
|
||||||
remediation: finding.remediation,
|
remediation: finding.remediation,
|
||||||
data: finding.data,
|
// Line-valued data moves with the code.
|
||||||
|
data: {
|
||||||
|
...finding.data,
|
||||||
|
if (catchLine is int) 'catchLine': catchLine + 500,
|
||||||
|
},
|
||||||
);
|
);
|
||||||
expect(moved.fingerprint, finding.fingerprint);
|
expect(
|
||||||
|
moved.fingerprint,
|
||||||
|
finding.fingerprint,
|
||||||
|
reason: '${finding.rule} at ${finding.location}',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('inserting lines above every finding keeps every identity', () async {
|
||||||
|
// The end-to-end version of the test above: really move the code, re-run
|
||||||
|
// the analysis, and compare. Positional data derived from the AST (such as
|
||||||
|
// a catch's place in its method) is exercised here, not simulated.
|
||||||
|
final temp = Directory.systemTemp.createTempSync('pipe-shifted');
|
||||||
|
_copyTree(Directory(fixturePath), temp);
|
||||||
|
for (final entity in Directory('${temp.path}/lib').listSync()) {
|
||||||
|
if (entity is! File || !entity.path.endsWith('.dart')) continue;
|
||||||
|
entity.writeAsStringSync(
|
||||||
|
'// shifted\n' * 7 + entity.readAsStringSync(),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
final shifted = await analyzeProject(
|
||||||
|
await loadProject(temp.path),
|
||||||
|
concurrency: 1,
|
||||||
|
);
|
||||||
|
try {
|
||||||
|
final lines = {
|
||||||
|
for (final finding in analysis.report.findings)
|
||||||
|
if (finding.line != null) finding.line!,
|
||||||
|
};
|
||||||
|
final shiftedLines = {
|
||||||
|
for (final finding in shifted.report.findings)
|
||||||
|
if (finding.line != null) finding.line!,
|
||||||
|
};
|
||||||
|
expect(shiftedLines, isNot(lines), reason: 'the code must have moved');
|
||||||
|
expect(
|
||||||
|
{for (final finding in shifted.report.findings) finding.fingerprint},
|
||||||
|
{for (final finding in analysis.report.findings) finding.fingerprint},
|
||||||
|
);
|
||||||
|
} finally {
|
||||||
|
await shifted.dispose();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('a version-1 baseline still accepts the findings it recorded', () {
|
||||||
|
// Fingerprints changed for rules that used to embed a line; an older
|
||||||
|
// baseline must keep suppressing them rather than report them all as new.
|
||||||
|
final legacy = {
|
||||||
|
for (final finding in analysis.report.findings) finding.legacyFingerprint,
|
||||||
|
};
|
||||||
|
final baselined = reportWith(legacy);
|
||||||
|
expect(baselined.newFindings, isEmpty);
|
||||||
|
|
||||||
|
final entries = [
|
||||||
|
for (final fingerprint in legacy)
|
||||||
|
BaselineEntry(
|
||||||
|
fingerprint: fingerprint,
|
||||||
|
rule: fingerprint.split('|').first,
|
||||||
|
file: fingerprint.split('|')[1],
|
||||||
|
label: fingerprint,
|
||||||
|
),
|
||||||
|
];
|
||||||
|
final verification = verifyAgainstBaseline(
|
||||||
|
report: analysis.report,
|
||||||
|
baseline: entries,
|
||||||
|
);
|
||||||
|
expect(verification.resolved, isEmpty);
|
||||||
|
expect(verification.newFindings, isEmpty);
|
||||||
});
|
});
|
||||||
|
|
||||||
test('a member name is a semantic key, so moving that code keeps identity', () {
|
test('a member name is a semantic key, so moving that code keeps identity', () {
|
||||||
|
|
@ -105,9 +181,9 @@ void main() {
|
||||||
});
|
});
|
||||||
|
|
||||||
test('two findings in one method still get distinct fingerprints', () {
|
test('two findings in one method still get distinct fingerprints', () {
|
||||||
// A method can hold more than one swallowed catch, so this rule carries an
|
// A method can hold more than one swallowed catch, so this rule carries the
|
||||||
// explicit catchLine: it needs position to stay distinguishable while other
|
// catch's index within its member: position enough to tell the two apart,
|
||||||
// rules stay line-independent.
|
// without the absolute line that broke identity whenever code moved.
|
||||||
final catches = analysis.report.findings
|
final catches = analysis.report.findings
|
||||||
.where(
|
.where(
|
||||||
(f) =>
|
(f) =>
|
||||||
|
|
@ -131,3 +207,16 @@ void main() {
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
void _copyTree(Directory from, Directory to) {
|
||||||
|
to.createSync(recursive: true);
|
||||||
|
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).writeAsBytesSync(entity.readAsBytesSync());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue