merge/audit-and-state-map #7
4 changed files with 151 additions and 19 deletions
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.
commit
939cdcb251
|
|
@ -1485,6 +1485,45 @@ class _RiverpodCollector extends RecursiveAstVisitor<void> {
|
|||
/// 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<FunctionBody>();
|
||||
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<void> {
|
|||
}
|
||||
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<void> {
|
|||
}
|
||||
}
|
||||
|
||||
/// 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<void> {
|
||||
_VariableUses(this.name);
|
||||
|
||||
final String name;
|
||||
final List<SimpleIdentifier> 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<FileOutcome> outcomes) {
|
||||
final types = <_TypeInfo>[];
|
||||
|
|
|
|||
|
|
@ -12,6 +12,8 @@ class AsyncData<T> {
|
|||
class AsyncValue<T> {
|
||||
const AsyncValue();
|
||||
|
||||
T? get value => null;
|
||||
|
||||
AsyncData<T>? get asData => null;
|
||||
|
||||
bool get hasError => false;
|
||||
|
|
@ -24,6 +26,14 @@ class AsyncValue<T> {
|
|||
throw UnimplementedError();
|
||||
}
|
||||
|
||||
class AsyncError<T> extends AsyncValue<T> {
|
||||
const AsyncError();
|
||||
}
|
||||
|
||||
class AsyncLoading<T> extends AsyncValue<T> {
|
||||
const AsyncLoading();
|
||||
}
|
||||
|
||||
class FutureProvider<T> {
|
||||
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<int>((ref) async => 1);
|
||||
final switchedProvider = FutureProvider<int>((ref) async => 1);
|
||||
final storedProvider = FutureProvider<int>((ref) async => 1);
|
||||
final storedUnhandledProvider = FutureProvider<int>((ref) async => 1);
|
||||
|
|
|
|||
|
|
@ -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(<String>['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', () {
|
||||
|
|
|
|||
|
|
@ -77,7 +77,16 @@ const Map<String, List<_Id>> _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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue