From cb30269f645d6e85b66d0b685e9762f071365895 Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 13:30:21 -0600 Subject: [PATCH 01/14] fix(event-rules): follow the event hierarchy and every owning bloc Two ways the event rules reported handled events as unhandled (error severity, high confidence): 1. A handler on the event base or an intermediate group. `on` with a switch inside handles every subclass, but handlers were matched to dispatches by exact name, so every dispatched subclass was reported by event-without-handler and the base handler itself by dead-handler. Variants now record the supertype they were enumerated through, and coverage follows it: an event is handled when it or an ancestor is registered, and a handler is live when anything at or below it is dispatched or constructed. A group handler still does not cover a sibling outside the group (UploadAbandoned stays reported). 2. Blocs sharing an event type. Only the first owner of an event type was kept, so a second bloc's handlers were ignored: an event only it handles was reported unhandled, and its dead handlers were never examined. There is now one event union per owning bloc. A dispatch does not record which bloc it targets, so an event any sharing bloc handles counts as handled, and an unhandled event is reported once per shared union rather than once per bloc. Dead handlers are still judged per bloc. Reproduced first on base_event.dart: 4 false findings before (SyncStarted, UploadRetryNow unhandled; SyncEvent, UploadRetry dead) and the shared case (ShareDismissed falsely unhandled, ShareBetaBloc's dead ShareExpired missed). Each is now a contract control. No change on nuwayze (102 findings) or tick (2): byte-identical before and after, since neither uses these patterns. --- lib/src/rules.dart | 50 ++++++- lib/src/state_flow.dart | 18 ++- .../stuck_loading/lib/base_event.dart | 127 ++++++++++++++++++ test/rule_contract_test.dart | 38 +++++- test/state_flow_test.dart | 13 +- 5 files changed, 230 insertions(+), 16 deletions(-) create mode 100644 test/fixtures/stuck_loading/lib/base_event.dart diff --git a/lib/src/rules.dart b/lib/src/rules.dart index bc03308..f981082 100644 --- a/lib/src/rules.dart +++ b/lib/src/rules.dart @@ -573,15 +573,53 @@ FindingReport runChecks({ handledByBloc.putIfAbsent(handler.bloc, () => {}).add(handler.event); } + // A dispatch does not record which bloc it targets, so when several blocs + // share an event type, an event any of them handles is handled. + final ownersByUnion = >{}; + for (final union in flow.eventUnions) { + ownersByUnion.putIfAbsent(union.name, () => []).add(union.owner); + } + final reportedUnhandled = {}; + for (final union in flow.eventUnions) { final variantNames = {for (final v in union.variants) v.name}; final handled = handledByBloc[union.owner] ?? {}; + final owners = ownersByUnion[union.name]!; + final handledByAnyOwner = { + for (final owner in owners) ...?handledByBloc[owner], + }; final dispatchedHere = dispatched.keys .where(variantNames.contains) .toSet(); + // `on` or `on` handles every subtype, so coverage follows the + // hierarchy: an event is handled when it or any ancestor up to the union + // base is registered, and a handler is live when anything at or below it + // is dispatched. Exact-name matching reported every dispatched subclass + // of a base handler as unhandled (error severity) and the base handler + // itself as dead. + final parentOf = { + for (final variant in union.variants) + if (variant.parent != null) variant.name: variant.parent!, + }; + Iterable selfAndAncestors(String event) sync* { + final seen = {}; + String? current = event; + while (current != null && seen.add(current)) { + yield current; + current = parentOf[current]; + } + } + + bool isHandled(String event) => + selfAndAncestors(event).any(handledByAnyOwner.contains); + bool covers(String handler, String event) => + selfAndAncestors(event).contains(handler); + if (enabled.contains(ruleEventWithoutHandler)) { - for (final event in dispatchedHere.difference(handled)) { + for (final event in dispatchedHere.where((e) => !isHandled(e))) { + // Reported once per shared union, not once per owning bloc. + if (!reportedUnhandled.add('${union.name}.$event')) continue; final sites = dispatched[event]!; findings.add( Finding( @@ -589,7 +627,8 @@ FindingReport runChecks({ severity: PipeSeverity.error, confidence: 'high', message: - '$event is dispatched but ${union.owner} registers no handler ' + '$event is dispatched but ${owners.join(', ')} ' + '${owners.length == 1 ? 'registers' : 'register'} no handler ' 'for it, so the interaction does nothing.', file: sites.first.path, line: sites.first.line, @@ -607,7 +646,9 @@ FindingReport runChecks({ } if (enabled.contains(ruleDeadHandler)) { - for (final event in handled.difference(dispatchedHere)) { + for (final event in handled.where( + (h) => !dispatchedHere.any((e) => covers(h, e)), + )) { final registration = flow.handlers.firstWhere( (handler) => handler.bloc == union.owner && handler.event == event, ); @@ -622,7 +663,8 @@ FindingReport runChecks({ }; final constructedElsewhere = constructedByFile.entries.any( (entry) => - !declaredIn.contains(entry.key) && entry.value.contains(event), + !declaredIn.contains(entry.key) && + entry.value.any((built) => covers(event, built)), ); if (constructedElsewhere) continue; findings.add( diff --git a/lib/src/state_flow.dart b/lib/src/state_flow.dart index c6a3967..1dd0694 100644 --- a/lib/src/state_flow.dart +++ b/lib/src/state_flow.dart @@ -26,16 +26,22 @@ enum UnionStyle { /// Something that can be matched on the state type. class StateVariant { - StateVariant({required this.name, this.declaredIn, this.note}); + StateVariant({required this.name, this.declaredIn, this.note, this.parent}); final String name; final String? declaredIn; final String? note; + /// The direct supertype this variant was enumerated through: the union base + /// or an intermediate group. Null for enum constants and freezed redirects. + /// Rules use it so a handler on a base or group type covers its subtypes. + final String? parent; + Map toJson() => { 'name': name, if (declaredIn != null) 'declaredIn': declaredIn, if (note != null) 'note': note, + if (parent != null) 'parent': parent, }; } @@ -1119,7 +1125,9 @@ class _MentionCollector extends RecursiveAstVisitor { final current = queue.removeLast(); if (!visited.add(current)) continue; for (final type in bySupertype[current] ?? const <_TypeInfo>[]) { - variants.add(StateVariant(name: type.name, declaredIn: type.file)); + variants.add( + StateVariant(name: type.name, declaredIn: type.file, parent: current), + ); queue.add(type.name); } } @@ -1649,13 +1657,13 @@ StateFlow buildStateFlow(Project project, List outcomes) { ); } - // Event unions: the `E` of `Bloc` plus its subclass events. + // Event unions: the `E` of `Bloc` plus its subclass events. One per + // owning bloc: blocs that share an event type each register their own + // handlers, and keeping only the first owner ignored every other bloc's. final eventUnions = []; - final seenEvents = {}; for (final entry in blocEvent.entries) { final eventType = entry.value; if (eventType == null) continue; - if (!seenEvents.add(eventType)) continue; final enumeration = _enumerateVariants( eventType, byName, diff --git a/test/fixtures/stuck_loading/lib/base_event.dart b/test/fixtures/stuck_loading/lib/base_event.dart new file mode 100644 index 0000000..7882e1f --- /dev/null +++ b/test/fixtures/stuck_loading/lib/base_event.dart @@ -0,0 +1,127 @@ +import 'bloc.dart'; + +// --- a handler registered on the event BASE -------------------------------- +// +// `on` with a switch inside handles every subclass. Matching handlers +// to dispatches by exact name would report every dispatched subclass as +// unhandled (error severity) and the base handler itself as dead. This is the +// regression guard for handler coverage following the event hierarchy. + +sealed class SyncEvent { + const SyncEvent(); +} + +class SyncStarted extends SyncEvent { + const SyncStarted(); +} + +class SyncCancelled extends SyncEvent { + const SyncCancelled(); +} + +/// An intermediate group: `on` covers [RetryNow] only. +abstract class RetryEvent extends SyncEvent { + const RetryEvent(); +} + +class RetryNow extends RetryEvent { + const RetryNow(); +} + +class SyncState { + const SyncState(); +} + +class SyncBloc extends Bloc { + SyncBloc() : super(const SyncState()); + + void register() { + on((event, emit) { + switch (event) { + case SyncStarted(): + case SyncCancelled(): + case RetryEvent(): + emit(const SyncState()); + } + }); + } + + void start() => add(const SyncStarted()); +} + +// A second bloc whose only handler is on an intermediate group, so the rule must +// credit dispatches of the group's subclasses to it, and still report a sibling +// outside the group. + +sealed class UploadEvent { + const UploadEvent(); +} + +abstract class UploadRetry extends UploadEvent { + const UploadRetry(); +} + +class UploadRetryNow extends UploadRetry { + const UploadRetryNow(); +} + +/// Dispatched, and outside the handled `UploadRetry` group: truly unhandled. +class UploadAbandoned extends UploadEvent { + const UploadAbandoned(); +} + +class UploadBloc extends Bloc { + UploadBloc() : super(const SyncState()); + + void register() { + on((event, emit) => emit(const SyncState())); + } + + void retry() => add(const UploadRetryNow()); + + void abandon() => add(const UploadAbandoned()); +} + +// --- two blocs sharing one event type --------------------------------------- +// +// Keeping only the first owner of an event type ignored the second bloc's +// handlers: an event only the second bloc handles was reported unhandled (error +// severity), and a dead handler in the second bloc was never seen. + +sealed class ShareEvent { + const ShareEvent(); +} + +class SharePressed extends ShareEvent { + const SharePressed(); +} + +class ShareDismissed extends ShareEvent { + const ShareDismissed(); +} + +/// Handled only by [ShareBetaBloc], and never dispatched: dead there. +class ShareExpired extends ShareEvent { + const ShareExpired(); +} + +class ShareAlphaBloc extends Bloc { + ShareAlphaBloc() : super(const SyncState()); + + void register() { + on((event, emit) => emit(const SyncState())); + } + + void press() => add(const SharePressed()); +} + +class ShareBetaBloc extends Bloc { + ShareBetaBloc() : super(const SyncState()); + + void register() { + on((event, emit) => emit(const SyncState())); + on((event, emit) => emit(const SyncState())); + } + + void dismiss() => add(const ShareDismissed()); +} diff --git a/test/rule_contract_test.dart b/test/rule_contract_test.dart index d0f8cec..0b55c7b 100644 --- a/test/rule_contract_test.dart +++ b/test/rule_contract_test.dart @@ -49,8 +49,21 @@ const Map> _negatives = { // event rules actually consult the handler and dispatch information. // NeverDispatched (never handled) and UnhandledTap (never handled) were // vacuous. - ruleEventWithoutHandler: [('event', 'HandledTap')], - ruleDeadHandler: [('event', 'HandledTap')], + // SyncStarted/UploadRetryNow are handled through `on` and + // `on`; those base and group handlers are live because a + // subtype is dispatched (base_event.dart). + ruleEventWithoutHandler: [ + ('event', 'HandledTap'), + ('event', 'SyncStarted'), + ('event', 'UploadRetryNow'), + // Handled by ShareBetaBloc, which shares ShareEvent with ShareAlphaBloc. + ('event', 'ShareDismissed'), + ], + ruleDeadHandler: [ + ('event', 'HandledTap'), + ('event', 'SyncEvent'), + ('event', 'UploadRetry'), + ], // handledProvider covers all AsyncValue cases. ruleRiverpodAsyncGap: [('provider', 'handledProvider')], // `bump` notifies; `setQuiet` mutates but a notifying method calls it. @@ -96,6 +109,27 @@ void main() { } } + test('dead-handler: every bloc sharing an event type is checked', () { + // Only the first owner of ShareEvent used to be kept, so ShareBetaBloc's + // handlers were never examined. + final owners = { + for (final finding in findings) + if (finding.rule == ruleDeadHandler && + finding.data['event'] == 'ShareExpired') + finding.data['owner'], + }; + expect(owners, {'ShareBetaBloc'}); + }); + + test('event-without-handler: a group handler does not cover a sibling', () { + // UploadAbandoned sits outside the handled UploadRetry group, so following + // the hierarchy must not over-credit it. + expect( + valuesOf(ruleEventWithoutHandler, 'event'), + contains('UploadAbandoned'), + ); + }); + for (final entry in _positives.entries) { test('${entry.key}: fires on the positive control', () { final (key, value) = entry.value; diff --git a/test/state_flow_test.dart b/test/state_flow_test.dart index 04d4688..0e31711 100644 --- a/test/state_flow_test.dart +++ b/test/state_flow_test.dart @@ -103,11 +103,14 @@ void main() { final hits = report.findings .where((finding) => finding.rule == ruleEventWithoutHandler) .toList(); - // ItemBloc registers handlers for both of its events, so the only report - // must be the deliberate gap fixture. - expect(hits, hasLength(1)); - expect(hits.single.data['event'], 'UnhandledTap'); - expect(hits.single.data['owner'], 'GapBloc'); + // ItemBloc registers handlers for both of its events, and SyncBloc/UploadBloc + // cover their other events through base and group handlers, so the only + // reports must be the two deliberate gaps. + expect( + {for (final hit in hits) '${hit.data['owner']}.${hit.data['event']}'}, + {'GapBloc.UnhandledTap', 'UploadBloc.UploadAbandoned'}, + ); + expect(hits, hasLength(2)); }); test('every finding carries evidence', () { -- 2.52.0 From 8d64718977088c64b43c4be7b20f01fd6465ccda Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 13:30:31 -0600 Subject: [PATCH 02/14] 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` 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` handler closures were all named ``, so two handlers in one class shared a fingerprint and encodeBaseline kept only one of them. They are now named `on`. Existing baselines that accepted a handler as `` 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 `` findings; after: on, on. --- lib/src/rules.dart | 75 +++++++++++++++++-- .../stuck_loading/lib/flag_cubit.dart | 33 ++++++++ test/flag_state_test.dart | 26 ++++--- test/rule_contract_test.dart | 25 +++++++ 4 files changed, 143 insertions(+), 16 deletions(-) diff --git a/lib/src/rules.dart b/lib/src/rules.dart index f981082..2b91cb9 100644 --- a/lib/src/rules.dart +++ b/lib/src/rules.dart @@ -1083,10 +1083,25 @@ class _FlagLoadingCollector extends RecursiveAstVisitor { @override void visitFunctionExpression(FunctionExpression node) { - _analyze(node.body, ''); + _analyze(node.body, _closureName(node)); super.visitFunctionExpression(node); } + /// `on` for a bloc handler closure, so two handlers in one class get + /// distinct identities. As a bare `` 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 ''; + } + void _analyze(FunctionBody? body, String member) { if (body == null) return; final owner = _classStack.isEmpty ? '' : _classStack.last; @@ -1095,13 +1110,17 @@ class _FlagLoadingCollector extends RecursiveAstVisitor { // 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 { 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 { 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 { } class _HasAwait extends RecursiveAstVisitor { + _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 { 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 { + _SetsFlagTrue(this.fields); + + final Set 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 { diff --git a/test/fixtures/stuck_loading/lib/flag_cubit.dart b/test/fixtures/stuck_loading/lib/flag_cubit.dart index 0a7c2b8..bfa7010 100644 --- a/test/fixtures/stuck_loading/lib/flag_cubit.dart +++ b/test/fixtures/stuck_loading/lib/flag_cubit.dart @@ -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 [], this.loading = true}); @@ -43,4 +47,33 @@ class FlagCubit extends Bloc { Future refreshQuietly() async { await Future.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.delayed(Duration.zero); + } catch (error) { + emit(state.copyWith(loading: false)); + } + }); + } + + void _later(Future Function() work) {} + + /// BAD, twice: two unguarded handlers in one class. Each is its own finding + /// with its own fingerprint, not one `` that hides the other. + void registerHandlers() { + on((event, emit) async { + emit(state.copyWith(loading: true)); + await Future.delayed(Duration.zero); + }); + on((event, emit) async { + emit(state.copyWith(loading: true)); + await Future.delayed(Duration.zero); + }); + } } diff --git a/test/flag_state_test.dart b/test/flag_state_test.dart index 1038311..8cf1c74 100644 --- a/test/flag_state_test.dart +++ b/test/flag_state_test.dart @@ -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', + 'on', + }); + 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', () { diff --git a/test/rule_contract_test.dart b/test/rule_contract_test.dart index 0b55c7b..17a4714 100644 --- a/test/rule_contract_test.dart +++ b/test/rule_contract_test.dart @@ -40,10 +40,16 @@ const Map> _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 ``. ruleLoadingFlagNotReset: [ ('member', 'loadGuarded'), ('member', 'refreshQuietly'), ('member', '_warmUp'), + ('member', 'scheduleGuarded'), + ('member', 'registerHandlers'), + ('member', ''), ], // 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 `` 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(['on', 'on']), + ); + 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; -- 2.52.0 From c065576e961a3bfb702c531f3d0887127a6c8020 Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 13:30:41 -0600 Subject: [PATCH 03/14] fix(cli): refuse subset baselines and scope verify to --rule Two more ways a partial scan produced a false claim, the same hazard class as the --limit and unreadable-file cases verify already refuses: - `verify --rule X` compared the fresh X-only report against the whole baseline, so every accepted finding from any other rule was printed as RESOLVED ("these fixes are confirmed") when it had merely not been checked. The baseline is now filtered to the selected rules. - `--write-baseline` (check and guide) wrote whatever the run produced. Under --rule or --limit, or with files that could not be analysed, the baseline silently omitted every finding outside that subset, and the next full `check --baseline` reported them all as new. It now refuses: --rule/--limit before analysis, unreadable files after it. Each is covered by a test that fails against the previous cli.dart (subset_scan_test.dart, and the unreadable-file case in unreadable_test.dart). README documents the refusal. --- README.md | 6 ++ lib/src/cli.dart | 52 ++++++++++++++++-- test/subset_scan_test.dart | 109 +++++++++++++++++++++++++++++++++++++ test/unreadable_test.dart | 30 ++++++++++ 4 files changed, 193 insertions(+), 4 deletions(-) create mode 100644 test/subset_scan_test.dart diff --git a/README.md b/README.md index d8728f9..1adea9c 100644 --- a/README.md +++ b/README.md @@ -110,6 +110,12 @@ pipe check . --write-baseline .pipe-baseline.json # accept current state pipe check . --baseline .pipe-baseline.json --fail-on warning ``` +`--write-baseline` refuses a partial scan (`--rule`, `--limit`, or files that +could not be analysed): a baseline written from a subset omits every finding +outside it, and the next full run would report them all as new. `verify --rule` +likewise compares only that rule's accepted findings, instead of calling every +other rule's findings resolved. + `--fail-on none|info|warning|error` (default `none`) fails the build when a **new** finding at or above that severity is present. Baselined findings never trip the gate, so a ratcheted repo stays green while a regression fails. diff --git a/lib/src/cli.dart b/lib/src/cli.dart index 9edc235..b6bbfbe 100644 --- a/lib/src/cli.dart +++ b/lib/src/cli.dart @@ -106,9 +106,43 @@ Set? _readBaseline(ArgResults results) { return decodeBaseline(file.readAsStringSync()); } -void _writeBaseline(ArgResults results, String root, List findings) { +/// A baseline is the set of findings a repo accepts, so it must come from a +/// complete scan with every rule on. Written from a subset, every finding +/// outside that subset is missing from it, and the next full `check --baseline` +/// reports all of them as new. Refused before analysis so the flags fail fast. +void _refuseSubsetBaseline(ArgResults results) { + if (results['write-baseline'] == null) return; + final narrowing = [ + if ((results['rule'] as List).isNotEmpty) '--rule', + if (results['limit'] != null) '--limit', + ]; + if (narrowing.isEmpty) return; + throw PipeException( + '--write-baseline needs a complete scan: with ${narrowing.join(' and ')} ' + 'the baseline would omit every finding outside that subset', + remedy: 'drop ${narrowing.join(' and ')} when writing a baseline', + ); +} + +void _writeBaseline( + ArgResults results, + ProjectAnalysis analysis, + List findings, +) { final path = results['write-baseline'] as String?; if (path == null) return; + // Findings in a file that could not be analysed are absent, not fixed. + final unresolved = analysis.run.outcomes + .where((outcome) => !outcome.resolved && !outcome.skipped) + .length; + if (unresolved > 0) { + throw PipeException( + '--write-baseline needs a complete scan: $unresolved file(s) could not ' + 'be analysed, so their findings would be missing from the baseline', + remedy: 'run `pipe index` to see why they failed, then write it again', + ); + } + final root = analysis.project.root; File(path).writeAsStringSync(encodeBaseline(root, findings)); stdout.writeln( 'wrote baseline to $path (${findings.length} findings accepted)', @@ -353,6 +387,7 @@ class CheckCommand extends Command { @override Future run() async { final results = argResults!; + _refuseSubsetBaseline(results); final project = await loadProject( _target(results), directories: results['dir'] as List, @@ -377,7 +412,7 @@ class CheckCommand extends Command { final top = readPositiveInt(results, 'top', fallback: 40); stdout.write(report.describe(top: top)); } - _writeBaseline(results, project.root, report.findings); + _writeBaseline(results, analysis, report.findings); final threshold = switch (results['fail-on'] as String) { 'error' => PipeSeverity.error, 'warning' => PipeSeverity.warning, @@ -545,9 +580,17 @@ class VerifyCommand extends Command { ); } + // With --rule, only the selected rules ran, so an accepted finding from + // any other rule is merely unchecked. Comparing against the whole baseline + // printed every one of them as RESOLVED. final report = verifyAgainstBaseline( report: analysis.report, - baseline: entries, + baseline: selected.isEmpty + ? entries + : [ + for (final entry in entries) + if (selected.contains(entry.rule)) entry, + ], ); if (results['format'] as String == 'json') { stdout.writeln(_encode(report.toJson())); @@ -691,6 +734,7 @@ class GuideCommand extends Command { @override Future run() async { final results = argResults!; + _refuseSubsetBaseline(results); final project = await loadProject( _target(results), directories: results['dir'] as List, @@ -750,7 +794,7 @@ class GuideCommand extends Command { } stdout.writeln('persisted to yams as $name'); } - _writeBaseline(results, project.root, analysis.report.findings); + _writeBaseline(results, analysis, analysis.report.findings); } finally { await analysis.dispose(); } diff --git a/test/subset_scan_test.dart b/test/subset_scan_test.dart new file mode 100644 index 0000000..44cf02b --- /dev/null +++ b/test/subset_scan_test.dart @@ -0,0 +1,109 @@ +import 'dart:convert'; +import 'dart:io'; + +import 'package:pipe/pipe.dart'; +import 'package:pipe/src/cli.dart'; +import 'package:test/test.dart'; + +/// Collects what a command writes to stdout. +class _CapturedStdout implements Stdout { + final StringBuffer buffer = StringBuffer(); + + @override + void write(Object? object) => buffer.write(object); + + @override + void writeln([Object? object = '']) => buffer.writeln(object); + + @override + dynamic noSuchMethod(Invocation invocation) => null; +} + +Future<(int?, String)> _run(List args) async { + final out = _CapturedStdout(); + final code = await IOOverrides.runZoned( + () => PipeRunner().run(args), + stdout: () => out, + ); + return (code, out.buffer.toString()); +} + +const String _fixture = 'test/fixtures/stuck_loading'; + +void main() { + late String baseline; + + setUpAll(() async { + final dir = Directory.systemTemp.createTempSync('pipe-subset'); + baseline = '${dir.path}/baseline.json'; + final (code, _) = await _run([ + 'check', + _fixture, + '--write-baseline', + baseline, + '-c', + '1', + ]); + expect(code, 0); + }); + + test('verify --rule does not call other rules\' findings resolved', () async { + // Only dead-handler ran, so accepted findings from every other rule were + // unchecked, not fixed. Comparing against the whole baseline reported each + // of them as RESOLVED - a false claim about someone's fixes. + final entries = decodeBaselineEntries(File(baseline).readAsStringSync()); + expect( + entries.map((entry) => entry.rule).toSet(), + hasLength(greaterThan(1)), + reason: 'the control needs findings from more than the selected rule', + ); + + final (_, output) = await _run([ + 'verify', + _fixture, + '--baseline', + baseline, + '--rule', + ruleDeadHandler, + '--format', + 'json', + '-c', + '1', + ]); + final report = jsonDecode(output) as Map; + expect(report['resolved'], 0); + expect(report['new'], 0); + expect( + report['baselineTotal'], + entries.where((entry) => entry.rule == ruleDeadHandler).length, + ); + }); + + for (final narrowing in [ + ['--rule', ruleDeadHandler], + ['--limit', '3'], + ]) { + test('--write-baseline refuses ${narrowing.first}', () async { + final target = '${File(baseline).parent.path}/subset.json'; + await expectLater( + _run([ + 'check', + _fixture, + ...narrowing, + '--write-baseline', + target, + '-c', + '1', + ]), + throwsA( + isA().having( + (error) => error.toString(), + 'message', + contains('complete scan'), + ), + ), + ); + expect(File(target).existsSync(), isFalse); + }); + } +} diff --git a/test/unreadable_test.dart b/test/unreadable_test.dart index af3c90e..4cd18ee 100644 --- a/test/unreadable_test.dart +++ b/test/unreadable_test.dart @@ -72,6 +72,36 @@ void main() { ); }); + test('--write-baseline refuses a partially readable project', () async { + // The unread file's findings would be missing from the accepted set, so the + // next complete scan would report them all as new. + final root = unreadableCorpus('write-partial', 'lib/item_bloc.dart'); + if (root == null) { + markTestSkipped('chmod 000 does not restrict this process'); + return; + } + + final baseline = '$root/baseline.json'; + await expectLater( + PipeRunner().run([ + 'check', + root, + '--write-baseline', + baseline, + '-c', + '1', + ]), + throwsA( + isA().having( + (error) => error.toString(), + 'message', + contains('complete scan'), + ), + ), + ); + expect(File(baseline).existsSync(), isFalse); + }); + test('recall keeps evaluating cases whose evidence is readable', () async { // A repo with one unreadable file must not lose coverage: a hit stands, // because evidence that was read is evidence. Skipping every case for the -- 2.52.0 From 682be14f97819b2fb78cbde45929678c3d52139b Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 13:30:49 -0600 Subject: [PATCH 04/14] fix(adjudicate): fail when uncertified, and state its independence limit The adjudicator printed "controls FAILED" or "NO SIGNAL" and then exited 0, so a CI step reading the exit status took an uncertified run as a pass. It now exits 1 unless the controls are meaningful and pass, in both text and json output. It was also described as independent. Its evidence is independent (its own regex over source), but the list of unions, variants, events and owners it checks comes from pipe's `state` output, so a variant pipe fails to enumerate is never checked. The docstring and README now say so instead of "independent adjudicator". --- README.md | 10 +++++++--- tool/adjudicate.py | 15 +++++++++++---- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 1adea9c..994b5d7 100644 --- a/README.md +++ b/README.md @@ -153,7 +153,7 @@ Baselines and reports are JSON, so they diff and review like any other file. | Rule | What it means | Verified precision (with n) | | --- | --- | --- | -| `emitted-not-rendered` | A state the bloc emits that no UI code matches, so the user never sees it. | **79/79 (100%)** factual on a CLEAN nuwayze main via an independent adjudicator with passing controls (`tool/adjudicate.py`). Reported once per **root cause**, not once per variant: one finding per unobserved union (dormant feature) and one per marker class when the union is observed. When the UI reads the union's payload through **base-class fields** the finding drops to info, because the data does reach the user. On clean nuwayze main: 30 findings covering 79 variant-level claims. | +| `emitted-not-rendered` | A state the bloc emits that no UI code matches, so the user never sees it. | **79/79 (100%)** factual on a CLEAN nuwayze main via a separate regex adjudicator with passing controls (`tool/adjudicate.py`; see its independence limit below). Reported once per **root cause**, not once per variant: one finding per unobserved union (dormant feature) and one per marker class when the union is observed. When the UI reads the union's payload through **base-class fields** the finding drops to info, because the data does reach the user. On clean nuwayze main: 30 findings covering 79 variant-level claims. | | `rendered-not-emitted` | A UI branch no state can reach: dead UI. | n=0 on nuwayze (unions whose bloc emits through helper methods are suppressed with a reason). Fires on a fixture with a passing negative control. | | `loading-not-reset` | An in-flight state whose failure path never resolves, so the spinner sticks. | n=0 on both dogfood repos, both verified to guard correctly; proven able to fire by fixture. | | `loading-flag-not-reset` | The same consequence for `copyWith` + `bool loading` state, which has no variant to match on. | **7/7 (100%)** on dex after hand adjudication; 4 characterised false positives were eliminated by the current gate. n=0 on nuwayze, verified correct. | @@ -168,8 +168,12 @@ Baselines and reports are JSON, so they diff and review like any other file. ### How precision was measured -`tool/adjudicate.py ` re-derives the two highest-volume claims without -trusting pipe's bookkeeping, and **prints controls first**. It must find evidence +`tool/adjudicate.py ` re-derives the two highest-volume claims with its +own regex evidence instead of pipe's, and **prints controls first**. It is only +partly independent: it takes the list of unions, variants, events and owners to +check from pipe's `state` output, so a variant pipe fails to enumerate is never +checked. It exits non-zero when its controls fail or cannot discriminate, so a +CI step cannot mistake an uncertified run for a pass. It must find evidence for every variant pipe calls *rendered* and every event pipe calls *dispatched*; on clean nuwayze main that is 131/131 rendered (type tests, `case`/switch- expression patterns, or base-class payload reads) and 143/162 dispatched, the diff --git a/tool/adjudicate.py b/tool/adjudicate.py index 515e35e..a43b9f4 100644 --- a/tool/adjudicate.py +++ b/tool/adjudicate.py @@ -1,9 +1,12 @@ #!/usr/bin/env python3 -"""Independent adjudicator for pipe's two highest-volume rules. +"""Adjudicator for pipe's two highest-volume rules. Usage: python3 tool/adjudicate.py [--format json] +Exit status: 0 when the controls pass, 1 when they fail or cannot discriminate +(NO SIGNAL). A CI step must not read an uncertified run as a pass. + Precision claims are only meaningful if the thing doing the judging can demonstrate it discriminates. So this script prints CONTROLS first: it must find a UI type test for every variant pipe calls *rendered*, and a construction for @@ -18,7 +21,10 @@ dead-handler : event E is never constructed or dispatched in any file other than the one that declares E. It classifies every mention of every state/event name by regex on the source -line, so it does not trust pipe's own bookkeeping. It does deliberately NOT +line, so it does not trust pipe's evidence judgements. It is only partly +independent, though: the list of unions, variants, events and owners to check +comes from pipe's own `state` output, so a variant pipe fails to enumerate is +never checked here at all. It does deliberately NOT judge UX severity: whether an unrendered marker state matters is a judgement, and whether an unbound handler is worth deleting is a judgement. Only the factual claims are adjudicated. @@ -400,9 +406,10 @@ def main() -> int: ), } + certified = result["controls"]["controls_meaningful"] and result["controls"]["controls_pass"] if args.format == "json": print(json.dumps(result, indent=2)) - return 0 + return 0 if certified else 1 c = result["controls"] print(f"repo {root}") @@ -433,7 +440,7 @@ def main() -> int: print(f" CONTRADICTED {bad}") print() print("caveat: " + result["caveat"]) - return 0 + return 0 if certified else 1 if __name__ == "__main__": -- 2.52.0 From 70262a8c2f46fd364ea592a21818fbdbd310c3a8 Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 13:36:10 -0600 Subject: [PATCH 05/14] fix(sdk): refuse an unreadable SDK clearly, and fix the runtime fallback An SDK directory that exists but cannot be read passed discovery, and the analyzer then died with a raw FileSystemException stack trace. This happens for real under macOS privacy protection: a Flutter SDK under ~/Documents stats fine but every open fails, and every app whose package_config.json names that flutterRoot crashed: Unhandled exception: FileSystemException(path=.../dart-sdk/lib/_internal/allowed_experiments.json; ...) The chosen SDK is now probed before the analysis collection is built, and an unreadable one is a PipeException naming the path, where it was discovered from, and the --sdk remedy. The last-resort fallback to the running Dart SDK took the executable's directory (/bin) as the SDK root, which never contains lib/libraries.json, so it could never succeed. sdkFromExecutable now maps /bin/dart to . --- lib/src/analysis.dart | 1 + lib/src/sdk.dart | 42 +++++++++++++++++++++++++++-- test/sdk_test.dart | 61 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 test/sdk_test.dart diff --git a/lib/src/analysis.dart b/lib/src/analysis.dart index 7553a79..e2846ac 100644 --- a/lib/src/analysis.dart +++ b/lib/src/analysis.dart @@ -55,6 +55,7 @@ class Analyzer { explicit: sdkOverride, flutterRootHint: project.flutterRoot, ); + requireReadableSdk(resolution); final collection = AnalysisContextCollection( includedPaths: [project.root], resourceProvider: PhysicalResourceProvider.INSTANCE, diff --git a/lib/src/sdk.dart b/lib/src/sdk.dart index 58c7ee3..8ea5cac 100644 --- a/lib/src/sdk.dart +++ b/lib/src/sdk.dart @@ -45,6 +45,44 @@ String? _normalizedDirectory(String? path) { return _isDirectory(path) ? p.normalize(p.absolute(path)) : null; } +/// The SDK root holding a `/bin/dart` executable, or null when the +/// executable does not sit inside a Dart SDK. +/// +/// The executable's own directory is `/bin`, which never contains +/// `lib/libraries.json`; checking it meant this fallback could never succeed. +String? sdkFromExecutable(String executable) { + final root = p.dirname(p.dirname(p.normalize(executable))); + return _isDartSdk(root) ? root : null; +} + +/// Fails with an actionable error when the chosen SDK cannot be read. +/// +/// Existence is not enough: macOS privacy protection (for example a Flutter SDK +/// under ~/Documents) lets `stat` succeed while every open fails, so discovery +/// accepted the SDK and the analyzer later died with a raw FileSystemException. +void requireReadableSdk(SdkResolution resolution) { + final sdk = resolution.sdkPath; + if (sdk == null) return; + for (final probe in [ + p.join(sdk, 'version'), + p.join(sdk, 'lib', 'libraries.json'), + ]) { + final file = File(probe); + if (!file.existsSync()) continue; + try { + file.openSync().closeSync(); + } on FileSystemException catch (error) { + throw PipeException( + 'the Dart SDK at $sdk (from ${resolution.source}) cannot be read: ' + '${error.osError?.message ?? error.message}', + remedy: + 'grant this process access to that directory, or pass ' + '--sdk ', + ); + } + } +} + /// Given a Flutter root, return its bundled Dart SDK if one is present. String? dartSdkInFlutterRoot(String flutterRoot) { final candidate = p.join(flutterRoot, 'bin', 'cache', 'dart-sdk'); @@ -144,8 +182,8 @@ SdkResolution resolveSdk({String? explicit, String? flutterRootHint}) { ); } - final runtimeSdk = p.dirname(p.normalize(Platform.resolvedExecutable)); - if (_isDartSdk(runtimeSdk)) { + final runtimeSdk = sdkFromExecutable(Platform.resolvedExecutable); + if (runtimeSdk != null) { return SdkResolution( sdkPath: runtimeSdk, flutterRoot: null, diff --git a/test/sdk_test.dart b/test/sdk_test.dart new file mode 100644 index 0000000..d7bbf84 --- /dev/null +++ b/test/sdk_test.dart @@ -0,0 +1,61 @@ +import 'dart:io'; + +import 'package:pipe/pipe.dart'; +import 'package:pipe/src/sdk.dart'; +import 'package:test/test.dart'; + +/// A directory laid out like a Dart SDK: `bin/dart`, `lib/libraries.json`, +/// `version`. +Directory _fakeSdk(String label) { + final sdk = Directory.systemTemp.createTempSync('pipe-$label'); + File('${sdk.path}/version').writeAsStringSync('3.13.0\n'); + File('${sdk.path}/lib/libraries.json') + ..createSync(recursive: true) + ..writeAsStringSync('{}'); + File('${sdk.path}/bin/dart').createSync(recursive: true); + return sdk; +} + +void main() { + test('an SDK that exists but cannot be read is a clear error', () { + // macOS privacy protection (a Flutter SDK under ~/Documents) lets stat + // succeed while every open fails, so discovery accepted the SDK and the + // analyzer later died with a raw FileSystemException stack trace. + final sdk = _fakeSdk('unreadable-sdk'); + final version = File('${sdk.path}/version'); + Process.runSync('chmod', ['000', version.path]); + try { + version.readAsStringSync(); + markTestSkipped('chmod 000 does not restrict this process'); + return; + } on FileSystemException { + // Expected: the restriction is real, so the check below is meaningful. + } + + final resolution = resolveSdk(explicit: sdk.path); + expect( + () => requireReadableSdk(resolution), + throwsA( + isA() + .having((e) => e.message, 'message', contains(sdk.path)) + .having((e) => e.remedy, 'remedy', contains('--sdk')), + ), + ); + }); + + test('a readable SDK passes the check', () { + final sdk = _fakeSdk('readable-sdk'); + expect( + () => requireReadableSdk(resolveSdk(explicit: sdk.path)), + returnsNormally, + ); + }); + + test('the running dart binary maps to its SDK root, not to bin/', () { + // dirname(/bin/dart) is /bin, which never contains + // lib/libraries.json, so this fallback could never succeed. + final sdk = _fakeSdk('runtime-sdk'); + expect(sdkFromExecutable('${sdk.path}/bin/dart'), sdk.path); + expect(sdkFromExecutable('${sdk.path}/version'), isNull); + }); +} -- 2.52.0 From 6e2d47f7db9d9a285c948f903dd1d9bbf5d88554 Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 13:39:31 -0600 Subject: [PATCH 06/14] fix(state-flow): credit freezed factory calls to their redirect target freezed declares variants as redirecting factories on the base (`const factory DocState.loading() = DocLoading;`) and blocs emit through them. `emit(const DocState.loading())` was recorded as emitting `DocState`, which is never a variant, so no freezed emission matched and every variant the UI renders was reported by rendered-not-emitted as a dead branch. Factory redirects are now also kept by constructor name, and an instance creation of `Type.ctor` (or an unnamed redirecting factory) is credited to the class it redirects to, for emissions, `state =` assignments and dispatches alike. Reproduced on freezed_style.dart before the fix: DocLoading, DocReady and DocStale all reported as never emitted; after, only DocStale, which really is never emitted and stays as the positive control. No change on nuwayze, tick, dex or word_alive. --- lib/src/state_flow.dart | 88 +++++++++++++++---- .../stuck_loading/lib/freezed_style.dart | 41 +++++++++ .../stuck_loading/lib/freezed_ui.dart | 17 ++++ test/rule_contract_test.dart | 14 ++- 4 files changed, 144 insertions(+), 16 deletions(-) create mode 100644 test/fixtures/stuck_loading/lib/freezed_style.dart create mode 100644 test/fixtures/stuck_loading/lib/freezed_ui.dart diff --git a/lib/src/state_flow.dart b/lib/src/state_flow.dart index 1dd0694..8ab5f05 100644 --- a/lib/src/state_flow.dart +++ b/lib/src/state_flow.dart @@ -478,6 +478,7 @@ class _TypeInfo { required this.isSealed, required this.enumConstants, required this.factoryRedirects, + this.namedRedirects = const {}, this.loadingFields = const [], this.loadingDefaultsTrue = false, this.fields = const [], @@ -493,6 +494,10 @@ class _TypeInfo { final List enumConstants; final List factoryRedirects; + /// Factory constructor name (`''` for the unnamed one) to its redirect + /// target, so `DocState.loading()` can be credited to the variant it builds. + final Map namedRedirects; + /// Loading-style boolean fields declared on the type. final List loadingFields; @@ -609,8 +614,14 @@ class _TypeCollector extends RecursiveAstVisitor { /// Read freezed-style `factory X.y(...) = Z;` redirect targets from source. /// Parsed textually because the AST accessor is experimental. - static List _factoryRedirects(NodeList members) { - final redirects = []; + static List _factoryRedirects(NodeList members) => + _namedFactoryRedirects(members).values.toList(); + + /// Same parse, keyed by constructor name (`''` for the unnamed factory). + static Map _namedFactoryRedirects( + NodeList members, + ) { + final redirects = {}; for (final member in members) { if (member is! ConstructorDeclaration) continue; if (member.factoryKeyword == null) continue; @@ -619,7 +630,9 @@ class _TypeCollector extends RecursiveAstVisitor { r'=\s*([A-Za-z_$][A-Za-z0-9_$.]*)', ).firstMatch(declaration); final target = match?.group(1); - if (target != null) redirects.add(_baseName(target)); + if (target != null) { + redirects[member.name?.lexeme ?? ''] = _baseName(target); + } } return redirects; } @@ -644,6 +657,7 @@ class _TypeCollector extends RecursiveAstVisitor { isSealed: node.sealedKeyword != null, enumConstants: const [], factoryRedirects: _factoryRedirects(node.body.members), + namedRedirects: _namedFactoryRedirects(node.body.members), loadingFields: loadingFields, loadingDefaultsTrue: loadingDefaultsTrue, fields: _declaredFields(node.body.members), @@ -674,9 +688,16 @@ class _TypeCollector extends RecursiveAstVisitor { } class _EmissionCollector extends RecursiveAstVisitor { - _EmissionCollector(this.project, this.path, this.lineInfo, this.sites); + _EmissionCollector( + this.project, + this.path, + this.lineInfo, + this.sites, + this.redirects, + ); final Project project; + final Map redirects; final String path; final LineInfo lineInfo; final List sites; @@ -731,11 +752,11 @@ class _EmissionCollector extends RecursiveAstVisitor { /// Arguments arrive as [Argument] wrappers; positional arguments are also /// [Expression]s, which is how we get at the emitted value. - static _Extraction _fromArguments(List arguments) { + _Extraction _fromArguments(List arguments) { if (arguments.isEmpty) return _Extraction(const [], 'unknown'); final argument = arguments.first; return argument is Expression - ? _extractVariants(argument) + ? _extractVariants(argument, redirects) : _Extraction(const [], 'unknown'); } @@ -775,7 +796,7 @@ class _EmissionCollector extends RecursiveAstVisitor { @override void visitAssignmentExpression(AssignmentExpression node) { if (node.leftHandSide.toSource().trim() == 'state') { - final extraction = _extractVariants(node.rightHandSide); + final extraction = _extractVariants(node.rightHandSide, redirects); sites.add( EmissionSite( path: project.relative(path), @@ -805,12 +826,15 @@ class _Extraction { } class _VariantFinder extends RecursiveAstVisitor { + _VariantFinder(this.redirects); + + final Map redirects; final List found = []; var sawCopyWith = false; @override void visitInstanceCreationExpression(InstanceCreationExpression node) { - final name = node.constructorName.type.name.lexeme; + final name = _createdVariant(node, redirects); if (!found.contains(name)) found.add(name); super.visitInstanceCreationExpression(node); } @@ -822,15 +846,32 @@ class _VariantFinder extends RecursiveAstVisitor { } } -_Extraction _extractVariants(Expression expression) { +/// The variant an instance creation builds. A freezed-style factory +/// (`const factory DocState.loading() = DocLoading;`) builds its redirect +/// target, not the base type it is called on, so `DocState.loading()` is +/// credited to `DocLoading`. Recording `DocState` meant no freezed emission ever +/// matched its variant. +String _createdVariant( + InstanceCreationExpression node, + Map redirects, +) { + final type = node.constructorName.type.name.lexeme; + final constructor = node.constructorName.name?.name ?? ''; + return redirects['$type.$constructor'] ?? type; +} + +_Extraction _extractVariants( + Expression expression, + Map redirects, +) { if (expression is InstanceCreationExpression) { - return _Extraction([expression.constructorName.type.name.lexeme], 'exact'); + return _Extraction([_createdVariant(expression, redirects)], 'exact'); } if (expression is MethodInvocation && expression.methodName.name == 'copyWith') { return _Extraction(const [], 'copyWith'); } - final finder = _VariantFinder(); + final finder = _VariantFinder(redirects); expression.accept(finder); if (finder.sawCopyWith && finder.found.isEmpty) { return _Extraction(const [], 'copyWith'); @@ -1197,9 +1238,16 @@ class _HandlerCollector extends RecursiveAstVisitor { /// Records `add(Event())` dispatch sites. class _DispatchCollector extends RecursiveAstVisitor { - _DispatchCollector(this.project, this.path, this.lineInfo, this.dispatches); + _DispatchCollector( + this.project, + this.path, + this.lineInfo, + this.dispatches, + this.redirects, + ); final Project project; + final Map redirects; final String path; final LineInfo lineInfo; final List dispatches; @@ -1211,7 +1259,7 @@ class _DispatchCollector extends RecursiveAstVisitor { if (arguments.isNotEmpty) { final argument = arguments.first; if (argument is Expression) { - final extraction = _extractVariants(argument); + final extraction = _extractVariants(argument, redirects); for (final variant in extraction.variants) { dispatches.add( EventDispatch( @@ -1520,6 +1568,12 @@ StateFlow buildStateFlow(Project project, List outcomes) { for (final type in types) if (type.isEnum) type.name: type, }; + // `Type.constructor` -> the class a redirecting factory actually builds. + final redirects = { + for (final type in types) + for (final entry in type.namedRedirects.entries) + '${type.name}.${entry.key}': entry.value, + }; // Bloc/cubit owners and their state types. final blocState = {}; @@ -1693,10 +1747,14 @@ StateFlow buildStateFlow(Project project, List outcomes) { final result = entry.value; final unit = result.unit; final lineInfo = result.lineInfo; - unit.accept(_EmissionCollector(project, entry.key, lineInfo, emissions)); + unit.accept( + _EmissionCollector(project, entry.key, lineInfo, emissions, redirects), + ); unit.accept(_RenderCollector(project, entry.key, lineInfo, renders)); unit.accept(_HandlerCollector(project, entry.key, lineInfo, handlers)); - unit.accept(_DispatchCollector(project, entry.key, lineInfo, dispatches)); + unit.accept( + _DispatchCollector(project, entry.key, lineInfo, dispatches, redirects), + ); unit.accept( _RiverpodCollector( project, diff --git a/test/fixtures/stuck_loading/lib/freezed_style.dart b/test/fixtures/stuck_loading/lib/freezed_style.dart new file mode 100644 index 0000000..6830092 --- /dev/null +++ b/test/fixtures/stuck_loading/lib/freezed_style.dart @@ -0,0 +1,41 @@ +import 'bloc.dart'; + +// --- a freezed-style union -------------------------------------------------- +// +// freezed declares variants as redirecting factories on the base, and the bloc +// emits through them: `emit(const DocState.loading())` builds a DocLoading. +// Recording the base type (`DocState`) meant no emission ever matched a variant, +// so every variant the UI renders was reported as never emitted. The variant +// classes are written by hand here, as the generated file would declare them, +// because this fixture has no dependencies and no codegen. + +class DocEvent {} + +sealed class DocState { + const DocState(); + + const factory DocState.loading() = DocLoading; + const factory DocState.ready() = DocReady; + + /// Declared, rendered, and never emitted: the positive control that proves + /// redirect resolution did not simply suppress the rule. + const factory DocState.stale() = DocStale; +} + +class DocLoading extends DocState { + const DocLoading(); +} + +class DocReady extends DocState { + const DocReady(); +} + +class DocStale extends DocState { + const DocStale(); +} + +class DocBloc extends Bloc { + DocBloc() : super(const DocState.loading()); + + void load() => emit(const DocState.ready()); +} diff --git a/test/fixtures/stuck_loading/lib/freezed_ui.dart b/test/fixtures/stuck_loading/lib/freezed_ui.dart new file mode 100644 index 0000000..83142b5 --- /dev/null +++ b/test/fixtures/stuck_loading/lib/freezed_ui.dart @@ -0,0 +1,17 @@ +// UI for freezed_style.dart, kept in its own file because type tests in a file +// that declares a bloc do not count as UI. + +import 'bloc.dart'; +import 'freezed_style.dart'; + +class DocView { + const DocView(); + + Object build() => BlocBuilder( + builder: (context, state) => switch (state) { + DocLoading() => 'loading', + DocReady() => 'ready', + DocStale() => 'stale', + }, + ); +} diff --git a/test/rule_contract_test.dart b/test/rule_contract_test.dart index 17a4714..effb158 100644 --- a/test/rule_contract_test.dart +++ b/test/rule_contract_test.dart @@ -32,7 +32,13 @@ const Map> _negatives = { // PatternState HAS render sites and every variant it renders is emitted. // ItemState would be vacuous: it has no render sites, so it could never be // reported regardless of the emission logic. - ruleRenderedNotEmitted: [('union', 'PatternState')], + // DocLoading/DocReady are emitted through freezed-style redirecting factories + // (`DocState.loading()`), which must be credited to the redirect target. + ruleRenderedNotEmitted: [ + ('union', 'PatternState'), + ('variant', 'DocLoading'), + ('variant', 'DocReady'), + ], // GuardedCubit emits a loading variant and awaits inside a try whose catch // resolves it, so reporting it would be a false positive. FlagCubit would be // vacuous: it has no variant union and is skipped outright. @@ -127,6 +133,12 @@ void main() { expect(owners, {'ShareBetaBloc'}); }); + test('rendered-not-emitted: redirect resolution does not mute the rule', () { + // DocStale is rendered and never emitted, so crediting factory calls to + // their redirect targets must leave it reported. + expect(valuesOf(ruleRenderedNotEmitted, 'variant'), contains('DocStale')); + }); + test('event-without-handler: a group handler does not cover a sibling', () { // UploadAbandoned sits outside the handled UploadRetry group, so following // the hierarchy must not over-credit it. -- 2.52.0 From 939cdcb25119485e22a8ba2bd4bcef51bf94c573 Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 13:45:11 -0600 Subject: [PATCH 07/14] fix(riverpod): recognise AsyncError patterns and follow stored values riverpod-async-gap judged handling from the text of the expression a `ref.watch` flows into, which missed two common shapes: - Pattern handling. A switch with `AsyncError()` / `AsyncLoading()` cases - the form pipe's own remediation recommends - did not count as handling, and a switch STATEMENT on the watched value was never looked at, because the parent climb stops at the first non-expression. A provider handled that way in one widget and read with `.value` in another was reported as unhandled. - A value stored in a local. `final v = ref.watch(p); ... v.when(...)` stopped the climb at the declaration, so both the handling and the read were invisible: a handled provider could be reported, and an unhandled one read as `v.value` was missed (a recall gap, not only precision). The consumer now widens to an enclosing switch statement, the uses of a declared local are followed through the enclosing body, and AsyncError/AsyncLoading patterns count as handling. Fixture: patterned, switched and stored providers (each also read directly elsewhere, so the controls are not vacuous) were all reported before and are silent now; storedUnhandledProvider was missed and is now reported. Real repos: tick's categoriesStreamProvider is no longer reported; its categories screen handles it with `catsAsync.when(...)` through a variable, which the rule's any-site-handles definition counts as handled. nuwayze and dex unchanged. word_alive's internalUserProvider finding comes from a commit made to word_alive during this work, not from this change: the old code reports it too. --- lib/src/state_flow.dart | 69 +++++++++++++++---- .../stuck_loading/lib/riverpod_gaps.dart | 59 ++++++++++++++++ test/riverpod_test.dart | 21 ++++-- test/rule_contract_test.dart | 23 ++++++- 4 files changed, 152 insertions(+), 20 deletions(-) diff --git a/lib/src/state_flow.dart b/lib/src/state_flow.dart index 8ab5f05..4843fc1 100644 --- a/lib/src/state_flow.dart +++ b/lib/src/state_flow.dart @@ -1485,6 +1485,45 @@ class _RiverpodCollector extends RecursiveAstVisitor { /// comparing node identity, because analyzer 14 exposes dual AST origins /// where `parent.target == child` can fail for the same logical node. static ({bool error, bool loading, bool readsValue}) _handling(AstNode node) { + final scope = _consumer(node); + final sources = [scope.toSource()]; + // `final v = ref.watch(p); ... v.when(...)`: the handling lives on the + // variable's uses, which the parent climb alone never reached, so a stored + // value was neither handled nor read as far as this analysis could see. + final declaration = scope.parent; + if (declaration is VariableDeclaration) { + final body = declaration.thisOrAncestorOfType(); + if (body != null) { + final uses = _VariableUses(declaration.name.lexeme); + body.accept(uses); + for (final use in uses.found) { + if (use.offset == declaration.name.offset) continue; + sources.add(_consumer(use).toSource()); + } + } + } + final source = sources.join('\n'); + // `error:` / `loading:` may appear either on an AsyncValue `.when(...)` or + // as a callback on `ref.listen(..., error: ...)`, and a switch handles them + // with `AsyncError()` / `AsyncLoading()` patterns - the form pipe's own + // remediation recommends, which was not recognised. + final error = + RegExp(r'\berror\s*:').hasMatch(source) || + source.contains('.hasError') || + RegExp(r'\bAsyncError\b').hasMatch(source); + final loading = + RegExp(r'\bloading\s*:').hasMatch(source) || + source.contains('.isLoading') || + RegExp(r'\bAsyncLoading\b').hasMatch(source); + final readsValue = RegExp( + r'\.(asData|value|valueOrNull|requireValue)\b', + ).hasMatch(source); + return (error: error, loading: loading, readsValue: readsValue); + } + + /// The outermost expression [node]'s value flows into, widened to a switch + /// statement when that expression is what it switches on. + static AstNode _consumer(AstNode node) { AstNode scope = node; while (true) { final parent = scope.parent; @@ -1494,18 +1533,9 @@ class _RiverpodCollector extends RecursiveAstVisitor { } break; } - final source = scope.toSource(); - // `error:` / `loading:` may appear either on an AsyncValue `.when(...)` or - // as a callback on `ref.listen(..., error: ...)`, so match either form. - final error = - RegExp(r'\berror\s*:').hasMatch(source) || source.contains('.hasError'); - final loading = - RegExp(r'\bloading\s*:').hasMatch(source) || - source.contains('.isLoading'); - final readsValue = RegExp( - r'\.(asData|value|valueOrNull|requireValue)\b', - ).hasMatch(source); - return (error: error, loading: loading, readsValue: readsValue); + final parent = scope.parent; + if (parent is SwitchStatement) return parent; + return scope; } @override @@ -1539,6 +1569,21 @@ class _RiverpodCollector extends RecursiveAstVisitor { } } +/// Identifiers named [name]: the uses of a local variable (shadowing is not +/// modelled, which can only add sources, never hide one). +class _VariableUses extends RecursiveAstVisitor { + _VariableUses(this.name); + + final String name; + final List found = []; + + @override + void visitSimpleIdentifier(SimpleIdentifier node) { + if (node.name == name) found.add(node); + super.visitSimpleIdentifier(node); + } +} + /// Build the state-flow graph for a resolved project. StateFlow buildStateFlow(Project project, List outcomes) { final types = <_TypeInfo>[]; diff --git a/test/fixtures/stuck_loading/lib/riverpod_gaps.dart b/test/fixtures/stuck_loading/lib/riverpod_gaps.dart index b930378..2026b34 100644 --- a/test/fixtures/stuck_loading/lib/riverpod_gaps.dart +++ b/test/fixtures/stuck_loading/lib/riverpod_gaps.dart @@ -12,6 +12,8 @@ class AsyncData { class AsyncValue { const AsyncValue(); + T? get value => null; + AsyncData? get asData => null; bool get hasError => false; @@ -24,6 +26,14 @@ class AsyncValue { throw UnimplementedError(); } +class AsyncError extends AsyncValue { + const AsyncError(); +} + +class AsyncLoading extends AsyncValue { + const AsyncLoading(); +} + class FutureProvider { const FutureProvider(this.create); @@ -60,4 +70,53 @@ class SubscriptionView { loading: () => -1, error: (error, stack) => -2, ); + + /// GOOD: pattern-matches AsyncError and AsyncLoading in a switch expression, + /// the form the remediation itself recommends. + int renderPatterned() => switch (ref.watch(patternedProvider)) { + AsyncError() => -2, + AsyncLoading() => -1, + _ => 0, + }; + + /// GOOD: the same in a switch statement. + int renderSwitchStatement() { + switch (ref.watch(switchedProvider)) { + case AsyncError(): + return -2; + case AsyncLoading(): + return -1; + default: + return 0; + } + } + + /// GOOD: the watched value is stored in a variable and then handled. + int renderStored() { + final count = ref.watch(storedProvider); + return count.when( + data: (value) => value, + loading: () => -1, + error: (error, stack) => -2, + ); + } + + /// Direct value reads of the three GOOD providers above. Without a site that + /// reads the value, a provider is never reported whatever its handling, so + /// those controls would prove nothing. + int? peekPatterned() => ref.watch(patternedProvider).value; + int? peekSwitched() => ref.watch(switchedProvider).value; + int? peekStored() => ref.watch(storedProvider).value; + + /// BAD: stored in a variable and then read without handling, so following + /// the variable must not silence the rule. + int? renderStoredUnhandled() { + final count = ref.watch(storedUnhandledProvider); + return count.value; + } } + +final patternedProvider = FutureProvider((ref) async => 1); +final switchedProvider = FutureProvider((ref) async => 1); +final storedProvider = FutureProvider((ref) async => 1); +final storedUnhandledProvider = FutureProvider((ref) async => 1); diff --git a/test/riverpod_test.dart b/test/riverpod_test.dart index 6e2b3a5..5d9a463 100644 --- a/test/riverpod_test.dart +++ b/test/riverpod_test.dart @@ -44,17 +44,24 @@ void main() { .where((f) => f.rule == ruleRiverpodAsyncGap) .toList(); - expect(hits, hasLength(1)); - expect(hits.single.data['provider'], 'unhandledProvider'); + // The two deliberate gaps: a direct read and a read through a variable. + expect(hits.map((f) => f.data['provider']).toSet(), { + 'unhandledProvider', + 'storedUnhandledProvider', + }); + expect(hits, hasLength(2)); + final unhandled = hits.singleWhere( + (f) => f.data['provider'] == 'unhandledProvider', + ); expect( - hits.single.data['missing'], + unhandled.data['missing'], containsAll(['error', 'loading']), ); // A non-bool value type keeps this at high confidence and warning severity. - expect(hits.single.confidence, 'high'); - expect(hits.single.severity, PipeSeverity.warning); - expect(hits.single.evidence, isNotEmpty); - expect(hits.single.remediation, isNotNull); + expect(unhandled.confidence, 'high'); + expect(unhandled.severity, PipeSeverity.warning); + expect(unhandled.evidence, isNotEmpty); + expect(unhandled.remediation, isNotNull); }); test('the handled provider produces no gap', () { diff --git a/test/rule_contract_test.dart b/test/rule_contract_test.dart index effb158..0906f41 100644 --- a/test/rule_contract_test.dart +++ b/test/rule_contract_test.dart @@ -77,7 +77,16 @@ const Map> _negatives = { ('event', 'UploadRetry'), ], // handledProvider covers all AsyncValue cases. - ruleRiverpodAsyncGap: [('provider', 'handledProvider')], + // The others are handled with AsyncError/AsyncLoading patterns (switch + // expression and statement) or through a stored variable, and each is also + // read directly elsewhere, so a gap would be reported if the handling were + // missed. + ruleRiverpodAsyncGap: [ + ('provider', 'handledProvider'), + ('provider', 'patternedProvider'), + ('provider', 'switchedProvider'), + ('provider', 'storedProvider'), + ], // `bump` notifies; `setQuiet` mutates but a notifying method calls it. ruleNotifierNeverNotifies: [('member', 'bump'), ('member', 'setQuiet')], // LiveNotifier notifies, so its Consumer can rebuild. @@ -139,6 +148,18 @@ void main() { expect(valuesOf(ruleRenderedNotEmitted, 'variant'), contains('DocStale')); }); + test( + 'riverpod-async-gap: a value stored in a variable is still followed', + () { + // The parent climb stopped at the variable declaration, so a stored value + // read with `.value` was invisible to the rule. + expect( + valuesOf(ruleRiverpodAsyncGap, 'provider'), + contains('storedUnhandledProvider'), + ); + }, + ); + test('event-without-handler: a group handler does not cover a sibling', () { // UploadAbandoned sits outside the handled UploadRetry group, so following // the hierarchy must not over-credit it. -- 2.52.0 From c79878f93dfdf00ee5c6f420cf2be6a59c1d7215 Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 13:47:29 -0600 Subject: [PATCH 08/14] 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` handler closures are also named `on` 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. --- lib/src/rules.dart | 58 +++++++++++++++----------- test/baseline_test.dart | 6 +-- test/fingerprint_test.dart | 84 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 26 deletions(-) create mode 100644 test/fingerprint_test.dart diff --git a/lib/src/rules.dart b/lib/src/rules.dart index 2b91cb9..3433cf3 100644 --- a/lib/src/rules.dart +++ b/lib/src/rules.dart @@ -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 { } } +/// `on` for a bloc handler closure, so two handlers in one class get +/// distinct identities. As a bare `` 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 ''; +} + /// Finds awaited work on a loading-flag path that no catch clause resolves. class _FlagLoadingCollector extends RecursiveAstVisitor { _FlagLoadingCollector( @@ -1083,25 +1105,10 @@ class _FlagLoadingCollector extends RecursiveAstVisitor { @override void visitFunctionExpression(FunctionExpression node) { - _analyze(node.body, _closureName(node)); + _analyze(node.body, _closureMemberName(node)); super.visitFunctionExpression(node); } - /// `on` for a bloc handler closure, so two handlers in one class get - /// distinct identities. As a bare `` 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 ''; - } - void _analyze(FunctionBody? body, String member) { if (body == null) return; final owner = _classStack.isEmpty ? '' : _classStack.last; @@ -1334,7 +1341,7 @@ class _LoadingResetCollector extends RecursiveAstVisitor { @override void visitFunctionExpression(FunctionExpression node) { - _analyze(node.body, ''); + _analyze(node.body, _closureMemberName(node)); super.visitFunctionExpression(node); } @@ -1347,6 +1354,10 @@ class _LoadingResetCollector extends RecursiveAstVisitor { 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 { 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 { 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, }, ), ); diff --git a/test/baseline_test.dart b/test/baseline_test.dart index e45141b..cc5d9d6 100644 --- a/test/baseline_test.dart +++ b/test/baseline_test.dart @@ -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) => diff --git a/test/fingerprint_test.dart b/test/fingerprint_test.dart new file mode 100644 index 0000000..3ac0bef --- /dev/null +++ b/test/fingerprint_test.dart @@ -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> _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 original; + late List 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 = {}; + 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); + }); +} -- 2.52.0 From 17ff523a119693e14b63d6d833e24dd63a9afef2 Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 14:04:32 -0600 Subject: [PATCH 09/14] fix(check): lower confidence where unresolved code can hide evidence check never looked at analyzer errors, so a project that resolved only partly was analysed as if it were whole. pipe reads names off the syntax tree, and an unresolved `Foo()` is a method call rather than a construction, so a dispatch, emission or construction in such a file simply disappears - and every finding is an absence claim that the missing evidence could refute. Found live during this work: with the Flutter SDK unreadable, nuwayze resolved with 40,079 analyzer errors (package:flutter unresolvable) and check still reported 102 findings at full confidence, with no warning. Now, when any analysed file has an unresolved-reference diagnostic (uri_does_not_exist, undefined_*, creation_with_non_type, ...): - check warns on stderr and adds a report note naming the first one; - a finding is lowered one confidence step, with data.confidenceLowered = unresolved-references, when an unresolved file mentions one of its identity names (event, variant, union, owner, provider, member). pipe's evidence is name-based, so a file that never names X cannot hide evidence about X; lowering everything for one stray error would bury the signal (a first cut did exactly that, taking all 102 nuwayze findings down for a single unresolved file). Validated on mirrors of the dogfood repos with a readable SDK (0 analyzer errors): nuwayze 102 findings identical to the original main, dex 0, word_alive 1 and tick 1 (the intended riverpod change). The fixture test adds an unresolved file that constructs NeverDispatched: its dead-handler finding drops to medium while UnhandledTap, which that file never names, stays high. --- README.md | 9 ++++ lib/src/analysis.dart | 96 +++++++++++++++++++++++++++++++++++++++ lib/src/pipeline.dart | 12 +++++ lib/src/rules.dart | 70 ++++++++++++++++++++++++++++ test/unresolved_test.dart | 95 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 282 insertions(+) create mode 100644 test/unresolved_test.dart diff --git a/README.md b/README.md index 994b5d7..2a75783 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,15 @@ the standalone SDK. If the project has never been resolved, `pipe` says so and tells you to run `pub get` rather than silently degrading every `package:` import to `dynamic`. +A project that resolves only partly is reported, not silently analysed: an +unresolved `Foo()` is a method call rather than a construction, so evidence in +that file is invisible. `check` warns on stderr, notes how many files have +unresolved references, and lowers by one step the confidence of every finding +whose names such a file mentions (`data.confidenceLowered`). An SDK that exists +but cannot be read (for example a Flutter SDK under a privacy-protected macOS +folder, which makes `package:flutter` unresolvable) is refused up front with a +`--sdk` remedy. + ## CI and ratcheting An existing codebase will have findings on day one. Accept them once, then fail diff --git a/lib/src/analysis.dart b/lib/src/analysis.dart index e2846ac..d43b79a 100644 --- a/lib/src/analysis.dart +++ b/lib/src/analysis.dart @@ -4,6 +4,8 @@ import 'package:analyzer/dart/analysis/analysis_context.dart'; import 'package:analyzer/dart/analysis/analysis_context_collection.dart'; import 'package:analyzer/dart/analysis/results.dart'; import 'package:analyzer/dart/analysis/session.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/diagnostic/diagnostic.dart'; import 'package:analyzer/file_system/physical_file_system.dart'; @@ -35,6 +37,100 @@ class FileOutcome { List get diagnostics => unit?.diagnostics ?? const []; } +/// Diagnostic codes meaning a name or import did not resolve. Unlike a type +/// error, these change the syntax tree pipe reads: an unresolved `Foo()` is a +/// method call rather than a construction, so a dispatch or emission of `Foo` +/// disappears from the analysis. +const Set unresolvedReferenceCodes = { + 'uri_does_not_exist', + 'undefined_class', + 'undefined_identifier', + 'undefined_method', + 'undefined_function', + 'undefined_getter', + 'undefined_setter', + 'undefined_super_member', + 'undefined_prefixed_name', + 'creation_with_non_type', + 'extends_non_class', + 'implements_non_class', + 'mixin_of_non_class', + 'non_type_as_type_argument', +}; + +/// Resolved files that contain at least one unresolved reference. +class UnresolvedReferences { + UnresolvedReferences({ + required this.files, + required this.analysed, + this.first, + this.names = const {}, + }); + + /// Every identifier those files mention. pipe's evidence is name-based, so an + /// unresolved file can only hide evidence about a name it mentions. + final Set names; + + /// Paths (absolute) of resolved files with an unresolved reference. + final List files; + + /// How many files resolved at all. + final int analysed; + + /// The first such diagnostic as `path:line: message`, for the reader. + final String? first; + + bool get any => files.isNotEmpty; +} + +UnresolvedReferences findUnresolvedReferences(List outcomes) { + final files = []; + final names = {}; + String? first; + var analysed = 0; + for (final outcome in outcomes) { + final unit = outcome.unit; + if (unit == null) continue; + analysed++; + for (final diagnostic in unit.diagnostics) { + if (!unresolvedReferenceCodes.contains( + diagnostic.diagnosticCode.lowerCaseName, + )) { + continue; + } + files.add(outcome.path); + final line = unit.lineInfo.getLocation(diagnostic.offset).lineNumber; + first ??= '${outcome.path}:$line: ${diagnostic.message}'; + final collector = _Names(); + unit.unit.accept(collector); + names.addAll(collector.names); + break; + } + } + return UnresolvedReferences( + files: files, + analysed: analysed, + first: first, + names: names, + ); +} + +class _Names extends RecursiveAstVisitor { + final Set names = {}; + + @override + void visitSimpleIdentifier(SimpleIdentifier node) { + names.add(node.name); + super.visitSimpleIdentifier(node); + } + + @override + void visitNamedType(NamedType node) { + names.add(node.name.lexeme); + super.visitNamedType(node); + } +} + /// Owns the analyzer contexts for one target project. /// /// Mirrors the lifecycle Flutter's own tooling uses: one collection rooted at diff --git a/lib/src/pipeline.dart b/lib/src/pipeline.dart index a011072..3d2f8fc 100644 --- a/lib/src/pipeline.dart +++ b/lib/src/pipeline.dart @@ -1,3 +1,6 @@ +import 'dart:io'; + +import 'analysis.dart'; import 'index.dart'; import 'project.dart'; import 'rules.dart'; @@ -42,6 +45,15 @@ Future analyzeProject( ); try { final flow = buildStateFlow(project, run.outcomes); + final unresolved = findUnresolvedReferences(run.outcomes); + if (unresolved.any) { + stderr.writeln( + 'pipe: WARNING ${unresolved.files.length} of ${unresolved.analysed} ' + 'analysed file(s) have unresolved references, so findings naming ' + 'anything they mention may be false and have lowered confidence. Run ' + '`pipe index` to see why.', + ); + } final report = runChecks( project: project, outcomes: run.outcomes, diff --git a/lib/src/rules.dart b/lib/src/rules.dart index 3433cf3..c2685e5 100644 --- a/lib/src/rules.dart +++ b/lib/src/rules.dart @@ -129,6 +129,23 @@ class Finding { return file ?? fingerprint; } + /// This finding one confidence step lower, recording why. + Finding lowered(String reason) => Finding( + rule: rule, + severity: severity, + confidence: switch (confidence) { + 'high' => 'medium', + _ => 'low', + }, + message: message, + file: file, + line: line, + evidence: evidence, + remediation: remediation, + data: {...data, 'confidenceLowered': reason}, + baselined: baselined, + ); + Finding asBaselined() => Finding( rule: rule, severity: severity, @@ -821,6 +838,34 @@ FindingReport runChecks({ if (note.isNotEmpty) notes.add(note); } + // Every finding is a whole-program absence claim (never rendered, never + // dispatched, never notified), and an unresolved reference can hide the + // evidence that refutes it: an unresolved `Foo()` is a method call, not a + // construction. On a real app whose package:flutter could not be read, + // findings were reported at high confidence with no indication. pipe's + // evidence is name-based, so only a finding whose names an unresolved file + // mentions can have lost evidence there; lowering every finding for one + // stray error elsewhere would bury the signal. + final unresolved = findUnresolvedReferences(outcomes); + if (unresolved.any) { + var lowered = 0; + for (var i = 0; i < findings.length; i++) { + if (!_identityNames(findings[i]).any(unresolved.names.contains)) continue; + findings[i] = findings[i].lowered('unresolved-references'); + lowered++; + } + final first = unresolved.first!; + notes.add( + '${unresolved.files.length} of ${unresolved.analysed} analysed file(s) ' + 'have unresolved references (first: ' + '${first.replaceFirst('${project.root}/', '')}). An unresolved `Foo()` ' + 'is not seen as a construction, so evidence in those files can be ' + 'missed: $lowered finding(s) naming something they mention had their ' + 'confidence lowered one step. Run `pipe index` and resolve the project ' + '(`pub get`, a readable SDK) for full confidence.', + ); + } + final resolved = baseline == null ? findings : [ @@ -1022,6 +1067,31 @@ class _CalledNames extends RecursiveAstVisitor { } } +/// The names a finding makes a claim about: the values of its identity keys. +Set _identityNames(Finding finding) { + const keys = [ + 'union', + 'variant', + 'variants', + 'group', + 'event', + 'provider', + 'owner', + 'member', + 'members', + 'stateType', + 'valueType', + 'valueTypes', + ]; + final names = {}; + for (final key in keys) { + final value = finding.data[key]; + if (value is String) names.add(value); + if (value is List) names.addAll(value.whereType()); + } + return names; +} + /// `on` for a bloc handler closure, so two handlers in one class get /// distinct identities. As a bare `` they shared a fingerprint, and /// the baseline dropped all but one of them. diff --git a/test/unresolved_test.dart b/test/unresolved_test.dart new file mode 100644 index 0000000..75127e4 --- /dev/null +++ b/test/unresolved_test.dart @@ -0,0 +1,95 @@ +import 'dart:io'; + +import 'package:pipe/pipe.dart'; +import 'package:test/test.dart'; + +/// The fixture plus one file whose references do not resolve: it constructs +/// `NeverDispatched` without importing it, and imports a file that is missing. +String _corpusWithUnresolvedFile() { + final from = Directory('test/fixtures/stuck_loading'); + final to = Directory.systemTemp.createTempSync('pipe-unresolved'); + 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()); + } + } + File('${to.path}/lib/unresolved_ui.dart').writeAsStringSync(''' +import 'widgets_that_do_not_exist.dart'; + +// Unresolved, `NeverDispatched()` parses as a method call rather than a +// construction, so the dispatch below is invisible to the analysis. +void tapNever(dynamic bloc) => bloc.add(NeverDispatched()); +'''); + return to.path; +} + +void main() { + test( + 'a finding an unresolved file could refute is not high confidence', + () async { + // Every pipe finding is a whole-program absence claim, and an unresolved + // reference anywhere can hide the evidence that refutes it. On a real app + // whose package:flutter could not be read (40k analyzer errors), check + // reported findings at high confidence with no warning at all. + final analysis = await analyzeProject( + await loadProject(_corpusWithUnresolvedFile()), + concurrency: 1, + ); + try { + final findings = analysis.report.findings; + final dead = findings.singleWhere( + (f) => + f.rule == ruleDeadHandler && f.data['event'] == 'NeverDispatched', + ); + expect(dead.confidence, 'medium'); + expect(dead.data['confidenceLowered'], 'unresolved-references'); + // The unresolved file never mentions UnhandledTap, so it cannot hide + // evidence about it: one stray error must not bury every finding. + final untouched = findings.singleWhere( + (f) => + f.rule == ruleEventWithoutHandler && + f.data['event'] == 'UnhandledTap', + ); + expect(untouched.confidence, 'high'); + expect(untouched.data.containsKey('confidenceLowered'), isFalse); + expect( + analysis.report.notes.join('\n'), + allOf( + contains('unresolved references'), + contains('unresolved_ui.dart'), + contains('1 finding(s)'), + ), + ); + } finally { + await analysis.dispose(); + } + }, + ); + + test('a fully resolved project keeps its confidence', () async { + final analysis = await analyzeProject( + await loadProject('test/fixtures/stuck_loading'), + concurrency: 1, + ); + try { + expect( + analysis.report.findings.where((f) => f.confidence == 'high'), + isNotEmpty, + ); + expect( + analysis.report.findings.where( + (f) => f.data.containsKey('confidenceLowered'), + ), + isEmpty, + ); + } finally { + await analysis.dispose(); + } + }); +} -- 2.52.0 From 56dcfc85a698934185444df2c74de82cef9536ec Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 14:17:33 -0600 Subject: [PATCH 10/14] refactor(rules): judge loading and construction facts, not syntax trees First step of the state-map seam: rules should read only a language-neutral model, so a second front end (C++) can feed them. The two loading rules built findings inside tree visitors, and dead-handler and the unresolved-reference check walked every unit from runChecks. Their recognisers now live in the Dart front end (dart_facts.dart) and emit neutral facts (facts.dart) carried on StateFlow: - SwallowedFailure: a busy variant is emitted, then an awaited failure is swallowed (loading-not-reset). The "does this owner hold rendered state" gate moved to the rule, since it is judgement, not recognition. - UnsettledFlag: a loading flag left set across unguarded awaited work (loading-flag-not-reset); the rule derives confidence from setsFlag and guardedTrySeen. - constructedByFile and UnresolvedSummary, with project-relative paths. Every fact serialises to JSON and appears in `pipe state`. rules.dart no longer imports any analyzer AST library; only the provider rules still take resolved units, which is the next step. No behaviour change, checked byte for byte: `check --format json` is identical on the fixture and on resolved mirrors of nuwayze, tick, dex and word_alive. 128 tests pass. --- lib/src/dart_facts.dart | 409 ++++++++++++++++++++++++++++++ lib/src/facts.dart | 153 +++++++++++ lib/src/rules.dart | 544 +++++----------------------------------- lib/src/state_flow.dart | 70 +++++- 4 files changed, 691 insertions(+), 485 deletions(-) create mode 100644 lib/src/dart_facts.dart create mode 100644 lib/src/facts.dart diff --git a/lib/src/dart_facts.dart b/lib/src/dart_facts.dart new file mode 100644 index 0000000..379b470 --- /dev/null +++ b/lib/src/dart_facts.dart @@ -0,0 +1,409 @@ +/// The Dart front end's pattern recognisers for the loading rules. +/// +/// These walk syntax trees and emit the language-neutral facts in facts.dart; +/// the rules in rules.dart judge the facts and never see a tree. +library; + +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/source/line_info.dart'; + +import 'facts.dart'; +import 'state_flow.dart'; + +/// Type names constructed in a unit, so a handler is not called dead when its +/// event is built somewhere and dispatched through a variable. +class ConstructionCollector extends RecursiveAstVisitor { + final Set constructed = {}; + + @override + void visitInstanceCreationExpression(InstanceCreationExpression node) { + constructed.add(node.constructorName.type.name.lexeme); + super.visitInstanceCreationExpression(node); + } +} + +class _TryCollector extends RecursiveAstVisitor { + final List tries = []; + + @override + void visitTryStatement(TryStatement node) { + tries.add(node); + super.visitTryStatement(node); + } + + // Nested functions are analysed on their own. + @override + void visitFunctionExpression(FunctionExpression node) {} + + @override + void visitFunctionDeclaration(FunctionDeclaration node) {} +} + +class _CalledNames extends RecursiveAstVisitor { + final Set names = {}; + + @override + void visitMethodInvocation(MethodInvocation node) { + final target = node.target; + if (target == null || target is ThisExpression) { + names.add(node.methodName.name); + } + super.visitMethodInvocation(node); + } + + @override + void visitFunctionExpressionInvocation(FunctionExpressionInvocation node) { + final function = node.function; + if (function is SimpleIdentifier) names.add(function.name); + super.visitFunctionExpressionInvocation(node); + } +} + +/// `on` for a bloc handler closure, so two handlers in one class get +/// distinct identities. As a bare `` 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 ''; +} + +/// Finds awaited work on a loading-flag path that no catch clause resolves. +class UnsettledFlagCollector extends RecursiveAstVisitor { + UnsettledFlagCollector( + this.relativePath, + this.lineInfo, + this.flagStatesByOwner, + this.facts, + ); + + final String relativePath; + final LineInfo lineInfo; + final Map flagStatesByOwner; + final List facts; + final List _classStack = []; + + /// Methods invoked from a constructor body: for those, a default-true loading + /// flag is in effect even though the method never sets it itself. + final Map> _constructorCalls = {}; + + static final RegExp _stateAssign = RegExp(r'(^|[^.\w])state\s*=(?!=)'); + static final RegExp _throwing = RegExp(r'\b(rethrow|throw)\b'); + static final RegExp _errorish = RegExp( + r'(Error|Failure|Fail|Cancel|Retry|Recover|Reset|Clear)', + caseSensitive: false, + ); + + static bool _surfacesFailure(String source) { + if (source.contains('emit(') || source.contains('notifyListeners(')) { + return true; + } + if (_stateAssign.hasMatch(source)) return true; + if (_throwing.hasMatch(source)) return true; + return source.contains('add(') && _errorish.hasMatch(source); + } + + @override + void visitClassDeclaration(ClassDeclaration node) { + final name = node.namePart.typeName.lexeme; + final calls = {}; + for (final member in node.body.members) { + if (member is! ConstructorDeclaration) continue; + final collector = _CalledNames(); + member.body.accept(collector); + calls.addAll(collector.names); + } + if (calls.isNotEmpty) _constructorCalls[name] = calls; + _classStack.add(name); + super.visitClassDeclaration(node); + _classStack.removeLast(); + } + + @override + void visitMixinDeclaration(MixinDeclaration node) { + _classStack.add(node.name.lexeme); + super.visitMixinDeclaration(node); + _classStack.removeLast(); + } + + @override + void visitMethodDeclaration(MethodDeclaration node) { + _analyze(node.body, node.name.lexeme); + super.visitMethodDeclaration(node); + } + + @override + void visitFunctionExpression(FunctionExpression node) { + _analyze(node.body, _closureMemberName(node)); + super.visitFunctionExpression(node); + } + + void _analyze(FunctionBody? body, String member) { + if (body == null) return; + final owner = _classStack.isEmpty ? '' : _classStack.last; + final flag = flagStatesByOwner[owner]; + if (flag == null) return; + // Lifecycle teardown never renders a loading state. + if (member == 'close' || member == 'dispose') return; + + // 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. + // + // 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); + if (!setsFlagTrue && !inheritsInitialFlag) return; + + // Guarded when an awaited try has a catch that surfaces the failure. + final tries = _TryCollector(); + body.accept(tries); + var awaitedTryGuarded = false; + var awaitedTrySeen = false; + for (final statement in tries.tries) { + final awaitedInTry = _HasAwait(shallow: true); + statement.body.accept(awaitedInTry); + if (!awaitedInTry.found) continue; + awaitedTrySeen = true; + for (final catchClause in statement.catchClauses) { + if (_surfacesFailure(catchClause.body.toSource())) { + awaitedTryGuarded = true; + } + } + } + if (awaitedTryGuarded) return; + + // Nothing awaited at all means there is no failure path to leave dangling. + final awaitProbe = _HasAwait(shallow: true); + body.accept(awaitProbe); + if (!awaitProbe.found) return; + + facts.add( + UnsettledFlag( + owner: owner, + member: member, + path: relativePath, + line: lineInfo.getLocation(body.offset).lineNumber, + stateType: flag.name, + fields: flag.fields, + defaultsTrue: flag.defaultsTrue, + setsFlag: setsFlagTrue, + guardedTrySeen: awaitedTrySeen, + ), + ); + } +} + +/// Scans a single function body for in-flight loading emits and top-level +/// awaited try statements. +class _BodyScan extends RecursiveAstVisitor { + final List loadingEmits = []; + final List topLevelTries = []; + + static final RegExp _loading = RegExp( + r'(Loading|Processing|Submitting|Saving|Creating|Updating|InProgress)', + ); + + int _tryDepth = 0; + + void _maybeLoading(AstNode node, List arguments) { + if (arguments.isEmpty) return; + final first = arguments.first; + if (first is! Expression) return; + if (_loading.hasMatch(first.toSource())) loadingEmits.add(node.offset); + } + + @override + void visitMethodInvocation(MethodInvocation node) { + if (node.methodName.name == 'emit') { + _maybeLoading(node, node.argumentList.arguments); + } + super.visitMethodInvocation(node); + } + + @override + void visitFunctionExpressionInvocation(FunctionExpressionInvocation node) { + final function = node.function; + if (function is SimpleIdentifier && function.name == 'emit') { + _maybeLoading(node, node.argumentList.arguments); + } + super.visitFunctionExpressionInvocation(node); + } + + @override + void visitTryStatement(TryStatement node) { + final isTopLevel = _tryDepth == 0; + _tryDepth++; + if (isTopLevel) topLevelTries.add(node); + super.visitTryStatement(node); + _tryDepth--; + } + + // Nested functions are analysed in their own right by _analyze; descending + // into them here would report the same catch twice. + @override + void visitFunctionExpression(FunctionExpression node) {} + + @override + void visitFunctionDeclaration(FunctionDeclaration node) {} +} + +class _HasAwait extends RecursiveAstVisitor { + _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 + void visitAwaitExpression(AwaitExpression node) { + 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 { + _SetsFlagTrue(this.fields); + + final Set 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) {} +} + +/// Finds members that emit an in-progress variant and then await work whose +/// catch writes no state. Whether the owner holds rendered state is the rule's +/// judgement, not the front end's, so every owner is scanned. +class SwallowedFailureCollector extends RecursiveAstVisitor { + SwallowedFailureCollector(this.relativePath, this.lineInfo, this.facts); + + final String relativePath; + final LineInfo lineInfo; + final List facts; + final List _classStack = []; + + static final RegExp _stateAssign = RegExp(r'(^|[^.\w])state\s*=(?!=)'); + static final RegExp _throwing = RegExp(r'\b(rethrow|throw)\b'); + static final RegExp _errorish = RegExp( + r'(Error|Failure|Fail|Cancel|Retry|Recover|Reset|Clear)', + caseSensitive: false, + ); + + @override + void visitClassDeclaration(ClassDeclaration node) { + _classStack.add(node.namePart.typeName.lexeme); + super.visitClassDeclaration(node); + _classStack.removeLast(); + } + + @override + void visitMixinDeclaration(MixinDeclaration node) { + _classStack.add(node.name.lexeme); + super.visitMixinDeclaration(node); + _classStack.removeLast(); + } + + @override + void visitMethodDeclaration(MethodDeclaration node) { + _analyze(node.body, node.name.lexeme); + super.visitMethodDeclaration(node); + } + + @override + void visitFunctionExpression(FunctionExpression node) { + _analyze(node.body, _closureMemberName(node)); + super.visitFunctionExpression(node); + } + + void _analyze(FunctionBody? body, String member) { + if (body == null) return; + final owner = _classStack.isEmpty ? '' : _classStack.last; + + final scan = _BodyScan(); + 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(); + statement.body.accept(probe); + if (!probe.found) continue; + + for (final catchClause in statement.catchClauses) { + final source = catchClause.body.toSource(); + final emits = + source.contains('emit(') || source.contains('notifyListeners('); + final assignsState = _stateAssign.hasMatch(source); + final rethrows = _throwing.hasMatch(source); + final dispatchesFailure = + source.contains('add(') && _errorish.hasMatch(source); + if (emits || assignsState || rethrows || dispatchesFailure) continue; + swallowed++; + + facts.add( + SwallowedFailure( + owner: owner, + member: member, + path: relativePath, + line: lineInfo.getLocation(catchClause.offset).lineNumber, + ordinal: swallowed, + handler: _oneLine(source), + ), + ); + } + } + } + + static String _oneLine(String source) { + final collapsed = source.replaceAll(RegExp(r'\s+'), ' ').trim(); + final body = collapsed.isEmpty ? '' : collapsed; + return body.length <= 120 ? body : '${body.substring(0, 117)}...'; + } +} diff --git a/lib/src/facts.dart b/lib/src/facts.dart new file mode 100644 index 0000000..3097e50 --- /dev/null +++ b/lib/src/facts.dart @@ -0,0 +1,153 @@ +/// Language-neutral facts a front end extracts for the rules. +/// +/// The rules judge these; they never read a syntax tree. A front end for +/// another language (a clang-based C++ extractor, say) produces the same facts +/// and every rule, baseline, verify and suggest path works unchanged. Each fact +/// round-trips through JSON so a front end can run as a separate process. +library; + +/// A member puts its owner into an in-progress state, then awaits work whose +/// failure handler writes no state, rethrows, or dispatches a failure: the +/// busy state can outlive the failure (a spinner that never stops). +/// +/// Variant style: the busy state is an emitted variant (`emit(Loading())`). +class SwallowedFailure { + SwallowedFailure({ + required this.owner, + required this.member, + required this.path, + required this.line, + required this.ordinal, + required this.handler, + }); + + factory SwallowedFailure.fromJson(Map json) => + SwallowedFailure( + owner: json['owner']! as String, + member: json['member']! as String, + path: json['path']! as String, + line: json['line']! as int, + ordinal: json['ordinal']! as int, + handler: json['handler']! as String, + ); + + final String owner; + final String member; + + /// Project-relative path and line of the failure handler (the catch). + final String path; + final int line; + + /// 1-based position among the member's swallowed handlers: identity that + /// survives code moving, unlike the line. + final int ordinal; + + /// The handler's source on one line, as evidence. + final String handler; + + Map toJson() => { + 'owner': owner, + 'member': member, + 'path': path, + 'line': line, + 'ordinal': ordinal, + 'handler': handler, + }; +} + +/// A member leaves a loading FLAG set across awaited work that no failure +/// handler resolves. +/// +/// Flag style: the state is one class with a `bool loading`-like field, so +/// there is no variant to match on. +class UnsettledFlag { + UnsettledFlag({ + required this.owner, + required this.member, + required this.path, + required this.line, + required this.stateType, + required this.fields, + required this.defaultsTrue, + required this.setsFlag, + required this.guardedTrySeen, + }); + + factory UnsettledFlag.fromJson(Map json) => UnsettledFlag( + owner: json['owner']! as String, + member: json['member']! as String, + path: json['path']! as String, + line: json['line']! as int, + stateType: json['stateType']! as String, + fields: [...(json['fields']! as List).cast()], + defaultsTrue: json['defaultsTrue']! as bool, + setsFlag: json['setsFlag']! as bool, + guardedTrySeen: json['guardedTrySeen']! as bool, + ); + + final String owner; + final String member; + final String path; + final int line; + final String stateType; + final List fields; + final bool defaultsTrue; + + /// The member itself sets the flag (as opposed to inheriting a default-true + /// flag by running from the constructor). + final bool setsFlag; + + /// Some awaited work sat inside a try, though none of its catches resolved + /// the flag. Weaker evidence than no try at all. + final bool guardedTrySeen; + + Map toJson() => { + 'owner': owner, + 'member': member, + 'path': path, + 'line': line, + 'stateType': stateType, + 'fields': fields, + 'defaultsTrue': defaultsTrue, + 'setsFlag': setsFlag, + 'guardedTrySeen': guardedTrySeen, + }; +} + +/// Files whose references did not resolve. Evidence in them can be missing, +/// so findings naming anything they mention are weaker. +class UnresolvedSummary { + UnresolvedSummary({ + required this.files, + required this.analysed, + this.first, + this.names = const {}, + }); + + factory UnresolvedSummary.fromJson(Map json) => + UnresolvedSummary( + files: [...(json['files']! as List).cast()], + analysed: json['analysed']! as int, + first: json['first'] as String?, + names: {...(json['names']! as List).cast()}, + ); + + /// Project-relative paths. + final List files; + final int analysed; + + /// The first unresolved reference as `path:line: message`. + final String? first; + + /// Every name those files mention. + final Set names; + + bool get any => files.isNotEmpty; + + Map toJson() => { + 'files': files, + 'analysed': analysed, + if (first != null) 'first': first, + 'names': names.toList()..sort(), + }; +} diff --git a/lib/src/rules.dart b/lib/src/rules.dart index c2685e5..12147b5 100644 --- a/lib/src/rules.dart +++ b/lib/src/rules.dart @@ -1,9 +1,5 @@ import 'dart:convert'; -import 'package:analyzer/dart/ast/ast.dart'; -import 'package:analyzer/dart/ast/visitor.dart'; -import 'package:analyzer/source/line_info.dart'; - import 'analysis.dart'; import 'errors.dart'; import 'project.dart'; @@ -360,16 +356,7 @@ FindingReport runChecks({ // Constructed type names per file, used to avoid reporting a handler as dead // when its event is built somewhere and dispatched through a variable. - final constructedByFile = >{}; - if (enabled.contains(ruleDeadHandler)) { - for (final outcome in outcomes) { - final unit = outcome.unit; - if (unit == null) continue; - final collector = _ConstructionCollector(); - unit.unit.accept(collector); - constructedByFile[project.relative(outcome.path)] = collector.constructed; - } - } + final constructedByFile = flow.constructedByFile; String site(EmissionSite emission) => '${emission.path}:${emission.line}: ${emission.snippet}'; @@ -734,16 +721,30 @@ FindingReport runChecks({ for (final emission in flow.emissions) if (emission.kind == 'notifyListeners') emission.owner, }; - for (final outcome in outcomes) { - final unit = outcome.unit; - if (unit == null) continue; - unit.unit.accept( - _LoadingResetCollector( - project, - outcome.path, - unit.lineInfo, - stateOwners, - findings, + for (final fact in flow.swallowedFailures) { + if (!stateOwners.contains(fact.owner)) continue; + findings.add( + Finding( + rule: ruleLoadingNotReset, + severity: PipeSeverity.warning, + confidence: 'high', + message: + '${fact.owner}.${fact.member} emits an in-progress state and then ' + 'awaits an operation whose catch clause never emits a terminal ' + 'state, so the UI can be left showing a spinner forever.', + file: fact.path, + line: fact.line, + evidence: ['${fact.path}:${fact.line}: ${fact.handler}'], + remediation: + 'Emit a failure/timeout state in the catch (or rethrow) so the ' + 'loading state is always resolved.', + data: { + 'owner': fact.owner, + 'member': fact.member, + // A method can hold more than one swallowed catch, so which one + // is part of this rule's identity. + 'catch': fact.ordinal, + }, ), ); } @@ -754,23 +755,38 @@ FindingReport runChecks({ // on. An awaited call on the loading path that no catch resolves can leave the // UI spinning forever, and this week's variant-based rule cannot see it. if (enabled.contains(ruleLoadingFlagNotReset)) { - final flagStatesByOwner = { - for (final state in flow.flagStates) state.owner: state, - }; - if (flagStatesByOwner.isNotEmpty) { - for (final outcome in outcomes) { - final unit = outcome.unit; - if (unit == null) continue; - unit.unit.accept( - _FlagLoadingCollector( - project, - outcome.path, - unit.lineInfo, - flagStatesByOwner, - findings, - ), - ); - } + for (final fact in flow.unsettledFlags) { + final fieldList = fact.fields.join(' / '); + findings.add( + Finding( + rule: ruleLoadingFlagNotReset, + severity: PipeSeverity.warning, + confidence: fact.setsFlag && !fact.guardedTrySeen ? 'high' : 'medium', + message: + '${fact.owner}.${fact.member} awaits work after putting ' + '${fact.stateType}`$fieldList` into its loading state, and no ' + 'catch clause surfaces a failure, so the UI can keep showing its ' + 'loading state after an error.', + file: fact.path, + line: fact.line, + evidence: [ + '${fact.path}:${fact.line}: ${fact.owner}.${fact.member} awaits ' + 'with ${fact.stateType}.$fieldList in flight and no catch that ' + 'emits a failure state', + ], + remediation: + 'Wrap the awaited call in try/catch and emit a state with ' + '`$fieldList` reset (or rethrow), so the loading state always ' + 'resolves.', + data: { + 'owner': fact.owner, + 'member': fact.member, + 'stateType': fact.stateType, + 'fields': fact.fields, + 'defaultsTrue': fact.defaultsTrue, + }, + ), + ); } } @@ -846,7 +862,7 @@ FindingReport runChecks({ // evidence is name-based, so only a finding whose names an unresolved file // mentions can have lost evidence there; lowering every finding for one // stray error elsewhere would bury the signal. - final unresolved = findUnresolvedReferences(outcomes); + final unresolved = flow.unresolved; if (unresolved.any) { var lowered = 0; for (var i = 0; i < findings.length; i++) { @@ -854,11 +870,10 @@ FindingReport runChecks({ findings[i] = findings[i].lowered('unresolved-references'); lowered++; } - final first = unresolved.first!; notes.add( '${unresolved.files.length} of ${unresolved.analysed} analysed file(s) ' - 'have unresolved references (first: ' - '${first.replaceFirst('${project.root}/', '')}). An unresolved `Foo()` ' + 'have unresolved references (first: ${unresolved.first}). An ' + 'unresolved `Foo()` ' 'is not seen as a construction, so evidence in those files can be ' 'missed: $lowered finding(s) naming something they mention had their ' 'confidence lowered one step. Run `pipe index` and resolve the project ' @@ -1020,53 +1035,6 @@ Set decodeBaseline(String content) { }; } -class _ConstructionCollector extends RecursiveAstVisitor { - final Set constructed = {}; - - @override - void visitInstanceCreationExpression(InstanceCreationExpression node) { - constructed.add(node.constructorName.type.name.lexeme); - super.visitInstanceCreationExpression(node); - } -} - -class _TryCollector extends RecursiveAstVisitor { - final List tries = []; - - @override - void visitTryStatement(TryStatement node) { - tries.add(node); - super.visitTryStatement(node); - } - - // Nested functions are analysed on their own. - @override - void visitFunctionExpression(FunctionExpression node) {} - - @override - void visitFunctionDeclaration(FunctionDeclaration node) {} -} - -class _CalledNames extends RecursiveAstVisitor { - final Set names = {}; - - @override - void visitMethodInvocation(MethodInvocation node) { - final target = node.target; - if (target == null || target is ThisExpression) { - names.add(node.methodName.name); - } - super.visitMethodInvocation(node); - } - - @override - void visitFunctionExpressionInvocation(FunctionExpressionInvocation node) { - final function = node.function; - if (function is SimpleIdentifier) names.add(function.name); - super.visitFunctionExpressionInvocation(node); - } -} - /// The names a finding makes a claim about: the values of its identity keys. Set _identityNames(Finding finding) { const keys = [ @@ -1091,395 +1059,3 @@ Set _identityNames(Finding finding) { } return names; } - -/// `on` for a bloc handler closure, so two handlers in one class get -/// distinct identities. As a bare `` 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 ''; -} - -/// Finds awaited work on a loading-flag path that no catch clause resolves. -class _FlagLoadingCollector extends RecursiveAstVisitor { - _FlagLoadingCollector( - this.project, - this.path, - this.lineInfo, - this.flagStatesByOwner, - this.findings, - ); - - final Project project; - final String path; - final LineInfo lineInfo; - final Map flagStatesByOwner; - final List findings; - final List _classStack = []; - - /// Methods invoked from a constructor body: for those, a default-true loading - /// flag is in effect even though the method never sets it itself. - final Map> _constructorCalls = {}; - - static final RegExp _stateAssign = RegExp(r'(^|[^.\w])state\s*=(?!=)'); - static final RegExp _throwing = RegExp(r'\b(rethrow|throw)\b'); - static final RegExp _errorish = RegExp( - r'(Error|Failure|Fail|Cancel|Retry|Recover|Reset|Clear)', - caseSensitive: false, - ); - - static bool _surfacesFailure(String source) { - if (source.contains('emit(') || source.contains('notifyListeners(')) { - return true; - } - if (_stateAssign.hasMatch(source)) return true; - if (_throwing.hasMatch(source)) return true; - return source.contains('add(') && _errorish.hasMatch(source); - } - - @override - void visitClassDeclaration(ClassDeclaration node) { - final name = node.namePart.typeName.lexeme; - final calls = {}; - for (final member in node.body.members) { - if (member is! ConstructorDeclaration) continue; - final collector = _CalledNames(); - member.body.accept(collector); - calls.addAll(collector.names); - } - if (calls.isNotEmpty) _constructorCalls[name] = calls; - _classStack.add(name); - super.visitClassDeclaration(node); - _classStack.removeLast(); - } - - @override - void visitMixinDeclaration(MixinDeclaration node) { - _classStack.add(node.name.lexeme); - super.visitMixinDeclaration(node); - _classStack.removeLast(); - } - - @override - void visitMethodDeclaration(MethodDeclaration node) { - _analyze(node.body, node.name.lexeme); - super.visitMethodDeclaration(node); - } - - @override - void visitFunctionExpression(FunctionExpression node) { - _analyze(node.body, _closureMemberName(node)); - super.visitFunctionExpression(node); - } - - void _analyze(FunctionBody? body, String member) { - if (body == null) return; - final owner = _classStack.isEmpty ? '' : _classStack.last; - final flag = flagStatesByOwner[owner]; - if (flag == null) return; - // Lifecycle teardown never renders a loading state. - if (member == 'close' || member == 'dispose') return; - - // 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. - // - // 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); - if (!setsFlagTrue && !inheritsInitialFlag) return; - - // Guarded when an awaited try has a catch that surfaces the failure. - final tries = _TryCollector(); - body.accept(tries); - var awaitedTryGuarded = false; - var awaitedTrySeen = false; - for (final statement in tries.tries) { - final awaitedInTry = _HasAwait(shallow: true); - statement.body.accept(awaitedInTry); - if (!awaitedInTry.found) continue; - awaitedTrySeen = true; - for (final catchClause in statement.catchClauses) { - if (_surfacesFailure(catchClause.body.toSource())) { - awaitedTryGuarded = true; - } - } - } - if (awaitedTryGuarded) return; - - // Nothing awaited at all means there is no failure path to leave dangling. - final awaitProbe = _HasAwait(shallow: true); - body.accept(awaitProbe); - if (!awaitProbe.found) return; - - final fieldList = flag.fields.join(' / '); - findings.add( - Finding( - rule: ruleLoadingFlagNotReset, - severity: PipeSeverity.warning, - confidence: setsFlagTrue && !awaitedTrySeen ? 'high' : 'medium', - message: - '$owner.$member awaits work after putting ${flag.name}`$fieldList` ' - 'into its loading state, and no catch clause surfaces a failure, so ' - 'the UI can keep showing its loading state after an error.', - file: project.relative(path), - line: lineInfo.getLocation(body.offset).lineNumber, - evidence: [ - '${project.relative(path)}:' - '${lineInfo.getLocation(body.offset).lineNumber}: ' - '$owner.$member awaits with ${flag.name}.$fieldList in flight and ' - 'no catch that emits a failure state', - ], - remediation: - 'Wrap the awaited call in try/catch and emit a state with ' - '`$fieldList` reset (or rethrow), so the loading state always ' - 'resolves.', - data: { - 'owner': owner, - 'member': member, - 'stateType': flag.name, - 'fields': flag.fields, - 'defaultsTrue': flag.defaultsTrue, - }, - ), - ); - } -} - -/// Scans a single function body for in-flight loading emits and top-level -/// awaited try statements. -class _BodyScan extends RecursiveAstVisitor { - final List loadingEmits = []; - final List topLevelTries = []; - - static final RegExp _loading = RegExp( - r'(Loading|Processing|Submitting|Saving|Creating|Updating|InProgress)', - ); - - int _tryDepth = 0; - - void _maybeLoading(AstNode node, List arguments) { - if (arguments.isEmpty) return; - final first = arguments.first; - if (first is! Expression) return; - if (_loading.hasMatch(first.toSource())) loadingEmits.add(node.offset); - } - - @override - void visitMethodInvocation(MethodInvocation node) { - if (node.methodName.name == 'emit') { - _maybeLoading(node, node.argumentList.arguments); - } - super.visitMethodInvocation(node); - } - - @override - void visitFunctionExpressionInvocation(FunctionExpressionInvocation node) { - final function = node.function; - if (function is SimpleIdentifier && function.name == 'emit') { - _maybeLoading(node, node.argumentList.arguments); - } - super.visitFunctionExpressionInvocation(node); - } - - @override - void visitTryStatement(TryStatement node) { - final isTopLevel = _tryDepth == 0; - _tryDepth++; - if (isTopLevel) topLevelTries.add(node); - super.visitTryStatement(node); - _tryDepth--; - } - - // Nested functions are analysed in their own right by _analyze; descending - // into them here would report the same catch twice. - @override - void visitFunctionExpression(FunctionExpression node) {} - - @override - void visitFunctionDeclaration(FunctionDeclaration node) {} -} - -class _HasAwait extends RecursiveAstVisitor { - _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 - void visitAwaitExpression(AwaitExpression node) { - 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 { - _SetsFlagTrue(this.fields); - - final Set 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 { - _LoadingResetCollector( - this.project, - this.path, - this.lineInfo, - this.stateOwners, - this.findings, - ); - - final Project project; - final String path; - final LineInfo lineInfo; - final Set stateOwners; - final List findings; - final List _classStack = []; - - static final RegExp _stateAssign = RegExp(r'(^|[^.\w])state\s*=(?!=)'); - static final RegExp _throwing = RegExp(r'\b(rethrow|throw)\b'); - static final RegExp _errorish = RegExp( - r'(Error|Failure|Fail|Cancel|Retry|Recover|Reset|Clear)', - caseSensitive: false, - ); - - @override - void visitClassDeclaration(ClassDeclaration node) { - _classStack.add(node.namePart.typeName.lexeme); - super.visitClassDeclaration(node); - _classStack.removeLast(); - } - - @override - void visitMixinDeclaration(MixinDeclaration node) { - _classStack.add(node.name.lexeme); - super.visitMixinDeclaration(node); - _classStack.removeLast(); - } - - @override - void visitMethodDeclaration(MethodDeclaration node) { - _analyze(node.body, node.name.lexeme); - super.visitMethodDeclaration(node); - } - - @override - void visitFunctionExpression(FunctionExpression node) { - _analyze(node.body, _closureMemberName(node)); - super.visitFunctionExpression(node); - } - - void _analyze(FunctionBody? body, String member) { - if (body == null) return; - final owner = _classStack.isEmpty ? '' : _classStack.last; - if (!stateOwners.contains(owner)) return; - - final scan = _BodyScan(); - 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(); - statement.body.accept(probe); - if (!probe.found) continue; - - for (final catchClause in statement.catchClauses) { - final source = catchClause.body.toSource(); - final emits = - source.contains('emit(') || source.contains('notifyListeners('); - final assignsState = _stateAssign.hasMatch(source); - final rethrows = _throwing.hasMatch(source); - final dispatchesFailure = - source.contains('add(') && _errorish.hasMatch(source); - if (emits || assignsState || rethrows || dispatchesFailure) continue; - swallowed++; - - findings.add( - Finding( - rule: ruleLoadingNotReset, - severity: PipeSeverity.warning, - confidence: 'high', - message: - '$owner.$member emits an in-progress state and then awaits an ' - 'operation whose catch clause never emits a terminal state, so ' - 'the UI can be left showing a spinner forever.', - file: project.relative(path), - line: lineInfo.getLocation(catchClause.offset).lineNumber, - evidence: [ - '${project.relative(path)}:' - '${lineInfo.getLocation(catchClause.offset).lineNumber}: ' - '${_oneLine(source)}', - ], - remediation: - 'Emit a failure/timeout state in the catch (or rethrow) so the ' - 'loading state is always resolved.', - data: { - 'owner': owner, - 'member': member, - // A method can hold more than one swallowed catch, so which one - // is part of this rule's identity. - 'catch': swallowed, - }, - ), - ); - } - } - } - - static String _oneLine(String source) { - final collapsed = source.replaceAll(RegExp(r'\s+'), ' ').trim(); - final body = collapsed.isEmpty ? '' : collapsed; - return body.length <= 120 ? body : '${body.substring(0, 117)}...'; - } -} diff --git a/lib/src/state_flow.dart b/lib/src/state_flow.dart index 4843fc1..d07fb3d 100644 --- a/lib/src/state_flow.dart +++ b/lib/src/state_flow.dart @@ -4,6 +4,8 @@ import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/source/line_info.dart'; import 'analysis.dart'; +import 'dart_facts.dart'; +import 'facts.dart'; import 'project.dart'; /// How a state union is expressed in source. @@ -354,7 +356,12 @@ class StateFlow { required this.ownerReferences, required this.flagStates, required this.riverpod, - }); + this.constructedByFile = const {}, + this.swallowedFailures = const [], + this.unsettledFlags = const [], + UnresolvedSummary? unresolved, + }) : unresolved = + unresolved ?? UnresolvedSummary(files: const [], analysed: 0); final String root; final List unions; @@ -376,6 +383,19 @@ class StateFlow { /// Riverpod providers and their consumption sites. final RiverpodModel riverpod; + /// Project-relative file -> type names constructed in it. + final Map> constructedByFile; + + /// Busy variant emitted, then an awaited failure swallowed (loading-not-reset). + final List swallowedFailures; + + /// Loading flag left set across unguarded awaited work + /// (loading-flag-not-reset). + final List unsettledFlags; + + /// Files whose references did not resolve, and the names they mention. + final UnresolvedSummary unresolved; + Map toJson() => { 'root': root, 'totals': { @@ -400,6 +420,13 @@ class StateFlow { 'ownerReferences': ownerReferences, 'flagStates': [for (final state in flagStates) state.toJson()], 'riverpod': riverpod.toJson(), + 'constructedByFile': { + for (final entry in constructedByFile.entries) + entry.key: entry.value.toList()..sort(), + }, + 'swallowedFailures': [for (final fact in swallowedFailures) fact.toJson()], + 'unsettledFlags': [for (final fact in unsettledFlags) fact.toJson()], + 'unresolved': unresolved.toJson(), }; /// Human summary, ordered by the most suspicious signal first. @@ -1788,10 +1815,33 @@ StateFlow buildStateFlow(Project project, List outcomes) { final dispatches = []; final riverpodProviders = []; final riverpodSites = []; + final constructedByFile = >{}; + final swallowedFailures = []; + final unsettledFlags = []; + final flagStatesByOwner = { + for (final state in flagStates) state.owner: state, + }; for (final entry in unitsByPath.entries) { final result = entry.value; final unit = result.unit; final lineInfo = result.lineInfo; + final relative = project.relative(entry.key); + final constructions = ConstructionCollector(); + unit.accept(constructions); + constructedByFile[relative] = constructions.constructed; + unit.accept( + SwallowedFailureCollector(relative, lineInfo, swallowedFailures), + ); + if (flagStatesByOwner.isNotEmpty) { + unit.accept( + UnsettledFlagCollector( + relative, + lineInfo, + flagStatesByOwner, + unsettledFlags, + ), + ); + } unit.accept( _EmissionCollector(project, entry.key, lineInfo, emissions, redirects), ); @@ -1886,5 +1936,23 @@ StateFlow buildStateFlow(Project project, List outcomes) { ownerReferences: ownerReferences, flagStates: flagStates, riverpod: RiverpodModel(providers: riverpodProviders, sites: riverpodSites), + constructedByFile: constructedByFile, + swallowedFailures: swallowedFailures, + unsettledFlags: unsettledFlags, + unresolved: _unresolvedSummary(project, outcomes), + ); +} + +UnresolvedSummary _unresolvedSummary( + Project project, + List outcomes, +) { + final found = findUnresolvedReferences(outcomes); + final first = found.first; + return UnresolvedSummary( + files: [for (final file in found.files) project.relative(file)], + analysed: found.analysed, + first: first?.replaceFirst('${project.root}/', ''), + names: found.names, ); } -- 2.52.0 From c3f1fb63197093bf6401afee2c3b20cac17c9af4 Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 14:23:39 -0600 Subject: [PATCH 11/14] refactor(rules): provider rules judge facts; runChecks takes only the model Second step of the state-map seam. The five ChangeNotifier/provider rules walked resolved units from runProviderRules. Their recognisers move to the Dart front end (dart_provider_facts.dart) and emit a NotifierModel of neutral facts (facts.dart): - NotifierFact / NotifierMethod / FieldMutation: an observable object that must announce its own changes, its methods, what each calls, mutates, and whether it notifies directly. Deliberately general enough for a Qt QObject or a hand-rolled observer subject. - consumer sites, provider declarations, member uses per file. - NonSubscribingRead: every read made while building without subscribing; whether the type is observable is now the rule's filter, not the recogniser's. - ProvidersCreatedInBuild: providers constructed in a build method. The transitive notify closure, the observable/lifecycle/wrapper judgements and all messages stay in provider_rules.dart, which no longer imports the analyzer. runChecks now takes only a StateFlow: no Project, no FileOutcome. The scanned directories ride on the model (StateFlow.directories) for the dead-handler scope claim. facts.dart is exported publicly. No behaviour change, checked byte for byte: `check --format json` is identical on the fixture and on resolved mirrors of nuwayze, tick, dex and word_alive. 128 tests pass. --- lib/pipe.dart | 3 +- lib/src/dart_provider_facts.dart | 535 ++++++++++++++++++++ lib/src/facts.dart | 295 +++++++++++ lib/src/pipeline.dart | 2 - lib/src/provider_rules.dart | 820 ++++--------------------------- lib/src/rules.dart | 14 +- lib/src/state_flow.dart | 17 +- test/baseline_test.dart | 8 +- test/policy_test.dart | 8 +- test/state_flow_test.dart | 2 +- 10 files changed, 955 insertions(+), 749 deletions(-) create mode 100644 lib/src/dart_provider_facts.dart diff --git a/lib/pipe.dart b/lib/pipe.dart index 114b447..cbec57d 100644 --- a/lib/pipe.dart +++ b/lib/pipe.dart @@ -24,7 +24,8 @@ export 'src/index.dart' export 'src/model.dart' show DeclarationInfo, FileModel, ProjectModel, buildProjectModel; export 'src/pipeline.dart' show ProjectAnalysis, analyzeProject; -export 'src/provider_rules.dart' show ProviderDeclaration, providerRuleNote; +export 'src/facts.dart'; +export 'src/provider_rules.dart' show providerRuleNote; export 'src/recall.dart' show RecallCase, RecallOutcome, RecallReport, loadRecallCases, runRecall; export 'src/project.dart' show Project, loadProject; diff --git a/lib/src/dart_provider_facts.dart b/lib/src/dart_provider_facts.dart new file mode 100644 index 0000000..f3583e5 --- /dev/null +++ b/lib/src/dart_provider_facts.dart @@ -0,0 +1,535 @@ +/// The Dart front end's recognisers for the `ChangeNotifier` / `provider` +/// rules. They emit the neutral facts in facts.dart; provider_rules.dart judges +/// them and never sees a syntax tree. +library; + +import 'package:analyzer/dart/analysis/results.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/source/line_info.dart'; + +import 'facts.dart'; +import 'project.dart'; + +/// Recognises `ChangeNotifier` classes (by supertype or mixin name). +class _NotifierCollector extends RecursiveAstVisitor { + _NotifierCollector(this.relativePath, this.lineInfo, this.notifiers); + + final String relativePath; + final LineInfo lineInfo; + final List notifiers; + + static final RegExp _notify = RegExp( + r'notifyListeners', + caseSensitive: false, + ); + + static bool _isChangeNotifier(ClassDeclaration node) { + final superclass = node.extendsClause?.superclass.toSource() ?? ''; + if (superclass.contains('ChangeNotifier')) return true; + return (node.withClause?.mixinTypes ?? const []).any( + (type) => type.toSource().contains('ChangeNotifier'), + ); + } + + @override + void visitClassDeclaration(ClassDeclaration node) { + if (!_isChangeNotifier(node)) { + super.visitClassDeclaration(node); + return; + } + + final fields = {}; + for (final member in node.body.members) { + if (member is! FieldDeclaration) continue; + for (final variable in member.fields.variables) { + fields.add(variable.name.lexeme); + } + } + + final methods = {}; + for (final member in node.body.members) { + if (member is! MethodDeclaration) continue; + if (member.isGetter) continue; + if (member.isStatic) continue; + + final source = member.toSource(); + final called = _CalledNames(); + member.body.accept(called); + final mutations = []; + for (final field in fields) { + // `_x = ...` or `this._x = ...`, but not `==`. + final pattern = RegExp( + r'(^|[^.\w])(this\.)?' + RegExp.escape(field) + r'\s*=(?!=)', + ); + if (pattern.hasMatch(source)) { + mutations.add( + FieldMutation( + field, + lineInfo.getLocation(member.offset).lineNumber, + ), + ); + } + } + methods[member.name.lexeme] = NotifierMethod( + name: member.name.lexeme, + calledNames: called.names, + directNotify: _notify.hasMatch(source), + mutations: mutations, + ); + } + + // What the UI can observe. + final observable = { + for (final field in fields) + if (!field.startsWith('_')) field, + }; + final apiMembers = { + for (final field in fields) + if (!field.startsWith('_')) field, + }; + for (final member in node.body.members) { + if (member is! MethodDeclaration) continue; + if (!member.isGetter) continue; + if (member.name.lexeme.startsWith('_')) continue; + apiMembers.add(member.name.lexeme); + final source = member.toSource(); + for (final field in fields) { + if (RegExp(r'\b' + RegExp.escape(field) + r'\b').hasMatch(source)) { + observable.add(field); + } + } + } + + if (methods.isNotEmpty) { + notifiers.add( + NotifierFact( + name: node.namePart.typeName.lexeme, + file: relativePath, + line: lineInfo.getLocation(node.offset).lineNumber, + methods: methods.values.toList(), + observableFields: observable, + apiMembers: apiMembers, + ), + ); + } + super.visitClassDeclaration(node); + } +} + +/// Method names invoked on `this` (or unqualified) within a body. +class _CalledNames extends RecursiveAstVisitor { + final Set names = {}; + + @override + void visitMethodInvocation(MethodInvocation node) { + final target = node.target; + if (target == null || target is ThisExpression) { + names.add(node.methodName.name); + } + super.visitMethodInvocation(node); + } + + @override + void visitFunctionExpressionInvocation(FunctionExpressionInvocation node) { + final function = node.function; + if (function is SimpleIdentifier) names.add(function.name); + super.visitFunctionExpressionInvocation(node); + } +} + +/// Consumer sites that read a notifier, used to say whether a Consumer can ever +/// rebuild. +class _ConsumerCollector extends RecursiveAstVisitor { + _ConsumerCollector(this.relativePath, this.lineInfo, this.consumedBy); + + final String relativePath; + final LineInfo lineInfo; + final Map> consumedBy; + + static const Set _widgets = { + 'Consumer', + 'Selector', + 'ChangeNotifierProvider', + 'ListenableBuilder', + 'AnimatedBuilder', + }; + + @override + void visitInstanceCreationExpression(InstanceCreationExpression node) { + final type = node.constructorName.type; + final name = type.name.lexeme; + if (!_widgets.contains(name)) { + super.visitInstanceCreationExpression(node); + return; + } + for (final argument + in type.typeArguments?.arguments ?? const []) { + final typeName = argument.toSource().split('<').first.trim(); + consumedBy + .putIfAbsent(typeName, () => []) + .add('$relativePath:${lineInfo.getLocation(node.offset).lineNumber}'); + } + super.visitInstanceCreationExpression(node); + } + + @override + void visitMethodInvocation(MethodInvocation node) { + final name = node.methodName.name; + if (name == 'watch' || name == 'read' || name == 'of') { + for (final argument + in node.typeArguments?.arguments ?? const []) { + final typeName = argument.toSource().split('<').first.trim(); + consumedBy + .putIfAbsent(typeName, () => []) + .add( + '$relativePath:${lineInfo.getLocation(node.offset).lineNumber}', + ); + } + } + super.visitMethodInvocation(node); + } +} + +/// Collects `provider` package provider declarations (all provider types). +class _ProviderDeclarationCollector extends RecursiveAstVisitor { + _ProviderDeclarationCollector( + this.relativePath, + this.lineInfo, + this.declarations, + ); + + final String relativePath; + final LineInfo lineInfo; + final List declarations; + + static const Set kinds = { + 'Provider', + 'ChangeNotifierProvider', + 'ListenableProvider', + 'ValueListenableProvider', + 'FutureProvider', + 'StreamProvider', + 'StateProvider', + 'ProxyProvider', + 'ChangeNotifierProxyProvider', + 'InheritedProvider', + }; + + @override + void visitInstanceCreationExpression(InstanceCreationExpression node) { + final type = node.constructorName.type; + var kind = type.name.lexeme; + if (kind.endsWith('ProxyProvider')) kind = 'ProxyProvider'; + if (!kinds.contains(kind)) { + super.visitInstanceCreationExpression(node); + return; + } + + // The provider package takes `create:`/`update:`; Riverpod passes a + // positional `(ref)` callback. That plus a `package:provider` import keeps + // the two apart. + final named = {}; + for (final argument in node.argumentList.arguments) { + if (argument is NamedArgument) named.add(argument.name.lexeme); + } + final looksLikeRiverpod = + !named.contains('create') && + !named.contains('update') && + !named.contains('value') && + node.constructorName.name == null; + if (looksLikeRiverpod) { + super.visitInstanceCreationExpression(node); + return; + } + + final arguments = type.typeArguments?.arguments ?? const []; + declarations.add( + ProviderDeclaration( + kind: kind, + valueType: arguments.isEmpty + ? null + : arguments.first.toSource().split('<').first.trim(), + file: relativePath, + line: lineInfo.getLocation(node.offset).lineNumber, + ), + ); + super.visitInstanceCreationExpression(node); + } +} + +/// Finds `context.read()` / `Provider.of(context, listen: false)` used +/// inside a `build` method for a type that is a `Listenable`. +/// +/// `read` never subscribes, so a widget that displays such a value will not +/// rebuild when it changes. This targets ChangeNotifier-provided state, which is +/// why it only fires for listenable provider types or known ChangeNotifiers. +class _ReadInBuildCollector extends RecursiveAstVisitor { + _ReadInBuildCollector(this.relativePath, this.lineInfo, this.reads); + + final String relativePath; + final LineInfo lineInfo; + final List reads; + + bool _isBuildMethod(MethodDeclaration node) { + if (node.name.lexeme != 'build') return false; + final returnType = node.returnType?.toSource() ?? ''; + if (returnType.contains('Widget')) return true; + final parameters = node.parameters; + if (parameters == null) return false; + for (final parameter in parameters.parameters) { + if (parameter.toSource().contains('BuildContext')) return true; + } + return false; + } + + @override + void visitMethodDeclaration(MethodDeclaration node) { + if (!_isBuildMethod(node)) { + super.visitMethodDeclaration(node); + return; + } + final collector = _ReadCallCollector(); + node.body.accept(collector); + for (final call in collector.calls) { + reads.add( + NonSubscribingRead( + type: call.type, + call: call.source, + path: relativePath, + line: lineInfo.getLocation(call.offset).lineNumber, + ), + ); + } + // Still descend: a nested widget may have its own build method. + super.visitMethodDeclaration(node); + } +} + +class _ReadCall { + _ReadCall(this.type, this.offset, this.source); + + final String type; + final int offset; + final String source; +} + +/// Collects non-subscribing reads made directly in a build body. +/// +/// It deliberately does NOT descend into nested functions: a read inside a +/// closure (a button callback, a BlocListener listener, a Selector builder) is +/// not made while the build method runs, so not subscribing there is usually +/// exactly right. Hand-checking the first real findings showed every one of them +/// was a closure read, which is why this is a precision guard rather than an +/// oversight - it costs recall on reads inside a `Builder(builder: ...)`. +class _ReadCallCollector extends RecursiveAstVisitor { + final List<_ReadCall> calls = []; + + @override + void visitFunctionExpression(FunctionExpression node) {} + + @override + void visitFunctionDeclaration(FunctionDeclaration node) {} + + @override + void visitMethodInvocation(MethodInvocation node) { + final arguments = node.typeArguments?.arguments ?? const []; + if (arguments.isNotEmpty) { + final type = arguments.first.toSource().split('<').first.trim(); + final name = node.methodName.name; + final target = node.target?.toSource(); + + // context.read() + if (name == 'read' && (target == 'context' || target == 'this.context')) { + calls.add(_ReadCall(type, node.offset, node.toSource())); + } + + // Provider.of(context, listen: false) + if (name == 'of' && (target == 'Provider' || target == 'provider')) { + final listens = node.argumentList.arguments.any( + (argument) => + argument is NamedArgument && + argument.name.lexeme == 'listen' && + argument.argumentExpression.toSource().trim() == 'false', + ); + if (listens) calls.add(_ReadCall(type, node.offset, node.toSource())); + } + } + super.visitMethodInvocation(node); + } +} + +/// Collects member reads (`x.someGetter`, `x.someField`) per file, so a public +/// notifier getter that nothing reads can be identified as dead state. +class _MemberUseCollector extends RecursiveAstVisitor { + _MemberUseCollector(this.uses); + + final Set uses; + + @override + void visitPropertyAccess(PropertyAccess node) { + uses.add(node.propertyName.name); + super.visitPropertyAccess(node); + } + + @override + void visitPrefixedIdentifier(PrefixedIdentifier node) { + uses.add(node.identifier.name); + super.visitPrefixedIdentifier(node); + } +} + +/// Finds a provider constructed directly inside a build method. +/// +/// Without `.value` the provider's `create` callback runs on every rebuild, so a +/// new notifier (and a new listener registration) is created each time and any +/// state it held is lost. +class _ProviderInBuildCollector extends RecursiveAstVisitor { + _ProviderInBuildCollector(this.relativePath, this.lineInfo, this.facts); + + final String relativePath; + final LineInfo lineInfo; + final List facts; + final List _classStack = []; + + static const Set kinds = { + 'Provider', + 'ChangeNotifierProvider', + 'ListenableProvider', + 'ValueListenableProvider', + 'ProxyProvider', + 'ChangeNotifierProxyProvider', + }; + + bool _isBuildMethod(MethodDeclaration node) { + if (node.name.lexeme != 'build') return false; + if ((node.returnType?.toSource() ?? '').contains('Widget')) return true; + final parameters = node.parameters; + if (parameters == null) return false; + return parameters.parameters.any( + (parameter) => parameter.toSource().contains('BuildContext'), + ); + } + + @override + void visitClassDeclaration(ClassDeclaration node) { + _classStack.add(node.namePart.typeName.lexeme); + super.visitClassDeclaration(node); + _classStack.removeLast(); + } + + @override + void visitMethodDeclaration(MethodDeclaration node) { + if (!_isBuildMethod(node)) { + super.visitMethodDeclaration(node); + return; + } + final collector = _ProviderCreationCollector(); + node.body.accept(collector); + if (collector.creations.isEmpty) { + super.visitMethodDeclaration(node); + return; + } + + final owner = _classStack.isEmpty ? '' : _classStack.last; + facts.add( + ProvidersCreatedInBuild( + owner: owner, + path: relativePath, + line: lineInfo.getLocation(node.offset).lineNumber, + creations: [ + for (final creation in collector.creations) + ProviderCreation( + kind: creation.kind, + valueType: creation.valueType, + line: lineInfo.getLocation(creation.offset).lineNumber, + ), + ], + ), + ); + super.visitMethodDeclaration(node); + } +} + +class _Creation { + _Creation(this.kind, this.valueType, this.offset, this.source); + + final String kind; + final String valueType; + final int offset; + final String source; +} + +class _ProviderCreationCollector extends RecursiveAstVisitor { + final List<_Creation> creations = []; + + // A closure runs later, so a provider created there is a different problem. + @override + void visitFunctionExpression(FunctionExpression node) {} + + @override + void visitFunctionDeclaration(FunctionDeclaration node) {} + + @override + void visitInstanceCreationExpression(InstanceCreationExpression node) { + final type = node.constructorName.type; + var kind = type.name.lexeme; + if (kind.endsWith('ProxyProvider')) kind = 'ProxyProvider'; + // `.value` passes an existing instance, which is correct. + if (node.constructorName.name?.name == 'value') { + super.visitInstanceCreationExpression(node); + return; + } + if (_ProviderInBuildCollector.kinds.contains(kind)) { + final arguments = + type.typeArguments?.arguments ?? const []; + creations.add( + _Creation( + kind, + arguments.isEmpty + ? '?' + : arguments.first.toSource().split('<').first.trim(), + node.offset, + node.toSource(), + ), + ); + } + super.visitInstanceCreationExpression(node); + } +} + +/// Collect the notifier/provider facts from resolved units, in unit order. +NotifierModel collectNotifierModel( + Project project, + Map unitsByPath, +) { + final notifiers = []; + final consumedBy = >{}; + final declarations = []; + final memberUses = >{}; + final reads = []; + final createdInBuild = []; + for (final entry in unitsByPath.entries) { + final unit = entry.value.unit; + final lineInfo = entry.value.lineInfo; + final relative = project.relative(entry.key); + unit.accept(_NotifierCollector(relative, lineInfo, notifiers)); + unit.accept(_ConsumerCollector(relative, lineInfo, consumedBy)); + unit.accept( + _ProviderDeclarationCollector(relative, lineInfo, declarations), + ); + final uses = {}; + unit.accept(_MemberUseCollector(uses)); + memberUses[relative] = uses; + unit.accept(_ReadInBuildCollector(relative, lineInfo, reads)); + unit.accept(_ProviderInBuildCollector(relative, lineInfo, createdInBuild)); + } + return NotifierModel( + notifiers: notifiers, + consumedBy: consumedBy, + declarations: declarations, + memberUses: memberUses, + nonSubscribingReads: reads, + createdInBuild: createdInBuild, + ); +} diff --git a/lib/src/facts.dart b/lib/src/facts.dart index 3097e50..e010e45 100644 --- a/lib/src/facts.dart +++ b/lib/src/facts.dart @@ -151,3 +151,298 @@ class UnresolvedSummary { 'names': names.toList()..sort(), }; } + +/// An observable object that must announce its own changes: a Dart +/// `ChangeNotifier`, a Qt `QObject` with signals, a hand-rolled observer +/// subject. Whether a mutation is ever announced is the rules' judgement. +class NotifierFact { + NotifierFact({ + required this.name, + required this.file, + required this.line, + required this.methods, + required this.observableFields, + required this.apiMembers, + }); + + factory NotifierFact.fromJson(Map json) => NotifierFact( + name: json['name']! as String, + file: json['file']! as String, + line: json['line']! as int, + methods: [ + for (final method in json['methods']! as List) + NotifierMethod.fromJson(method as Map), + ], + observableFields: {...(json['observableFields']! as List).cast()}, + apiMembers: {...(json['apiMembers']! as List).cast()}, + ); + + final String name; + final String file; + final int line; + + /// Instance methods, in declaration order. + final List methods; + + /// State a consumer can observe (public fields, and private fields a public + /// getter exposes). + final Set observableFields; + + /// The public read surface: public getters and fields. + final Set apiMembers; + + Map toJson() => { + 'name': name, + 'file': file, + 'line': line, + 'methods': [for (final method in methods) method.toJson()], + 'observableFields': observableFields.toList()..sort(), + 'apiMembers': apiMembers.toList()..sort(), + }; +} + +class NotifierMethod { + NotifierMethod({ + required this.name, + required this.calledNames, + required this.directNotify, + required this.mutations, + }); + + factory NotifierMethod.fromJson(Map json) => NotifierMethod( + name: json['name']! as String, + calledNames: {...(json['calledNames']! as List).cast()}, + directNotify: json['directNotify']! as bool, + mutations: [ + for (final mutation in json['mutations']! as List) + FieldMutation.fromJson(mutation as Map), + ], + ); + + final String name; + + /// Methods it calls on its own object. + final Set calledNames; + + /// It announces a change itself (`notifyListeners()`, `emit signal`). + final bool directNotify; + + /// Fields it assigns, in the order they were found. + final List mutations; + + Map toJson() => { + 'name': name, + 'calledNames': calledNames.toList()..sort(), + 'directNotify': directNotify, + 'mutations': [for (final mutation in mutations) mutation.toJson()], + }; +} + +class FieldMutation { + FieldMutation(this.name, this.line); + + factory FieldMutation.fromJson(Map json) => + FieldMutation(json['name']! as String, json['line']! as int); + + final String name; + final int line; + + Map toJson() => {'name': name, 'line': line}; +} + +/// A read of a provided value made directly while building UI, without +/// subscribing to it (`context.read()`). Whether T is observable is the +/// rule's judgement. +class NonSubscribingRead { + NonSubscribingRead({ + required this.type, + required this.call, + required this.path, + required this.line, + }); + + factory NonSubscribingRead.fromJson(Map json) => + NonSubscribingRead( + type: json['type']! as String, + call: json['call']! as String, + path: json['path']! as String, + line: json['line']! as int, + ); + + final String type; + final String call; + final String path; + final int line; + + Map toJson() => { + 'type': type, + 'call': call, + 'path': path, + 'line': line, + }; +} + +/// A build method that constructs providers without reusing an instance, so +/// their create callbacks rerun on every rebuild. +class ProvidersCreatedInBuild { + ProvidersCreatedInBuild({ + required this.owner, + required this.path, + required this.line, + required this.creations, + }); + + factory ProvidersCreatedInBuild.fromJson(Map json) => + ProvidersCreatedInBuild( + owner: json['owner']! as String, + path: json['path']! as String, + line: json['line']! as int, + creations: [ + for (final creation in json['creations']! as List) + ProviderCreation.fromJson(creation as Map), + ], + ); + + final String owner; + final String path; + final int line; + + /// In source order. + final List creations; + + Map toJson() => { + 'owner': owner, + 'path': path, + 'line': line, + 'creations': [for (final creation in creations) creation.toJson()], + }; +} + +class ProviderCreation { + ProviderCreation({ + required this.kind, + required this.valueType, + required this.line, + }); + + factory ProviderCreation.fromJson(Map json) => + ProviderCreation( + kind: json['kind']! as String, + valueType: json['valueType']! as String, + line: json['line']! as int, + ); + + final String kind; + final String valueType; + final int line; + + Map toJson() => { + 'kind': kind, + 'valueType': valueType, + 'line': line, + }; +} + +/// Everything the notifier/provider rules judge. +class NotifierModel { + NotifierModel({ + this.notifiers = const [], + this.consumedBy = const {}, + this.declarations = const [], + this.memberUses = const {}, + this.nonSubscribingReads = const [], + this.createdInBuild = const [], + }); + + factory NotifierModel.fromJson(Map json) => NotifierModel( + notifiers: [ + for (final notifier in json['notifiers']! as List) + NotifierFact.fromJson(notifier as Map), + ], + consumedBy: { + for (final entry in (json['consumedBy']! as Map).entries) + entry.key as String: [...(entry.value as List).cast()], + }, + declarations: [ + for (final declaration in json['declarations']! as List) + ProviderDeclaration.fromJson(declaration as Map), + ], + memberUses: { + for (final entry in (json['memberUses']! as Map).entries) + entry.key as String: {...(entry.value as List).cast()}, + }, + nonSubscribingReads: [ + for (final read in json['nonSubscribingReads']! as List) + NonSubscribingRead.fromJson(read as Map), + ], + createdInBuild: [ + for (final build in json['createdInBuild']! as List) + ProvidersCreatedInBuild.fromJson(build as Map), + ], + ); + + final List notifiers; + + /// Observed type -> consumer sites (`path:line`), in source order. + final Map> consumedBy; + final List declarations; + + /// Project-relative file -> member names read in it. + final Map> memberUses; + final List nonSubscribingReads; + final List createdInBuild; + + Map toJson() => { + 'notifiers': [for (final notifier in notifiers) notifier.toJson()], + 'consumedBy': consumedBy, + 'declarations': [ + for (final declaration in declarations) declaration.toJson(), + ], + 'memberUses': { + for (final entry in memberUses.entries) + entry.key: entry.value.toList()..sort(), + }, + 'nonSubscribingReads': [ + for (final read in nonSubscribingReads) read.toJson(), + ], + 'createdInBuild': [for (final build in createdInBuild) build.toJson()], + }; +} + +/// A provider declaration (`ChangeNotifierProvider(create: ...)`). +class ProviderDeclaration { + ProviderDeclaration({ + required this.kind, + required this.valueType, + required this.file, + required this.line, + }); + + factory ProviderDeclaration.fromJson(Map json) => + ProviderDeclaration( + kind: json['kind']! as String, + valueType: json['valueType'] as String?, + file: json['file']! as String, + line: json['line']! as int, + ); + + /// `Provider`, `ChangeNotifierProvider`, `ProxyProvider`, ... + final String kind; + final String? valueType; + final String file; + final int line; + + /// Providers whose value is a `Listenable`, so consumers must subscribe. + bool get isListenable => const { + 'ChangeNotifierProvider', + 'ListenableProvider', + 'ValueListenableProvider', + }.contains(kind); + + Map toJson() => { + 'kind': kind, + if (valueType != null) 'valueType': valueType, + 'file': file, + 'line': line, + }; +} diff --git a/lib/src/pipeline.dart b/lib/src/pipeline.dart index 3d2f8fc..031028a 100644 --- a/lib/src/pipeline.dart +++ b/lib/src/pipeline.dart @@ -55,8 +55,6 @@ Future analyzeProject( ); } final report = runChecks( - project: project, - outcomes: run.outcomes, flow: flow, enabledRules: enabledRules, baseline: baseline, diff --git a/lib/src/provider_rules.dart b/lib/src/provider_rules.dart index c3ad258..fe0316a 100644 --- a/lib/src/provider_rules.dart +++ b/lib/src/provider_rules.dart @@ -1,11 +1,3 @@ -import 'package:analyzer/dart/ast/ast.dart'; -import 'package:analyzer/dart/ast/visitor.dart'; -import 'package:analyzer/source/line_info.dart'; - -import 'analysis.dart'; -import 'project.dart'; -import 'rules.dart'; - /// `ChangeNotifier` / `provider` rules. /// /// These target the `provider` half of the state-management surface, where the @@ -14,12 +6,13 @@ import 'rules.dart'; /// code often routes through a private wrapper (`_notifyListeners()`), so /// "did it notify?" has to be answered transitively over the class's own call /// graph rather than by looking for the literal call. -class _FieldMutation { - _FieldMutation(this.name, this.line); +/// +/// The rules judge the facts in [NotifierModel]; the Dart recognisers that +/// produce them live in dart_provider_facts.dart. +library; - final String name; - final int line; -} +import 'facts.dart'; +import 'rules.dart'; /// A notify wrapper `_notifyListeners()` may be declared in a base class or /// mixin, so it is recognised by naming convention. Anchored on purpose: a @@ -34,670 +27,19 @@ const Set _lifecycleMethods = { 'debugFillProperties', }; -class _MethodInfo { - _MethodInfo({ - required this.name, - required this.source, - required this.offset, - required this.calledNames, - required this.directNotify, - required this.mutations, - }); - - final String name; - final String source; - final int offset; - final Set calledNames; - final bool directNotify; - final List<_FieldMutation> mutations; -} - -class _NotifierInfo { - _NotifierInfo({ - required this.name, - required this.file, - required this.offset, - required this.line, - required this.methods, - required this.observableFields, - required this.apiMembers, - }); - - final String name; - final String file; - final int offset; - final int line; - final Map methods; - - /// Fields the UI can actually observe: public fields, plus private fields that - /// a public getter exposes. Internal bookkeeping (`_disposed`, `_idleTimer`) - /// is excluded, because mutating it invisible does not affect the user. - final Set observableFields; - - /// The public read surface: public getters and public fields. This is what a - /// consumer could display, so a member here that nothing reads is dead state. - final Set apiMembers; - - /// Methods that reach a `notifyListeners` call through the class's own calls. - final Set notifying = {}; - - /// True when the class declares a notify wrapper (e.g. `_notifyListeners`). - bool get hasNotifyWrapper => - methods.values.any((m) => m.directNotify) || - methods.keys.any((name) => name.toLowerCase().contains('notify')); -} - -class _NotifierCollector extends RecursiveAstVisitor { - _NotifierCollector(this.project, this.path, this.lineInfo, this.notifiers); - - final Project project; - final String path; - final LineInfo lineInfo; - final List<_NotifierInfo> notifiers; - - static final RegExp _notify = RegExp( - r'notifyListeners', - caseSensitive: false, - ); - - static bool _isChangeNotifier(ClassDeclaration node) { - final superclass = node.extendsClause?.superclass.toSource() ?? ''; - if (superclass.contains('ChangeNotifier')) return true; - return (node.withClause?.mixinTypes ?? const []).any( - (type) => type.toSource().contains('ChangeNotifier'), - ); - } - - @override - void visitClassDeclaration(ClassDeclaration node) { - if (!_isChangeNotifier(node)) { - super.visitClassDeclaration(node); - return; - } - - final fields = {}; - for (final member in node.body.members) { - if (member is! FieldDeclaration) continue; - for (final variable in member.fields.variables) { - fields.add(variable.name.lexeme); - } - } - - final methods = {}; - for (final member in node.body.members) { - if (member is! MethodDeclaration) continue; - if (member.isGetter) continue; - if (member.isStatic) continue; - - final source = member.toSource(); - final called = _CalledNames(); - member.body.accept(called); - final mutations = <_FieldMutation>[]; - for (final field in fields) { - // `_x = ...` or `this._x = ...`, but not `==`. - final pattern = RegExp( - r'(^|[^.\w])(this\.)?' + RegExp.escape(field) + r'\s*=(?!=)', - ); - if (pattern.hasMatch(source)) { - mutations.add( - _FieldMutation( - field, - lineInfo.getLocation(member.offset).lineNumber, - ), - ); - } - } - methods[member.name.lexeme] = _MethodInfo( - name: member.name.lexeme, - source: source, - offset: member.offset, - calledNames: called.names, - directNotify: _notify.hasMatch(source), - mutations: mutations, - ); - } - - // What the UI can observe. - final observable = { - for (final field in fields) - if (!field.startsWith('_')) field, - }; - final apiMembers = { - for (final field in fields) - if (!field.startsWith('_')) field, - }; - for (final member in node.body.members) { - if (member is! MethodDeclaration) continue; - if (!member.isGetter) continue; - if (member.name.lexeme.startsWith('_')) continue; - apiMembers.add(member.name.lexeme); - final source = member.toSource(); - for (final field in fields) { - if (RegExp(r'\b' + RegExp.escape(field) + r'\b').hasMatch(source)) { - observable.add(field); - } - } - } - - if (methods.isNotEmpty) { - notifiers.add( - _NotifierInfo( - name: node.namePart.typeName.lexeme, - file: project.relative(path), - offset: node.offset, - line: lineInfo.getLocation(node.offset).lineNumber, - methods: methods, - observableFields: observable, - apiMembers: apiMembers, - ), - ); - } - super.visitClassDeclaration(node); - } -} - -/// Method names invoked on `this` (or unqualified) within a body. -class _CalledNames extends RecursiveAstVisitor { - final Set names = {}; - - @override - void visitMethodInvocation(MethodInvocation node) { - final target = node.target; - if (target == null || target is ThisExpression) { - names.add(node.methodName.name); - } - super.visitMethodInvocation(node); - } - - @override - void visitFunctionExpressionInvocation(FunctionExpressionInvocation node) { - final function = node.function; - if (function is SimpleIdentifier) names.add(function.name); - super.visitFunctionExpressionInvocation(node); - } -} - -/// Consumer sites that read a notifier, used to say whether a Consumer can ever -/// rebuild. -class _ConsumerCollector extends RecursiveAstVisitor { - _ConsumerCollector(this.project, this.path, this.lineInfo, this.consumedBy); - - final Project project; - final String path; - final LineInfo lineInfo; - final Map> consumedBy; - - static const Set _widgets = { - 'Consumer', - 'Selector', - 'ChangeNotifierProvider', - 'ListenableBuilder', - 'AnimatedBuilder', - }; - - @override - void visitInstanceCreationExpression(InstanceCreationExpression node) { - final type = node.constructorName.type; - final name = type.name.lexeme; - if (!_widgets.contains(name)) { - super.visitInstanceCreationExpression(node); - return; - } - for (final argument - in type.typeArguments?.arguments ?? const []) { - final typeName = argument.toSource().split('<').first.trim(); - consumedBy - .putIfAbsent(typeName, () => []) - .add( - '${project.relative(path)}:' - '${lineInfo.getLocation(node.offset).lineNumber}', - ); - } - super.visitInstanceCreationExpression(node); - } - - @override - void visitMethodInvocation(MethodInvocation node) { - final name = node.methodName.name; - if (name == 'watch' || name == 'read' || name == 'of') { - for (final argument - in node.typeArguments?.arguments ?? const []) { - final typeName = argument.toSource().split('<').first.trim(); - consumedBy - .putIfAbsent(typeName, () => []) - .add( - '${project.relative(path)}:' - '${lineInfo.getLocation(node.offset).lineNumber}', - ); - } - } - super.visitMethodInvocation(node); - } -} - -/// A `provider` package provider declaration. -class ProviderDeclaration { - ProviderDeclaration({ - required this.kind, - required this.valueType, - required this.file, - required this.line, - }); - - /// `Provider`, `ChangeNotifierProvider`, `ProxyProvider`, ... - final String kind; - final String? valueType; - final String file; - final int line; - - /// Providers whose value is a `Listenable`, so consumers must subscribe. - bool get isListenable => const { - 'ChangeNotifierProvider', - 'ListenableProvider', - 'ValueListenableProvider', - }.contains(kind); - - Map toJson() => { - 'kind': kind, - if (valueType != null) 'valueType': valueType, - 'file': file, - 'line': line, - }; -} - -/// Collects `provider` package provider declarations (all provider types). -class _ProviderDeclarationCollector extends RecursiveAstVisitor { - _ProviderDeclarationCollector( - this.project, - this.path, - this.lineInfo, - this.declarations, - ); - - final Project project; - final String path; - final LineInfo lineInfo; - final List declarations; - - static const Set kinds = { - 'Provider', - 'ChangeNotifierProvider', - 'ListenableProvider', - 'ValueListenableProvider', - 'FutureProvider', - 'StreamProvider', - 'StateProvider', - 'ProxyProvider', - 'ChangeNotifierProxyProvider', - 'InheritedProvider', - }; - - @override - void visitInstanceCreationExpression(InstanceCreationExpression node) { - final type = node.constructorName.type; - var kind = type.name.lexeme; - if (kind.endsWith('ProxyProvider')) kind = 'ProxyProvider'; - if (!kinds.contains(kind)) { - super.visitInstanceCreationExpression(node); - return; - } - - // The provider package takes `create:`/`update:`; Riverpod passes a - // positional `(ref)` callback. That plus a `package:provider` import keeps - // the two apart. - final named = {}; - for (final argument in node.argumentList.arguments) { - if (argument is NamedArgument) named.add(argument.name.lexeme); - } - final looksLikeRiverpod = - !named.contains('create') && - !named.contains('update') && - !named.contains('value') && - node.constructorName.name == null; - if (looksLikeRiverpod) { - super.visitInstanceCreationExpression(node); - return; - } - - final arguments = type.typeArguments?.arguments ?? const []; - declarations.add( - ProviderDeclaration( - kind: kind, - valueType: arguments.isEmpty - ? null - : arguments.first.toSource().split('<').first.trim(), - file: project.relative(path), - line: lineInfo.getLocation(node.offset).lineNumber, - ), - ); - super.visitInstanceCreationExpression(node); - } -} - -/// Finds `context.read()` / `Provider.of(context, listen: false)` used -/// inside a `build` method for a type that is a `Listenable`. -/// -/// `read` never subscribes, so a widget that displays such a value will not -/// rebuild when it changes. This targets ChangeNotifier-provided state, which is -/// why it only fires for listenable provider types or known ChangeNotifiers. -class _ReadInBuildCollector extends RecursiveAstVisitor { - _ReadInBuildCollector( - this.project, - this.path, - this.lineInfo, - this.listenableTypes, - this.notifierNames, - this.findings, - ); - - final Project project; - final String path; - final LineInfo lineInfo; - final Set listenableTypes; - final Set notifierNames; - final List findings; - - bool _isBuildMethod(MethodDeclaration node) { - if (node.name.lexeme != 'build') return false; - final returnType = node.returnType?.toSource() ?? ''; - if (returnType.contains('Widget')) return true; - final parameters = node.parameters; - if (parameters == null) return false; - for (final parameter in parameters.parameters) { - if (parameter.toSource().contains('BuildContext')) return true; - } - return false; - } - - bool _isListened(String type) => - listenableTypes.contains(type) || notifierNames.contains(type); - - @override - void visitMethodDeclaration(MethodDeclaration node) { - if (!_isBuildMethod(node)) { - super.visitMethodDeclaration(node); - return; - } - final collector = _ReadCallCollector(); - node.body.accept(collector); - for (final call in collector.calls) { - final type = call.type; - if (!_isListened(type)) continue; - findings.add( - Finding( - rule: ruleProviderReadInBuild, - severity: PipeSeverity.warning, - confidence: 'medium', - message: - '$type is read without subscribing inside build ' - '(${call.source}). This widget will not rebuild when $type ' - 'changes, so anything it renders from that value can go stale.', - file: project.relative(path), - line: lineInfo.getLocation(call.offset).lineNumber, - evidence: [ - '${project.relative(path)}:' - '${lineInfo.getLocation(call.offset).lineNumber}: ' - '${call.source}', - ], - remediation: - 'Use context.watch<$type>() (or Provider.of<$type>(context)) so ' - 'the widget subscribes — or keep read() and make sure nothing ' - 'rendered depends on the value.', - data: {'valueType': type, 'call': call.source, 'member': 'build'}, - ), - ); - } - // Still descend: a nested widget may have its own build method. - super.visitMethodDeclaration(node); - } -} - -class _ReadCall { - _ReadCall(this.type, this.offset, this.source); - - final String type; - final int offset; - final String source; -} - -/// Collects non-subscribing reads made directly in a build body. -/// -/// It deliberately does NOT descend into nested functions: a read inside a -/// closure (a button callback, a BlocListener listener, a Selector builder) is -/// not made while the build method runs, so not subscribing there is usually -/// exactly right. Hand-checking the first real findings showed every one of them -/// was a closure read, which is why this is a precision guard rather than an -/// oversight - it costs recall on reads inside a `Builder(builder: ...)`. -class _ReadCallCollector extends RecursiveAstVisitor { - final List<_ReadCall> calls = []; - - @override - void visitFunctionExpression(FunctionExpression node) {} - - @override - void visitFunctionDeclaration(FunctionDeclaration node) {} - - @override - void visitMethodInvocation(MethodInvocation node) { - final arguments = node.typeArguments?.arguments ?? const []; - if (arguments.isNotEmpty) { - final type = arguments.first.toSource().split('<').first.trim(); - final name = node.methodName.name; - final target = node.target?.toSource(); - - // context.read() - if (name == 'read' && (target == 'context' || target == 'this.context')) { - calls.add(_ReadCall(type, node.offset, node.toSource())); - } - - // Provider.of(context, listen: false) - if (name == 'of' && (target == 'Provider' || target == 'provider')) { - final listens = node.argumentList.arguments.any( - (argument) => - argument is NamedArgument && - argument.name.lexeme == 'listen' && - argument.argumentExpression.toSource().trim() == 'false', - ); - if (listens) calls.add(_ReadCall(type, node.offset, node.toSource())); - } - } - super.visitMethodInvocation(node); - } -} - /// Member names a user would notice going unrendered. final RegExp _looksImportantMember = RegExp( r'(error|failure|message|loading|empty|count|total|status)', caseSensitive: false, ); -/// Collects member reads (`x.someGetter`, `x.someField`) per file, so a public -/// notifier getter that nothing reads can be identified as dead state. -class _MemberUseCollector extends RecursiveAstVisitor { - _MemberUseCollector(this.uses); - - final Set uses; - - @override - void visitPropertyAccess(PropertyAccess node) { - uses.add(node.propertyName.name); - super.visitPropertyAccess(node); - } - - @override - void visitPrefixedIdentifier(PrefixedIdentifier node) { - uses.add(node.identifier.name); - super.visitPrefixedIdentifier(node); - } -} - -/// Finds a provider constructed directly inside a build method. -/// -/// Without `.value` the provider's `create` callback runs on every rebuild, so a -/// new notifier (and a new listener registration) is created each time and any -/// state it held is lost. -class _ProviderInBuildCollector extends RecursiveAstVisitor { - _ProviderInBuildCollector( - this.project, - this.path, - this.lineInfo, - this.findings, - ); - - final Project project; - final String path; - final LineInfo lineInfo; - final List findings; - final List _classStack = []; - - static const Set kinds = { - 'Provider', - 'ChangeNotifierProvider', - 'ListenableProvider', - 'ValueListenableProvider', - 'ProxyProvider', - 'ChangeNotifierProxyProvider', - }; - - bool _isBuildMethod(MethodDeclaration node) { - if (node.name.lexeme != 'build') return false; - if ((node.returnType?.toSource() ?? '').contains('Widget')) return true; - final parameters = node.parameters; - if (parameters == null) return false; - return parameters.parameters.any( - (parameter) => parameter.toSource().contains('BuildContext'), - ); - } - - @override - void visitClassDeclaration(ClassDeclaration node) { - _classStack.add(node.namePart.typeName.lexeme); - super.visitClassDeclaration(node); - _classStack.removeLast(); - } - - @override - void visitMethodDeclaration(MethodDeclaration node) { - if (!_isBuildMethod(node)) { - super.visitMethodDeclaration(node); - return; - } - final collector = _ProviderCreationCollector(); - node.body.accept(collector); - if (collector.creations.isEmpty) { - super.visitMethodDeclaration(node); - return; - } - - // One finding per build method, not one per provider: on a real app root - // this is a single decision (hoist them or not), and 45 findings for one - // root widget buries everything else. The claim is factual - create does - // rerun on rebuild - but whether it matters depends on how often the widget - // rebuilds, which is why this is info and not a warning. - final owner = _classStack.isEmpty ? '' : _classStack.last; - final kinds = {}; - final valueTypes = []; - for (final creation in collector.creations) { - kinds.update(creation.kind, (v) => v + 1, ifAbsent: () => 1); - if (!valueTypes.contains(creation.valueType)) { - valueTypes.add(creation.valueType); - } - } - valueTypes.sort(); - final summary = kinds.entries.map((e) => '${e.key} x${e.value}').join(', '); - final first = collector.creations.first; - - findings.add( - Finding( - rule: ruleProviderCreatedInBuild, - severity: PipeSeverity.info, - confidence: 'medium', - message: - '$owner.build constructs ${collector.creations.length} provider(s) ' - 'without `.value` ($summary), so their create callbacks run again if ' - 'this widget rebuilds - new instances and listener registrations, ' - 'and any state they held is lost.', - file: project.relative(path), - line: lineInfo.getLocation(node.offset).lineNumber, - evidence: [ - for (final creation in collector.creations.take(3)) - '${project.relative(path)}:' - '${lineInfo.getLocation(creation.offset).lineNumber}: ' - '${creation.kind}<${creation.valueType}>', - ], - remediation: - 'Hoist these above the widget that rebuilds (a parent build or the ' - 'app root), or use the `.value` constructor with instances created ' - 'once. Harmless only if this widget never rebuilds.', - data: { - 'owner': owner, - 'member': 'build', - 'kindCounts': summary, - 'valueTypes': valueTypes, - 'providerCount': collector.creations.length, - 'firstKind': first.kind, - }, - ), - ); - super.visitMethodDeclaration(node); - } -} - -class _ProviderCreation { - _ProviderCreation(this.kind, this.valueType, this.offset, this.source); - - final String kind; - final String valueType; - final int offset; - final String source; -} - -class _ProviderCreationCollector extends RecursiveAstVisitor { - final List<_ProviderCreation> creations = []; - - // A closure runs later, so a provider created there is a different problem. - @override - void visitFunctionExpression(FunctionExpression node) {} - - @override - void visitFunctionDeclaration(FunctionDeclaration node) {} - - @override - void visitInstanceCreationExpression(InstanceCreationExpression node) { - final type = node.constructorName.type; - var kind = type.name.lexeme; - if (kind.endsWith('ProxyProvider')) kind = 'ProxyProvider'; - // `.value` passes an existing instance, which is correct. - if (node.constructorName.name?.name == 'value') { - super.visitInstanceCreationExpression(node); - return; - } - if (_ProviderInBuildCollector.kinds.contains(kind)) { - final arguments = - type.typeArguments?.arguments ?? const []; - creations.add( - _ProviderCreation( - kind, - arguments.isEmpty - ? '?' - : arguments.first.toSource().split('<').first.trim(), - node.offset, - node.toSource(), - ), - ); - } - super.visitInstanceCreationExpression(node); - } -} - /// Append `ChangeNotifier` findings to [findings]. /// /// Returns a coverage note so that "0 findings" can be told apart from "the rule /// never ran": it reports how many notifier classes and candidate mutators were /// actually examined. String runProviderRules({ - required Project project, - required List outcomes, + required NotifierModel model, required List findings, required Set enabledRules, FixPolicy policy = FixPolicy.wireUp, @@ -715,44 +57,25 @@ String runProviderRules({ return ''; } - final notifiers = <_NotifierInfo>[]; - final consumedBy = >{}; - final declarations = []; - final memberUses = >{}; - for (final outcome in outcomes) { - final unit = outcome.unit; - if (unit == null) continue; - unit.unit.accept( - _NotifierCollector(project, outcome.path, unit.lineInfo, notifiers), - ); - unit.unit.accept( - _ConsumerCollector(project, outcome.path, unit.lineInfo, consumedBy), - ); - unit.unit.accept( - _ProviderDeclarationCollector( - project, - outcome.path, - unit.lineInfo, - declarations, - ), - ); - final uses = {}; - unit.unit.accept(_MemberUseCollector(uses)); - memberUses[project.relative(outcome.path)] = uses; - } + final notifiers = model.notifiers; + final consumedBy = model.consumedBy; + final declarations = model.declarations; + final memberUses = model.memberUses; // A method notifies if it calls notifyListeners or calls a notifying method. + final notifyingOf = >{}; for (final notifier in notifiers) { + final notifying = notifyingOf[notifier] = {}; final stack = [ - for (final method in notifier.methods.values) + for (final method in notifier.methods) if (method.directNotify) method.name, - for (final name in notifier.methods.keys) - if (_notifyWrapperName.hasMatch(name)) name, + for (final method in notifier.methods) + if (_notifyWrapperName.hasMatch(method.name)) method.name, ]; while (stack.isNotEmpty) { final name = stack.removeLast(); - if (!notifier.notifying.add(name)) continue; - for (final method in notifier.methods.values) { + if (!notifying.add(name)) continue; + for (final method in notifier.methods) { if (method.calledNames.contains(name)) stack.add(method.name); } } @@ -766,21 +89,30 @@ String runProviderRules({ declaration.valueType!, }; final notifierNames = {for (final notifier in notifiers) notifier.name}; - if (listenableTypes.isNotEmpty || notifierNames.isNotEmpty) { - for (final outcome in outcomes) { - final unit = outcome.unit; - if (unit == null) continue; - unit.unit.accept( - _ReadInBuildCollector( - project, - outcome.path, - unit.lineInfo, - listenableTypes, - notifierNames, - findings, - ), - ); - } + bool isListened(String type) => + listenableTypes.contains(type) || notifierNames.contains(type); + for (final read in model.nonSubscribingReads) { + final type = read.type; + if (!isListened(type)) continue; + findings.add( + Finding( + rule: ruleProviderReadInBuild, + severity: PipeSeverity.warning, + confidence: 'medium', + message: + '$type is read without subscribing inside build ' + '(${read.call}). This widget will not rebuild when $type ' + 'changes, so anything it renders from that value can go stale.', + file: read.path, + line: read.line, + evidence: ['${read.path}:${read.line}: ${read.call}'], + remediation: + 'Use context.watch<$type>() (or Provider.of<$type>(context)) so ' + 'the widget subscribes — or keep read() and make sure nothing ' + 'rendered depends on the value.', + data: {'valueType': type, 'call': read.call, 'member': 'build'}, + ), + ); } } @@ -834,15 +166,54 @@ String runProviderRules({ // R013 - a provider constructed inside build. if (wantCreatedInBuild) { - for (final outcome in outcomes) { - final unit = outcome.unit; - if (unit == null) continue; - unit.unit.accept( - _ProviderInBuildCollector( - project, - outcome.path, - unit.lineInfo, - findings, + for (final build in model.createdInBuild) { + // One finding per build method, not one per provider: on a real app root + // this is a single decision (hoist them or not), and 45 findings for one + // root widget buries everything else. The claim is factual - create does + // rerun on rebuild - but whether it matters depends on how often the + // widget rebuilds, which is why this is info and not a warning. + final kinds = {}; + final valueTypes = []; + for (final creation in build.creations) { + kinds.update(creation.kind, (v) => v + 1, ifAbsent: () => 1); + if (!valueTypes.contains(creation.valueType)) { + valueTypes.add(creation.valueType); + } + } + valueTypes.sort(); + final summary = kinds.entries + .map((e) => '${e.key} x${e.value}') + .join(', '); + final first = build.creations.first; + findings.add( + Finding( + rule: ruleProviderCreatedInBuild, + severity: PipeSeverity.info, + confidence: 'medium', + message: + '${build.owner}.build constructs ${build.creations.length} ' + 'provider(s) without `.value` ($summary), so their create ' + 'callbacks run again if this widget rebuilds - new instances and ' + 'listener registrations, and any state they held is lost.', + file: build.path, + line: build.line, + evidence: [ + for (final creation in build.creations.take(3)) + '${build.path}:${creation.line}: ' + '${creation.kind}<${creation.valueType}>', + ], + remediation: + 'Hoist these above the widget that rebuilds (a parent build or ' + 'the app root), or use the `.value` constructor with instances ' + 'created once. Harmless only if this widget never rebuilds.', + data: { + 'owner': build.owner, + 'member': 'build', + 'kindCounts': summary, + 'valueTypes': valueTypes, + 'providerCount': build.creations.length, + 'firstKind': first.kind, + }, ), ); } @@ -854,7 +225,8 @@ String runProviderRules({ final consumers = consumedBy[notifier.name] ?? const []; final isConsumed = consumers.isNotEmpty; if (isConsumed) consumedCount++; - final inert = isConsumed && notifier.notifying.isEmpty; + final notifying = notifyingOf[notifier]!; + final inert = isConsumed && notifying.isEmpty; // When nothing in the notifier ever notifies, one class-level finding beats // one per mutator, so the consumer-facing rule covers it instead. @@ -880,7 +252,7 @@ String runProviderRules({ } if (!wantNeverNotifies) continue; - for (final method in notifier.methods.values) { + for (final method in notifier.methods) { if (method.mutations.isEmpty) continue; // Lifecycle teardown must not notify: notifying after dispose throws, so // a missing notify here is correct, not a bug. @@ -891,15 +263,15 @@ String runProviderRules({ .toList(); if (observable.isEmpty) continue; candidates++; - if (notifier.notifying.contains(method.name)) continue; + if (notifying.contains(method.name)) continue; // The UI entry points are the public methods; private helpers are an // implementation detail of some other path. if (method.name.startsWith('_')) continue; // If a notifying method calls this one, the caller is responsible for // announcing the change, so this is not a missing notification. - final calledByNotifier = notifier.methods.values.any( + final calledByNotifier = notifier.methods.any( (caller) => - notifier.notifying.contains(caller.name) && + notifying.contains(caller.name) && caller.calledNames.contains(method.name), ); if (calledByNotifier) continue; diff --git a/lib/src/rules.dart b/lib/src/rules.dart index 12147b5..766e0e7 100644 --- a/lib/src/rules.dart +++ b/lib/src/rules.dart @@ -1,8 +1,6 @@ import 'dart:convert'; -import 'analysis.dart'; import 'errors.dart'; -import 'project.dart'; import 'provider_rules.dart'; import 'state_flow.dart'; @@ -315,9 +313,10 @@ bool _looksLoading(String variant) => RegExp( ).hasMatch(variant); /// Build a finding per rule over an already-built state-flow graph. +/// +/// Rules read only [flow]: no syntax tree, no resolved unit. Any front end that +/// can produce a [StateFlow] (or its JSON) gets every rule. FindingReport runChecks({ - required Project project, - required List outcomes, required StateFlow flow, Set? enabledRules, Set? baseline, @@ -685,7 +684,7 @@ FindingReport runChecks({ confidence: 'high', message: '${union.owner} handles $event, but nothing under ' - '${project.directories.join(', ')} ever dispatches or ' + '${flow.directories.join(', ')} ever dispatches or ' 'constructs $event.', file: registration.path, line: registration.line, @@ -845,8 +844,7 @@ FindingReport runChecks({ enabled.contains(ruleNotifierFieldUnread) || enabled.contains(ruleProviderCreatedInBuild)) { final note = runProviderRules( - project: project, - outcomes: outcomes, + model: flow.notifierModel, findings: findings, enabledRules: enabled, policy: policy, @@ -902,7 +900,7 @@ FindingReport runChecks({ }); return FindingReport( - root: project.root, + root: flow.root, findings: resolved, suppressed: suppressed, notes: notes, diff --git a/lib/src/state_flow.dart b/lib/src/state_flow.dart index d07fb3d..ffe6e77 100644 --- a/lib/src/state_flow.dart +++ b/lib/src/state_flow.dart @@ -5,6 +5,7 @@ import 'package:analyzer/source/line_info.dart'; import 'analysis.dart'; import 'dart_facts.dart'; +import 'dart_provider_facts.dart'; import 'facts.dart'; import 'project.dart'; @@ -356,14 +357,21 @@ class StateFlow { required this.ownerReferences, required this.flagStates, required this.riverpod, + this.directories = const ['lib'], this.constructedByFile = const {}, this.swallowedFailures = const [], this.unsettledFlags = const [], UnresolvedSummary? unresolved, + NotifierModel? notifierModel, }) : unresolved = - unresolved ?? UnresolvedSummary(files: const [], analysed: 0); + unresolved ?? UnresolvedSummary(files: const [], analysed: 0), + notifierModel = notifierModel ?? NotifierModel(); final String root; + + /// Directories the front end scanned, so a claim like "nothing dispatches X" + /// can say where it looked. + final List directories; final List unions; final List eventUnions; final List emissions; @@ -396,8 +404,12 @@ class StateFlow { /// Files whose references did not resolve, and the names they mention. final UnresolvedSummary unresolved; + /// ChangeNotifier / provider facts. + final NotifierModel notifierModel; + Map toJson() => { 'root': root, + 'directories': directories, 'totals': { 'unions': unions.length, 'variants': unions.fold( @@ -427,6 +439,7 @@ class StateFlow { 'swallowedFailures': [for (final fact in swallowedFailures) fact.toJson()], 'unsettledFlags': [for (final fact in unsettledFlags) fact.toJson()], 'unresolved': unresolved.toJson(), + 'notifiers': notifierModel.toJson(), }; /// Human summary, ordered by the most suspicious signal first. @@ -1926,6 +1939,7 @@ StateFlow buildStateFlow(Project project, List outcomes) { return StateFlow( root: project.root, + directories: project.directories, unions: unions, eventUnions: eventUnions, emissions: emissions, @@ -1940,6 +1954,7 @@ StateFlow buildStateFlow(Project project, List outcomes) { swallowedFailures: swallowedFailures, unsettledFlags: unsettledFlags, unresolved: _unresolvedSummary(project, outcomes), + notifierModel: collectNotifierModel(project, unitsByPath), ); } diff --git a/test/baseline_test.dart b/test/baseline_test.dart index cc5d9d6..857cd30 100644 --- a/test/baseline_test.dart +++ b/test/baseline_test.dart @@ -15,12 +15,8 @@ void main() { await analysis.dispose(); }); - FindingReport reportWith(Set? baseline) => runChecks( - project: analysis.project, - outcomes: analysis.run.outcomes, - flow: analysis.flow, - baseline: baseline, - ); + FindingReport reportWith(Set? baseline) => + runChecks(flow: analysis.flow, baseline: baseline); test('encodes a baseline whose fingerprints cover every finding', () { final encoded = encodeBaseline( diff --git a/test/policy_test.dart b/test/policy_test.dart index 499a1af..e519d03 100644 --- a/test/policy_test.dart +++ b/test/policy_test.dart @@ -19,12 +19,8 @@ void main() { await analysis.dispose(); }); - FindingReport reportWith(FixPolicy policy) => runChecks( - project: analysis.project, - outcomes: analysis.run.outcomes, - flow: analysis.flow, - policy: policy, - ); + FindingReport reportWith(FixPolicy policy) => + runChecks(flow: analysis.flow, policy: policy); String briefWith(FixPolicy policy) => buildGuidance( root: '/root', diff --git a/test/state_flow_test.dart b/test/state_flow_test.dart index 0e31711..316f5ce 100644 --- a/test/state_flow_test.dart +++ b/test/state_flow_test.dart @@ -13,7 +13,7 @@ void main() { project = await loadProject(fixturePath); run = await resolveProject(project, concurrency: 1); flow = buildStateFlow(project, run.outcomes); - report = runChecks(project: project, outcomes: run.outcomes, flow: flow); + report = runChecks(flow: flow); }); tearDownAll(() async { -- 2.52.0 From 0d891919e91b8bb01b7493e1608460d98dc7420c Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 14:31:03 -0600 Subject: [PATCH 12/14] feat(check): run the rules on a state model written by any front end Third step of the state-map seam, and the proof that it holds. stateFlowFromJson rebuilds a StateFlow from its toJson form (computed members - totals, coverage differences, Riverpod gaps - are recomputed), and `pipe check --model ` runs every rule on such a file instead of analysing Dart. A front end for another language now only has to write that JSON to get every rule, --baseline ratcheting and --fail-on gating. StateUnion.toJson now carries uiReadsBaseFields (only when true), which the round trip showed it had been dropping. Checked byte for byte, through a file on disk: `pipe state --format json` then `pipe check --model` reports exactly what `pipe check` does on the fixture and on resolved mirrors of nuwayze (102 findings), tick, dex and word_alive. state_map_roundtrip_test.dart asserts the same in process (model JSON is idempotent; findings identical; every rule fires, so the comparison is not vacuous), and model_cli_test.dart covers the CLI path. --model refuses --write-baseline: a model does not record whether its front end read every file, so the baseline could silently be partial - the hazard the source path already refuses. A malformed model is a PipeException, not a crash. 134 tests pass. --- README.md | 27 ++++- lib/pipe.dart | 4 +- lib/src/cli.dart | 100 ++++++++++++---- lib/src/state_flow.dart | 182 +++++++++++++++++++++++++++++ test/model_cli_test.dart | 116 ++++++++++++++++++ test/state_map_roundtrip_test.dart | 44 +++++++ 6 files changed, 449 insertions(+), 24 deletions(-) create mode 100644 test/model_cli_test.dart create mode 100644 test/state_map_roundtrip_test.dart diff --git a/README.md b/README.md index 2a75783..a6401e2 100644 --- a/README.md +++ b/README.md @@ -375,7 +375,10 @@ lib/src/index.dart resolution health, timing, diagnostics lib/src/model.dart declaration model lib/src/state_flow.dart the state-flow IR (unions, emissions, render sites, events, flags, Riverpod) -lib/src/rules.dart breakage detectors, findings, baselines +lib/src/facts.dart language-neutral facts the rules judge (JSON round-trip) +lib/src/dart_facts.dart Dart front end: loading/construction recognisers +lib/src/dart_provider_facts.dart Dart front end: ChangeNotifier/provider recognisers +lib/src/rules.dart breakage detectors, findings, baselines (model only) lib/src/recall.dart labelled-bug recall measurement lib/src/verify.dart baseline verification (resolved / still / new) lib/src/suggest.dart ranked fix order per file @@ -386,6 +389,28 @@ tool/adjudicate.py independent precision adjudicator tool/build_release.sh native AOT build ``` +### The state model is the seam + +Rules never read a syntax tree. The Dart front end (`state_flow.dart`, +`dart_facts.dart`, `dart_provider_facts.dart`) turns resolved code into a state +model: unions and their variants, where each is emitted and rendered, event +dispatch and handling, busy paths whose failure is swallowed, observable objects +and whether their mutations are announced. `runChecks` takes only that model. + +The model is plain JSON, and a model that went through JSON reports exactly +what the in-process one does - checked byte for byte on the fixture +(`test/state_map_roundtrip_test.dart`) and on four real apps: + +```bash +pipe state . --format json > model.json # any front end can write this +pipe check --model model.json # every rule, baseline and gate +``` + +So a front end for another language only has to write that JSON; it gets every +rule, `--baseline` ratcheting and `--fail-on` gating without touching Dart. +`check --model` refuses `--write-baseline`, because a model does not record +whether its front end read every file. + Built on `package:analyzer` with whole-project resolution, not a `custom_lint`/analyzer plugin: plugins have no project-wide index, `flutter analyze` does not report new-system plugin diagnostics, and a CLI can pin its own diff --git a/lib/pipe.dart b/lib/pipe.dart index cbec57d..258edba 100644 --- a/lib/pipe.dart +++ b/lib/pipe.dart @@ -60,6 +60,7 @@ export 'src/state_flow.dart' EventDispatch, EventHandler, EventUnion, + FlagState, RenderBranch, RenderSite, StateFlow, @@ -67,7 +68,8 @@ export 'src/state_flow.dart' StateVariant, UnionCoverage, UnionStyle, - buildStateFlow; + buildStateFlow, + stateFlowFromJson; export 'src/state_flow.dart' show RiverpodGap, RiverpodModel, RiverpodProvider, RiverpodSite; export 'src/suggest.dart' show FilePlan, FixAction, WorkPlan, buildWorkPlan; diff --git a/lib/src/cli.dart b/lib/src/cli.dart index b6bbfbe..8070b38 100644 --- a/lib/src/cli.dart +++ b/lib/src/cli.dart @@ -374,6 +374,12 @@ class CheckCommand extends Command { ..addOption( 'write-baseline', help: 'Write the current findings to this baseline file.', + ) + ..addOption( + 'model', + help: + 'Check a state model (`pipe state --format json`, or any front end ' + 'that writes it) instead of analysing Dart source.', ); } @@ -388,12 +394,57 @@ class CheckCommand extends Command { Future run() async { final results = argResults!; _refuseSubsetBaseline(results); + final selected = _readRules(results); + final baseline = _readBaseline(results); + + final model = results['model'] as String?; + if (model != null) { + // A model does not record whether its front end read every file, so a + // baseline written from it could silently omit findings. + if (results['write-baseline'] != null) { + throw PipeException( + '--write-baseline cannot be used with --model: a model does not ' + 'record whether every file was read, so the baseline could be ' + 'partial', + remedy: 'write the baseline from a source analysis instead', + ); + } + final file = File(model); + if (!file.existsSync()) { + throw PipeException('model file not found: $model'); + } + final Object? json; + try { + json = jsonDecode(file.readAsStringSync()); + } on FormatException catch (error) { + throw PipeException('model is not JSON: ${error.message}'); + } + if (json is! Map) { + throw PipeException('model must be a JSON object: $model'); + } + final StateFlow flow; + try { + flow = stateFlowFromJson(json); + } on TypeError catch (error) { + throw PipeException( + 'model is missing a field or has one of the wrong type: $error', + remedy: + 'write it with `pipe state --format json`, or match that shape', + ); + } + final report = runChecks( + flow: flow, + enabledRules: selected.isEmpty ? null : selected, + baseline: baseline, + policy: _readPolicy(results), + ); + return _emitCheck(results, report); + } + final project = await loadProject( _target(results), directories: results['dir'] as List, ); - final selected = _readRules(results); - final baseline = _readBaseline(results); final analysis = await analyzeProject( project, sdkOverride: results['sdk'] as String?, @@ -405,32 +456,37 @@ class CheckCommand extends Command { policy: _readPolicy(results), ); try { - final report = analysis.report; - if (results['format'] as String == 'json') { - stdout.writeln(_encode(report.toJson())); - } else { - final top = readPositiveInt(results, 'top', fallback: 40); - stdout.write(report.describe(top: top)); - } - _writeBaseline(results, analysis, report.findings); - final threshold = switch (results['fail-on'] as String) { - 'error' => PipeSeverity.error, - 'warning' => PipeSeverity.warning, - 'info' => PipeSeverity.info, - _ => null, - }; - if (threshold != null && report.hasNewAtOrAbove(threshold)) return 1; - if ((results['fail-on-finding'] as bool) && - report.hasNewAtOrAbove(PipeSeverity.error)) { - return 1; - } + final code = _emitCheck(results, analysis.report); + _writeBaseline(results, analysis, analysis.report.findings); + return code; } finally { await analysis.dispose(); } - return 0; } } +/// Print a check report and apply the --fail-on gate. +int _emitCheck(ArgResults results, FindingReport report) { + final top = readPositiveInt(results, 'top', fallback: 40); + if (results['format'] as String == 'json') { + stdout.writeln(_encode(report.toJson())); + } else { + stdout.write(report.describe(top: top)); + } + final threshold = switch (results['fail-on'] as String) { + 'error' => PipeSeverity.error, + 'warning' => PipeSeverity.warning, + 'info' => PipeSeverity.info, + _ => null, + }; + if (threshold != null && report.hasNewAtOrAbove(threshold)) return 1; + if ((results['fail-on-finding'] as bool) && + report.hasNewAtOrAbove(PipeSeverity.error)) { + return 1; + } + return 0; +} + /// Measure detection against a labelled set of known bugs. class RecallCommand extends Command { RecallCommand() { diff --git a/lib/src/state_flow.dart b/lib/src/state_flow.dart index ffe6e77..50600f4 100644 --- a/lib/src/state_flow.dart +++ b/lib/src/state_flow.dart @@ -89,6 +89,7 @@ class StateUnion { 'ownerKind': ownerKind, if (eventType != null) 'eventType': eventType, 'variants': [for (final variant in variants) variant.toJson()], + if (uiReadsBaseFields) 'uiReadsBaseFields': true, }; } @@ -1971,3 +1972,184 @@ UnresolvedSummary _unresolvedSummary( names: found.names, ); } + +// --------------------------------------------------------------------------- +// Decoding +// --------------------------------------------------------------------------- + +/// Rebuild a [StateFlow] from its [StateFlow.toJson] form. +/// +/// This is the seam for other front ends: anything that writes this JSON (a +/// separate process for another language, say) can be checked by every rule +/// with `runChecks(flow: stateFlowFromJson(json))`. Computed members (`totals`, +/// coverage differences, Riverpod `gaps`, `isAsync`) are ignored on input and +/// recomputed. +StateFlow stateFlowFromJson(Map json) { + List> objects(Object? value) => [ + for (final item in (value as List?) ?? const []) + item as Map, + ]; + List strings(Object? value) => [ + ...((value as List?) ?? const []).cast(), + ]; + StateVariant variant(Map v) => StateVariant( + name: v['name']! as String, + declaredIn: v['declaredIn'] as String?, + note: v['note'] as String?, + parent: v['parent'] as String?, + ); + + final unions = [ + for (final u in objects(json['unions'])) + StateUnion( + name: u['name']! as String, + declaredIn: u['declaredIn']! as String, + style: UnionStyle.values.byName(u['style']! as String), + owner: u['owner']! as String, + ownerKind: u['ownerKind']! as String, + eventType: u['eventType'] as String?, + variants: [for (final v in objects(u['variants'])) variant(v)], + uiReadsBaseFields: (u['uiReadsBaseFields'] as bool?) ?? false, + ), + ]; + StateUnion unionFor(String name, String owner) => + unions.firstWhere((union) => union.name == name && union.owner == owner); + + return StateFlow( + root: json['root']! as String, + directories: strings(json['directories'] ?? const ['lib']), + unions: unions, + eventUnions: [ + for (final u in objects(json['eventUnions'])) + EventUnion( + name: u['name']! as String, + declaredIn: u['declaredIn']! as String, + owner: u['owner']! as String, + variants: [for (final v in objects(u['variants'])) variant(v)], + ), + ], + emissions: [ + for (final e in objects(json['emissions'])) + EmissionSite( + path: e['path']! as String, + line: e['line']! as int, + owner: e['owner']! as String, + member: e['member']! as String, + kind: e['kind']! as String, + confidence: e['confidence']! as String, + variants: strings(e['variants']), + snippet: e['snippet']! as String, + ), + ], + renders: [ + for (final r in objects(json['renders'])) + RenderSite( + path: r['path']! as String, + line: r['line']! as int, + widget: r['widget']! as String, + stateType: r['stateType']! as String, + branches: [ + for (final b in objects(r['branches'])) + RenderBranch( + variant: b['variant']! as String, + line: b['line']! as int, + trivial: b['trivial']! as bool, + negated: (b['negated'] as bool?) ?? false, + ), + ], + ), + ], + handlers: [ + for (final h in objects(json['handlers'])) + EventHandler( + bloc: h['bloc']! as String, + event: h['event']! as String, + path: h['path']! as String, + line: h['line']! as int, + ), + ], + dispatches: [ + for (final d in objects(json['dispatches'])) + EventDispatch( + event: d['event']! as String, + path: d['path']! as String, + line: d['line']! as int, + confidence: d['confidence']! as String, + snippet: d['snippet']! as String, + ), + ], + coverages: [ + for (final c in objects(json['coverage'])) + UnionCoverage( + union: unionFor(c['union']! as String, c['owner']! as String), + declared: {...strings(c['declared'])}, + emitted: {...strings(c['emitted'])}, + initial: {...strings(c['initial'])}, + rendered: {...strings(c['rendered'])}, + renderedInBuilder: {...strings(c['renderedInBuilder'])}, + trivialInBuilder: {...strings(c['trivialInBuilder'])}, + renderSites: c['renderSites']! as int, + emissionSites: c['emissionSites']! as int, + ), + ], + ownerReferences: { + for (final entry + in ((json['ownerReferences'] as Map?) ?? const {}).entries) + entry.key as String: strings(entry.value), + }, + flagStates: [ + for (final f in objects(json['flagStates'])) + FlagState( + name: f['name']! as String, + owner: f['owner']! as String, + fields: strings(f['fields']), + defaultsTrue: f['defaultsTrue']! as bool, + ), + ], + riverpod: RiverpodModel( + providers: [ + for (final p in objects((json['riverpod'] as Map?)?['providers'])) + RiverpodProvider( + name: p['name']! as String, + kind: p['kind']! as String, + declaredIn: p['declaredIn']! as String, + line: p['line']! as int, + valueType: p['valueType'] as String?, + ), + ], + sites: [ + for (final site in objects((json['riverpod'] as Map?)?['sites'])) + RiverpodSite( + provider: site['provider']! as String, + path: site['path']! as String, + line: site['line']! as int, + watches: site['watch']! as bool, + handlesError: site['handlesError']! as bool, + handlesLoading: site['handlesLoading']! as bool, + readsValue: site['readsValue']! as bool, + ), + ], + ), + constructedByFile: { + for (final entry + in ((json['constructedByFile'] as Map?) ?? const {}).entries) + entry.key as String: {...strings(entry.value)}, + }, + swallowedFailures: [ + for (final f in objects(json['swallowedFailures'])) + SwallowedFailure.fromJson(f), + ], + unsettledFlags: [ + for (final f in objects(json['unsettledFlags'])) + UnsettledFlag.fromJson(f), + ], + unresolved: json['unresolved'] == null + ? null + : UnresolvedSummary.fromJson( + json['unresolved']! as Map, + ), + notifierModel: json['notifiers'] == null + ? null + : NotifierModel.fromJson(json['notifiers']! as Map), + ); +} diff --git a/test/model_cli_test.dart b/test/model_cli_test.dart new file mode 100644 index 0000000..5999502 --- /dev/null +++ b/test/model_cli_test.dart @@ -0,0 +1,116 @@ +import 'dart:io'; + +import 'package:pipe/pipe.dart'; +import 'package:pipe/src/cli.dart'; +import 'package:test/test.dart'; + +/// Collects what a command writes to stdout. +class _CapturedStdout implements Stdout { + final StringBuffer buffer = StringBuffer(); + + @override + void write(Object? object) => buffer.write(object); + + @override + void writeln([Object? object = '']) => buffer.writeln(object); + + @override + dynamic noSuchMethod(Invocation invocation) => null; +} + +Future<(int?, String)> _run(List args) async { + final out = _CapturedStdout(); + final code = await IOOverrides.runZoned( + () => PipeRunner().run(args), + stdout: () => out, + ); + return (code, out.buffer.toString()); +} + +const String _fixture = 'test/fixtures/stuck_loading'; + +void main() { + late String model; + + setUpAll(() async { + final dir = Directory.systemTemp.createTempSync('pipe-model'); + model = '${dir.path}/model.json'; + final (code, json) = await _run([ + 'state', + _fixture, + '--format', + 'json', + '-c', + '1', + ]); + expect(code, 0); + File(model).writeAsStringSync(json); + }); + + test('check --model reports exactly what a source check reports', () async { + // The front-end seam as a separate process would use it: the model goes + // through a file, and the rules must not notice. + final (_, fromSource) = await _run([ + 'check', + _fixture, + '--format', + 'json', + '-c', + '1', + ]); + final (_, fromModel) = await _run([ + 'check', + '--model', + model, + '--format', + 'json', + ]); + expect(fromModel, fromSource); + }); + + test('check --model honours --rule and --fail-on', () async { + final (code, output) = await _run([ + 'check', + '--model', + model, + '--rule', + ruleEventWithoutHandler, + '--fail-on', + 'error', + '--format', + 'json', + ]); + expect(code, 1, reason: 'event-without-handler fires at error severity'); + expect(output, contains(ruleEventWithoutHandler)); + expect(output, isNot(contains(ruleDeadHandler))); + }); + + test('check --model refuses --write-baseline', () async { + final target = '${File(model).parent.path}/baseline.json'; + await expectLater( + _run(['check', '--model', model, '--write-baseline', target]), + throwsA( + isA().having( + (error) => error.toString(), + 'message', + contains('--model'), + ), + ), + ); + expect(File(target).existsSync(), isFalse); + }); + + test('check --model rejects a file that is not a model', () async { + final dir = File(model).parent.path; + File('$dir/array.json').writeAsStringSync('[1, 2]'); + // An object missing required fields must be a clear error, not a crash. + File('$dir/partial.json').writeAsStringSync('{"unions": [{"name": 1}]}'); + for (final bad in ['array.json', 'partial.json']) { + await expectLater( + _run(['check', '--model', '$dir/$bad']), + throwsA(isA()), + reason: bad, + ); + } + }); +} diff --git a/test/state_map_roundtrip_test.dart b/test/state_map_roundtrip_test.dart new file mode 100644 index 0000000..c72f3d5 --- /dev/null +++ b/test/state_map_roundtrip_test.dart @@ -0,0 +1,44 @@ +import 'dart:convert'; + +import 'package:pipe/pipe.dart'; +import 'package:test/test.dart'; + +/// The state-map seam: rules read only the model, so a model that went through +/// JSON - as one written by a separate front end would - must produce exactly +/// the same report as the in-process model. If a rule quietly depended on +/// something the JSON does not carry, this is where it shows. +void main() { + late ProjectAnalysis analysis; + + setUpAll(() async { + analysis = await analyzeProject( + await loadProject('test/fixtures/stuck_loading'), + concurrency: 1, + ); + }); + + tearDownAll(() async { + await analysis.dispose(); + }); + + StateFlow throughJson(StateFlow flow) => stateFlowFromJson( + jsonDecode(jsonEncode(flow.toJson())) as Map, + ); + + test('the model survives JSON unchanged', () { + final once = jsonEncode(analysis.flow.toJson()); + final twice = jsonEncode(throughJson(analysis.flow).toJson()); + expect(twice, once); + }); + + test('rules on the decoded model report exactly the same findings', () { + final direct = jsonEncode(runChecks(flow: analysis.flow).toJson()); + final decoded = jsonEncode( + runChecks(flow: throughJson(analysis.flow)).toJson(), + ); + expect(decoded, direct); + // Not vacuous: every rule fires somewhere on the fixture. + final rules = runChecks(flow: analysis.flow).findings.map((f) => f.rule); + expect(rules.toSet(), containsAll(allRules)); + }); +} -- 2.52.0 From 4b499c1e2161dcba7049a7f61756a9f3d36e6a1f Mon Sep 17 00:00:00 2001 From: trvon Date: Tue, 22 Sep 2026 19:16:53 -0600 Subject: [PATCH 13/14] feat(cpp): experimental C++ front end for scoped-enum state tool/cpp_front.py is a second front end written against the state model, not the rules: it reads a compile database with libclang (Homebrew LLVM) and writes the JSON that `pipe check --model` takes. Every rule, baseline and gate works on it unchanged. Mapping, per project `enum class` (identity from clang's resolved AST, so two enums both named QueryIntent are never confused): - a reference that produces a value (assignment, atomic store, argument, return, initialiser) is an emission; a cast from an int is an indirect one; - a `case`, `==` or `!=` is an observation; - a busy enumerator written to the method's OWN state, followed by a top-level try whose catch neither writes state nor rethrows, is a swallowed failure; - sources the build does not compile are recorded as unresolved, so findings naming them drop a confidence step. One rule-side change, optional and Dart-neutral: an EmissionSite may name its stateType. rendered-not-emitted attributed indirect emissions by owner (a bloc owns one state type), so a cast to one enum made "never emitted" unprovable for every other enum in the same C++ class. With stateType set, the suppression is scoped to that enum. Dart output is still byte identical on the fixture and all four dogfood apps. Run on ~/work/tools/yams (375 translation units from build/debug, 0 clang errors, about two minutes): 118 state enums, 82 findings. A grep-based hand check of 13 found 12 factually correct - e.g. the IPC connection FSM never enters ImplState::ReadingHeader, the downloader never reports ProgressStage::Verifying though three CLI commands render it, ExtractScopeType qualifiers are parsed but never acted on - and one false positive (a nested JSON-parse fallback taken for a stuck busy state), now excluded by requiring the method's own state and a top-level try. Bugs found on the way and fixed: ccache/compiler wrappers passed as inputs, relative -I paths resolved against the wrong directory (7 TUs silently lost third_party headers), a symlinked path putting every TU "outside" the project, and an empty compile database producing an empty "clean" model with exit 0 - all now fail loudly or are handled. test/cpp_front_test.dart runs the front end on test/fixtures/cpp_state with positive and negative controls for each rule; skipped without LLVM. 138 tests pass. --- README.md | 35 ++ lib/src/rules.dart | 8 +- lib/src/state_flow.dart | 8 + test/cpp_front_test.dart | 95 ++++ test/fixtures/cpp_state/src/engine.cpp | 104 +++++ tool/cpp_front.py | 604 +++++++++++++++++++++++++ 6 files changed, 853 insertions(+), 1 deletion(-) create mode 100644 test/cpp_front_test.dart create mode 100644 test/fixtures/cpp_state/src/engine.cpp create mode 100644 tool/cpp_front.py diff --git a/README.md b/README.md index a6401e2..5e7ba8e 100644 --- a/README.md +++ b/README.md @@ -411,6 +411,41 @@ rule, `--baseline` ratcheting and `--fail-on` gating without touching Dart. `check --model` refuses `--write-baseline`, because a model does not record whether its front end read every file. +### Experimental: C++ state enums + +`tool/cpp_front.py` is a second front end, written against the model rather than +the rules. It reads a compile database with libclang (Homebrew LLVM) and maps +each project `enum class` to a union: a reference that produces a value +(assignment, atomic `store`, argument, return, initialiser) is an emission; a +`case`, `==` or `!=` is an observation; a busy enumerator set on the method's +own state followed by a top-level `catch` that neither writes state nor +rethrows is a swallowed failure. Identity comes from clang's resolved AST, so +two enums both named `QueryIntent` are never confused. + +```bash +tool/cpp_front.py ~/work/tools/yams --build-dir ~/work/tools/yams/build/debug -o yams.json +pipe check --model yams.json +``` + +Read the rules as: `rendered-not-emitted` - an enumerator something branches on +that nothing ever produces (a dead branch, or a state an FSM can never enter); +`emitted-not-rendered` - a value produced that nothing ever branches on +(computed and ignored, or reaching output only through a name table). Sources +the build does not compile are recorded as unresolved, so findings naming them +drop a confidence step. `test/cpp_front_test.dart` holds the positive and +negative controls (`test/fixtures/cpp_state`); it is skipped without LLVM. + +On yams (375 translation units, 0 clang errors, about two minutes) the first +run produced 82 findings. A grep-based hand check of 13 found 12 factually +correct and 1 false positive (a nested JSON-parse fallback taken for a stuck +busy state), which the front end now excludes. Factual is not the same as a +bug: of the 12, `TransportState::Error` is covered by `!= Connected` checks and +`InboundSessionStage` is fuzzing instrumentation. The substantive ones include +the IPC connection FSM never entering `ImplState::ReadingHeader`, the downloader +never reporting `ProgressStage::Verifying` although three CLI commands render +it, and `ExtractScopeType` query qualifiers being parsed but never acted on. +n=13 from one repo bounds the false-positive rate; it does not measure it. + Built on `package:analyzer` with whole-project resolution, not a `custom_lint`/analyzer plugin: plugins have no project-wide index, `flutter analyze` does not report new-system plugin diagnostics, and a CLI can pin its own diff --git a/lib/src/rules.dart b/lib/src/rules.dart index 766e0e7..7792d26 100644 --- a/lib/src/rules.dart +++ b/lib/src/rules.dart @@ -523,8 +523,14 @@ FindingReport runChecks({ if (enabled.contains(ruleRenderedNotEmitted)) { for (final coverage in flow.coverages) { if (coverage.renderedNeverEmitted.isEmpty) continue; + // An emission that names its state type belongs to that union only; one + // that does not (a Dart `emit(_helper())`) is attributed by owner, since a + // bloc owns one state type. A C++ class holds several enums, and a cast + // to one of them must not make every other one unprovable. final ownerEmissions = flow.emissions.where( - (emission) => emission.owner == coverage.union.owner, + (emission) => emission.stateType == null + ? emission.owner == coverage.union.owner + : emission.stateType == coverage.union.name, ); final uncertain = ownerEmissions.any( (emission) => diff --git a/lib/src/state_flow.dart b/lib/src/state_flow.dart index 50600f4..bdcc1d8 100644 --- a/lib/src/state_flow.dart +++ b/lib/src/state_flow.dart @@ -104,6 +104,7 @@ class EmissionSite { required this.confidence, required this.variants, required this.snippet, + this.stateType, }); final String path; @@ -111,6 +112,11 @@ class EmissionSite { final String owner; final String member; + /// The state type this emission produces, when the front end knows it even + /// though the variant is unknown (a C++ `static_cast(n)`). Null means + /// "attribute by owner", which is right when an owner has one state type. + final String? stateType; + /// `emit`, `assign` or `notifyListeners`. final String kind; @@ -128,6 +134,7 @@ class EmissionSite { 'confidence': confidence, if (variants.isNotEmpty) 'variants': variants, 'snippet': snippet, + if (stateType != null) 'stateType': stateType, }; } @@ -2039,6 +2046,7 @@ StateFlow stateFlowFromJson(Map json) { confidence: e['confidence']! as String, variants: strings(e['variants']), snippet: e['snippet']! as String, + stateType: e['stateType'] as String?, ), ], renders: [ diff --git a/test/cpp_front_test.dart b/test/cpp_front_test.dart new file mode 100644 index 0000000..dfa1ba0 --- /dev/null +++ b/test/cpp_front_test.dart @@ -0,0 +1,95 @@ +import 'dart:convert'; +import 'dart:io'; + +import 'package:pipe/pipe.dart'; +import 'package:test/test.dart'; + +/// Contract for the experimental C++ front end (tool/cpp_front.py): it writes a +/// state model, and the unchanged rules must fire on each positive control in +/// test/fixtures/cpp_state and stay silent on each negative one. +/// +/// Skipped, not passed, when Homebrew LLVM (libclang + its Python bindings) or +/// python3 is unavailable. +void main() { + final llvm = Platform.environment['PIPE_LLVM'] ?? '/opt/homebrew/opt/llvm'; + final available = + File('$llvm/lib/libclang.dylib').existsSync() && + Process.runSync('python3', ['--version']).exitCode == 0; + + late FindingReport report; + + setUpAll(() async { + if (!available) return; + final root = Directory.systemTemp.createTempSync('pipe-cpp'); + Directory('${root.path}/src').createSync(); + File( + 'test/fixtures/cpp_state/src/engine.cpp', + ).copySync('${root.path}/src/engine.cpp'); + File('${root.path}/compile_commands.json').writeAsStringSync( + jsonEncode([ + { + 'directory': root.path, + 'file': 'src/engine.cpp', + 'arguments': ['c++', '-std=c++20', '-c', 'src/engine.cpp'], + }, + ]), + ); + final model = '${root.path}/model.json'; + final result = Process.runSync('python3', [ + 'tool/cpp_front.py', + root.path, + '--include', + 'src', + '-o', + model, + ]); + expect(result.exitCode, 0, reason: '${result.stderr}'); + expect('${result.stderr}', contains('0 clang error(s)')); + report = runChecks( + flow: stateFlowFromJson( + jsonDecode(File(model).readAsStringSync()) as Map, + ), + ); + }); + + Iterable hits(String rule) => + report.findings.where((finding) => finding.rule == rule); + + test('an enum produced but never examined is reported', () { + expect( + hits(ruleEmittedNotRendered).map((f) => f.data['union']), + contains('fx::Unread'), + ); + }, skip: available ? false : 'Homebrew LLVM / python3 not available'); + + test('an enumerator examined but never produced is a dead branch', () { + final ghost = hits( + ruleRenderedNotEmitted, + ).where((f) => f.data['variant'] == 'Ghost'); + expect(ghost, hasLength(1)); + expect(ghost.single.file, 'src/engine.cpp'); + expect(ghost.single.line, isNotNull); + }, skip: available ? false : 'Homebrew LLVM / python3 not available'); + + test('a cast to one enum does not mute "never emitted" for another', () { + // Casted::Y is produced only through static_cast, so it is unprovable + // and suppressed. Phase lives in the same class and must still be judged. + expect( + hits(ruleRenderedNotEmitted).map((f) => f.data['union']), + isNot(contains('fx::Casted')), + ); + expect(report.suppressed.where((s) => s.contains('fx::Phase')), isEmpty); + }, skip: available ? false : 'Homebrew LLVM / python3 not available'); + + test( + 'a swallowed failure on the method\'s own busy state fires, and only it', + () { + // startGuarded resolves the state, startNested's try is a nested local + // fallback, and Reporter::run sets a field of a local event object. + expect(hits(ruleLoadingNotReset).map((f) => f.data['member']).toList(), [ + 'startSwallowed', + ]); + }, + skip: available ? false : 'Homebrew LLVM / python3 not available', + ); +} diff --git a/test/fixtures/cpp_state/src/engine.cpp b/test/fixtures/cpp_state/src/engine.cpp new file mode 100644 index 0000000..8c5aa9c --- /dev/null +++ b/test/fixtures/cpp_state/src/engine.cpp @@ -0,0 +1,104 @@ +// Fixture for tool/cpp_front.py: each case is a positive or negative control +// for a rule run on the C++ state model (see test/cpp_front_test.dart). + +namespace fx { + +// Ghost is compared against but never produced: rendered-not-emitted. +enum class Phase { Idle, Starting, Running, Failed, Ghost }; + +// Produced, never compared or switched on: emitted-not-rendered. +enum class Unread { A, B }; + +// Y is compared against and produced only through a cast from an int, so the +// rule must suppress itself rather than call the branch dead. +enum class Casted { X, Y }; + +void mayThrow(); + +class Engine { + public: + // Negative: the catch resolves the busy state. + void startGuarded() { + phase_ = Phase::Starting; + try { + mayThrow(); + phase_ = Phase::Running; + } catch (...) { + phase_ = Phase::Failed; + } + } + + // Positive: busy state set, then a top-level try whose catch swallows the + // failure, so Starting can outlive it (loading-not-reset). + void startSwallowed() { + phase_ = Phase::Starting; + try { + mayThrow(); + phase_ = Phase::Running; + } catch (...) { + } + } + + // Negative: a nested try is a local fallback, not the method's failure path. + void startNested() { + phase_ = Phase::Starting; + if (ready_) { + try { + mayThrow(); + } catch (...) { + } + } + phase_ = Phase::Running; + } + + bool ghost() const { return phase_ == Phase::Ghost; } + + bool idle() const { + switch (phase_) { + case Phase::Idle: + return true; + case Phase::Starting: + case Phase::Running: + case Phase::Failed: + return false; + default: + return false; + } + } + + void setUnread() { + unread_ = Unread::A; + unread_ = Unread::B; + } + + void load(int n) { casted_ = static_cast(n); } + bool isY() const { return casted_ == Casted::Y; } + + private: + bool ready_ = false; + Phase phase_ = Phase::Idle; + Unread unread_ = Unread::A; + Casted casted_ = Casted::X; +}; + +// Negative: `ev.stage` is a field of a local event object handed to a callback, +// not state Reporter holds, so its try cannot leave Reporter stuck. +struct Event { + Phase stage; +}; +void notify(const Event&); + +class Reporter { + public: + void run() { + Event ev; + ev.stage = Phase::Starting; + notify(ev); + try { + mayThrow(); + } catch (...) { + } + } +}; + +} // namespace fx diff --git a/tool/cpp_front.py b/tool/cpp_front.py new file mode 100644 index 0000000..961c2c4 --- /dev/null +++ b/tool/cpp_front.py @@ -0,0 +1,604 @@ +#!/usr/bin/env python3 +"""Experimental C++ front end: writes a pipe state model from clang's AST. + +Usage: + tool/cpp_front.py [--build-dir DIR] [--include src,include] + [--limit N] [--jobs N] [-o model.json] + pipe check --model model.json + +pipe's rules read only the state model, so this front end never touches them. +It uses libclang's *resolved* AST: every enumerator reference is linked to its +EnumConstantDecl, so identity is exact rather than matched by name. + +What it maps (scoped enums only, `enum class`, declared in the project): + + union an `enum class`; variants are its enumerators + emission a reference that PRODUCES the value: assigned to a field or + variable, stored into an atomic, passed, returned, initialised + observation a reference that EXAMINES the value: a `case` label, `==`, `!=` + (these fill pipe's "rendered" set: something branches on it) + swallowed a member writes a busy enumerator (Starting, Loading, ...) to a + failure field, then has a try whose catch writes no state and does not + rethrow (loading-not-reset) + +Consequences worth knowing before reading the findings: +- `emitted-not-rendered` here means "produced, but nothing ever branches on or + compares it". A value that reaches the user through a lookup table or + reflection (magic_enum) is invisible to this, so treat those as leads. +- `rendered-not-emitted` means "compared against, but never produced anywhere": + a dead branch, unless the value comes from a cast (`static_cast(n)`), which + is recorded as an indirect emission so the rule suppresses itself. +- The rule messages still speak of "UI"; for C++ read "nothing observes it". + +Requires Homebrew LLVM's libclang and its Python bindings (found automatically). +""" + +from __future__ import annotations + +import argparse +import glob +import json +import os +import re +import shlex +import subprocess +import sys +from collections import Counter, defaultdict +from concurrent.futures import ProcessPoolExecutor, as_completed +from pathlib import Path + +LLVM = Path(os.environ.get("PIPE_LLVM", "/opt/homebrew/opt/llvm")) + + +def _load_cindex(): + for candidate in sorted(glob.glob(str(LLVM / "lib/python3.*/site-packages")), reverse=True): + if candidate not in sys.path: + sys.path.insert(0, candidate) + import clang.cindex as cindex # noqa: E402 + + # Pool workers are reused, and libclang may be configured only once. + if not cindex.Config.loaded: + cindex.Config.set_library_file(str(LLVM / "lib/libclang.dylib")) + return cindex + + +_SDK = None + +BUSY = re.compile( + r"^(Loading|Starting|StartingUp|Initializing|Initialising|Connecting|Stopping|" + r"ShuttingDown|Busy|Pending|InProgress|Processing|Saving|Updating|Creating|" + r"Syncing|Indexing|Warming|WarmingUp|Draining|Migrating|Recovering)$" +) + +# Kinds that only wrap an expression; the classifier looks through them. +_TRANSPARENT = { + "UNEXPOSED_EXPR", + "PAREN_EXPR", + "CSTYLE_CAST_EXPR", + "CXX_FUNCTIONAL_CAST_EXPR", + "CONDITIONAL_OPERATOR", + "INIT_LIST_EXPR", +} + + +def _resource_dir() -> str: + return subprocess.check_output([str(LLVM / "bin/clang"), "-print-resource-dir"], text=True).strip() + + +def _sdk_root() -> str | None: + """Apple's compiler finds the macOS SDK itself; libclang needs it spelled out, + or every standard header (, sqlite3.h) is missing.""" + try: + return subprocess.check_output(["xcrun", "--show-sdk-path"], text=True).strip() + except (OSError, subprocess.CalledProcessError): + return None + + +def _args_for(entry: dict, resource_dir: str) -> list[str]: + """Compiler flags only: drop wrappers (ccache), the compiler, the source, + and output/dependency options that make no sense for a syntax-only parse.""" + args = entry.get("arguments") or shlex.split(entry["command"]) + while args and (os.path.basename(args[0]) in ("ccache", "sccache", "distcc") + or not args[0].startswith("-") and args[0] != entry["file"] + and os.path.basename(args[0]) in ("c++", "g++", "clang++", "cc", "gcc", "clang")): + args = args[1:] + out: list[str] = [] + skip = False + directory = entry["directory"] + # Include paths are relative to the build directory, but libclang resolves + # them against this process's working directory: made absolute, or headers + # like third_party/... silently go missing. + path_flags = ("-I", "-isystem", "-iquote", "-idirafter", "-include") + absolute = [] + pending = None + for arg in args: + if pending is not None: + absolute.append(arg if os.path.isabs(arg) else os.path.normpath(os.path.join(directory, arg))) + pending = None + continue + if arg in path_flags: + absolute.append(arg) + pending = arg + continue + for flag in path_flags: + if arg.startswith(flag) and len(arg) > len(flag) and flag != "-include": + value = arg[len(flag):] + if not os.path.isabs(value): + arg = flag + os.path.normpath(os.path.join(directory, value)) + break + absolute.append(arg) + args = absolute + for arg in args: + if skip: + skip = False + continue + if arg in ("-o", "-MF", "-MT", "-MQ"): + skip = True + continue + if arg in ("-c", "-MD", "-MMD", "-fdiagnostics-color=always") or \ + arg.startswith(("-o", "-MF", "-MT", "-MQ")): + continue + if arg == entry["file"] or not arg.startswith("-") and arg.endswith((".cpp", ".cc", ".cxx")): + continue + out.append(arg) + out += ["-resource-dir", resource_dir, "-Wno-everything", "-fsyntax-only"] + sdk = _SDK + if sdk and "-isysroot" not in out: + out += ["-isysroot", sdk] + return out + + +def _qualified(cursor) -> str: + parts = [] + c = cursor + while c is not None and c.kind.name != "TRANSLATION_UNIT": + if c.spelling: + parts.append(c.spelling) + c = c.semantic_parent + return "::".join(reversed(parts)) + + +def _enclosing(stack, kinds): + for c in reversed(stack): + if c.kind.name in kinds: + return c + return None + + +def _operator(cursor) -> str | None: + try: + op = cursor.binary_operator + return op.name if op is not None else None + except AttributeError: + return None + + +def _top_level_try(stack) -> bool: + """True when the try sits directly in its function's body, not nested in a + loop, branch, block or another try.""" + for i in range(len(stack) - 1, -1, -1): + kind = stack[i].kind.name + if kind in ("CXX_METHOD", "FUNCTION_DECL", "CONSTRUCTOR", "LAMBDA_EXPR"): + # stack[i+1] is the body; the try must be its direct child. + return len(stack) - 1 == i + 1 and stack[i + 1].kind.name == "COMPOUND_STMT" + if kind == "CXX_TRY_STMT": + return False + return False + + +def _field_owner(expr, cindex): + """For `x.state_` / `state_` / `this->state_`: the class declaring the field.""" + for node in expr.walk_preorder(): + if node.kind.name == "MEMBER_REF_EXPR": + ref = node.referenced + if ref is not None and ref.kind.name == "FIELD_DECL": + return ref.semantic_parent, ref.spelling + return None, None + + +def analyse_tu(task): + """Parse one translation unit; return the facts located in project files.""" + file, directory, args, root, includes = task + cindex = _load_cindex() + index = cindex.Index.create() + try: + tu = index.parse( + os.path.normpath(os.path.join(directory, file)), + args=args, + options=cindex.TranslationUnit.PARSE_INCOMPLETE, + ) + except cindex.TranslationUnitLoadError as error: + return {"file": file, "error": str(error)} + + root = os.path.realpath(root) + prefixes = tuple(os.path.join(root, p) + os.sep for p in includes) + + def in_project(cursor) -> str | None: + loc = cursor.location + if loc.file is None: + return None + path = os.path.realpath(loc.file.name) + if not path.startswith(prefixes): + return None + return os.path.relpath(path, root) + + enums = {} + refs = [] + busy_tries = [] + + def classify(stack, ref): + """Return (role, owner, field, kind) for an enumerator reference.""" + child = ref + for parent in reversed(stack): + kind = parent.kind.name + if kind in _TRANSPARENT: + child = parent + continue + if kind == "CASE_STMT": + first = next(iter(parent.get_children()), None) + if first is not None and first.hash == child.hash: + return ("observe", None, None, "case") + return ("produce", None, None, "other") + if kind == "BINARY_OPERATOR": + op = _operator(parent) + children = list(parent.get_children()) + if op in ("EQ", "NE"): + return ("observe", None, None, "compare") + if op == "Assign" and len(children) == 2 and children[1].hash == child.hash: + owner, field = _field_owner(children[0], cindex) + return ("produce", owner, field, "assign") + return ("produce", None, None, "other") + if kind == "CALL_EXPR" and parent.spelling in ("store", "exchange"): + owner, field = _field_owner(parent, cindex) + return ("produce", owner, field, "assign") + if kind == "MEMBER_REF": + ref_decl = parent.referenced + if ref_decl is not None and ref_decl.kind.name == "FIELD_DECL": + return ("produce", ref_decl.semantic_parent, ref_decl.spelling, "initial") + if kind == "FIELD_DECL": + return ("produce", parent.semantic_parent, parent.spelling, "initial") + if kind in ("CALL_EXPR", "RETURN_STMT", "VAR_DECL", "COMPOUND_STMT", + "IF_STMT", "CXX_METHOD", "FUNCTION_DECL", "CONSTRUCTOR"): + return ("produce", None, None, "other") + child = parent + return ("produce", None, None, "other") + + def member_name(stack): + fn = _enclosing(stack, {"CXX_METHOD", "FUNCTION_DECL", "CONSTRUCTOR", + "DESTRUCTOR", "FUNCTION_TEMPLATE", "LAMBDA_EXPR"}) + if fn is None: + return "", None + if fn.kind.name == "LAMBDA_EXPR": + outer = _enclosing(stack[: stack.index(fn)], {"CXX_METHOD", "FUNCTION_DECL", "CONSTRUCTOR"}) + name = outer.spelling if outer is not None else "" + return f"{name}::", outer + return fn.spelling, fn + + def scan_try(stack, try_cursor, rel): + """A try inside a member that wrote a busy value earlier.""" + body = list(try_cursor.get_children()) + handlers = [c for c in body if c.kind.name == "CXX_CATCH_STMT"] + swallowed = [] + for handler in handlers: + writes_state = False + rethrows = False + for node in handler.walk_preorder(): + if node.kind.name == "CXX_THROW_EXPR": + rethrows = True + if node.kind.name == "DECL_REF_EXPR": + ref = node.referenced + if ref is not None and ref.kind.name == "ENUM_CONSTANT_DECL": + writes_state = True + if node.kind.name in ("CALL_EXPR",) and re.search( + r"(set|mark|fail|error|transition|reset|clear)", node.spelling or "", re.I + ): + writes_state = True + if not writes_state and not rethrows: + tokens = " ".join(t.spelling for t in handler.get_tokens()) + swallowed.append((handler.location.line, tokens[:117] + ("..." if len(tokens) > 117 else ""))) + return swallowed + + def visit(cursor, stack, busy_seen): + rel = in_project(cursor) + if rel is None and cursor.kind.name != "TRANSLATION_UNIT": + return + kind = cursor.kind.name + if kind == "ENUM_DECL" and cursor.is_scoped_enum() and cursor.is_definition(): + name = _qualified(cursor) + enums[name] = { + "name": name, + "declaredIn": rel, + "line": cursor.location.line, + "variants": [c.spelling for c in cursor.get_children() + if c.kind.name == "ENUM_CONSTANT_DECL"], + } + elif kind == "DECL_REF_EXPR": + ref = cursor.referenced + if ref is not None and ref.kind.name == "ENUM_CONSTANT_DECL": + parent_enum = ref.semantic_parent + if parent_enum is not None and parent_enum.is_scoped_enum(): + role, owner, field, how = classify(stack, cursor) + member, fn = member_name(stack) + klass = _enclosing(stack, {"CLASS_DECL", "STRUCT_DECL", "CLASS_TEMPLATE"}) + if fn is not None and fn.semantic_parent is not None and \ + fn.semantic_parent.kind.name in ("CLASS_DECL", "STRUCT_DECL", "CLASS_TEMPLATE"): + klass = fn.semantic_parent + record = { + "enum": _qualified(parent_enum), + "variant": ref.spelling, + "role": role, + "how": how, + "owner": _qualified(owner) if owner is not None else None, + "field": field, + "member": member, + "memberOwner": _qualified(klass) if klass is not None else None, + "path": rel, + "line": cursor.location.line, + "col": cursor.location.column, + } + refs.append(record) + # Only the method's OWN state counts: `ev.stage = Connecting` on a + # local event object handed to a callback is not state this + # class can leave stuck. + if role == "produce" and how == "assign" and BUSY.match(ref.spelling) \ + and owner is not None and klass is not None \ + and _qualified(owner) == _qualified(klass): + busy_seen.append(record) + elif kind == "CXX_STATIC_CAST_EXPR" or kind == "CSTYLE_CAST_EXPR": + target = cursor.type.get_canonical() + decl = target.get_declaration() + if decl is not None and decl.kind.name == "ENUM_DECL" and decl.is_scoped_enum(): + children = list(cursor.get_children()) + if children and children[-1].type.get_canonical().kind.name != "ENUM": + refs.append({ + "enum": _qualified(decl), "variant": None, "role": "cast", "how": "cast", + "owner": None, "field": None, "member": member_name(stack)[0], + "memberOwner": None, "path": rel, "line": cursor.location.line, + "col": cursor.location.column, + }) + elif kind == "CXX_TRY_STMT" and busy_seen and _top_level_try(stack): + # (a top-level try of the method: nested tries are local fallbacks, + # like a JSON parse guarded with `catch (...) { skip = false; }`) + member, fn = member_name(stack) + for busy in busy_seen: + if busy["member"] != member: + continue + for line, handler in scan_try(stack, cursor, rel): + busy_tries.append({ + "owner": busy["owner"], "member": member, "path": rel, + "line": line, "handler": handler, + }) + break + new_busy = busy_seen + if kind in ("CXX_METHOD", "FUNCTION_DECL", "CONSTRUCTOR", "LAMBDA_EXPR"): + new_busy = [] + stack.append(cursor) + for child in cursor.get_children(): + visit(child, stack, new_busy) + stack.pop() + + visit(tu.cursor, [], []) + errors = [f"{d.location.file}:{d.location.line}: {d.spelling}" + for d in tu.diagnostics if d.severity >= 3] + return {"file": file, "enums": list(enums.values()), "refs": refs, "tries": busy_tries, + "errors": len(errors), "errorSamples": errors[:3]} + + +def build_model(root: Path, results: list[dict], includes: list[str]) -> dict: + enums: dict[str, dict] = {} + seen_refs = set() + refs = [] + tries = {} + failed = [] + for result in results: + if "error" in result: + failed.append(result["file"]) + continue + for enum in result["enums"]: + enums.setdefault(enum["name"], enum) + for ref in result["refs"]: + key = (ref["path"], ref["line"], ref["col"], ref["enum"], ref["variant"]) + if key in seen_refs: + continue + seen_refs.add(key) + refs.append(ref) + for t in result["tries"]: + tries.setdefault((t["path"], t["line"]), t) + + by_enum = defaultdict(list) + for ref in refs: + by_enum[ref["enum"]].append(ref) + + unions, coverage, emissions, renders = [], [], [], [] + for name, enum in sorted(enums.items()): + if len(enum["variants"]) < 2: + continue + erefs = by_enum.get(name, []) + owners = Counter(r["owner"] for r in erefs if r["role"] == "produce" and r["owner"]) + # The class whose field holds this state; without one it is a plain value + # type (a mode flag, a return code), not state anyone could leave stale. + if not owners: + continue + owner = owners.most_common(1)[0][0] + variants = enum["variants"] + emitted, initial, observed = set(), set(), set() + observe_sites = 0 + emit_sites = 0 + for r in erefs: + if r["role"] == "observe": + observed.add(r["variant"]) + observe_sites += 1 + renders.append({ + "path": r["path"], "line": r["line"], "widget": r["how"], + "stateType": name, + "branches": [{"variant": r["variant"], "line": r["line"], "trivial": False}], + }) + elif r["role"] in ("produce", "cast"): + emit_sites += 1 + if r["variant"]: + emitted.add(r["variant"]) + if r["how"] == "initial": + initial.add(r["variant"]) + emissions.append({ + "path": r["path"], "line": r["line"], "owner": owner, + "member": r["member"], "kind": "assign" if r["how"] == "assign" else "emit", + "confidence": "exact" if r["role"] == "produce" else "indirect", + "variants": [r["variant"]] if r["variant"] else [], + "snippet": f"{r['field'] or ''} = {name.split('::')[-1]}::{r['variant']}" + if r["variant"] else f"static_cast<{name.split('::')[-1]}>(...)", + # A class holds several enums: name the union so a cast to one + # does not make "never emitted" unprovable for the others. + "stateType": name, + }) + union = { + "name": name, "declaredIn": enum["declaredIn"], "style": "enumUnion", + "owner": owner, "ownerKind": "Cubit", + "variants": [{"name": v, "declaredIn": enum["declaredIn"]} for v in variants], + } + unions.append(union) + coverage.append({ + "union": name, "owner": owner, "style": "enumUnion", + "declared": sorted(variants), "emitted": sorted(emitted), "initial": sorted(initial), + "rendered": sorted(observed), "renderedInBuilder": [], "trivialInBuilder": [], + "emissionSites": emit_sites, "renderSites": observe_sites, + }) + + owners = {u["owner"] for u in unions} + swallowed = [] + ordinals = Counter() + for (path, line), t in sorted(tries.items()): + if t["owner"] not in owners: + continue + ordinals[(t["owner"], t["member"])] += 1 + swallowed.append({ + "owner": t["owner"], "member": t["member"], "path": path, "line": line, + "ordinal": ordinals[(t["owner"], t["member"])], "handler": t["handler"], + }) + + # Files outside an owner's own declaration that mention it: pipe uses these + # to tell a dormant (unwired) union from a partly observed one. + references = defaultdict(set) + for r in refs: + if r["role"] == "observe" and r["enum"] in {u["name"] for u in unions}: + owner = next(u["owner"] for u in unions if u["name"] == r["enum"]) + references[owner].add(r["path"]) + + return { + "root": str(root), + "directories": includes, + "frontEnd": {"name": "pipe-cpp-front (experimental)", "failedTranslationUnits": failed}, + "unions": unions, + "eventUnions": [], + "emissions": emissions, + "renders": renders, + "handlers": [], + "dispatches": [], + "coverage": coverage, + "ownerReferences": {k: sorted(v) for k, v in references.items()}, + "flagStates": [], + "riverpod": {"providers": [], "sites": []}, + "constructedByFile": {}, + "swallowedFailures": swallowed, + "unsettledFlags": [], + } + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("root") + parser.add_argument("--build-dir", help="directory holding compile_commands.json") + parser.add_argument("--include", default="src,include", + help="project subdirectories whose code is analysed") + parser.add_argument("--limit", type=int) + parser.add_argument("--match", help="only translation units whose path contains this") + parser.add_argument("--jobs", type=int, default=os.cpu_count()) + parser.add_argument("-o", "--output", default="-") + args = parser.parse_args() + + root = Path(args.root).resolve() + db_dir = Path(args.build_dir) if args.build_dir else root + db_path = db_dir / "compile_commands.json" + if not db_path.is_file(): + print(f"cpp-front: no compile database at {db_path}; generate one " + f"(meson/CMake write it to the build dir) and pass --build-dir", + file=sys.stderr) + return 2 + database = json.loads(db_path.read_text()) + includes = [p for p in args.include.split(",") if p] + resource_dir = _resource_dir() + global _SDK + _SDK = _sdk_root() + + tasks = [] + seen = set() + for entry in database: + # Resolved on both sides: a symlinked path (macOS /var -> /private/var) + # otherwise lands "outside" the resolved root and the TU is skipped. + path = os.path.realpath(os.path.join(entry["directory"], entry["file"])) + if path in seen or not path.endswith((".cpp", ".cc", ".cxx")): + continue + rel = os.path.relpath(path, root) + if not any(rel.startswith(p + os.sep) for p in includes): + continue + if args.match and args.match not in rel: + continue + seen.add(path) + tasks.append((entry["file"], entry["directory"], _args_for(entry, resource_dir), + str(root), includes)) + if args.limit: + tasks = tasks[: args.limit] + if not tasks: + # An empty model reads as a clean project; refuse instead. + print(f"cpp-front: no translation unit under {', '.join(includes)} in " + f"{db_dir / 'compile_commands.json'}; nothing was analysed", file=sys.stderr) + return 2 + + results = [] + with ProcessPoolExecutor(max_workers=args.jobs) as pool: + futures = [pool.submit(analyse_tu, t) for t in tasks] + for i, future in enumerate(as_completed(futures), 1): + results.append(future.result()) + if i % 25 == 0 or i == len(tasks): + print(f"cpp-front: {i}/{len(tasks)} translation units", file=sys.stderr) + + model = build_model(root, results, includes) + + # Sources the build does not compile (optional backends, benchmarks) were + # never analysed, so evidence in them is missing. Recorded as unresolved with + # the names they mention, pipe lowers confidence for any finding naming them. + unanalysed = [] + names: set[str] = set() + for include in includes: + for path in sorted((root / include).rglob("*")): + if path.suffix not in (".cpp", ".cc", ".cxx") or os.path.realpath(path) in seen: + continue + unanalysed.append(os.path.relpath(path, root)) + names.update(re.findall(r"[A-Za-z_][A-Za-z0-9_]*", path.read_text(errors="replace"))) + failed_files = [os.path.normpath(f) for f in model["frontEnd"]["failedTranslationUnits"]] + if unanalysed or failed_files: + files = unanalysed + failed_files + model["unresolved"] = { + "files": files, + "analysed": len(tasks) - len(failed_files), + "first": f"{files[0]}: not compiled by this build, so not analysed", + "names": sorted(names), + } + failed = model["frontEnd"]["failedTranslationUnits"] + errors = sum(r.get("errors", 0) for r in results) + for r in results: + for sample in r.get("errorSamples", [])[:1]: + print(f"cpp-front: clang error in {r['file']}: {sample}", file=sys.stderr) + print(f"cpp-front: {len(model['unions'])} state enums, {len(model['emissions'])} " + f"emissions, {len(model['renders'])} observations, " + f"{len(model['swallowedFailures'])} swallowed failures; " + f"{len(failed)} TU(s) failed, {errors} clang error(s)", file=sys.stderr) + text = json.dumps(model, indent=2) + if args.output == "-": + print(text) + else: + Path(args.output).write_text(text) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) -- 2.52.0 From 1779da7e4dcd5f008d5858315b3806a92c06793b Mon Sep 17 00:00:00 2001 From: trvon Date: Wed, 23 Sep 2026 20:40:40 -0600 Subject: [PATCH 14/14] docs: trim the README and split usage, rules, design and development out The README had grown to 500 lines mixing an overview, a CLI reference, rule evidence, measurement method, architecture, build notes and progress notes. It is now a 72-line overview (what pipe is, coverage, quick start, the rule list, links), and the rest lives in: - docs/usage.md: commands, flags (checked against `pipe help`), SDK and resolution behaviour, CI baselines, verify, fix order, agent briefs, remediation policy. - docs/rules.md: each rule's evidence, the factual-vs-bug distinction, how precision and recall are measured, provider and other limits. - docs/design.md: motivation, architecture, the state-model seam, the experimental C++ front end. - docs/development.md: building and performance, tests, hooks. Dropped as progress notes: the negative-result and recall-bug narratives, before/after finding counts, review-round history, and stale figures (a "5/5 recall" that depends on which branch a repo has checked out, "84 tests", a duplicated recall.dart line, and the adjudicator described as fully independent). --- README.md | 522 ++++---------------------------------------- docs/design.md | 92 ++++++++ docs/development.md | 54 +++++ docs/rules.md | 82 +++++++ docs/usage.md | 119 ++++++++++ 5 files changed, 394 insertions(+), 475 deletions(-) create mode 100644 docs/design.md create mode 100644 docs/development.md create mode 100644 docs/rules.md create mode 100644 docs/usage.md diff --git a/README.md b/README.md index 5e7ba8e..765a95c 100644 --- a/README.md +++ b/README.md @@ -1,500 +1,72 @@ # pipe -Static state-flow analysis for Dart and Flutter apps. `pipe` resolves a project, -maps how state flows through its state management, and reports the breakages that -make an app behave worse than the code suggests — the ones that show up as a -spinner that never stops, an error the user never sees, or a tap that does -nothing. +Static state-flow analysis for Dart and Flutter apps. `pipe` maps how state +moves through an app's state management and reports the breakages that make it +behave worse than the code suggests: a spinner that never stops, an error the +user never sees, a tap that does nothing. -It is built to guide agents and reviewers on large codebases, not to score style. +It is built to guide agents and reviewers on large codebases, not to score +style. Every finding carries `file:line` evidence, a confidence, a severity and +a remediation. -## What it covers +## Coverage | State management | Support | | --- | --- | -| BLoC / Cubit (`on`, `emit`, sealed or unsealed state unions) | yes | -| State as a single class with a `bool loading`-style flag (`copyWith` style) | yes | -| Riverpod (`Provider`, `FutureProvider`, `StreamProvider`, `NotifierProvider`, `.family`) | yes | -| `provider` + `ChangeNotifier` | yes, with the limits below | -| GoRouter / navigation, widget layout, theming | no — out of scope | +| BLoC / Cubit (`on`, `emit`, sealed, abstract, enum or freezed unions) | yes | +| Single state class with a `bool loading`-style flag (`copyWith`) | yes | +| Riverpod (`FutureProvider`, `StreamProvider`, `NotifierProvider`, `.family`, ...) | yes | +| `provider` + `ChangeNotifier` | yes, with [limits](docs/rules.md#provider-and-changenotifier) | +| C++ `enum class` state (experimental, via libclang) | [see design](docs/design.md#c-front-end-experimental) | +| Navigation, layout, theming | no | -### `provider` / `ChangeNotifier` support and its limits - -Modelled: every provider-package declaration type is collected and reported by -type in the coverage note (`Provider`, `ChangeNotifierProvider`, -`ListenableProvider`, `ValueListenableProvider`, `FutureProvider`, -`StreamProvider`, `StateProvider`, `ProxyProvider` and its variants, `.value` -constructors), along with `Consumer`, `Selector`, `ChangeNotifierProvider`, -`ListenableBuilder` and `AnimatedBuilder` as consumption sites. Declaration -detection uses the type name plus `create:`/`update:`/`value:` named arguments to -tell the provider package apart from Riverpod's positional `(ref)` callback — it -is not import-verified, so an unrelated class named `Provider` taking a `create:` -argument would be miscounted in that note. - -Rules: `notifier-never-notifies`, `notifier-inert-consumer`, -`provider-read-in-build`, `notifier-field-unread`, `provider-created-in-build`. - -Still not implemented, and not claimed: any analysis of `ProxyProvider` -`update` functions returning unchanged values, and rebuild-frequency reasoning -(which is why `provider-created-in-build` is info rather than a warning — the -pattern is harmless on a widget that never rebuilds and a real leak on one that -rebuilds often, and static analysis cannot tell them apart). - -The `provider-read-in-build` rule does not descend into closures, because a read -inside a callback or a `Selector` builder is not made while build runs. That is a -deliberate precision choice: it costs recall on reads inside a -`Builder(builder: ...)`, which is noted rather than hidden. - -Not modelled today: `bloc_concurrency` transformers, freezed unions whose -generated file is absent, and `ref.read` call sites (dispatch, not render). - -## Usage +## Quick start ```bash dart pub get +tool/build_release.sh # prints the binary's path under build/ +# below, `pipe` is that binary on your PATH (or `dart run bin/pipe.dart`) -# Is the project even analyzable? (resolution health, timing, diagnostics) -pipe index - -# The resolved project model (declarations) as JSON -pipe graph - -# The state-flow graph: unions, emissions, render sites, coverage, Riverpod -pipe state - -# Ranked breakages with evidence and remediation -pipe check - -# A prompt-sized brief for an agent, optionally persisted into the yams corpus -pipe guide --budget 6000 --yams - -# How many labelled known bugs does pipe actually detect? -pipe recall --root --strict - -# After a fix: which accepted findings are gone? -pipe verify --baseline .pipe-baseline.json --expect-resolved - -# What to fix first, per file (ranked, with the scoring explained) -pipe suggest --format agent --budget 3000 +pipe index # can the project be analysed? +pipe check # ranked findings +pipe suggest # what to fix first, per file +pipe guide --format agent # a brief to paste into an agent prompt ``` -Common flags: `--format text|json` (`agent|md|json` for `guide`), `-c N` -(concurrency, default 4), `--sdk ` (override SDK discovery), `--dir` -(repeatable), `--include-generated`, `--limit N`, `--rule ` (repeatable), -`--top N`. - -Command-specific flags: `index --fail-on-error` exits 1 when the resolved project -has analyzer errors (the default reports the count and exits 0); -`suggest --budget N --max-actions N --top N` bound the agent plan and the text -report; `guide --budget N --max-findings N` bound the agent brief, and `--yams` -with `--yams-name ` persists that brief to the yams corpus; -`recall --manifest --root ` locate the labelled-bug manifest and the -directory holding the repos it names, and `recall --strict` exits non-zero when a -case it expects to still find is missed. Every numeric flag rejects a value that -is not a positive integer rather than falling back to its default. - -`pipe` finds the right SDK itself: it reads `flutterRoot` from the target's -`.dart_tool/package_config.json`, then `FLUTTER_ROOT`, then `flutter` on `PATH`. -Flutter projects need Flutter's bundled Dart SDK because `dart:ui` is not part of -the standalone SDK. If the project has never been resolved, `pipe` says so and -tells you to run `pub get` rather than silently degrading every `package:` import -to `dynamic`. - -A project that resolves only partly is reported, not silently analysed: an -unresolved `Foo()` is a method call rather than a construction, so evidence in -that file is invisible. `check` warns on stderr, notes how many files have -unresolved references, and lowers by one step the confidence of every finding -whose names such a file mentions (`data.confidenceLowered`). An SDK that exists -but cannot be read (for example a Flutter SDK under a privacy-protected macOS -folder, which makes `package:flutter` unresolvable) is refused up front with a -`--sdk` remedy. - -## CI and ratcheting - -An existing codebase will have findings on day one. Accept them once, then fail -only on what is new: +In CI, accept today's findings once and fail only on new ones: ```bash -pipe check . --write-baseline .pipe-baseline.json # accept current state +pipe check . --write-baseline .pipe-baseline.json pipe check . --baseline .pipe-baseline.json --fail-on warning +pipe verify . --baseline .pipe-baseline.json --expect-resolved # after a fix ``` -`--write-baseline` refuses a partial scan (`--rule`, `--limit`, or files that -could not be analysed): a baseline written from a subset omits every finding -outside it, and the next full run would report them all as new. `verify --rule` -likewise compares only that rule's accepted findings, instead of calling every -other rule's findings resolved. - -`--fail-on none|info|warning|error` (default `none`) fails the build when a -**new** finding at or above that severity is present. Baselined findings never -trip the gate, so a ratcheted repo stays green while a regression fails. -Because today's signal is mostly warnings, `--fail-on warning` is the useful -setting; `--fail-on error` only catches the strongest rules (`--fail-on-finding` -is an alias for it). - -`pipe verify` closes the other half of the loop — proving a fix rather than only -reporting a problem. It compares a fresh analysis against the baseline and prints -what is `resolved`, what `still` remains, and what is `new`, exiting non-zero with -`--expect-resolved` while anything accepted is still reported: - -```text -$ pipe verify . --baseline .pipe-baseline.json -baseline 12 accepted findings -resolved 1 no longer reported — these fixes are confirmed -still 11 still reported -new 0 not in the baseline -RESOLVED - + lib/flag_cubit.dart:25 loadUnguarded [loading-flag-not-reset] -``` - -Baselines are self-describing (each entry carries the rule plus a readable label), -so a resolved finding is named rather than shown as an opaque hash. - -A baseline stores a **fingerprint** per finding (`rule + file + semantic -identity` such as union/variant/provider/owner/member), so moving code down a -file does not resurrect accepted findings. `--fail-on-finding` only counts new -error-severity findings, and severity counts in reports are computed over new -findings, so a ratcheted baseline does not keep a build red. - -Baselines and reports are JSON, so they diff and review like any other file. - ## Rules -| Rule | What it means | Verified precision (with n) | -| --- | --- | --- | -| `emitted-not-rendered` | A state the bloc emits that no UI code matches, so the user never sees it. | **79/79 (100%)** factual on a CLEAN nuwayze main via a separate regex adjudicator with passing controls (`tool/adjudicate.py`; see its independence limit below). Reported once per **root cause**, not once per variant: one finding per unobserved union (dormant feature) and one per marker class when the union is observed. When the UI reads the union's payload through **base-class fields** the finding drops to info, because the data does reach the user. On clean nuwayze main: 30 findings covering 79 variant-level claims. | -| `rendered-not-emitted` | A UI branch no state can reach: dead UI. | n=0 on nuwayze (unions whose bloc emits through helper methods are suppressed with a reason). Fires on a fixture with a passing negative control. | -| `loading-not-reset` | An in-flight state whose failure path never resolves, so the spinner sticks. | n=0 on both dogfood repos, both verified to guard correctly; proven able to fire by fixture. | -| `loading-flag-not-reset` | The same consequence for `copyWith` + `bool loading` state, which has no variant to match on. | **7/7 (100%)** on dex after hand adjudication; 4 characterised false positives were eliminated by the current gate. n=0 on nuwayze, verified correct. | -| `event-without-handler` | A dispatched event with no `on`, so the interaction does nothing. | n=0 across the corpus; verified non-vacuous (168 dispatch sites on nuwayze all map to a registered handler). | -| `dead-handler` | An `on` nothing ever dispatches or constructs. | **77/77 (100%)** on nuwayze, same adjudicator with passing controls. | -| `riverpod-async-gap` | An async provider read in the UI whose error and/or loading state is never rendered. | n=8 on word_alive, n=2 on tick, hand-adjudicated: **warning/high** only for data-carrying providers; `bool` providers read as optimistic guards are downgraded to info/medium after a confirmed false positive. | -| `notifier-never-notifies` | A `ChangeNotifier` method that changes observable state without ever notifying listeners. | Fixture contract only: **zero true positives** on the corpus (nuwayze 30 observable mutators, word_alive 36 — all of which notify). Gated on observable fields with lifecycle hooks excluded, after the first run was 9/9 false positives. | -| `notifier-inert-consumer` | A notifier consumed by a `Consumer`/`Selector` with no notifying path at all, so those consumers can never rebuild. | Fixture contract only; no real-code true positives yet. | -| `provider-read-in-build` | A `Listenable` read with `context.read` / `Provider.of(listen: false)` **directly in a build body**, so the widget never subscribes and can render stale state. | 1 finding on nuwayze from **447** raw `context.read` calls; 0 on word_alive/dex. Hand-verified: the first 4 findings were all reads inside **closures** (a `BlocListener` callback, a `Selector` builder), so a closure guard was added and only the direct-in-build case survives. | -| `notifier-field-unread` | A public getter on a consumed `ChangeNotifier` that nothing reads, so the state it exposes cannot affect what the user sees. | 3 on nuwayze (e.g. `UserProvider.isInitializing`, `AppLocaleProvider.preferredLocale`), 0 on word_alive/dex. Reported once per notifier, and only for notifiers the UI actually consumes. **medium** confidence: a name match elsewhere can mask a genuinely unread member. | -| `provider-created-in-build` | A provider constructed inside `build` without `.value`, so `create` reruns on every rebuild and state is lost. | 1 finding on nuwayze — `AppProviders.build` constructs **45** providers — from 45 raw sites, because it is reported **once per build method**. Deliberately **info**, not warning: the claim is factual, but a root `StatelessWidget` rarely rebuilds, so impact depends on rebuild frequency. | - -### How precision was measured - -`tool/adjudicate.py ` re-derives the two highest-volume claims with its -own regex evidence instead of pipe's, and **prints controls first**. It is only -partly independent: it takes the list of unions, variants, events and owners to -check from pipe's `state` output, so a variant pipe fails to enumerate is never -checked. It exits non-zero when its controls fail or cannot discriminate, so a -CI step cannot mistake an uncertified run for a pass. It must find evidence -for every variant pipe calls *rendered* and every event pipe calls *dispatched*; -on clean nuwayze main that is 131/131 rendered (type tests, `case`/switch- -expression patterns, or base-class payload reads) and 143/162 dispatched, the -other 19 being shared event names whose construction sites a text-level checker -cannot honestly attribute — reported as *unattributable*, not as a pass. - -The control is deliberately three-way — **found / unattributable / missing** — and -only *missing* fails it, because an oracle that cannot discriminate must say so -rather than report a clean bill of health. A corpus with no control material at -all exits with **NO SIGNAL** and calls precision **undefined, not perfect**. If a control is -below 100% the numbers after it are void — which is exactly what happened on the -first attempt: an earlier version of the checker matched the `on` -registration as if it were a dispatch and reported 9% precision. The tool also -classifies comment-stripped lines, because a commented-out dispatch -(`cartview.dart:512`) is not a dispatch. - -Re-run the adjudicator on a **clean tree**: on a repo with uncommitted edits its -controls correctly fail and it refuses to certify, which is the behaviour you -want from an oracle. - -The adjudicator deliberately judges the **factual claim only** ("never -type-tested in UI" / "never constructed elsewhere"). It does not judge UX -severity, and it runs on one repo, so it bounds false-positive risk rather than -eliminating it. - -### A factually correct finding is not always a bug - -`emitted-not-rendered` at high confidence means "no UI code observes this state -type". That is verifiable and true — but it can be *deliberate*. On nuwayze the -two such blocs turned out to be dormant scaffolding the repo had already -documented as unreachable: - -- `AdminSettingsBloc` never had a UI consumer (the shipped admin Settings tab - uses `ArtistSettingsBloc` and reads the repository directly), and the project's - own action contract classifies `UpdateFeatureFlag` / `SaveAdminSettings` as - `"not-reachable"`. -- `CommentsBloc` was deliberately retired when a `StreamBuilder`-driven comments - UI replaced it; the last consumer was deleted in a cleanup commit. - -So pipe now reports **one finding per unobserved union** instead of one per -variant (that change took nuwayze from 159 findings to 146, collapsing 15 variant -findings into 2 decisions), and the remediation says to check the repository's -docs and action contracts before deleting anything. Treat those as decisions to -confirm, not regressions to fix. - -**A negative result worth recording.** The obvious next tier looked promising: a -never-matched variant that declares *no* fields beyond its base cannot hide any -data of its own, so it should be harmless. Measured on nuwayze, **all 67** of the -medium-tier findings qualified — every one was a marker state carrying no payload -of its own. A signal that is true of 100% of the population cannot separate -signal from noise, as a *discriminator*. It was still the right question though: -an audit of the same repo found the real cause, which is that the UI reads the -union's payload off the **base class** (`state.currentBooking`, -`state.isRefreshing`), so the data reaches the user without the variant ever being -matched. `uiReadsBaseFields` now records that per union and drops those findings -to info with an explanation, which removed 5 false-positive warnings on nuwayze. - -**A recall bug the same audit surfaced.** The switch -`case ChatListLoaded value => ...` binds a type on a *variable* pattern, not the -object pattern `ChatListLoaded()` this matcher looked for, so an exhaustive switch -was invisible and two variants were wrongly reported as unrendered. Both the -mention collector and the branch collector now handle `DeclaredVariablePattern`. -The fixture has a regression guard (`patternView`) so this cannot come back. - -This is also why the precision numbers above are stated as *factual* precision: -they say how often the claim holds, not how often the issue is worth acting on. - -Every finding carries `file:line`, a `path:line: snippet` evidence string, a -confidence, a UX severity, and a remediation. If a rule cannot be validated it is -demoted or deleted, not defended. - -### Recall, measured not assumed - -Precision alone is not evidence. Two mechanisms pin recall: - -1. **Rule contracts in CI.** `test/rule_contract_test.dart` requires every rule to - have a positive control (it must fire) *and* a negative control (an adjacent - case it must stay silent on), from fixture code written for exactly that pair — - for example `loading-flag-not-reset` must fire on `loadUnguarded` and must not - fire on `loadGuarded` or `refreshQuietly`; `event-without-handler` must fire on - `UnhandledTap` and must not fire on `NeverDispatched`, which is handled. A rule - added without both controls fails the build. -2. **Labelled real bugs.** `pipe recall` evaluates `recall/manifest.json` — real - bugs previously found in real codebases — and reports `ok/total` per rule, - failing (`--strict`) on any miss. Repos absent from the machine are reported as - **skipped**, never as passes. - Each case carries a `status`: - - `open` — the bug still exists, so pipe must **find** it. - - `fixed` — the bug was repaired, so pipe must **not** find it again. A `fixed` - case that reappears is reported as a **REGRESSION**, which turns every repair - into a permanent guard against the real code rather than a deleted - expectation. - -Current result: **5/5 accounted for** (2 open bugs found, 3 fixed bugs still -quiet), 0 regressions — measured against the **working tree**, so a repo checked -out on an unmerged fix branch reports its cases as missed. The two nuwayze cases -are wired up on branches (`wire/nuwayze-*`) that are not merged yet, so they still -read as `open` here; they flip to `fixed` when those land. - -Recall is still measured against a small labelled set. It bounds the risk of -false negatives; it does not prove they are absent. - -### What pipe deliberately does not reimplement - -`unawaited_futures`, `use_build_context_synchronously`, `cancel_subscriptions`, -`close_sinks`, BLoC naming/API hygiene (`bloc_lint`), declaration-level dead code -(`undead`, `dallow`). Those are per-file checks the ecosystem already does well. -pipe targets whole-program state reachability that per-file linters structurally -cannot express. - -## Default remediation policy - -**Wire it up.** An unreachable state, handler or control is usually an *unfinished -feature* rather than dead weight, so pipe's default guidance is to make it -reachable: render the missing states, add the missing branch, dispatch the event -from a control. Removal is the exception and is worded as needing explicit -approval rather than offered as an equal choice. - -`--policy wire-up` (default) / `--policy remove` flips the emphasis for teams that -would rather delete. It changes the wording of every remediation that presents a -choice, the `decision:` line in the agent brief, and the heading of the -dead-handler section (`Unreachable handlers - wire up the control` versus -`Cleanly deletable`). - -This is a policy, not a finding: pipe still reports the same facts either way. - -## Fix order - -`pipe suggest` turns a findings list into a work plan: findings are collapsed to -root causes (one action per union / provider / handler), each cause is scored, and -the plan is presented both globally and **per file**. Only new findings are -planned, so a ratcheted baseline yields an empty plan. - -```text -1. [warning/high] emitted-not-rendered — CommentsState (CommentsBloc) - why: 125 = warning 50 x high (x1.0), one fix clears 6 findings (+50), user-visible failure (+25) - fix: Either render CommentsLoading in the consuming widget (or delete it and its emits) ... - where: lib/blocs/comment/comment_bloc.dart:143, ... +2 more - clears 6 finding(s) (CommentSubmitError, CommentSubmitSuccess, CommentSubmitting, - CommentsError, CommentsLoaded, CommentsLoading); verify with `pipe check --rule emitted-not-rendered` -``` - -The score is deliberately simple and always printed, so the order can be argued -with rather than trusted blindly: - -| term | effect | +| Rule | Meaning | | --- | --- | -| severity | `error` 100, `warning` 50, `info` 10 — dominates the **base** score | -| confidence | high ×1.0, medium ×0.7, low ×0.4 — a low-confidence finding may not be real | -| breadth | +10 per extra finding the one fix clears (capped at +50) | -| user-visible failure | +25 when the rule flagged the failure as UX-critical | -| certainty | +15 for rules whose fix needs no product judgement (dead handler, event without handler, loading not reset) | +| `emitted-not-rendered` | A state is emitted but no UI code matches it, so the user never sees it. | +| `rendered-not-emitted` | A UI branch no emitted state can reach: dead UI. | +| `loading-not-reset` | An in-progress state whose failure path never resolves, so the spinner sticks. | +| `loading-flag-not-reset` | The same, for a `bool loading` flag left set after an error. | +| `event-without-handler` | A dispatched event with no `on`, so the interaction does nothing. | +| `dead-handler` | An `on` that nothing dispatches or constructs. | +| `riverpod-async-gap` | An async provider read in the UI whose error or loading state is never handled. | +| `notifier-never-notifies` | A `ChangeNotifier` method changes observable state without notifying. | +| `notifier-inert-consumer` | A consumed notifier that never notifies, so its consumers never rebuild. | +| `provider-read-in-build` | A `Listenable` read without subscribing inside `build`, so it can render stale. | +| `notifier-field-unread` | A public member of a consumed notifier that nothing reads. | +| `provider-created-in-build` | A provider constructed in `build`, so its state is recreated on rebuild. | -Because the bonuses are additive, a wide and certain `info` group (10 base + 15 -certainty + 50 breadth = 75) can outrank a narrow `warning` (35): severity -dominates the base, not the final total. That is deliberate — one fix that clears -nine findings is usually the better first move — but it is stated rather than -implied, and the `why:` line prints the arithmetic for every action. +Measured precision, recall and the limits of each rule are in +[docs/rules.md](docs/rules.md). -## Agent guidance +## Documentation -`pipe guide` emits a brief designed to be pasted into an agent prompt: findings -grouped by root cause, dense `file:line` anchors (`file:18,29,32`), a rule legend -carrying the standard fix, a `decision:` line that resolves "fix vs delete" where -the project makes that decidable, and a `verify:` command. Truncation is always -explicit — never silently dropped. - -It was verified by giving independent agents *only* the brief text and no tool -access. Five rounds of that found real defects (a legend that did not match the -entries shown, hidden edit targets, a missing consumer file), all fixed. Two -limits are inherent and stated in the brief rather than hidden: - -1. Deciding whether to wire up or delete an unwired bloc needs **product intent**. - pipe reports the evidence a human needs (registered in DI/providers, 0 render - sites) but cannot supply the intent. -2. Adding a missing UI branch requires reading the consumer widget, so pipe names - the file and line and does not inline widget source. - -Deletion-class findings need neither, which is why they are the verified -actionable class. - -## Architecture - -```text -bin/pipe.dart entrypoint -lib/src/project.dart package graph validation, source discovery -lib/src/sdk.dart SDK discovery (Flutter-aware) -lib/src/analysis.dart AnalysisContextCollection lifecycle, per-file resolution -lib/src/index.dart resolution health, timing, diagnostics -lib/src/model.dart declaration model -lib/src/state_flow.dart the state-flow IR (unions, emissions, render sites, - events, flags, Riverpod) -lib/src/facts.dart language-neutral facts the rules judge (JSON round-trip) -lib/src/dart_facts.dart Dart front end: loading/construction recognisers -lib/src/dart_provider_facts.dart Dart front end: ChangeNotifier/provider recognisers -lib/src/rules.dart breakage detectors, findings, baselines (model only) -lib/src/recall.dart labelled-bug recall measurement -lib/src/verify.dart baseline verification (resolved / still / new) -lib/src/suggest.dart ranked fix order per file -lib/src/recall.dart labelled-bug recall measurement -lib/src/pipeline.dart shared analyze -> graph -> rules pipeline -lib/src/guide.dart agent brief / markdown report / yams emission -tool/adjudicate.py independent precision adjudicator -tool/build_release.sh native AOT build -``` - -### The state model is the seam - -Rules never read a syntax tree. The Dart front end (`state_flow.dart`, -`dart_facts.dart`, `dart_provider_facts.dart`) turns resolved code into a state -model: unions and their variants, where each is emitted and rendered, event -dispatch and handling, busy paths whose failure is swallowed, observable objects -and whether their mutations are announced. `runChecks` takes only that model. - -The model is plain JSON, and a model that went through JSON reports exactly -what the in-process one does - checked byte for byte on the fixture -(`test/state_map_roundtrip_test.dart`) and on four real apps: - -```bash -pipe state . --format json > model.json # any front end can write this -pipe check --model model.json # every rule, baseline and gate -``` - -So a front end for another language only has to write that JSON; it gets every -rule, `--baseline` ratcheting and `--fail-on` gating without touching Dart. -`check --model` refuses `--write-baseline`, because a model does not record -whether its front end read every file. - -### Experimental: C++ state enums - -`tool/cpp_front.py` is a second front end, written against the model rather than -the rules. It reads a compile database with libclang (Homebrew LLVM) and maps -each project `enum class` to a union: a reference that produces a value -(assignment, atomic `store`, argument, return, initialiser) is an emission; a -`case`, `==` or `!=` is an observation; a busy enumerator set on the method's -own state followed by a top-level `catch` that neither writes state nor -rethrows is a swallowed failure. Identity comes from clang's resolved AST, so -two enums both named `QueryIntent` are never confused. - -```bash -tool/cpp_front.py ~/work/tools/yams --build-dir ~/work/tools/yams/build/debug -o yams.json -pipe check --model yams.json -``` - -Read the rules as: `rendered-not-emitted` - an enumerator something branches on -that nothing ever produces (a dead branch, or a state an FSM can never enter); -`emitted-not-rendered` - a value produced that nothing ever branches on -(computed and ignored, or reaching output only through a name table). Sources -the build does not compile are recorded as unresolved, so findings naming them -drop a confidence step. `test/cpp_front_test.dart` holds the positive and -negative controls (`test/fixtures/cpp_state`); it is skipped without LLVM. - -On yams (375 translation units, 0 clang errors, about two minutes) the first -run produced 82 findings. A grep-based hand check of 13 found 12 factually -correct and 1 false positive (a nested JSON-parse fallback taken for a stuck -busy state), which the front end now excludes. Factual is not the same as a -bug: of the 12, `TransportState::Error` is covered by `!= Connected` checks and -`InboundSessionStage` is fuzzing instrumentation. The substantive ones include -the IPC connection FSM never entering `ImplState::ReadingHeader`, the downloader -never reporting `ProgressStage::Verifying` although three CLI commands render -it, and `ExtractScopeType` query qualifiers being parsed but never acted on. -n=13 from one repo bounds the false-positive rate; it does not measure it. - -Built on `package:analyzer` with whole-project resolution, not a -`custom_lint`/analyzer plugin: plugins have no project-wide index, `flutter -analyze` does not report new-system plugin diagnostics, and a CLI can pin its own -analyzer instead of chasing the SDK's pinned range. - -Resolution mirrors Flutter's own tooling (dispose and rebuild the context -collection when `package_config.json` changes, `changeFile` + -`applyPendingFileChanges`, a fresh session after -`InconsistentAnalysisException`). - -## Performance - -Run pipe as a native binary, not through `dart run`. Dart JIT startup is paid on -every invocation and dominates short runs: - -```bash -tool/build_release.sh # dart build cli, falling back to dart compile exe -``` - -Measured on the same machine (macOS arm64): - -| scenario | `dart run` (JIT) | AOT binary | -| --- | --- | --- | -| 2-file project | 6.4s | **0.2s** | -| 814-file Flutter app, end to end | ~23-29s | **10.4s** | -| per-file p50 on that app | ~50-75ms | **16ms** | - -The binary is ~18MB and self-contained. - -### Where the remaining time goes - -The first file of a large project costs ~3.1s (analyzer warmup: SDK and package -summaries), and the rest is aggregate per-file resolution. SDK *discovery* is not -the floor - it is a couple of file checks against `package_config.json` - so the -obvious "cache the SDK path" optimisation buys nothing. Cutting the warmup would -need a persistent analysis cache, which is a much larger and riskier change. - -Resolution health on the 814-file app: 812/814 resolved, 0 errors, 0 unresolved -URIs. Two runs are byte-identical after dropping timing, and `pipe check` output -is byte-identical across runs. - -## Tests - -```bash -dart test -``` - -84 tests across resolution/IR extraction, rule contracts (positive *and* -negative controls per rule), Riverpod and provider modelling, agent-brief -rendering, fix-order scoring, baseline round-tripping (including legacy -fingerprints-only baselines), verification, and recall evaluation, against a -dependency-free fixture project. +- [Usage](docs/usage.md): commands, flags, SDK discovery, CI ratcheting, + verification, fix order, agent briefs. +- [Rules](docs/rules.md): what each rule claims, the evidence behind it, and how + precision and recall are measured. +- [Design](docs/design.md): why pipe exists, how it is built, the state-model + seam, and the experimental C++ front end. +- [Development](docs/development.md): building, performance, tests and hooks. diff --git a/docs/design.md b/docs/design.md new file mode 100644 index 0000000..562caa3 --- /dev/null +++ b/docs/design.md @@ -0,0 +1,92 @@ +# Design + +## Motivation + +The state bugs users feel (a spinner that never stops, an error never shown, a +tap that does nothing) are whole-program facts: a state emitted in one file and +never matched in any other, an event dispatched with no handler anywhere. A +per-file linter cannot express them, so pipe resolves the whole project and +reasons over all of it at once. + +pipe is a CLI on `package:analyzer`, not a `custom_lint` or analyzer plugin: +plugins have no project-wide index, `flutter analyze` does not report +new-system plugin diagnostics, and a CLI can pin its own analyzer version. + +## Architecture + +```text +bin/pipe.dart entrypoint +lib/src/cli.dart commands and flags +lib/src/project.dart package validation, source discovery +lib/src/sdk.dart SDK discovery (Flutter-aware) +lib/src/analysis.dart analysis contexts, per-file resolution +lib/src/index.dart resolution health, timing, diagnostics +lib/src/model.dart declaration model (`graph`) +lib/src/state_flow.dart Dart front end: the state model and its JSON +lib/src/dart_facts.dart Dart front end: loading and construction facts +lib/src/dart_provider_facts.dart Dart front end: ChangeNotifier / provider facts +lib/src/facts.dart language-neutral facts (JSON round-trip) +lib/src/rules.dart rules, findings, baselines (model only) +lib/src/provider_rules.dart ChangeNotifier / provider rules (model only) +lib/src/pipeline.dart analyse -> model -> rules +lib/src/verify.dart baseline verification +lib/src/suggest.dart fix order +lib/src/guide.dart agent brief, markdown report, yams emission +lib/src/recall.dart labelled-bug recall +tool/adjudicate.py precision adjudicator +tool/cpp_front.py experimental C++ front end +tool/build_release.sh native AOT build +``` + +Resolution mirrors Flutter's own tooling: the context collection is rebuilt +when `package_config.json` changes, file changes go through `changeFile` and +`applyPendingFileChanges`, and an `InconsistentAnalysisException` gets a fresh +session. + +## The state model + +Rules never read a syntax tree. A front end turns code into a state model: +unions and variants, where each is emitted and matched, events dispatched and +handled, busy paths whose failure is swallowed, and observable objects with +whether their mutations are announced. `runChecks` takes only that model. + +The model is plain JSON, and a model read back from JSON reports exactly what +the in-process one does (`test/state_map_roundtrip_test.dart`): + +```bash +pipe state . --format json > model.json # or any front end that writes it +pipe check --model model.json # every rule, baseline and gate +``` + +A front end for another language only has to write that JSON. + +## C++ front end (experimental) + +`tool/cpp_front.py` reads a compile database with libclang (Homebrew LLVM) and +maps each project `enum class` to a union: + +- **emission**: a reference that produces the value (assignment, atomic + `store`, argument, return, initialiser); a cast from an integer is an + indirect emission, which makes "never emitted" unprovable for that enum; +- **observation**: a `case`, `==` or `!=`; +- **swallowed failure**: a busy enumerator written to the method's own state, + then a top-level `catch` that neither writes state nor rethrows. + +Identity comes from clang's resolved AST, so two enums with the same name are +never confused. Sources the build does not compile are recorded as unresolved. + +```bash +tool/cpp_front.py --build-dir /build/debug -o model.json +pipe check --model model.json +``` + +For C++, read `rendered-not-emitted` as "branched on but never produced" (a +dead branch, or a state a state machine never enters) and `emitted-not-rendered` +as "produced but never branched on" (computed and ignored, or reaching output +only through a name table or reflection). Rule messages still say "UI". + +On yams (375 translation units, about two minutes) a hand check of 13 findings +found 12 factually correct and 1 false positive, which the front end now +excludes. That sample bounds the false-positive rate; it does not measure it. +Controls live in `test/fixtures/cpp_state` and `test/cpp_front_test.dart`, +skipped when LLVM is absent. diff --git a/docs/development.md b/docs/development.md new file mode 100644 index 0000000..84bad35 --- /dev/null +++ b/docs/development.md @@ -0,0 +1,54 @@ +# Development + +## Building + +```bash +dart pub get +tool/build_release.sh # `dart build cli`, falling back to `dart compile exe` +``` + +Use the native binary rather than `dart run`: JIT startup is paid on every +invocation. On macOS arm64: + +| Scenario | `dart run` | AOT binary | +| --- | --- | --- | +| 2-file project | 6.4s | 0.2s | +| 814-file Flutter app, end to end | ~23-29s | 10.4s | +| per-file p50 on that app | ~50-75ms | 16ms | + +The binary is about 18MB and self-contained. On a large project the first file +costs about 3s of analyzer warm-up (SDK and package summaries). Cutting that +would need a persistent analysis cache. + +## Tests + +```bash +dart test +``` + +The suite runs against `test/fixtures/stuck_loading`, a dependency-free project +with hand-written stand-ins for BLoC, Riverpod and provider, so it resolves with +no `pub get` and no network. Do not add a `package:` import to it. + +What the suite holds pipe to: + +- every rule has a positive and a negative control (`rule_contract_test.dart`); +- no fingerprint depends on position, and none collide (`fingerprint_test.dart`); +- the state model survives JSON unchanged, with identical findings + (`state_map_roundtrip_test.dart`); +- partial, unreadable and unresolved scans are reported, never passed silently. + +`test/cpp_front_test.dart` runs the C++ front end on `test/fixtures/cpp_state` +and is skipped without Homebrew LLVM. + +## Hooks and CI + +`git config core.hooksPath .githooks` enables the hooks. `pre-commit` runs +`scripts/lint.sh --hook` (format and analyze); `pre-push` runs +`scripts/pre-push-ci-gate.sh` (format, analyze and the full test suite). +Commit messages follow Conventional Commits (`commitlint.config.js`). + +## Measuring precision and recall + +See [rules.md](rules.md#how-precision-is-measured) for `tool/adjudicate.py` and +`pipe recall`. diff --git a/docs/rules.md b/docs/rules.md new file mode 100644 index 0000000..538223c --- /dev/null +++ b/docs/rules.md @@ -0,0 +1,82 @@ +# Rules + +Every rule states a factual, whole-program claim ("no UI code matches this +state"). Precision below is **factual** precision: how often the claim holds, +not how often it is worth acting on. A rule that cannot be validated is demoted +or removed. + +## Evidence per rule + +Measured on the dogfood apps (nuwayze, dex, word_alive, tick). + +| Rule | Evidence | +| --- | --- | +| `emitted-not-rendered` | 79/79 claims factual on nuwayze main (adjudicator with passing controls). One finding per unobserved union and per unobserved marker class. Drops to info when the UI reads the union's payload through base-class fields. | +| `rendered-not-emitted` | Fixture contract; suppressed with a reason when the owner emits through helper methods. | +| `loading-not-reset` | Fixture contract; the dogfood apps guard correctly. | +| `loading-flag-not-reset` | 7/7 on dex, hand-adjudicated. | +| `event-without-handler` | Fixture contract; 168 dispatch sites on nuwayze all map to a handler. Follows the event hierarchy (`on` covers subtypes) and blocs sharing an event type. | +| `dead-handler` | 77/77 on nuwayze (adjudicator). The message names the directories scanned; events built only in tests are not features. | +| `riverpod-async-gap` | 10 hand-adjudicated findings (word_alive, tick). Recognises `.when`, `AsyncError`/`AsyncLoading` patterns and values stored in locals. `bool` providers are downgraded to info. | +| `notifier-never-notifies` | Fixture contract only; no real-code true positives yet. Limited to observable fields; lifecycle hooks excluded. | +| `notifier-inert-consumer` | Fixture contract only. | +| `provider-read-in-build` | 1 finding from 447 `context.read` calls on nuwayze. Reads inside closures are excluded by design. | +| `notifier-field-unread` | 3 on nuwayze; medium confidence, since a name match elsewhere can mask an unread member. | +| `provider-created-in-build` | Info, once per build method: the claim is factual, but impact depends on how often the widget rebuilds. | + +## A factual finding is not always a bug + +`emitted-not-rendered` at high confidence can be deliberate: dormant scaffolding +or a retired feature. Those findings are one decision per union, and the +remediation asks to check the repo's docs and action contracts before deleting +anything. + +## How precision is measured + +`tool/adjudicate.py ` re-derives the two highest-volume claims +(`emitted-not-rendered`, `dead-handler`) with its own regex evidence, and prints +controls first: it must find evidence for every variant pipe calls rendered and +every event pipe calls dispatched. Controls are three-way (found / +unattributable / missing), and only *missing* fails. A corpus with no control +material reports **NO SIGNAL** and undefined precision. The script exits +non-zero whenever it cannot certify. + +Limits: it is only partly independent (the list of variants, events and owners +to check comes from pipe's `state` output), it judges the factual claim only, +not severity, and it needs a clean working tree. + +## Recall + +1. **Rule contracts.** `test/rule_contract_test.dart` requires each rule to fire + on a positive control and stay silent on an adjacent negative control. A rule + without both fails the build. +2. **Labelled real bugs.** `pipe recall` evaluates `recall/manifest.json`. An + `open` case must be found; a `fixed` case must stay quiet, and reappearing is + a **REGRESSION**. Repos missing from the machine are *skipped*, never passed. + Results reflect each repo's working tree. + +The labelled set is small: it bounds the risk of false negatives, it does not +prove their absence. + +## provider and ChangeNotifier + +Modelled: every provider-package declaration type (`Provider`, +`ChangeNotifierProvider`, `ListenableProvider`, `ValueListenableProvider`, +`FutureProvider`, `StreamProvider`, `StateProvider`, `ProxyProvider` and +variants, `.value`), and `Consumer`, `Selector`, `ListenableBuilder` and +`AnimatedBuilder` as consumers. Declarations are told apart from Riverpod by +`create:`/`update:`/`value:` arguments, not by import, so an unrelated class +named `Provider` taking `create:` would be miscounted. + +Not modelled: `ProxyProvider` `update` functions returning unchanged values, and +rebuild frequency. + +## Other limits + +Not modelled: `bloc_concurrency` transformers, freezed unions whose generated +file is absent, and `ref.read` call sites. + +pipe deliberately does not reimplement per-file checks the ecosystem already +does well: `unawaited_futures`, `use_build_context_synchronously`, +`cancel_subscriptions`, `close_sinks`, `bloc_lint`, or declaration-level dead +code (`undead`, `dallow`). diff --git a/docs/usage.md b/docs/usage.md new file mode 100644 index 0000000..1bf6d1d --- /dev/null +++ b/docs/usage.md @@ -0,0 +1,119 @@ +# Usage + +## Commands + +| Command | What it does | +| --- | --- | +| `pipe index ` | Resolve the project and report analysis health, timing and diagnostics. `--fail-on-error` exits 1 on analyzer errors. | +| `pipe graph ` | The resolved declaration model, as JSON. | +| `pipe state ` | The state model: unions, emissions, render sites, events, Riverpod, notifier facts. `--format json` writes the model other commands can read back. | +| `pipe check ` | Ranked findings with evidence and remediation. | +| `pipe suggest ` | Findings collapsed to root causes and ranked, globally and per file. | +| `pipe guide ` | A prompt-sized brief for an agent; `--yams` persists it to the yams corpus. | +| `pipe verify --baseline F` | Which accepted findings were resolved, remain, or are new. | +| `pipe recall --root ` | How many labelled known bugs pipe detects (see [rules](rules.md#recall)). | + +Common flags: `--format text|json` (`agent|md|json` for `guide`), `-c/--concurrency N` +(default 4), `--sdk `, `--dir ` (repeatable, default `lib`), +`--include-generated`, `--rule ` (repeatable), `--top N`, and `--limit N` +(debugging only; results are marked partial). + +Command-specific: `check --fail-on`, `--baseline`, `--write-baseline`, +`--model`; `guide --budget --max-findings --yams --yams-name`; +`suggest --budget --max-actions`; `verify --expect-resolved`; +`recall --manifest --root --strict`; `--policy wire-up|remove` on `check`, +`guide` and `suggest`. + +Every numeric flag rejects a value that is not a positive integer instead of +falling back to its default. + +## SDK and resolution + +pipe finds the SDK itself: `flutterRoot` from the target's +`.dart_tool/package_config.json`, then `FLUTTER_ROOT`, then `flutter` on `PATH`. +Flutter projects need Flutter's bundled Dart SDK, because `dart:ui` is not part +of the standalone one. Override with `--sdk`. + +pipe refuses to report a clean result it did not earn: + +- A project never resolved (`pub get` not run) is an error, not a scan in which + every `package:` import is `dynamic`. +- An SDK that exists but cannot be read (for example under a privacy-protected + macOS folder) is refused with a `--sdk` remedy. +- Files that fail to resolve make the results **PARTIAL**, with a warning. +- Files with unresolved references (`uri_does_not_exist`, `undefined_*`) produce + a warning and a report note, and every finding naming something such a file + mentions drops one confidence step (`data.confidenceLowered`). An unresolved + `Foo()` is a method call, not a construction, so evidence there is invisible. + +## CI and baselines + +```bash +pipe check . --write-baseline .pipe-baseline.json +pipe check . --baseline .pipe-baseline.json --fail-on warning +``` + +`--fail-on none|info|warning|error` (default `none`) fails when a **new** finding +at or above that severity is present. Baselined findings never trip it. +`--fail-on-finding` is an alias for `--fail-on error`. + +A baseline stores one fingerprint per finding: rule, file and semantic identity +(union, variant, event, provider, owner, member). Fingerprints never include a +line number, so moving code does not resurrect accepted findings. Entries also +carry a readable label, so `verify` names what was resolved. + +`--write-baseline` refuses a partial scan (`--rule`, `--limit`, or unreadable +files), because the baseline would omit every finding outside it. `check --model` +refuses it too, since a model does not record whether every file was read. + +## Verifying a fix + +```text +$ pipe verify . --baseline .pipe-baseline.json +baseline 12 accepted findings +resolved 1 no longer reported — these fixes are confirmed +still 11 still reported +new 0 not in the baseline +RESOLVED + + lib/flag_cubit.dart:25 loadUnguarded [loading-flag-not-reset] +``` + +`--expect-resolved` exits non-zero while anything accepted is still reported. +`verify` refuses `--limit` and partially readable projects, and `--rule` +compares only that rule's accepted findings. + +## Fix order + +`pipe suggest` collapses findings to root causes (one action per union, +provider or handler), scores each, and prints the arithmetic so the order can be +argued with: + +| Term | Effect | +| --- | --- | +| severity | `error` 100, `warning` 50, `info` 10 (the base score) | +| confidence | high ×1.0, medium ×0.7, low ×0.4 | +| breadth | +10 per extra finding the fix clears, capped at +50 | +| user-visible failure | +25 when the rule marks the failure as user-facing | +| certainty | +15 when the fix needs no product judgement | + +The bonuses are additive, so a wide, certain `info` group can outrank a narrow +`warning`. Only new findings are planned, so a ratcheted repo yields an empty +plan. + +## Agent briefs + +`pipe guide` groups findings by root cause with dense anchors +(`file:18,29,32`), a rule legend carrying the standard fix, a `decision:` line +where the project makes "fix vs delete" decidable, and a `verify:` command. +Truncation is always stated. + +Two limits are stated in the brief: whether to wire up or delete an unwired +bloc needs product intent, and adding a missing UI branch means reading the +consumer widget, which pipe names but does not inline. + +## Remediation policy + +The default is `--policy wire-up`: an unreachable state, handler or control is +usually an unfinished feature, so the remediation is to make it reachable, and +removal is worded as needing approval. `--policy remove` flips the emphasis. The +facts reported are the same either way. -- 2.52.0