merge/audit-and-state-map #7
3 changed files with 122 additions and 26 deletions
fix(baseline): make every fingerprint independent of position
The fingerprint is documented as surviving line shifts, but eight of the fixture's findings carried a line number, so moving code down a file turned accepted findings into new ones on the next ratcheted check: - dormant emitted-not-rendered (keyed by union + owner, neither counted as identifying, so the line was appended) - notifier-inert-consumer and notifier-field-unread (owner only) - loading-not-reset, whose `catchLine` key is a line by another name `union` and `owner` now count as identifying (each of those rules emits at most one finding per union or notifier per file), and loading-not-reset keys on the catch's ordinal within its member. Its `on<E>` handler closures are also named `on<E>` like loading-flag-not-reset's, so the ordinal cannot collide across handlers. The existing move test only bumped `line`, not `catchLine` in the data, so it passed while real moves did not. fingerprint_test.dart now shifts every file of the fixture for real and requires (a) every fingerprint to survive and (b) no two findings to share one. (a) fails on the previous rules.dart with exactly the eight findings above; (b) passed there too, and guards the ordinal and closure naming that replace the line. Baselines written before this change see the affected findings once as new; re-accept them with --write-baseline. No committed baselines exist in the dogfood repos.
commit
c79878f93d
|
|
@ -74,7 +74,7 @@ class Finding {
|
|||
'stateType',
|
||||
'valueType',
|
||||
'call',
|
||||
'catchLine',
|
||||
'catch',
|
||||
]) {
|
||||
final value = data[key];
|
||||
if (value != null) {
|
||||
|
|
@ -84,7 +84,14 @@ class Finding {
|
|||
}
|
||||
// Findings with no discriminating key (two methods of the same rule in the
|
||||
// same file, say) would collide, so fall back to the position.
|
||||
// test/fingerprint_test.dart checks, across the whole fixture, that no
|
||||
// fingerprint depends on position and that none collide.
|
||||
const discriminating = {
|
||||
// One dormant emitted-not-rendered finding per union, and one
|
||||
// notifier-inert / notifier-field-unread finding per notifier: both are
|
||||
// semantic identity, so they must not fall back to the line.
|
||||
'union',
|
||||
'owner',
|
||||
'variant',
|
||||
'group',
|
||||
'event',
|
||||
|
|
@ -95,9 +102,9 @@ class Finding {
|
|||
// 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.
|
||||
// method) carries the catch's ordinal within that member, not its line.
|
||||
'member',
|
||||
'catchLine',
|
||||
'catch',
|
||||
};
|
||||
if (seen.intersection(discriminating).isEmpty && line != null) {
|
||||
parts.add('line=$line');
|
||||
|
|
@ -1015,6 +1022,21 @@ class _CalledNames extends RecursiveAstVisitor<void> {
|
|||
}
|
||||
}
|
||||
|
||||
/// `on<E>` for a bloc handler closure, so two handlers in one class get
|
||||
/// distinct identities. As a bare `<closure>` they shared a fingerprint, and
|
||||
/// the baseline dropped all but one of them.
|
||||
String _closureMemberName(FunctionExpression node) {
|
||||
final list = node.parent;
|
||||
final call = list?.parent;
|
||||
if (list is ArgumentList && call is MethodInvocation) {
|
||||
final types = call.typeArguments?.arguments;
|
||||
if (call.methodName.name == 'on' && types != null && types.length == 1) {
|
||||
return 'on<${types.single.toSource()}>';
|
||||
}
|
||||
}
|
||||
return '<closure>';
|
||||
}
|
||||
|
||||
/// Finds awaited work on a loading-flag path that no catch clause resolves.
|
||||
class _FlagLoadingCollector extends RecursiveAstVisitor<void> {
|
||||
_FlagLoadingCollector(
|
||||
|
|
@ -1083,25 +1105,10 @@ class _FlagLoadingCollector extends RecursiveAstVisitor<void> {
|
|||
|
||||
@override
|
||||
void visitFunctionExpression(FunctionExpression node) {
|
||||
_analyze(node.body, _closureName(node));
|
||||
_analyze(node.body, _closureMemberName(node));
|
||||
super.visitFunctionExpression(node);
|
||||
}
|
||||
|
||||
/// `on<E>` for a bloc handler closure, so two handlers in one class get
|
||||
/// distinct identities. As a bare `<closure>` they shared a fingerprint, and
|
||||
/// the baseline dropped all but one of them.
|
||||
static String _closureName(FunctionExpression node) {
|
||||
final list = node.parent;
|
||||
final call = list?.parent;
|
||||
if (list is ArgumentList && call is MethodInvocation) {
|
||||
final types = call.typeArguments?.arguments;
|
||||
if (call.methodName.name == 'on' && types != null && types.length == 1) {
|
||||
return 'on<${types.single.toSource()}>';
|
||||
}
|
||||
}
|
||||
return '<closure>';
|
||||
}
|
||||
|
||||
void _analyze(FunctionBody? body, String member) {
|
||||
if (body == null) return;
|
||||
final owner = _classStack.isEmpty ? '<top>' : _classStack.last;
|
||||
|
|
@ -1334,7 +1341,7 @@ class _LoadingResetCollector extends RecursiveAstVisitor<void> {
|
|||
|
||||
@override
|
||||
void visitFunctionExpression(FunctionExpression node) {
|
||||
_analyze(node.body, '<closure>');
|
||||
_analyze(node.body, _closureMemberName(node));
|
||||
super.visitFunctionExpression(node);
|
||||
}
|
||||
|
||||
|
|
@ -1347,6 +1354,10 @@ class _LoadingResetCollector extends RecursiveAstVisitor<void> {
|
|||
body.accept(scan);
|
||||
if (scan.loadingEmits.isEmpty) return;
|
||||
|
||||
// Position within the member, not the line: two swallowed catches in one
|
||||
// method need distinct fingerprints, and a line number changed whenever
|
||||
// anything above the method moved.
|
||||
var swallowed = 0;
|
||||
for (final statement in scan.topLevelTries) {
|
||||
if (statement.catchClauses.isEmpty) continue;
|
||||
final probe = _HasAwait();
|
||||
|
|
@ -1362,6 +1373,7 @@ class _LoadingResetCollector extends RecursiveAstVisitor<void> {
|
|||
final dispatchesFailure =
|
||||
source.contains('add(') && _errorish.hasMatch(source);
|
||||
if (emits || assignsState || rethrows || dispatchesFailure) continue;
|
||||
swallowed++;
|
||||
|
||||
findings.add(
|
||||
Finding(
|
||||
|
|
@ -1385,9 +1397,9 @@ class _LoadingResetCollector extends RecursiveAstVisitor<void> {
|
|||
data: {
|
||||
'owner': owner,
|
||||
'member': member,
|
||||
// A method can hold more than one swallowed catch, so position is
|
||||
// part of this rule's identity.
|
||||
'catchLine': lineInfo.getLocation(catchClause.offset).lineNumber,
|
||||
// A method can hold more than one swallowed catch, so which one
|
||||
// is part of this rule's identity.
|
||||
'catch': swallowed,
|
||||
},
|
||||
),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -105,9 +105,9 @@ void main() {
|
|||
});
|
||||
|
||||
test('two findings in one method still get distinct fingerprints', () {
|
||||
// A method can hold more than one swallowed catch, so this rule carries an
|
||||
// explicit catchLine: it needs position to stay distinguishable while other
|
||||
// rules stay line-independent.
|
||||
// A method can hold more than one swallowed catch, so this rule carries the
|
||||
// catch's ordinal within the member: distinguishable, and still stable when
|
||||
// the method moves (test/fingerprint_test.dart).
|
||||
final catches = analysis.report.findings
|
||||
.where(
|
||||
(f) =>
|
||||
|
|
|
|||
84
test/fingerprint_test.dart
Normal file
84
test/fingerprint_test.dart
Normal file
|
|
@ -0,0 +1,84 @@
|
|||
import 'dart:io';
|
||||
|
||||
import 'package:pipe/pipe.dart';
|
||||
import 'package:test/test.dart';
|
||||
|
||||
/// The fixture copied with [padding] blank lines inserted at the top of every
|
||||
/// Dart source, so every finding moves down without any semantic change.
|
||||
String _shiftedFixture(int padding) {
|
||||
final from = Directory('test/fixtures/stuck_loading');
|
||||
final to = Directory.systemTemp.createTempSync('pipe-shifted');
|
||||
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);
|
||||
if (entity.path.endsWith('.dart')) {
|
||||
// After any `library;` / import header would change nothing; a comment
|
||||
// block at the very top shifts every line in the file.
|
||||
File(target).writeAsStringSync(
|
||||
'${List.filled(padding, '//').join('\n')}\n'
|
||||
'${entity.readAsStringSync()}',
|
||||
);
|
||||
} else {
|
||||
File(target).writeAsBytesSync(entity.readAsBytesSync());
|
||||
}
|
||||
}
|
||||
}
|
||||
return to.path;
|
||||
}
|
||||
|
||||
Future<List<Finding>> _findings(String root) async {
|
||||
final analysis = await analyzeProject(
|
||||
await loadProject(root),
|
||||
concurrency: 1,
|
||||
);
|
||||
try {
|
||||
return analysis.report.findings;
|
||||
} finally {
|
||||
await analysis.dispose();
|
||||
}
|
||||
}
|
||||
|
||||
void main() {
|
||||
late List<Finding> original;
|
||||
late List<Finding> shifted;
|
||||
|
||||
setUpAll(() async {
|
||||
original = await _findings('test/fixtures/stuck_loading');
|
||||
shifted = await _findings(_shiftedFixture(7));
|
||||
});
|
||||
|
||||
test('moving code down a file does not change any fingerprint', () {
|
||||
// A baseline must keep accepting a finding when unrelated edits move it.
|
||||
// Any fingerprint carrying a line number resurrected accepted findings as
|
||||
// new on the next check.
|
||||
expect(
|
||||
shifted.map((f) => f.line),
|
||||
isNot(equals(original.map((f) => f.line))),
|
||||
reason: 'the control must actually move the findings',
|
||||
);
|
||||
final before = {for (final f in original) f.fingerprint: f.rule};
|
||||
final after = {for (final f in shifted) f.fingerprint: f.rule};
|
||||
final moved = {
|
||||
for (final entry in before.entries)
|
||||
if (!after.containsKey(entry.key)) entry.key: entry.value,
|
||||
};
|
||||
expect(moved, isEmpty, reason: 'fingerprints that depend on position');
|
||||
});
|
||||
|
||||
test('no two findings share a fingerprint', () {
|
||||
// A collision makes the baseline keep one finding and silently drop the
|
||||
// other, so a second real defect can never be reported as new.
|
||||
final counts = <String, int>{};
|
||||
for (final f in original) {
|
||||
counts[f.fingerprint] = (counts[f.fingerprint] ?? 0) + 1;
|
||||
}
|
||||
expect({
|
||||
for (final e in counts.entries)
|
||||
if (e.value > 1) e.key: e.value,
|
||||
}, isEmpty);
|
||||
});
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue