fix/nuwayze-dogfood-findings #11
4 changed files with 255 additions and 10 deletions
fix(provider): count same-file reads for notifier-field-unread
Found by dogfooding on nuwayze: `pipe check` reported _AgentHubController is consumed in the UI but 8 of its public member(s) are read by name nowhere outside the class ... for a private ChangeNotifier whose widget, declared in the same file (lib/screens/settings/agent_hub_screen.dart), renders every one of those getters (`controller.authorizations`, `controller.hasLoadError`, ...). The rule claims "outside the class" but discounted the notifier's whole file: member reads were collected per file and the file that declares the notifier was skipped. A private controller can only be rendered from its own library, so that exclusion hid exactly the reads that matter. Member reads are now kept per scope: reads inside a ChangeNotifier class body go under `memberUseScope(file, Class)`, everything else under the file. The rule discounts only the notifier's own scope, so a widget in the same file, or in a `part` of the library, counts as a reader, while a notifier reading itself (`this.x`, `other.x`) still does not. The model JSON keeps its shape (a map of name sets); only the keys gained the class scope. Tests (test/field_unread_scope_test.dart, a generated project): a same-file widget read and a part-file read are not reported; a member read nowhere and a member read only inside its own class are. On nuwayze-w3 the finding is gone and nothing else moves: 65 findings with the current release, 64 with this change, the only difference being _AgentHubController. 221 tests pass.
commit
1332dcbff0
|
|
@ -373,22 +373,49 @@ class _ReadCallCollector extends RecursiveAstVisitor<void> {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Collects member reads (`x.someGetter`, `x.someField`) per file, so a public
|
/// Collects member reads (`x.someGetter`, `x.someField`) per scope, so a public
|
||||||
/// notifier getter that nothing reads can be identified as dead state.
|
/// notifier getter that nothing reads can be identified as dead state.
|
||||||
|
///
|
||||||
|
/// Reads inside a `ChangeNotifier` class body are kept under that class's own
|
||||||
|
/// scope ([memberUseScope]) and everything else under the file, so the rule can
|
||||||
|
/// discount a notifier reading itself without discounting the widget declared
|
||||||
|
/// next to it in the same file.
|
||||||
class _MemberUseCollector extends RecursiveAstVisitor<void> {
|
class _MemberUseCollector extends RecursiveAstVisitor<void> {
|
||||||
_MemberUseCollector(this.uses);
|
_MemberUseCollector(this.relativePath, this.uses);
|
||||||
|
|
||||||
final Set<String> uses;
|
final String relativePath;
|
||||||
|
final Map<String, Set<String>> uses;
|
||||||
|
String? _notifier;
|
||||||
|
|
||||||
|
Set<String> get _scope => uses.putIfAbsent(
|
||||||
|
_notifier == null ? relativePath : memberUseScope(relativePath, _notifier!),
|
||||||
|
() => <String>{},
|
||||||
|
);
|
||||||
|
|
||||||
|
@override
|
||||||
|
void visitClassDeclaration(ClassDeclaration node) {
|
||||||
|
if (!_NotifierCollector._isChangeNotifier(node)) {
|
||||||
|
super.visitClassDeclaration(node);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
final outer = _notifier;
|
||||||
|
_notifier = node.namePart.typeName.lexeme;
|
||||||
|
try {
|
||||||
|
super.visitClassDeclaration(node);
|
||||||
|
} finally {
|
||||||
|
_notifier = outer;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
@override
|
@override
|
||||||
void visitPropertyAccess(PropertyAccess node) {
|
void visitPropertyAccess(PropertyAccess node) {
|
||||||
uses.add(node.propertyName.name);
|
_scope.add(node.propertyName.name);
|
||||||
super.visitPropertyAccess(node);
|
super.visitPropertyAccess(node);
|
||||||
}
|
}
|
||||||
|
|
||||||
@override
|
@override
|
||||||
void visitPrefixedIdentifier(PrefixedIdentifier node) {
|
void visitPrefixedIdentifier(PrefixedIdentifier node) {
|
||||||
uses.add(node.identifier.name);
|
_scope.add(node.identifier.name);
|
||||||
super.visitPrefixedIdentifier(node);
|
super.visitPrefixedIdentifier(node);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -532,9 +559,9 @@ NotifierModel collectNotifierModel(
|
||||||
unit.accept(
|
unit.accept(
|
||||||
_ProviderDeclarationCollector(relative, lineInfo, declarations),
|
_ProviderDeclarationCollector(relative, lineInfo, declarations),
|
||||||
);
|
);
|
||||||
final uses = <String>{};
|
final uses = <String, Set<String>>{relative: <String>{}};
|
||||||
unit.accept(_MemberUseCollector(uses));
|
unit.accept(_MemberUseCollector(relative, uses));
|
||||||
memberUses[relative] = uses;
|
memberUses.addAll(uses);
|
||||||
unit.accept(_ReadInBuildCollector(relative, lineInfo, reads));
|
unit.accept(_ReadInBuildCollector(relative, lineInfo, reads));
|
||||||
unit.accept(_ProviderInBuildCollector(relative, lineInfo, createdInBuild));
|
unit.accept(_ProviderInBuildCollector(relative, lineInfo, createdInBuild));
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -353,6 +353,11 @@ class ProviderCreation {
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The [NotifierModel.memberUses] key for reads inside [notifier]'s own class
|
||||||
|
/// body in [file]. A file key ends in `.dart`, so the two never collide, and a
|
||||||
|
/// class name is unique within its library.
|
||||||
|
String memberUseScope(String file, String notifier) => '$file#$notifier';
|
||||||
|
|
||||||
/// Everything the notifier/provider rules judge.
|
/// Everything the notifier/provider rules judge.
|
||||||
class NotifierModel {
|
class NotifierModel {
|
||||||
NotifierModel({
|
NotifierModel({
|
||||||
|
|
@ -397,7 +402,11 @@ class NotifierModel {
|
||||||
final Map<String, List<String>> consumedBy;
|
final Map<String, List<String>> consumedBy;
|
||||||
final List<ProviderDeclaration> declarations;
|
final List<ProviderDeclaration> declarations;
|
||||||
|
|
||||||
/// Project-relative file -> member names read in it.
|
/// Scope -> member names read in it. A scope is a project-relative file for
|
||||||
|
/// reads outside any notifier class, or [memberUseScope] for reads inside
|
||||||
|
/// one, so a notifier's reads of itself can be told apart from reads by the
|
||||||
|
/// rest of its file (a private controller and the widget that renders it
|
||||||
|
/// usually share one).
|
||||||
final Map<String, Set<String>> memberUses;
|
final Map<String, Set<String>> memberUses;
|
||||||
final List<NonSubscribingRead> nonSubscribingReads;
|
final List<NonSubscribingRead> nonSubscribingReads;
|
||||||
final List<ProvidersCreatedInBuild> createdInBuild;
|
final List<ProvidersCreatedInBuild> createdInBuild;
|
||||||
|
|
|
||||||
|
|
@ -121,10 +121,14 @@ String runProviderRules({
|
||||||
if (wantFieldUnread) {
|
if (wantFieldUnread) {
|
||||||
for (final notifier in notifiers) {
|
for (final notifier in notifiers) {
|
||||||
if ((consumedBy[notifier.name] ?? const <String>[]).isEmpty) continue;
|
if ((consumedBy[notifier.name] ?? const <String>[]).isEmpty) continue;
|
||||||
|
// Only the class's own body is discounted. Excluding its whole file
|
||||||
|
// reported a private controller whose widget, in the same file, renders
|
||||||
|
// every one of its getters.
|
||||||
|
final ownScope = memberUseScope(notifier.file, notifier.name);
|
||||||
final unread = <String>[];
|
final unread = <String>[];
|
||||||
for (final member in notifier.apiMembers) {
|
for (final member in notifier.apiMembers) {
|
||||||
final read = memberUses.entries
|
final read = memberUses.entries
|
||||||
.where((entry) => entry.key != notifier.file)
|
.where((entry) => entry.key != ownScope)
|
||||||
.any((entry) => entry.value.contains(member));
|
.any((entry) => entry.value.contains(member));
|
||||||
if (!read) unread.add(member);
|
if (!read) unread.add(member);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
205
test/field_unread_scope_test.dart
Normal file
205
test/field_unread_scope_test.dart
Normal file
|
|
@ -0,0 +1,205 @@
|
||||||
|
import 'dart:io';
|
||||||
|
|
||||||
|
import 'package:pipe/pipe.dart';
|
||||||
|
import 'package:test/test.dart';
|
||||||
|
|
||||||
|
/// notifier-field-unread discounts reads inside the notifier's own class body,
|
||||||
|
/// and nothing else. It used to discount the notifier's whole file, so a
|
||||||
|
/// private controller rendered by the widget declared next to it was reported
|
||||||
|
/// as unread (nuwayze's `_AgentHubController`, eight getters, all rendered).
|
||||||
|
///
|
||||||
|
/// Each case uses member names no other case reads, because a read is matched
|
||||||
|
/// by name anywhere in the scan.
|
||||||
|
const Map<String, String> _files = {
|
||||||
|
'lib/support.dart': '''
|
||||||
|
class ChangeNotifier {
|
||||||
|
void notifyListeners() {}
|
||||||
|
}
|
||||||
|
|
||||||
|
class BuildContext {
|
||||||
|
T watch<T>() => throw UnimplementedError();
|
||||||
|
}
|
||||||
|
''',
|
||||||
|
// A private controller and the widget that renders it, in one file.
|
||||||
|
'lib/same_file.dart': '''
|
||||||
|
import 'support.dart';
|
||||||
|
|
||||||
|
class _SameFileController extends ChangeNotifier {
|
||||||
|
int _sameCount = 0;
|
||||||
|
String? _sameLoadError;
|
||||||
|
|
||||||
|
int get sameCount => _sameCount;
|
||||||
|
String? get sameLoadError => _sameLoadError;
|
||||||
|
|
||||||
|
void refresh() {
|
||||||
|
_sameCount++;
|
||||||
|
notifyListeners();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
class SameFileView {
|
||||||
|
String build(BuildContext context) {
|
||||||
|
final controller = context.watch<_SameFileController>();
|
||||||
|
return '\${controller.sameCount} \${controller.sameLoadError}';
|
||||||
|
}
|
||||||
|
}
|
||||||
|
''',
|
||||||
|
// A controller whose only reader is a part of its library.
|
||||||
|
'lib/part_host.dart': '''
|
||||||
|
import 'support.dart';
|
||||||
|
|
||||||
|
part 'part_host_view.dart';
|
||||||
|
|
||||||
|
class PartController extends ChangeNotifier {
|
||||||
|
int _partTotal = 0;
|
||||||
|
|
||||||
|
int get partTotal => _partTotal;
|
||||||
|
String get partStatus => 'ok';
|
||||||
|
|
||||||
|
void bump() {
|
||||||
|
_partTotal++;
|
||||||
|
notifyListeners();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
''',
|
||||||
|
'lib/part_host_view.dart': '''
|
||||||
|
part of 'part_host.dart';
|
||||||
|
|
||||||
|
class PartView {
|
||||||
|
String build(BuildContext context) {
|
||||||
|
final controller = context.watch<PartController>();
|
||||||
|
return '\${controller.partTotal} \${controller.partStatus}';
|
||||||
|
}
|
||||||
|
}
|
||||||
|
''',
|
||||||
|
// `unreadHiddenCount` is read nowhere, though its sibling is rendered next
|
||||||
|
// door: the true positive the same-file fix must keep.
|
||||||
|
'lib/unread.dart': '''
|
||||||
|
import 'support.dart';
|
||||||
|
|
||||||
|
class UnreadController extends ChangeNotifier {
|
||||||
|
int _unreadShown = 0;
|
||||||
|
int _unreadHiddenCount = 0;
|
||||||
|
|
||||||
|
int get unreadShown => _unreadShown;
|
||||||
|
int get unreadHiddenCount => _unreadHiddenCount;
|
||||||
|
|
||||||
|
void bump() {
|
||||||
|
_unreadShown++;
|
||||||
|
_unreadHiddenCount++;
|
||||||
|
notifyListeners();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
class UnreadView {
|
||||||
|
String build(BuildContext context) =>
|
||||||
|
'\${context.watch<UnreadController>().unreadShown}';
|
||||||
|
}
|
||||||
|
''',
|
||||||
|
// `selfErrorMessage` is read only by the class itself, which is not a
|
||||||
|
// render.
|
||||||
|
'lib/self_read.dart': '''
|
||||||
|
import 'support.dart';
|
||||||
|
|
||||||
|
class SelfReadController extends ChangeNotifier {
|
||||||
|
String? _selfErrorMessage;
|
||||||
|
|
||||||
|
String get selfLabel => 'label';
|
||||||
|
String? get selfErrorMessage => _selfErrorMessage;
|
||||||
|
|
||||||
|
// ignore: unnecessary_this
|
||||||
|
String describe() => 'error: \${this.selfErrorMessage}';
|
||||||
|
|
||||||
|
bool sameErrorAs(SelfReadController other) =>
|
||||||
|
other.selfErrorMessage == _selfErrorMessage;
|
||||||
|
|
||||||
|
void fail(String message) {
|
||||||
|
_selfErrorMessage = message;
|
||||||
|
notifyListeners();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
class SelfReadView {
|
||||||
|
String build(BuildContext context) =>
|
||||||
|
context.watch<SelfReadController>().selfLabel;
|
||||||
|
}
|
||||||
|
''',
|
||||||
|
};
|
||||||
|
|
||||||
|
String _project() {
|
||||||
|
final root = Directory.systemTemp.createTempSync('pipe-field-unread');
|
||||||
|
File('${root.path}/pubspec.yaml').writeAsStringSync('''
|
||||||
|
name: field_unread
|
||||||
|
publish_to: none
|
||||||
|
environment:
|
||||||
|
sdk: ^3.0.0
|
||||||
|
''');
|
||||||
|
File('${root.path}/.dart_tool/package_config.json')
|
||||||
|
..createSync(recursive: true)
|
||||||
|
..writeAsStringSync('''
|
||||||
|
{
|
||||||
|
"configVersion": 2,
|
||||||
|
"packages": [
|
||||||
|
{
|
||||||
|
"name": "field_unread",
|
||||||
|
"rootUri": "../",
|
||||||
|
"packageUri": "lib/",
|
||||||
|
"languageVersion": "3.0"
|
||||||
|
}
|
||||||
|
]
|
||||||
|
}
|
||||||
|
''');
|
||||||
|
for (final entry in _files.entries) {
|
||||||
|
File('${root.path}/${entry.key}')
|
||||||
|
..createSync(recursive: true)
|
||||||
|
..writeAsStringSync(entry.value);
|
||||||
|
}
|
||||||
|
return root.path;
|
||||||
|
}
|
||||||
|
|
||||||
|
void main() {
|
||||||
|
late String root;
|
||||||
|
late ProjectAnalysis analysis;
|
||||||
|
late Map<String, List<Object?>> unreadByOwner;
|
||||||
|
|
||||||
|
setUpAll(() async {
|
||||||
|
root = _project();
|
||||||
|
analysis = await analyzeProject(
|
||||||
|
await loadProject(root),
|
||||||
|
concurrency: 1,
|
||||||
|
enabledRules: {ruleNotifierFieldUnread},
|
||||||
|
);
|
||||||
|
unreadByOwner = {
|
||||||
|
for (final finding in analysis.report.findings)
|
||||||
|
if (finding.rule == ruleNotifierFieldUnread)
|
||||||
|
finding.data['owner']! as String: finding.data['members']! as List,
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
tearDownAll(() async {
|
||||||
|
await analysis.dispose();
|
||||||
|
Directory(root).deleteSync(recursive: true);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('the scan resolved every file', () {
|
||||||
|
// Otherwise a missing finding below could be an analysis failure.
|
||||||
|
expect(analysis.report.coverage!.resolved, _files.length);
|
||||||
|
expect(analysis.report.notes.join('\n'), isNot(contains('unresolved')));
|
||||||
|
});
|
||||||
|
|
||||||
|
test('reads by a widget in the notifier\'s own file count', () {
|
||||||
|
expect(unreadByOwner, isNot(contains('_SameFileController')));
|
||||||
|
});
|
||||||
|
|
||||||
|
test('reads in a part of the notifier\'s library count', () {
|
||||||
|
expect(unreadByOwner, isNot(contains('PartController')));
|
||||||
|
});
|
||||||
|
|
||||||
|
test('a member read nowhere is still reported', () {
|
||||||
|
expect(unreadByOwner['UnreadController'], ['unreadHiddenCount']);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('a member read only inside its own class is still reported', () {
|
||||||
|
expect(unreadByOwner['SelfReadController'], ['selfErrorMessage']);
|
||||||
|
});
|
||||||
|
}
|
||||||
Loading…
Add table
Add a link
Reference in a new issue