merge/audit-and-state-map #7
4 changed files with 143 additions and 16 deletions
fix(loading-flag-not-reset): analyse closures once, and name handlers
The method-level check read the whole body text, closures included, for both the `loading: true` probe and the await probe, while the try collector skips closures. So a method whose closure set the flag and guarded its own await was reported at high confidence as unguarded, and a method that only registers `on<E>` closures was reported alongside each closure. Every probe now stops at nested functions, which are analysed on their own; the flag probe is a `field: true` named-argument visitor instead of a regex over source. `on<E>` handler closures were all named `<closure>`, so two handlers in one class shared a fingerprint and encodeBaseline kept only one of them. They are now named `on<E>`. Existing baselines that accepted a handler as `<closure>` will see it once as new. Fixture: scheduleGuarded (guarded closure) and registerHandlers are now negative controls; two unguarded handlers must yield two findings with distinct fingerprints. Before: scheduleGuarded, registerHandlers and two colliding `<closure>` findings; after: on<FlagRefresh>, on<FlagReload>.
commit
8d64718977
|
|
@ -1083,10 +1083,25 @@ class _FlagLoadingCollector extends RecursiveAstVisitor<void> {
|
|||
|
||||
@override
|
||||
void visitFunctionExpression(FunctionExpression node) {
|
||||
_analyze(node.body, '<closure>');
|
||||
_analyze(node.body, _closureName(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;
|
||||
|
|
@ -1095,13 +1110,17 @@ class _FlagLoadingCollector extends RecursiveAstVisitor<void> {
|
|||
// Lifecycle teardown never renders a loading state.
|
||||
if (member == 'close' || member == 'dispose') return;
|
||||
|
||||
final source = body.toSource();
|
||||
// Only methods that actually put the loading flag into the true state can
|
||||
// leave the UI spinning. A method that merely runs in a class whose state
|
||||
// has such a flag (a repository write, a teardown call) cannot.
|
||||
final setsFlagTrue = RegExp(
|
||||
'(${flag.fields.join('|')})\\s*:\\s*true',
|
||||
).hasMatch(source);
|
||||
//
|
||||
// Every probe here stops at nested functions, which are analysed on their
|
||||
// own. The flag probe and the await probe used to read closure bodies while
|
||||
// the try collector skipped them, so a method whose closure set the flag
|
||||
// and guarded its own await was reported at high confidence.
|
||||
final flagProbe = _SetsFlagTrue(flag.fields.toSet());
|
||||
body.accept(flagProbe);
|
||||
final setsFlagTrue = flagProbe.found;
|
||||
final inheritsInitialFlag =
|
||||
flag.defaultsTrue &&
|
||||
(_constructorCalls[owner]?.contains(member) ?? false);
|
||||
|
|
@ -1113,7 +1132,7 @@ class _FlagLoadingCollector extends RecursiveAstVisitor<void> {
|
|||
var awaitedTryGuarded = false;
|
||||
var awaitedTrySeen = false;
|
||||
for (final statement in tries.tries) {
|
||||
final awaitedInTry = _HasAwait();
|
||||
final awaitedInTry = _HasAwait(shallow: true);
|
||||
statement.body.accept(awaitedInTry);
|
||||
if (!awaitedInTry.found) continue;
|
||||
awaitedTrySeen = true;
|
||||
|
|
@ -1126,7 +1145,7 @@ class _FlagLoadingCollector extends RecursiveAstVisitor<void> {
|
|||
if (awaitedTryGuarded) return;
|
||||
|
||||
// Nothing awaited at all means there is no failure path to leave dangling.
|
||||
final awaitProbe = _HasAwait();
|
||||
final awaitProbe = _HasAwait(shallow: true);
|
||||
body.accept(awaitProbe);
|
||||
if (!awaitProbe.found) return;
|
||||
|
||||
|
|
@ -1219,6 +1238,12 @@ class _BodyScan extends RecursiveAstVisitor<void> {
|
|||
}
|
||||
|
||||
class _HasAwait extends RecursiveAstVisitor<void> {
|
||||
_HasAwait({this.shallow = false});
|
||||
|
||||
/// Stop at nested functions: an await inside a closure is not awaited by the
|
||||
/// body that declares the closure.
|
||||
final bool shallow;
|
||||
|
||||
bool found = false;
|
||||
|
||||
@override
|
||||
|
|
@ -1226,6 +1251,42 @@ class _HasAwait extends RecursiveAstVisitor<void> {
|
|||
found = true;
|
||||
super.visitAwaitExpression(node);
|
||||
}
|
||||
|
||||
@override
|
||||
void visitFunctionExpression(FunctionExpression node) {
|
||||
if (!shallow) super.visitFunctionExpression(node);
|
||||
}
|
||||
|
||||
@override
|
||||
void visitFunctionDeclaration(FunctionDeclaration node) {
|
||||
if (!shallow) super.visitFunctionDeclaration(node);
|
||||
}
|
||||
}
|
||||
|
||||
/// A `field: true` named argument for one of the loading fields, outside any
|
||||
/// nested function.
|
||||
class _SetsFlagTrue extends RecursiveAstVisitor<void> {
|
||||
_SetsFlagTrue(this.fields);
|
||||
|
||||
final Set<String> fields;
|
||||
bool found = false;
|
||||
|
||||
@override
|
||||
void visitNamedArgument(NamedArgument node) {
|
||||
final value = node.argumentExpression;
|
||||
if (fields.contains(node.name.lexeme) &&
|
||||
value is BooleanLiteral &&
|
||||
value.value) {
|
||||
found = true;
|
||||
}
|
||||
super.visitNamedArgument(node);
|
||||
}
|
||||
|
||||
@override
|
||||
void visitFunctionExpression(FunctionExpression node) {}
|
||||
|
||||
@override
|
||||
void visitFunctionDeclaration(FunctionDeclaration node) {}
|
||||
}
|
||||
|
||||
class _LoadingResetCollector extends RecursiveAstVisitor<void> {
|
||||
|
|
|
|||
33
test/fixtures/stuck_loading/lib/flag_cubit.dart
vendored
33
test/fixtures/stuck_loading/lib/flag_cubit.dart
vendored
|
|
@ -6,6 +6,10 @@ import 'bloc.dart';
|
|||
/// that the variant-based rules cannot see.
|
||||
class FlagEvent {}
|
||||
|
||||
class FlagRefresh extends FlagEvent {}
|
||||
|
||||
class FlagReload extends FlagEvent {}
|
||||
|
||||
class FlagState {
|
||||
const FlagState({this.items = const <int>[], this.loading = true});
|
||||
|
||||
|
|
@ -43,4 +47,33 @@ class FlagCubit extends Bloc<FlagEvent, FlagState> {
|
|||
Future<void> refreshQuietly() async {
|
||||
await Future<void>.delayed(Duration.zero);
|
||||
}
|
||||
|
||||
/// GOOD: the flag, the await and the guarding catch all live in a closure,
|
||||
/// which is analysed on its own. The enclosing method sets nothing and awaits
|
||||
/// nothing itself, so reporting it would be a false positive.
|
||||
void scheduleGuarded() {
|
||||
_later(() async {
|
||||
emit(state.copyWith(loading: true));
|
||||
try {
|
||||
await Future<void>.delayed(Duration.zero);
|
||||
} catch (error) {
|
||||
emit(state.copyWith(loading: false));
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
void _later(Future<void> Function() work) {}
|
||||
|
||||
/// BAD, twice: two unguarded handlers in one class. Each is its own finding
|
||||
/// with its own fingerprint, not one `<closure>` that hides the other.
|
||||
void registerHandlers() {
|
||||
on<FlagRefresh>((event, emit) async {
|
||||
emit(state.copyWith(loading: true));
|
||||
await Future<void>.delayed(Duration.zero);
|
||||
});
|
||||
on<FlagReload>((event, emit) async {
|
||||
emit(state.copyWith(loading: true));
|
||||
await Future<void>.delayed(Duration.zero);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -30,15 +30,23 @@ void main() {
|
|||
.where((finding) => finding.rule == ruleLoadingFlagNotReset)
|
||||
.toList();
|
||||
|
||||
// loadGuarded resets the flag in its catch and refreshQuietly never sets it,
|
||||
// so exactly one finding should survive.
|
||||
expect(hits, hasLength(1));
|
||||
expect(hits.single.data['member'], 'loadUnguarded');
|
||||
expect(hits.single.data['stateType'], 'FlagState');
|
||||
expect(hits.single.severity, PipeSeverity.warning);
|
||||
expect(hits.single.confidence, 'high');
|
||||
expect(hits.single.evidence, isNotEmpty);
|
||||
expect(hits.single.remediation, isNotNull);
|
||||
// loadGuarded resets the flag in its catch, refreshQuietly never sets it,
|
||||
// and scheduleGuarded's closure guards itself, so only the unguarded method
|
||||
// and the two deliberately unguarded `on<>` handlers survive.
|
||||
expect(hits.map((hit) => hit.data['member']).toSet(), {
|
||||
'loadUnguarded',
|
||||
'on<FlagRefresh>',
|
||||
'on<FlagReload>',
|
||||
});
|
||||
expect(hits, hasLength(3));
|
||||
final unguarded = hits.singleWhere(
|
||||
(hit) => hit.data['member'] == 'loadUnguarded',
|
||||
);
|
||||
expect(unguarded.data['stateType'], 'FlagState');
|
||||
expect(unguarded.severity, PipeSeverity.warning);
|
||||
expect(unguarded.confidence, 'high');
|
||||
expect(unguarded.evidence, isNotEmpty);
|
||||
expect(unguarded.remediation, isNotNull);
|
||||
});
|
||||
|
||||
test('flag findings carry no false positive for guarded code', () {
|
||||
|
|
|
|||
|
|
@ -40,10 +40,16 @@ const Map<String, List<_Id>> _negatives = {
|
|||
// loadGuarded resolves the flag, refreshQuietly never sets it, and `_warmUp`
|
||||
// is constructor-invoked on a flag defaulting to FALSE, so nothing is in
|
||||
// flight.
|
||||
// scheduleGuarded's closure sets the flag and guards its own await, and
|
||||
// registerHandlers only registers closures: neither enclosing method is in
|
||||
// flight itself. Every closure gets a real name, never `<closure>`.
|
||||
ruleLoadingFlagNotReset: [
|
||||
('member', 'loadGuarded'),
|
||||
('member', 'refreshQuietly'),
|
||||
('member', '_warmUp'),
|
||||
('member', 'scheduleGuarded'),
|
||||
('member', 'registerHandlers'),
|
||||
('member', '<closure>'),
|
||||
],
|
||||
// HandledTap is handled AND dispatched, so it is the control that proves both
|
||||
// event rules actually consult the handler and dispatch information.
|
||||
|
|
@ -130,6 +136,25 @@ void main() {
|
|||
);
|
||||
});
|
||||
|
||||
test('loading-flag-not-reset: two handlers in one class stay distinct', () {
|
||||
// As `<closure>` they shared one fingerprint, so a baseline kept only one.
|
||||
final hits = findings
|
||||
.where(
|
||||
(finding) =>
|
||||
finding.rule == ruleLoadingFlagNotReset &&
|
||||
finding.data['owner'] == 'FlagCubit',
|
||||
)
|
||||
.toList();
|
||||
expect(
|
||||
hits.map((finding) => finding.data['member']),
|
||||
containsAll(<String>['on<FlagRefresh>', 'on<FlagReload>']),
|
||||
);
|
||||
expect(
|
||||
hits.map((finding) => finding.fingerprint).toSet(),
|
||||
hasLength(hits.length),
|
||||
);
|
||||
});
|
||||
|
||||
for (final entry in _positives.entries) {
|
||||
test('${entry.key}: fires on the positive control', () {
|
||||
final (key, value) = entry.value;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue