From 1332dcbff082d9a30f3aec53072c1d41f4998392 Mon Sep 17 00:00:00 2001 From: trvon Date: Fri, 25 Sep 2026 20:09:13 -0600 Subject: [PATCH 1/3] 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. --- lib/src/dart_provider_facts.dart | 43 +++++-- lib/src/facts.dart | 11 +- lib/src/provider_rules.dart | 6 +- test/field_unread_scope_test.dart | 205 ++++++++++++++++++++++++++++++ 4 files changed, 255 insertions(+), 10 deletions(-) create mode 100644 test/field_unread_scope_test.dart diff --git a/lib/src/dart_provider_facts.dart b/lib/src/dart_provider_facts.dart index ab1ef88..3087920 100644 --- a/lib/src/dart_provider_facts.dart +++ b/lib/src/dart_provider_facts.dart @@ -373,22 +373,49 @@ class _ReadCallCollector extends RecursiveAstVisitor { } } -/// 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. +/// +/// 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 { - _MemberUseCollector(this.uses); + _MemberUseCollector(this.relativePath, this.uses); - final Set uses; + final String relativePath; + final Map> uses; + String? _notifier; + + Set get _scope => uses.putIfAbsent( + _notifier == null ? relativePath : memberUseScope(relativePath, _notifier!), + () => {}, + ); + + @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 void visitPropertyAccess(PropertyAccess node) { - uses.add(node.propertyName.name); + _scope.add(node.propertyName.name); super.visitPropertyAccess(node); } @override void visitPrefixedIdentifier(PrefixedIdentifier node) { - uses.add(node.identifier.name); + _scope.add(node.identifier.name); super.visitPrefixedIdentifier(node); } } @@ -532,9 +559,9 @@ NotifierModel collectNotifierModel( unit.accept( _ProviderDeclarationCollector(relative, lineInfo, declarations), ); - final uses = {}; - unit.accept(_MemberUseCollector(uses)); - memberUses[relative] = uses; + final uses = >{relative: {}}; + unit.accept(_MemberUseCollector(relative, uses)); + memberUses.addAll(uses); unit.accept(_ReadInBuildCollector(relative, lineInfo, reads)); unit.accept(_ProviderInBuildCollector(relative, lineInfo, createdInBuild)); } diff --git a/lib/src/facts.dart b/lib/src/facts.dart index 0dc6268..9279bc7 100644 --- a/lib/src/facts.dart +++ b/lib/src/facts.dart @@ -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. class NotifierModel { NotifierModel({ @@ -397,7 +402,11 @@ class NotifierModel { final Map> consumedBy; final List 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> memberUses; final List nonSubscribingReads; final List createdInBuild; diff --git a/lib/src/provider_rules.dart b/lib/src/provider_rules.dart index 16511d3..b3b0c85 100644 --- a/lib/src/provider_rules.dart +++ b/lib/src/provider_rules.dart @@ -121,10 +121,14 @@ String runProviderRules({ if (wantFieldUnread) { for (final notifier in notifiers) { if ((consumedBy[notifier.name] ?? const []).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 = []; for (final member in notifier.apiMembers) { final read = memberUses.entries - .where((entry) => entry.key != notifier.file) + .where((entry) => entry.key != ownScope) .any((entry) => entry.value.contains(member)); if (!read) unread.add(member); } diff --git a/test/field_unread_scope_test.dart b/test/field_unread_scope_test.dart new file mode 100644 index 0000000..eec575c --- /dev/null +++ b/test/field_unread_scope_test.dart @@ -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 _files = { + 'lib/support.dart': ''' +class ChangeNotifier { + void notifyListeners() {} +} + +class BuildContext { + T watch() => 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(); + 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().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().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> 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']); + }); +} -- 2.52.0 From adf898be1990c430f44ee0ff2e8c5ed39ed24063 Mon Sep 17 00:00:00 2001 From: trvon Date: Fri, 25 Sep 2026 20:11:34 -0600 Subject: [PATCH 2/3] feat(verify): default --baseline to /.pipe-baseline.json `pipe verify .` failed with "verify needs --baseline" even in a repo whose baseline sits where the README's CI recipe writes it. nuwayze's scripts/testing/pipe_state_flow_gate.sh commits exactly that file, .pipe-baseline.json at the repo root, so every verify there had to repeat the path. Without --baseline, verify now reads /.pipe-baseline.json when it exists and says so on stderr ("pipe: no --baseline given; using ..."), so a run never silently picks a file nobody named. With neither, the error names both remedies: pass --baseline, or write the conventional file with `pipe check --write-baseline /.pipe-baseline.json` before the fix. pipe had no baseline-name constant; defaultBaselineName is now the one place it is spelled. Tests (test/verify_default_baseline_test.dart): an explicit --baseline wins and announces nothing; the conventional file is used and announced; with neither the error carries both remedies; and end to end, `pipe verify ` against a baseline written at the copy's root reads it (--expect-resolved fails on the 28 still-present findings). README and docs/usage.md show the shorter form. On nuwayze-w3, `pipe verify /Users/.../nuwayze-w3` with no flag reads its .pipe-baseline.json (65 accepted findings). 225 tests pass. --- README.md | 2 +- docs/usage.md | 6 +- lib/src/cli.dart | 45 +++++++++-- test/verify_default_baseline_test.dart | 104 +++++++++++++++++++++++++ 4 files changed, 147 insertions(+), 10 deletions(-) create mode 100644 test/verify_default_baseline_test.dart diff --git a/README.md b/README.md index 077512f..a2dd62b 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ In CI, accept today's findings once and fail only on new ones: ```bash 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 +pipe verify . --expect-resolved # after a fix; reads .pipe-baseline.json ``` ## Rules diff --git a/docs/usage.md b/docs/usage.md index 5435340..4b598e6 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -10,7 +10,7 @@ | `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 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)). | | `pipe queries ` | Every Firestore query the code can issue, as typed records; an inventory, no rules (see [queries](queries.md)). | @@ -109,6 +109,10 @@ RESOLVED + lib/flag_cubit.dart:25 loadUnguarded [loading-flag-not-reset] ``` +Without `--baseline`, `verify` reads `/.pipe-baseline.json`, the file the +CI recipe above writes, and says so on stderr; with neither, it fails and names +both. So `pipe verify .` is enough in a repo that follows the recipe. + `--expect-resolved` exits non-zero while anything accepted is still reported or was not scanned. `verify` only calls a finding resolved when it looked for it: diff --git a/lib/src/cli.dart b/lib/src/cli.dart index febc4e5..d64537c 100644 --- a/lib/src/cli.dart +++ b/lib/src/cli.dart @@ -625,6 +625,36 @@ class RecallCommand extends Command { } } +/// The conventional baseline file name, at the project root. It is what the +/// README's CI recipe writes, and what `verify` reads when `--baseline` is +/// omitted. +const String defaultBaselineName = '.pipe-baseline.json'; + +/// The baseline `verify` compares against: [explicit] when given, else +/// [defaultBaselineName] under the project [target] when that exists. Using +/// the default is announced on [log] (stderr), so a run never silently picks a +/// file the user did not name. +String resolveVerifyBaseline( + String? explicit, + String target, { + StringSink? log, +}) { + if (explicit != null) return explicit; + final conventional = p.join(target, defaultBaselineName); + if (File(conventional).existsSync()) { + (log ?? stderr).writeln('pipe: no --baseline given; using $conventional'); + return conventional; + } + throw PipeException( + 'verify needs a baseline: no --baseline given and no $defaultBaselineName ' + 'at the project root ($conventional)', + remedy: + 'pass --baseline with the findings you fixed, or write ' + '$conventional with `pipe check $target --write-baseline ' + '$conventional` before the fix', + ); +} + /// Report which accepted findings are gone after a fix. class VerifyCommand extends Command { VerifyCommand() { @@ -632,7 +662,9 @@ class VerifyCommand extends Command { argParser ..addOption( 'baseline', - help: 'Baseline of accepted findings to verify against (required).', + help: + 'Baseline of accepted findings to verify against ' + '(default: /$defaultBaselineName when it exists).', ) ..addMultiOption('rule', help: 'Limit to specific rules (repeatable).') ..addOption('top', defaultsTo: '25', help: 'Rows per section.') @@ -653,13 +685,10 @@ class VerifyCommand extends Command { @override Future run() async { final results = argResults!; - final baselinePath = results['baseline'] as String?; - if (baselinePath == null) { - throw PipeException( - 'verify needs --baseline', - remedy: 'pass the baseline that contains the findings you fixed', - ); - } + final baselinePath = resolveVerifyBaseline( + results['baseline'] as String?, + _target(results), + ); // Verifying a partial scan is unsound, and silently so: with --limit the // findings that were never scanned are absent from the fresh report, which // `verify` reports as RESOLVED - claiming fixes that were never made. diff --git a/test/verify_default_baseline_test.dart b/test/verify_default_baseline_test.dart new file mode 100644 index 0000000..0bb6ea0 --- /dev/null +++ b/test/verify_default_baseline_test.dart @@ -0,0 +1,104 @@ +import 'dart:io'; + +import 'package:path/path.dart' as p; +import 'package:pipe/pipe.dart'; +import 'package:pipe/src/cli.dart'; +import 'package:test/test.dart'; + +/// A copy of the rule fixture, so a baseline can be written at its root. +String _fixtureCopy() { + final from = Directory('test/fixtures/stuck_loading'); + final to = Directory.systemTemp.createTempSync('pipe-verify-default'); + for (final entity in from.listSync(recursive: true)) { + final target = p.join(to.path, p.relative(entity.path, from: from.path)); + if (entity is Directory) { + Directory(target).createSync(recursive: true); + } else if (entity is File) { + File(target) + ..createSync(recursive: true) + ..writeAsBytesSync(entity.readAsBytesSync()); + } + } + return to.path; +} + +void main() { + // `pipe verify .` used to fail with "verify needs --baseline" even in a repo + // whose baseline sits at the conventional path the README's CI recipe + // writes (nuwayze's scripts/testing/pipe_state_flow_gate.sh). + group('resolveVerifyBaseline', () { + late Directory temp; + + setUp(() => temp = Directory.systemTemp.createTempSync('pipe-verify-bl')); + tearDown(() => temp.deleteSync(recursive: true)); + + test('an explicit --baseline wins, with nothing announced', () { + File(p.join(temp.path, defaultBaselineName)).writeAsStringSync('{}'); + final log = StringBuffer(); + expect( + resolveVerifyBaseline('other.json', temp.path, log: log), + 'other.json', + ); + expect(log.toString(), isEmpty); + }); + + test('without --baseline the conventional file is used and announced', () { + final conventional = p.join(temp.path, defaultBaselineName); + File(conventional).writeAsStringSync('{}'); + final log = StringBuffer(); + expect(resolveVerifyBaseline(null, temp.path, log: log), conventional); + expect(log.toString(), contains('using $conventional')); + }); + + test('without either, the error names both remedies', () { + expect( + () => resolveVerifyBaseline(null, temp.path, log: StringBuffer()), + throwsA( + isA() + .having((e) => e.message, 'message', contains('--baseline')) + .having( + (e) => e.remedy, + 'remedy', + allOf( + contains('--baseline'), + contains(p.join(temp.path, defaultBaselineName)), + ), + ), + ), + ); + }); + }); + + test('pipe verify reads /.pipe-baseline.json', () async { + final root = _fixtureCopy(); + try { + final baseline = p.join(root, defaultBaselineName); + expect( + await PipeRunner().run([ + 'check', + root, + '--write-baseline', + baseline, + '-c', + '1', + ]), + 0, + ); + // Nothing changed since the baseline, so every accepted finding remains + // and --expect-resolved fails: proof the default baseline was read, + // since an empty one would have nothing to expect. + expect( + await PipeRunner().run([ + 'verify', + root, + '--expect-resolved', + '-c', + '1', + ]), + 1, + ); + } finally { + Directory(root).deleteSync(recursive: true); + } + }); +} -- 2.52.0 From 31607b5bcb25627e8439986ce8eaa63e2fdd4e78 Mon Sep 17 00:00:00 2001 From: trvon Date: Fri, 25 Sep 2026 20:16:23 -0600 Subject: [PATCH 3/3] fix(queries): resolve a subdirectory path to its enclosing package Found by dogfooding on nuwayze: `pipe queries lib` from the repo root failed with no resolved package graph at .dart_tool/package_config.json remedy: run `flutter pub get` or `dart pub get` in .../nuwayze-w3/lib It failed even with the package resolved, because the path was taken as the project root: pipe looked for lib/.dart_tool/package_config.json, which never exists, and the remedy sent the user to run pub get in a directory that has no pubspec. enclosingPackage() (project.dart) splits a path into the package that contains it (the path itself when it holds a pubspec.yaml, else the nearest ancestor that does) and the directory it names under that root. `pipe queries` loads the package root and scans only that directory, so `pipe queries lib/services` is `--dir lib/services` on the package; an explicit --dir with such a path is relative to it. Without a resolved graph the error now names the package root. loadProject's own remedy uses the same lookup, so `pipe check lib` (and the other commands, which still take the path as the root) now says the path is inside the package at and suggests passing that with `--dir lib`, instead of pub get in lib. Tests: enclosingPackage on a root, a nested directory, a nested package and a missing path; loadProject's remedy for a directory inside a package; and `pipe queries` on the fixture's lib/data (root reported as the package, one file discovered), with `lib --dir data`, and on an unresolved package where the remedy names the root, not lib. On nuwayze-w3: `pipe queries lib` exits 0 with 313 queries over 847 files, identical to `pipe queries .`; `pipe queries lib/services` scans 161 files and every one of its 212 queries sits under lib/services. 233 tests pass. --- docs/queries.md | 5 +++ lib/pipe.dart | 3 +- lib/src/cli.dart | 39 ++++++++++++++++--- lib/src/project.dart | 43 ++++++++++++++++++++- test/firestore_queries_test.dart | 56 ++++++++++++++++++++++++++++ test/package_target_test.dart | 64 ++++++++++++++++++++++++++++++++ 6 files changed, 202 insertions(+), 8 deletions(-) create mode 100644 test/package_target_test.dart diff --git a/docs/queries.md b/docs/queries.md index 9ec73c9..f622f09 100644 --- a/docs/queries.md +++ b/docs/queries.md @@ -6,6 +6,11 @@ findings, no baseline, and deliberately no index analysis. Deciding whether a query needs a composite index, and probing a live project, belong to the consumer of this output, which can compare it against `firestore.indexes.json`. +The path may be a package root or a directory inside one: `pipe queries lib/data` +resolves the package that contains `lib/data` (the nearest `pubspec.yaml`) and +scans only that directory, as `--dir lib/data` would. `--dir` given with such a +path is relative to it. + What it recognises: - **cloud_firestore chains.** Receivers are identified by **static type and diff --git a/lib/pipe.dart b/lib/pipe.dart index 1790309..f97a233 100644 --- a/lib/pipe.dart +++ b/lib/pipe.dart @@ -46,7 +46,8 @@ 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; +export 'src/project.dart' + show PackageTarget, Project, enclosingPackage, loadProject; export 'src/sdk.dart' show SdkResolution, resolveSdk; export 'src/rules.dart' show diff --git a/lib/src/cli.dart b/lib/src/cli.dart index d64537c..af1b938 100644 --- a/lib/src/cli.dart +++ b/lib/src/cli.dart @@ -337,9 +337,18 @@ class GraphCommand extends Command { @override Future run() async { final results = argResults!; + // `pipe queries lib` means "the queries under lib of this package": the + // package graph lives at the package root, and the path is the scope. + final target = enclosingPackage(_target(results)); + final dirs = results['dir'] as List; + final scope = target.scope; final project = await loadProject( - _target(results), - directories: results['dir'] as List, + target.root, + directories: scope == null + ? dirs + : results.wasParsed('dir') + ? [for (final dir in dirs) p.join(scope, dir)] + : [scope], ); final run = await resolveProject( project, @@ -393,9 +402,18 @@ class StateCommand extends Command { @override Future run() async { final results = argResults!; + // `pipe queries lib` means "the queries under lib of this package": the + // package graph lives at the package root, and the path is the scope. + final target = enclosingPackage(_target(results)); + final dirs = results['dir'] as List; + final scope = target.scope; final project = await loadProject( - _target(results), - directories: results['dir'] as List, + target.root, + directories: scope == null + ? dirs + : results.wasParsed('dir') + ? [for (final dir in dirs) p.join(scope, dir)] + : [scope], ); final run = await resolveProject( project, @@ -1009,9 +1027,18 @@ class QueriesCommand extends Command { final adapters = configPath == null ? const [] : parseAdapterConfig(_readText(configPath, 'adapter config')); + // `pipe queries lib` means "the queries under lib of this package": the + // package graph lives at the package root, and the path is the scope. + final target = enclosingPackage(_target(results)); + final dirs = results['dir'] as List; + final scope = target.scope; final project = await loadProject( - _target(results), - directories: results['dir'] as List, + target.root, + directories: scope == null + ? dirs + : results.wasParsed('dir') + ? [for (final dir in dirs) p.join(scope, dir)] + : [scope], ); final run = await resolveProject( project, diff --git a/lib/src/project.dart b/lib/src/project.dart index fb2406d..fa6e0a0 100644 --- a/lib/src/project.dart +++ b/lib/src/project.dart @@ -56,6 +56,40 @@ bool _isHidden(String path, String root) { .any((segment) => segment.startsWith('.') && segment != '.'); } +/// A path split into the package that contains it and the directory under +/// that package's root it names. +class PackageTarget { + const PackageTarget(this.root, this.scope); + + /// The package root: the path itself when it holds a `pubspec.yaml`, else + /// the nearest ancestor that does, else the path unchanged. + final String root; + + /// The path relative to [root] when it named a directory inside the + /// package (`lib`, `lib/data`), or null when it named the root itself. + final String? scope; +} + +/// The package enclosing [inputPath]. +/// +/// `pipe queries lib`, run from a package root, used to treat `lib` as the +/// project: it looked for `lib/.dart_tool/package_config.json` and told the +/// user to run `pub get` in `lib`. The package graph and the remedy belong to +/// the package root, and `lib` is only the scope within it. +PackageTarget enclosingPackage(String inputPath) { + final path = p.normalize(p.absolute(inputPath)); + if (!Directory(path).existsSync() || + File(p.join(path, 'pubspec.yaml')).existsSync()) { + return PackageTarget(path, null); + } + for (var dir = p.dirname(path); ; dir = p.dirname(dir)) { + if (File(p.join(dir, 'pubspec.yaml')).existsSync()) { + return PackageTarget(dir, p.relative(path, from: dir)); + } + if (p.dirname(dir) == dir) return PackageTarget(path, null); + } +} + /// Load and validate a target project without touching the filesystem. /// /// Fails loudly when the project has not been resolved, because analyzing an @@ -74,9 +108,16 @@ Future loadProject( p.join(root, '.dart_tool', 'package_config.json'), ); if (!packageConfigFile.existsSync()) { + // A directory inside a package has no graph of its own; resolving there + // would not help, so point at the package that owns it. + final package = enclosingPackage(root); throw PipeException( 'no resolved package graph at .dart_tool/package_config.json', - remedy: 'run `flutter pub get` or `dart pub get` in $root', + remedy: package.scope == null + ? 'run `flutter pub get` or `dart pub get` in $root' + : '$root is inside the package at ${package.root}: pass that path ' + '(with `--dir ${package.scope}` to narrow the scan), and run ' + '`flutter pub get` or `dart pub get` there if it has not been', ); } diff --git a/test/firestore_queries_test.dart b/test/firestore_queries_test.dart index 564a938..9469346 100644 --- a/test/firestore_queries_test.dart +++ b/test/firestore_queries_test.dart @@ -602,5 +602,61 @@ adapters: expect(strict.exitCode, 2); expect(strict.stderr as String, contains('--require-firestore')); }); + + test('a directory inside the package is a scope, not a project', () async { + // `pipe queries lib` looked for lib/.dart_tool/package_config.json and + // failed. The graph is the package's; the path only narrows the scan. + final result = await pipe(['$fixturePath/lib/data']); + expect(result.exitCode, 0, reason: '${result.stderr}'); + final json = jsonDecode(result.stdout as String) as Map; + expect(json['root'], fixturePath); + final coverage = json['coverage'] as Map; + expect(coverage['discovered'], 1); + expect(coverage['complete'], isTrue); + // lib/data holds only the gateway, so the queries the rest of lib issues + // are out of scope. + final sites = [ + for (final query in json['queries'] as List) + (query as Map)['site'] as String, + ]; + expect(sites, everyElement(startsWith('lib/data/'))); + expect(model.queries, isNotEmpty); + expect(json['unresolvedFirestoreUris'], isEmpty); + }); + + test('--dir under a scoped path is relative to that path', () async { + final result = await pipe(['$fixturePath/lib', '--dir', 'data']); + expect(result.exitCode, 0, reason: '${result.stderr}'); + final json = jsonDecode(result.stdout as String) as Map; + expect((json['coverage'] as Map)['discovered'], 1); + }); + + test('an unresolved package is reported at its root, not the scope', () { + final copy = Directory.systemTemp.createTempSync('pipe-queries-fresh'); + try { + File('${copy.path}/pubspec.yaml').writeAsStringSync('name: fresh\n'); + File('${copy.path}/lib/fresh.dart') + ..createSync(recursive: true) + ..writeAsStringSync('void main() {}\n'); + final root = copy.resolveSymbolicLinksSync(); + expect( + () => PipeRunner().run(['queries', '$root/lib']), + throwsA( + isA() + .having((e) => e.message, 'message', contains('package graph')) + .having( + (e) => e.remedy, + 'remedy', + allOf( + endsWith('pub get` in $root'), + isNot(contains('$root/lib')), + ), + ), + ), + ); + } finally { + copy.deleteSync(recursive: true); + } + }); }); } diff --git a/test/package_target_test.dart b/test/package_target_test.dart new file mode 100644 index 0000000..ee7b1d0 --- /dev/null +++ b/test/package_target_test.dart @@ -0,0 +1,64 @@ +import 'dart:io'; + +import 'package:path/path.dart' as p; +import 'package:pipe/pipe.dart'; +import 'package:test/test.dart'; + +void main() { + late String root; + + setUp(() { + root = Directory.systemTemp + .createTempSync('pipe-package-target') + .resolveSymbolicLinksSync(); + File(p.join(root, 'pubspec.yaml')).writeAsStringSync('name: outer\n'); + Directory(p.join(root, 'lib', 'data')).createSync(recursive: true); + File(p.join(root, 'lib', 'data', 'a.dart')).writeAsStringSync(''); + File(p.join(root, 'nested', 'pubspec.yaml')) + ..createSync(recursive: true) + ..writeAsStringSync('name: nested\n'); + Directory(p.join(root, 'nested', 'lib')).createSync(); + }); + + tearDown(() => Directory(root).deleteSync(recursive: true)); + + group('enclosingPackage', () { + test('a package root is its own package, with no scope', () { + final target = enclosingPackage(root); + expect(target.root, root); + expect(target.scope, isNull); + }); + + test('a directory inside a package resolves to the package root', () { + final target = enclosingPackage(p.join(root, 'lib', 'data')); + expect(target.root, root); + expect(target.scope, p.join('lib', 'data')); + }); + + test('the nearest pubspec wins', () { + final target = enclosingPackage(p.join(root, 'nested', 'lib')); + expect(target.root, p.join(root, 'nested')); + expect(target.scope, 'lib'); + }); + + test('a missing path is returned unchanged for loadProject to reject', () { + final missing = p.join(root, 'nope'); + expect(enclosingPackage(missing).root, missing); + expect(enclosingPackage(missing).scope, isNull); + }); + }); + + test('loadProject on a directory inside a package names the package', () { + // `pipe check lib` used to say "run `dart pub get` in .../lib". + expect( + () => loadProject(p.join(root, 'lib')), + throwsA( + isA().having( + (e) => e.remedy, + 'remedy', + allOf(contains('inside the package at $root'), contains('--dir lib')), + ), + ), + ); + }); +} -- 2.52.0