From fb587a97a02a823b3ffa18e3cb9f51393c5c1305 Mon Sep 17 00:00:00 2001 From: Alexandru Mariuti Date: Wed, 26 Aug 2026 19:26:20 +0700 Subject: [PATCH 1/2] fix(solid_generator): qualify cross-file registry entries by origin library MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #110. The name-keyed registry could not distinguish a local plain class from a foreign @SolidState class sharing its simple name: blocking registration protected the local class but silently lost the genuinely reactive foreign read. Entries under a shadowed or multi-origin name are now recorded qualified by origin library (side-maps; the flat registry is stripped for flagged names so every other consumer is byte-identical for the unflagged common case), and the value rewriter resolves flagged names only when the receiver's tier-1 resolved staticType library matches a recorded origin — asset:/source URIs normalized to the registry's package: form. AST-only receivers never rewrite an ambiguous name, so the change can only add rewrites proven by real resolution, never introduce a wrong one. Two same-named @SolidState classes consumed in one file each resolve through their own receiver; the one-hop local-shadow fixture's foreign read now lowers while the local plain read stays untouched. Deliberate side-effect (changelogged): dispose auto-injection no longer recognizes a 2+-origin name as @Solid-annotated — pass dispose: explicitly for such classes. Validated pre-release against the downstream app via path overrides (byte-identical output + live shadow probe). --- SPEC.md | 4 +- packages/solid_generator/CHANGELOG.md | 2 + packages/solid_generator/lib/builder.dart | 295 ++++++++++++++---- .../lib/src/annotation_reader.dart | 24 ++ .../lib/src/build_rewriter.dart | 6 + .../lib/src/cross_file_consumer_rewriter.dart | 44 ++- .../lib/src/plain_class_rewriter.dart | 13 +- .../lib/src/state_class_rewriter.dart | 13 +- .../lib/src/stateless_rewriter.dart | 13 +- .../lib/src/value_rewriter.dart | 237 ++++++++++++-- .../main.dart | 16 + .../foo_a.dart | 8 + .../foo_b.dart | 8 + .../main.dart | 38 +++ .../consumer.dart | 65 ++-- .../foo_a.g.dart | 11 + .../foo_b.g.dart | 11 + .../main.g.dart | 38 +++ .../consumer.g.dart | 67 ++-- .../test/integration/golden_helpers.dart | 1 + .../test/value_rewriter_test.dart | 53 ++++ 21 files changed, 812 insertions(+), 155 deletions(-) create mode 100644 packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/foo_a.dart create mode 100644 packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/foo_b.dart create mode 100644 packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/main.dart create mode 100644 packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/foo_a.g.dart create mode 100644 packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/foo_b.g.dart create mode 100644 packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/main.g.dart diff --git a/SPEC.md b/SPEC.md index 2a69a24..5104a03 100644 --- a/SPEC.md +++ b/SPEC.md @@ -1089,7 +1089,9 @@ Rules: Clause 2's registry is the SAME class-name → reactive-member-name map (`classRegistry`) the builder threads through the `.value` cross-class rewrite (§5.1) — populated same-file by a fast member-scan and cross-file by walking imports (the resolver pass documented under §3.6's env-field rule), extended to also seed from every type created at a `Provider(...)` / `.environment()` call site in the file (not just `@SolidEnvironment` field types), so a controller that's provided but never consumed via `@SolidEnvironment` still gets recognized. It is further extended to seed from the declared type name of every class's instance field and constructor parameter in the file — the plain constructor-injection DI shape (`CustomersRepository({required AuthRepository authRepository})`, `final AuthRepository _authRepository;`), which has no `@SolidEnvironment` field and creates nothing at a `Provider(...)` / `.environment()` call site, so neither of the other two rules ever seeds it (issue #104). A `super.` constructor parameter seeds the same way: an explicit-typed one (`Foo(AuthRepository super.repo)`) reads its own annotation like any other parameter, and a bare one (`Foo(super.repo)`) is resolved — on a resolved unit, from the analyzer's own resolution of the forwarded parameter; on an unresolved unit (the pure-consumer probe), by locating the superclass declaration and matching the targeted super-constructor's corresponding parameter, recursing up a chain of further bare `super.` relays where needed — or, for a `this.x` field-formal target, the base class's same-named instance field (§2; issue #108). The superclass-location search is same-package-only and alias-aware (§2), and never runs at all once the resolved path has already answered the question — both matter in practice because this same code path fires for `{super.key}` on every widget, and without them would otherwise chase `StatefulWidget`/`State` into `package:flutter` on every unresolved probe. Once the wanted name is known, finding ITS OWN declaration gets one extra hop beyond the import walk this paragraph describes — through the file that hosts the superclass's own imports — never a general transitive walk (§2). A type name enters the registry ONLY when its declaration carries at least one `@SolidState` field or getter; a candidate name that matches nothing found is harmless. - Two rules narrow the cross-file import walk to mirror Dart's own name resolution, closing collision windows the annotation-blind seeding above would otherwise open: a simple name that the CURRENT file itself declares as a class/enum/mixin/etc. is dropped from the wanted set before the import walk starts — a local top-level declaration always shadows a same-name import (no error, no ambiguity), so cross-file attribution for that name is provably wrong and the import walk never even looks for it; and each import's `show`/`hide` combinators are honored — an import that hides the wanted name, or carries a `show` list that doesn't include it, cannot be credited as that name's source and is skipped for that name (its other names are unaffected). Matching is otherwise by SIMPLE class name, not library identity — two distinct types sharing a name across two DIFFERENT imported libraries, neither of them local to the current file and neither excluded by a combinator, could in principle still collide; a residual risk of the name-based registry design, not resolved by the two rules above. The bare-`super.x` seeder's own ambiguity fallback (§2; issue #108) widens this collision surface a little further when it fires: unable to pin down a single matching parameter, it seeds EVERY field's and constructor parameter's type name from the located superclass, including names that would otherwise never have been proposed as candidates for the current file at all. + One rule still narrows the cross-file import walk to mirror Dart's own name resolution: each import's `show`/`hide` combinators are honored — an import that hides the wanted name, or carries a `show` list that doesn't include it, cannot be credited as that name's source and is skipped for that name (its other names are unaffected). A simple name the CURRENT file itself declares as a class/enum/mixin/etc. no longer drops that name from the wanted set before the walk starts (issue #110) — the walk still looks for a same-named foreign class, and a match is recorded QUALIFIED by its origin library (`classRegistryOrigins`, `name -> originUri -> fields`) rather than folded into the flat, name-only `classRegistry`; a name with a local declaration, or with two-plus DISTINCT cross-file origins, is flagged in `classRegistryShadowedNames` and the flat `classRegistry` entry for it is withheld. + + That qualification is what narrows the residual name-based collision risk — but only for the §5.1 `.value` cross-class rewrite, and only where real type resolution reaches the receiver. `value_rewriter.dart` resolves a flagged name's cross-class read by comparing the receiver's resolved `staticType` (tier 1 — see §5.1) against the flagged name's recorded origins; two distinct types sharing a simple name across two DIFFERENT imported libraries now resolve to their OWN class's fields precisely, each through its own constructor-injected receiver, rather than colliding. The caveat is NOT eliminated, only narrowed to the contexts tier 1 cannot reach: an AST-only receiver (tiers 2-4 — a same-named-but-untyped shape, or any context with no resolver at all) can never supply a library URI, so a flagged name simply never rewrites there — conservative, not wrong, but still a residual gap relative to a fully library-qualified design. It is also NOT narrowed at all for THIS section's own clause 2 — the dispose auto-injection registry lookup (`_classRegistry.containsKey(typeName)`) is a plain name-based presence check with no per-origin awareness, and the main lowering path (above) has no resolver to consult in the first place, so two distinct types sharing a name could still cause dispose auto-injection to reason about the wrong one. The bare-`super.x` seeder's own ambiguity fallback (§2; issue #108) widens the underlying collision SURFACE a little further when it fires: unable to pin down a single matching parameter, it seeds EVERY field's and constructor parameter's type name from the located superclass, including names that would otherwise never have been proposed as candidates for the current file at all — the qualification above still applies to each of those seeded names individually. The injected `provider.dispose()` always compiles for Solid-lowered types because Section 10 attaches `implements Disposable` and a synthesized `dispose()` to every annotated class. For non-Solid types, the user declares `void dispose()` (or a known disposable base) on the source class for clause 1 to fire; otherwise nothing is injected (clause 3) unless the type is cross-file and unproven (clause 4), and the user may always opt in or out by writing `dispose:` explicitly. diff --git a/packages/solid_generator/CHANGELOG.md b/packages/solid_generator/CHANGELOG.md index c5a8278..f81cbeb 100644 --- a/packages/solid_generator/CHANGELOG.md +++ b/packages/solid_generator/CHANGELOG.md @@ -1,5 +1,7 @@ ## 3.0.0-dev.5 +- **FIX**: The cross-file registry's name-keyed same-simple-name collision — documented as a residual risk in §4.9 and reproduced as an accepted trade-off in the `cross_file_super_param_one_hop_local_shadow` fixture (dev.4/#108) — is now resolved wherever real type resolution reaches the receiver (#110). Previously, when a file locally declared a plain class under the same simple name as a foreign, `@SolidState`-bearing class reached via constructor injection, the two shadow rules added for #104/#105/#108 (`wantedTypes.removeAll(declaredInUnit)` in the main walk, the mirrored `declaredInConsumingUnit` guard in the one-hop extension) dropped the foreign class from the registry ENTIRELY before it was ever looked up — correctly preventing the wrong rewrite on the local class, but at the cost of also losing the genuinely reactive foreign read, which stayed silently un-lowered with no error. `_populateCrossFileTypes`'s cross-file walk (`_registerWantedClassesFrom`, shared by the main walk and the one-hop extension) no longer removes a wanted name from consideration on either front: a match under a locally-shadowed name, or a second DISTINCT match for a name already resolved once, is now recorded QUALIFIED by origin library into two new side-maps, `classRegistryOrigins` / `classCollectionFieldsOrigins` (`name -> originUri -> fields`, the origin URI computed the same `_sourceToLibAsset(...).uri` way `crossClassFieldTypeOriginUris` already does). A finalize pass at the end of `_populateCrossFileTypes` then flags any name with two-plus recorded origins, or with a local declaration of that name in the current file, into a new `classRegistryShadowedNames` set — and, for exactly those flagged names, strips the flat `classRegistry` / `classCollectionFields` entry back out, so every OTHER existing consumer of those two maps (dispose auto-injection, the tier 2-4 AST-only rewrite paths) sees precisely the same flat map they always did for an unflagged name. `value_rewriter.dart`'s cross-class rewrite (`_ValueRewriteVisitor._fieldsForCrossClassName`) is the one new consumer of the qualified side-maps: an unflagged name resolves exactly as before (no URI check, byte-identical); a FLAGGED name resolves only when the receiver's tier-1 `Expression.staticType` (real semantic resolution — never available on the AST-only tiers 2-4) is present AND its declaring library's URI, normalized from the resolver's `asset:/source/` form into the registry's own `package:/` form (`_normalizeLibraryUri`), matches one of the flagged name's recorded origins. This closes the `cross_file_super_param_one_hop_local_shadow` gap exactly (`thing.label` now lowers to `thing.label.value` while `f.label` correctly stays untouched — both classes share the simple name `Foo`) and, more generally, lets two unrelated `@SolidState` classes sharing a simple name across different files each resolve correctly through their own constructor-injected receiver in the same consuming file (see the new `cross_file_qualified_registry_disambiguation` fixture). An AST-only receiver, or a flagged name whose resolved library matches none of its recorded origins, still never rewrites — the pre-#110 conservative default for anything the registry cannot prove. One deliberate side-effect on dispose auto-injection: a flagged name's flat `classRegistry` entry is stripped, so a class whose simple name has two-plus distinct cross-file origins (no local shadow needed) is no longer recognized as `@Solid*`-annotated by the dispose rewriter and gets NO auto-injected `dispose:` (the visible-plain clause applies) — previously it non-deterministically fired for whichever origin an import walk registered first. Pass `dispose: (context, provider) => provider.dispose()` explicitly when providing such a same-named class. + - **FIX**: The dev.4 residual gap — a PURE CONSUMER whose ONLY link to a cross-file `@SolidState`-bearing class is a bare `super.x` constructor parameter (no type written at that position) — is now closed (#108), including issue #108's own primary example: a consumer that imports only its immediate superclass's file, never the file that actually declares the injected class. Detecting a pure consumer at all still requires the no-annotation fast path's UNRESOLVED syntactic probe, and a bare `super.x` still carries no type text at that position, but `_populateCrossFileTypes`'s seeding loop now resolves it from the AST alone: it reads the subclass's own `extends` clause for the superclass's simple name, locates that class's declaration (checking the current unit's own top-level declarations first — skipped when the `extends` clause is import-prefixed, since a prefixed reference can never resolve to a local declaration — then walking imports with the same shadowing/combinator/skip-set discipline the rest of the function already applies, PLUS two more: same-package only, since this package's `@SolidState` classes can never live in a cross-package file — critical in practice, since this same code path fires for `{super.key}` on every widget, and without the guard would otherwise chase `StatefulWidget`/`State` into `package:flutter`; and alias-aware, since an unprefixed name can only come from an unprefixed import and a prefixed name only from the import carrying that exact prefix — `_importExposesName`'s show/hide check alone can't tell a same-named decoy behind a DIFFERENT prefix from the real, unprefixed source), finds the super-constructor the subclass's initializer list targets (`super.named(...)` for a named constructor, else the implicit call to the unnamed one), and matches the bare parameter against that constructor's corresponding parameter — positionally among its other `super.` siblings (never ambiguous for code that compiles, since Dart itself rejects mixing a positional `super.x` with an explicit positional argument in the same invocation) or by name. When the matched parameter is itself an untyped `this.x` field formal, the same-named instance field's declared type is used instead; when it is ITSELF a further bare `super.x` (a chain of plain relaying subclasses forwarding the same parameter), the search recurses up the inheritance chain, up to five superclasses deep, before giving up. The syntactic walk is skipped entirely once the RESOLVED fallback (`_seedFromResolvedSuperFormal`, dev.3) already answered the question — running the full import-and-parse walk on top of a successful resolution was pure waste, and, unconditional, was the other half of the `{super.key}`-on-every-widget cost. Once the wanted class's simple name is known, locating ITS OWN declaration gets exactly one further hop beyond the import walk just described — through the file that hosts the superclass's own imports, same-package-only, never a general transitive walk: this is what closes issue #108's own primary example, where the injected class is declared in a file the pure consumer never imports at all, only its superclass's file does. A class reachable only through a LONGER chain of imports beyond that one extra hop remains a documented residual, not recognized. When the matched parameter's declared type is itself one of the superclass's OWN type parameters (`class Base { Base(this.repo); final T repo; }`), the wanted name is mapped BY INDEX to the subclass's own `extends Base` type argument, recursing through nested generics, instead of seeding the placeholder name verbatim — a bare type-parameter reference is a syntactically ordinary type name, so without this mapping it silently seeded a name (`T`) that matches no class anywhere and the real dependency (`AuthRepository`) was never sought at all (fix review addendum finding 2). When the superclass can't be located, the targeted constructor can't be pinned down, the matched parameter resolves to no usable type, or the type-parameter mapping has no type argument to resolve against, every candidate name from the located superclass (every constructor parameter, every instance field) is seeded rather than guessing wrong or skipping silently — over-seeding a name is harmless, since the registry only fills from classes that actually carry `@SolidState` members, though it does further widen the pre-existing same-simple-name registry-collision surface (§4.9) by proposing names that would otherwise never have been candidates for the file at all. The fix lives in two shared helpers, `_seedFromSyntacticSuperFormal` (the chain search) and `_populateCrossFileTypesOneHop` (the one-hop class-location extension, sharing its registration rule with the main cross-file walk via `_registerWantedClassesFrom`), both reached from `_populateCrossFileTypes`'s single seeding loop, so the probe and the main pipeline pick them up identically. ## 3.0.0-dev.4 diff --git a/packages/solid_generator/lib/builder.dart b/packages/solid_generator/lib/builder.dart index 3bb7ab9..273d5c1 100644 --- a/packages/solid_generator/lib/builder.dart +++ b/packages/solid_generator/lib/builder.dart @@ -165,6 +165,15 @@ class _SolidBuilder implements Builder { Map>? probedCrossFileCollections; Map>? probedCrossFileFieldTypes; Map>? probedCrossFileOriginUris; + // Origin-qualified counterparts (issue #110) — see + // `_populateCrossFileTypes`'s doc comment. Populated in lockstep with + // the four maps above; empty (never null) is a valid, common result — + // most files resolve every wanted name unambiguously — so these are + // merged unconditionally below rather than null-checked like the ones + // above. + Map>>? probedClassRegistryOrigins; + Map>>? probedClassCollectionFieldsOrigins; + Set? probedClassRegistryShadowedNames; if (!hasSolidAnnotation && !hasProviderHint) { // Cheap syntactic pre-check (#106): seed candidate cross-file wanted @@ -195,6 +204,9 @@ class _SolidBuilder implements Builder { final probeCollections = >{}; final probeFieldTypes = >{}; final probeOriginUris = >{}; + final probeRegistryOrigins = >>{}; + final probeCollectionFieldsOrigins = >>{}; + final probeShadowedNames = {}; await _populateCrossFileTypes( parsed.unit, buildStep, @@ -203,8 +215,20 @@ class _SolidBuilder implements Builder { probeFieldTypes, probeOriginUris, const {}, + probeRegistryOrigins, + probeCollectionFieldsOrigins, + probeShadowedNames, ); - if (probeRegistry.isEmpty) { + // A name issue #110's finalize pass flagged as ambiguous (shadowed by + // a local declaration, or collided across two-plus cross-file + // origins) is stripped from `probeRegistry` but still lives in + // `probeShadowedNames` / `probeRegistryOrigins` — exactly the + // `cross_file_super_param_one_hop_local_shadow` shape, where the + // file's ONLY cross-file find is a shadowed name. Bailing out here on + // `probeRegistry.isEmpty` alone (its pre-#110 condition) would treat + // that file as having nothing to lower at all, silently reintroducing + // the bug this issue fixes. + if (probeRegistry.isEmpty && probeShadowedNames.isEmpty) { await buildStep.writeAsString(outputId, source); return; } @@ -212,6 +236,9 @@ class _SolidBuilder implements Builder { probedCrossFileCollections = probeCollections; probedCrossFileFieldTypes = probeFieldTypes; probedCrossFileOriginUris = probeOriginUris; + probedClassRegistryOrigins = probeRegistryOrigins; + probedClassCollectionFieldsOrigins = probeCollectionFieldsOrigins; + probedClassRegistryShadowedNames = probeShadowedNames; } for (final diagnostic in parsed.errors) { @@ -284,6 +311,16 @@ class _SolidBuilder implements Builder { // import into the consumer's lib output so the synthesized // `Computed<(…, T, …)>` Record-Computed resolves at lib-time. final crossClassFieldTypeOriginUris = >{}; + // Origin-qualified counterparts of `sameFileRegistry` / + // `sameFileCollections` (issue #110) — `name -> originUri -> fields` — + // plus the set of names those two maps deliberately hold NO flat entry + // for. See `_populateCrossFileTypes`'s doc comment; threaded down to + // every reader / rewriter so `value_rewriter.dart`'s tier-1 URI match + // can resolve a flagged name wherever a reactive body reads it. + final sameFileRegistryOrigins = >>{}; + final sameFileCollectionFieldsOrigins = + >>{}; + final sameFileShadowedNames = {}; // Cross-file resolver: walks every `package:`/relative import of the // current source file, redirecting same-package imports from `lib/` to // `source/`, and pulls in `@SolidState` member names for every class @@ -313,6 +350,11 @@ class _SolidBuilder implements Builder { sameFileCollections.addAll(probedCrossFileCollections!); sameFileFieldTypes.addAll(probedCrossFileFieldTypes!); crossClassFieldTypeOriginUris.addAll(probedCrossFileOriginUris!); + sameFileRegistryOrigins.addAll(probedClassRegistryOrigins!); + sameFileCollectionFieldsOrigins.addAll( + probedClassCollectionFieldsOrigins!, + ); + sameFileShadowedNames.addAll(probedClassRegistryShadowedNames!); } else { await _populateCrossFileTypes( unit, @@ -322,6 +364,9 @@ class _SolidBuilder implements Builder { sameFileFieldTypes, crossClassFieldTypeOriginUris, hasProviderHint ? collectProviderCreatedTypeNames(unit) : const {}, + sameFileRegistryOrigins, + sameFileCollectionFieldsOrigins, + sameFileShadowedNames, ); } @@ -330,6 +375,9 @@ class _SolidBuilder implements Builder { source, sameFileRegistry, sameFileCollections, + sameFileRegistryOrigins, + sameFileCollectionFieldsOrigins, + sameFileShadowedNames, ); if (annotatedClasses.every((c) => c.hasNoAnnotations)) { // No reactive annotations resolved. The file may still need: @@ -377,6 +425,9 @@ class _SolidBuilder implements Builder { unit, classRegistry: sameFileRegistry, classCollectionFields: sameFileCollections, + classRegistryOrigins: sameFileRegistryOrigins, + classCollectionFieldsOrigins: sameFileCollectionFieldsOrigins, + classRegistryShadowedNames: sameFileShadowedNames, ); var current = lowered.text; if (hasProviderHint) { @@ -421,6 +472,9 @@ class _SolidBuilder implements Builder { crossClassFieldTypeOriginUris, buildStep.inputId, source, + sameFileRegistryOrigins, + sameFileCollectionFieldsOrigins, + sameFileShadowedNames, ); await buildStep.writeAsString(outputId, transformed); } @@ -576,6 +630,9 @@ List<_AnnotatedClass> _collectAnnotatedClasses( String source, Map> classRegistry, Map> classCollectionFields, + Map>> classRegistryOrigins, + Map>> classCollectionFieldsOrigins, + Set classRegistryShadowedNames, ) { final result = <_AnnotatedClass>[]; for (final decl in unit.declarations) { @@ -609,6 +666,18 @@ List<_AnnotatedClass> _collectAnnotatedClasses( final crossClassCollections = Map>.from( classCollectionFields, )..remove(selfClass); + // Origin-qualified counterparts (issue #110), same exclude-self + // discipline as the two flat views above. + final crossClassRegistryOrigins = + Map>>.from(classRegistryOrigins) + ..remove(selfClass); + final crossClassCollectionFieldsOrigins = + Map>>.from( + classCollectionFieldsOrigins, + )..remove(selfClass); + final crossClassShadowedNames = Set.from( + classRegistryShadowedNames, + )..remove(selfClass); // Pre-scan members once for `@SolidEnvironment` so each reader sees the // host class's env-field map (fieldName → typeText) up-front. The // env-field receiver shape (`.`) needs this @@ -651,6 +720,9 @@ List<_AnnotatedClass> _collectAnnotatedClasses( queryNames: queryNames, classRegistry: crossClassRegistry, classCollectionFields: crossClassCollections, + classRegistryOrigins: crossClassRegistryOrigins, + classCollectionFieldsOrigins: crossClassCollectionFieldsOrigins, + classRegistryShadowedNames: crossClassShadowedNames, environmentFields: environmentFieldsForBody, collectionFields: collectionFieldsSeen, widgetBoundFields: widgetBoundCtorNames, @@ -673,6 +745,9 @@ List<_AnnotatedClass> _collectAnnotatedClasses( queryNames: queryNames, classRegistry: crossClassRegistry, classCollectionFields: crossClassCollections, + classRegistryOrigins: crossClassRegistryOrigins, + classCollectionFieldsOrigins: crossClassCollectionFieldsOrigins, + classRegistryShadowedNames: crossClassShadowedNames, environmentFields: environmentFieldsForBody, collectionFields: collectionFieldsSeen, widgetBoundFields: widgetBoundCtorNames, @@ -688,6 +763,9 @@ List<_AnnotatedClass> _collectAnnotatedClasses( queryNames: queryNames, classRegistry: crossClassRegistry, classCollectionFields: crossClassCollections, + classRegistryOrigins: crossClassRegistryOrigins, + classCollectionFieldsOrigins: crossClassCollectionFieldsOrigins, + classRegistryShadowedNames: crossClassShadowedNames, environmentFields: environmentFieldsForBody, collectionFields: collectionFieldsSeen, widgetBoundFields: widgetBoundCtorNames, @@ -825,6 +903,9 @@ String _renderOutput( Map> crossClassFieldTypeOriginUris, AssetId inputId, String source, + Map>> classRegistryOrigins, + Map>> classCollectionFieldsOrigins, + Set classRegistryShadowedNames, ) { // Walk `unit.declarations` in source order. Class declarations are paired // with `annotatedClasses` (which `_collectAnnotatedClasses` populates in @@ -842,6 +923,9 @@ String _renderOutput( classCollectionFields, classFieldTypes, source, + classRegistryOrigins, + classCollectionFieldsOrigins, + classRegistryShadowedNames, ) else _passthroughResult(decl, source), @@ -981,6 +1065,9 @@ RewriteResult _resultForClass( Map> classCollectionFields, Map> classFieldTypes, String source, + Map>> classRegistryOrigins, + Map>> classCollectionFieldsOrigins, + Set classRegistryShadowedNames, ) { if (c.hasNoAnnotations) return _passthroughResult(c.decl, source); return _rewriteClass( @@ -994,6 +1081,9 @@ RewriteResult _resultForClass( classCollectionFields, classFieldTypes, source, + classRegistryOrigins, + classCollectionFieldsOrigins, + classRegistryShadowedNames, ); } @@ -1025,6 +1115,9 @@ RewriteResult _rewriteClass( Map> classCollectionFields, Map> classFieldTypes, String source, + Map>> classRegistryOrigins, + Map>> classCollectionFieldsOrigins, + Set classRegistryShadowedNames, ) { final kind = classKindOf(decl); final className = decl.name.lexeme; @@ -1041,6 +1134,9 @@ RewriteResult _rewriteClass( classCollectionFields, classFieldTypes, source, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); case ClassKind.plainClass: return rewritePlainClass( @@ -1054,6 +1150,9 @@ RewriteResult _rewriteClass( classCollectionFields, classFieldTypes, source, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); case ClassKind.stateClass: return rewriteStateClass( @@ -1067,6 +1166,9 @@ RewriteResult _rewriteClass( classCollectionFields, classFieldTypes, source, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); case ClassKind.statefulWidget: throw CodeGenerationError( @@ -1092,12 +1194,22 @@ RewriteResult _rewriteClass( /// The two registries are mutated in place. Same-file types take precedence: /// when a type name is already present, the cross-file pass does NOT /// overwrite it (in-file source is always the source of truth for the -/// current build). More generally, any simple name this unit itself -/// declares (class/enum/mixin/…) is dropped from the wanted set before the -/// import walk even starts — mirroring Dart's own name resolution, where a -/// local top-level declaration always shadows a same-name import — and each -/// import's `show`/`hide` combinators are honored so an import cannot be -/// credited as a name's source when it explicitly excludes that name (see +/// current build). +/// +/// A simple name this unit ALSO declares itself (class/enum/mixin/…) no +/// longer removes the name from the wanted set before the import walk (issue +/// #110 — it used to, dropping any foreign class of that name from the +/// registry entirely, silently losing its reactivity). The import walk now +/// always searches; a match found under a locally-shadowed name (or a name +/// two-plus distinct imports resolve to different classes for) is +/// registered QUALIFIED, by origin, into [classRegistryOrigins] / +/// [classCollectionFieldsOrigins] rather than the flat [classRegistry] / +/// [classCollectionFields] — see the finalize pass at the end of this +/// function and `value_rewriter.dart`'s [_ValueRewriteVisitor. +/// _fieldsForCrossClassName] for how a flagged name resolves at rewrite +/// time (a mandatory tier-1 library-URI match). Each import's `show`/`hide` +/// combinators are still honored, so an import cannot be credited as a +/// name's source when it explicitly excludes that name (see /// [_importExposesName]). /// /// `package:` imports of the **current package** are redirected from `lib/` @@ -1116,6 +1228,9 @@ Future _populateCrossFileTypes( Map> classFieldTypes, Map> crossClassFieldTypeOriginUris, Set extraWantedTypes, + Map>> classRegistryOrigins, + Map>> classCollectionFieldsOrigins, + Set classRegistryShadowedNames, ) async { // Walk every `@SolidEnvironment` field declaration in the unit. The // builder pre-scan does NOT pre-build env-field models — the readers do @@ -1124,10 +1239,17 @@ Future _populateCrossFileTypes( // present in [classRegistry] are skipped (the same-file pass is the // source of truth there). final wantedTypes = {}; - // Computed up front (issue #108 fix review finding 1): a name [unit] - // itself declares always shadows a same-name import, no matter how many - // hops away the import lives — see the `wantedTypes.removeAll(...)` call - // below, and [_populateCrossFileTypesOneHop]'s use of this same set. + // Computed up front (issue #108 fix review finding 1; repurposed by issue + // #110). A name [unit] itself declares always shadows a same-name import + // for ORDINARY (unqualified) name resolution — Dart's own rule — no + // matter how many hops away the import lives. Before issue #110 this set + // was used to strip such names from `wantedTypes` before the import walk + // even started, so a foreign class sharing the name was never looked up + // at all and its reactivity silently lost (see the + // `cross_file_super_param_one_hop_local_shadow` fixture). It is now + // consulted only by the finalize pass at the end of this function, AFTER + // the walk below has had a chance to find and qualify any such foreign + // class by origin — see [classRegistryOrigins]. final declaredInUnit = _collectDeclaredTypeNames(unit); for (final decl in unit.declarations) { if (decl is! ClassDeclaration) continue; @@ -1264,11 +1386,12 @@ Future _populateCrossFileTypes( hop.$2, step, wantedTypes, - declaredInUnit, classRegistry, classCollectionFields, classFieldTypes, crossClassFieldTypeOriginUris, + classRegistryOrigins, + classCollectionFieldsOrigins, ); } } @@ -1282,20 +1405,14 @@ Future _populateCrossFileTypes( } } } - // Local declarations always shadow same-name imports (standard Dart - // name-resolution: no error, no ambiguity — the current library's own - // top-level declaration simply wins). So a simple name that this unit - // itself declares as a class/enum/mixin/etc. can NEVER be the wanted - // type's cross-file source, no matter what any import brings in under - // that same simple name — see issue #104 fix review, finding 1 (same- - // simple-name shadowing collision: a local plain `class Address` plus an - // unrelated imported `@SolidState`-annotated `class Address` elsewhere - // must not attribute the import's reactive fields to the local class). - wantedTypes.removeAll(declaredInUnit); + // A simple name this unit itself declares as a class/enum/mixin/etc. no + // longer removes that name from `wantedTypes` (issue #110 — see this + // function's doc comment). `wantedTypes` may still be empty here (nothing + // needed cross-file resolution at all), in which case the walk below is a + // no-op. if (wantedTypes.isEmpty) return; for (final directive in unit.directives.whereType()) { - if (wantedTypes.isEmpty) break; final uri = directive.uri.stringValue; if (uri == null || uri.startsWith('dart:')) continue; final assetId = _resolveImportToSourceAsset(uri, step.inputId); @@ -1328,8 +1445,34 @@ Future _populateCrossFileTypes( classCollectionFields, classFieldTypes, crossClassFieldTypeOriginUris, + classRegistryOrigins, + classCollectionFieldsOrigins, ); } + + // Finalize per-name ambiguity (issue #110). A name qualifies for + // disambiguation — meaning the flat, name-keyed `classRegistry` / + // `classCollectionFields` deliberately hold NO entry for it, forcing + // every consumer through the origin-qualified `classRegistryOrigins` / + // `classCollectionFieldsOrigins` side-maps instead — iff either (a) + // `_registerWantedClassesFrom` (above, or via the one-hop extension) + // found the name under two or more DISTINCT origins (a genuine + // same-simple-name collision across this file's own imports), or (b) + // this file ALSO declares its own top-level type under that name (the + // shadowing scenario this issue exists to fix — previously handled by + // dropping the name from `wantedTypes` before the walk even started, so + // the foreign class's reactivity was lost outright rather than merely + // qualified). Every OTHER name keeps its flat entry exactly as + // `_registerWantedClassesFrom` wrote it — unconditionally unambiguous, + // byte-identical to this generator's behavior before issue #110. + for (final name in classRegistryOrigins.keys) { + final origins = classRegistryOrigins[name]!; + final isAmbiguous = origins.length > 1 || declaredInUnit.contains(name); + if (!isAmbiguous) continue; + classRegistryShadowedNames.add(name); + classRegistry.remove(name); + classCollectionFields.remove(name); + } } /// Scans [imported]'s top-level class declarations for names still present @@ -1346,9 +1489,35 @@ Future _populateCrossFileTypes( /// extension ([_populateCrossFileTypesOneHop], issue #108 fix review /// finding 1) so both apply IDENTICAL registration rules — including the /// "don't stop at the first same-named-but-unannotated decoy" discipline: a -/// match with zero `@SolidState` members does NOT remove the name from -/// [wantedTypes], so a caller that walks more than one import for the same -/// name keeps looking. +/// match with zero `@SolidState` members contributes nothing. +/// +/// [wantedTypes] is NEVER mutated here (issue #110 — it used to remove a +/// name as soon as one reactive match was found, which meant a SECOND +/// distinct class sharing that same simple name, reached through a LATER +/// import of the same file, was never even looked at). Every call site now +/// keeps searching every one of its own imports for every wanted name, so a +/// genuine same-simple-name collision across two distinct cross-file +/// classes is fully discovered — each origin recorded into +/// [classRegistryOrigins] / [classCollectionFieldsOrigins] — rather than +/// silently resolved to whichever import happened to be scanned first. This +/// is a bounded cost (this file's own import count, never transitive); a +/// name found in exactly one import — the overwhelming common case — pays +/// no extra cost beyond scanning the (typically short) remainder of the +/// import list. +/// +/// [classRegistry] / [classCollectionFields] / [classFieldTypes] receive +/// EVERY match found, keyed by simple name only, exactly as before issue +/// #110 — including a match under a name that turns out to be ambiguous. +/// [_populateCrossFileTypes]'s finalize pass, which runs after every call +/// site here has contributed, is what strips such a name back out of +/// [classRegistry] / [classCollectionFields] once it recognizes the +/// ambiguity; this function does not need to know about that decision. +/// [classRegistryOrigins] / [classCollectionFieldsOrigins] accumulate every +/// match's fields keyed by BOTH simple name and origin URI +/// (`_sourceToLibAsset(importedAssetId).uri.toString()` — the same +/// asset-derived URI form [crossClassFieldTypeOriginUris] already uses), +/// unconditionally — this is the qualified data the finalize pass and +/// `value_rewriter.dart`'s tier-1 URI match need for a name it flags. void _registerWantedClassesFrom( CompilationUnit imported, AssetId importedAssetId, @@ -1359,6 +1528,8 @@ void _registerWantedClassesFrom( Map> classCollectionFields, Map> classFieldTypes, Map> crossClassFieldTypeOriginUris, + Map>> classRegistryOrigins, + Map>> classCollectionFieldsOrigins, ) { for (final decl in imported.declarations) { if (decl is! ClassDeclaration) continue; @@ -1400,6 +1571,17 @@ void _registerWantedClassesFrom( if (fieldTypeTexts.isNotEmpty) { classFieldTypes[className] = fieldTypeTexts; } + // Origin-qualified counterpart (issue #110) — recorded unconditionally + // for every match, ambiguous or not; see this function's doc comment + // and [_populateCrossFileTypes]'s finalize pass. + final originUri = _sourceToLibAsset(importedAssetId).uri.toString(); + (classRegistryOrigins[className] ??= >{})[originUri] = + scalarNames; + if (collectionNames.isNotEmpty) { + (classCollectionFieldsOrigins[className] ??= + >{})[originUri] = + collectionNames; + } // For each `@SolidState` field whose declared type is NOT declared // inside the same class file, capture the file's same-package import // URIs as candidate origins. The consumer's lib output will inject @@ -1433,7 +1615,6 @@ void _registerWantedClassesFrom( ); } } - wantedTypes.remove(className); } } } @@ -1462,49 +1643,47 @@ void _registerWantedClassesFrom( /// one-hop extension exists ONLY to serve the superclass seeder, which /// itself only ever locates a same-package superclass in the first place. /// -/// Consuming-unit shadow guard (issue #108 fix review addendum, finding 1): -/// [declaredInConsumingUnit] is the ORIGINAL scanned file's own declared -/// type names — computed once in [_populateCrossFileTypes] and threaded -/// through unchanged. Filtering [wantedTypes] against [hostUnit]'s own -/// declared names (below) is NOT enough on its own: this function runs -/// *during* [_populateCrossFileTypes]'s per-declaration seeding loop, -/// strictly BEFORE that function's own `wantedTypes.removeAll( -/// declaredInUnit)` shadow filter runs at the end of the loop. Without this -/// extra guard, a name the ORIGINAL consuming file declares locally (e.g. -/// its own plain `class Foo`) could still be registered into -/// [classRegistry] from a `@SolidState`-bearing same-named `Foo` reached -/// through [hostUnit]'s imports — the registration is irreversible even -/// though the name is removed from `wantedTypes` moments later, because -/// [classRegistry] is a separate map already written by then. Filtering -/// here, before [_registerWantedClassesFrom] ever runs, closes that -/// ordering hole. +/// Consuming-unit shadow (issue #108 fix review addendum, finding 1; +/// repurposed by issue #110): a name the ORIGINAL scanned file declares +/// locally (e.g. its own plain `class Foo`) used to be stripped from +/// [wantedTypes] before this function even ran, so a match found here +/// through [hostUnit]'s imports was never registered at all — silently +/// dropping a genuinely reactive foreign class's fields (see the +/// `cross_file_super_param_one_hop_local_shadow` fixture). This function no +/// longer applies that guard: a match found here is registered exactly like +/// any other, and [_populateCrossFileTypes]'s finalize pass — which runs +/// once, after every one-hop call site here has already contributed to the +/// SAME [classRegistryOrigins] / [classCollectionFieldsOrigins] instances — +/// is what flags the name as ambiguous against the original file's own +/// declared names, qualifying rather than discarding the registration. Future _populateCrossFileTypesOneHop( CompilationUnit hostUnit, AssetId hostAssetId, BuildStep step, Set wantedTypes, - Set declaredInConsumingUnit, Map> classRegistry, Map> classCollectionFields, Map> classFieldTypes, Map> crossClassFieldTypeOriginUris, + Map>> classRegistryOrigins, + Map>> classCollectionFieldsOrigins, ) async { - // Same local-shadowing discipline the main walk applies to the original - // file, applied here relative to [hostUnit]: a name [hostUnit] declares - // itself can never be the wanted type's source via one of ITS imports — - // and in practice this is always already a no-op, since a name declared - // directly in [hostUnit] would already have been found by the main - // walk's own scan of [hostUnit]'s declarations before this function is - // ever reached. Kept for defense and to make the discipline explicit. - wantedTypes - ..removeAll(_collectDeclaredTypeNames(hostUnit)) - // The guard this function actually exists to add — see the doc - // comment above. - ..removeAll(declaredInConsumingUnit); + // Same local-shadowing discipline the main walk once applied to the + // original file, applied here relative to [hostUnit] itself (distinct + // from the original file — see the doc comment above): a name [hostUnit] + // declares directly can never be the wanted type's source via one of ITS + // OWN imports, because within [hostUnit]'s own scope that name + // unambiguously resolves to [hostUnit]'s local declaration, full stop — + // this is ordinary Dart name resolution, not an issue-#110-style + // cross-FILE collision. In practice this is always already a no-op, since + // a name declared directly in [hostUnit] would already have been found by + // the main walk's own scan of [hostUnit]'s declarations before this + // function is ever reached. Kept for defense and to make the discipline + // explicit. + wantedTypes.removeAll(_collectDeclaredTypeNames(hostUnit)); if (wantedTypes.isEmpty) return; for (final directive in hostUnit.directives.whereType()) { - if (wantedTypes.isEmpty) break; final uri = directive.uri.stringValue; if (uri == null || uri.startsWith('dart:')) continue; final assetId = _resolveImportToSourceAsset(uri, hostAssetId); @@ -1534,6 +1713,8 @@ Future _populateCrossFileTypesOneHop( classCollectionFields, classFieldTypes, crossClassFieldTypeOriginUris, + classRegistryOrigins, + classCollectionFieldsOrigins, ); } } diff --git a/packages/solid_generator/lib/src/annotation_reader.dart b/packages/solid_generator/lib/src/annotation_reader.dart index 90f18ab..ee93705 100644 --- a/packages/solid_generator/lib/src/annotation_reader.dart +++ b/packages/solid_generator/lib/src/annotation_reader.dart @@ -137,6 +137,9 @@ GetterModel? readSolidStateGetter( Map environmentFields = const {}, Set collectionFields = const {}, Set widgetBoundFields = const {}, + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, }) { if (!decl.isGetter || decl.isStatic) return null; final annotation = findAnnotationByName(solidStateName, decl.metadata); @@ -177,6 +180,9 @@ GetterModel? readSolidStateGetter( environmentFields: environmentFields, collectionFields: collectionFields, widgetBoundFields: widgetBoundFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); return GetterModel( @@ -232,6 +238,9 @@ _readReactiveBody( Map environmentFields = const {}, Set collectionFields = const {}, Set widgetBoundFields = const {}, + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, }) { final AstNode node; final bool isBlockBody; @@ -255,6 +264,9 @@ _readReactiveBody( environmentFields: environmentFields, collectionFields: collectionFields, widgetBoundFields: widgetBoundFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); // Zero-deps Effect / Computed are rejected. A reactive dep is either a // `.value`-rewritten state read, a tracked query-call invocation, OR a @@ -307,6 +319,9 @@ EffectModel? readSolidEffectMethod( Map environmentFields = const {}, Set collectionFields = const {}, Set widgetBoundFields = const {}, + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, }) { if (decl.isGetter || decl.isSetter || decl.isStatic) return null; final annotation = findAnnotationByName(solidEffectName, decl.metadata); @@ -341,6 +356,9 @@ EffectModel? readSolidEffectMethod( environmentFields: environmentFields, collectionFields: collectionFields, widgetBoundFields: widgetBoundFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); return EffectModel( @@ -377,6 +395,9 @@ QueryModel? readSolidQueryMethod( Map environmentFields = const {}, Set widgetBoundFields = const {}, Set collectionFields = const {}, + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, }) { if (decl.isGetter || decl.isSetter || decl.isStatic) return null; final annotation = findAnnotationByName(solidQueryName, decl.metadata); @@ -420,6 +441,9 @@ QueryModel? readSolidQueryMethod( environmentFields: environmentFields, collectionFields: collectionFields, widgetBoundFields: widgetBoundFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); // A self-cycle is rejected at codegen — solidart would re-run diff --git a/packages/solid_generator/lib/src/build_rewriter.dart b/packages/solid_generator/lib/src/build_rewriter.dart index de1fa17..23d0d1b 100644 --- a/packages/solid_generator/lib/src/build_rewriter.dart +++ b/packages/solid_generator/lib/src/build_rewriter.dart @@ -108,6 +108,9 @@ BuildMethodRewrite rewriteBuildMethod( Set widgetBoundFields = const {}, Set collectionFields = const {}, Map> classCollectionFields = const {}, + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, }) { final methodStart = buildMethod.offset; final methodEnd = buildMethod.end; @@ -123,6 +126,9 @@ BuildMethodRewrite rewriteBuildMethod( widgetBoundFields: widgetBoundFields, collectionFields: collectionFields, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); final wrapPlan = computeWrapPlan( buildMethod, diff --git a/packages/solid_generator/lib/src/cross_file_consumer_rewriter.dart b/packages/solid_generator/lib/src/cross_file_consumer_rewriter.dart index 539b046..6d935e4 100644 --- a/packages/solid_generator/lib/src/cross_file_consumer_rewriter.dart +++ b/packages/solid_generator/lib/src/cross_file_consumer_rewriter.dart @@ -57,18 +57,27 @@ PureConsumerLowering lowerPureConsumers( CompilationUnit unit, { required Map> classRegistry, required Map> classCollectionFields, + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, }) { final widgetResult = collectPureConsumerWidgetEdits( unit, text, classRegistry: classRegistry, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); final crossFileEdits = collectPureConsumerCrossFileEdits( unit, text, classRegistry: classRegistry, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); if (widgetResult.edits.isEmpty && crossFileEdits.isEmpty) { return (text: text, emittedSignalBuilder: false); @@ -116,15 +125,23 @@ PureConsumerLowering lowerPureConsumers( /// are a known, accepted gap: the reproduced issue and every reported /// real-world shape are DI-shaped (a class holding another class). /// -/// Returns an empty edit list when [classRegistry] is empty or no visited -/// member contains a rewrite. +/// Returns an empty edit list when [classRegistry] AND +/// [classRegistryShadowedNames] are both empty (issue #110 — a name flagged +/// as ambiguous is stripped from [classRegistry] but still needs a visit: +/// see `builder.dart::_populateCrossFileTypes`'s finalize pass) or no +/// visited member contains a rewrite. List collectPureConsumerCrossFileEdits( CompilationUnit unit, String text, { required Map> classRegistry, required Map> classCollectionFields, + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, }) { - if (classRegistry.isEmpty) return const []; + if (classRegistry.isEmpty && classRegistryShadowedNames.isEmpty) { + return const []; + } final edits = []; for (final decl in unit.declarations) { if (decl is! ClassDeclaration) continue; @@ -141,6 +158,9 @@ List collectPureConsumerCrossFileEdits( text, classRegistry: classRegistry, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); edits.addAll(result.edits); } @@ -184,15 +204,20 @@ List collectPureConsumerCrossFileEdits( /// `widget.`-prefixing is needed. /// /// Returns `(edits: const [], emittedSignalBuilder: false)` when -/// [classRegistry] is empty or no visited member contains a rewrite. +/// [classRegistry] AND [classRegistryShadowedNames] are both empty (issue +/// #110 — see [collectPureConsumerCrossFileEdits]'s doc comment) or no +/// visited member contains a rewrite. ({List edits, bool emittedSignalBuilder}) collectPureConsumerWidgetEdits( CompilationUnit unit, String text, { required Map> classRegistry, required Map> classCollectionFields, + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, }) { - if (classRegistry.isEmpty) { + if (classRegistry.isEmpty && classRegistryShadowedNames.isEmpty) { return (edits: const [], emittedSignalBuilder: false); } final edits = []; @@ -212,6 +237,9 @@ collectPureConsumerWidgetEdits( text, classRegistry: classRegistry, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); if (rewritten.emittedWrap) emittedSignalBuilder = true; if (rewritten.text != original) { @@ -227,6 +255,9 @@ collectPureConsumerWidgetEdits( text, classRegistry: classRegistry, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); edits.addAll(result.edits); continue; @@ -238,6 +269,9 @@ collectPureConsumerWidgetEdits( text, classRegistry: classRegistry, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); edits.addAll(result.edits); } diff --git a/packages/solid_generator/lib/src/plain_class_rewriter.dart b/packages/solid_generator/lib/src/plain_class_rewriter.dart index 0a56a9a..da9adf8 100644 --- a/packages/solid_generator/lib/src/plain_class_rewriter.dart +++ b/packages/solid_generator/lib/src/plain_class_rewriter.dart @@ -61,8 +61,11 @@ RewriteResult rewritePlainClass( Map> classRegistry, Map> classCollectionFields, Map> classFieldTypes, - String source, -) { + String source, { + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, +}) { final className = classDecl.name.lexeme; // `@SolidState` getters on plain classes (Computed lowering) ARE supported // via the same `emitComputedField` path the stateless rewriter uses — see @@ -195,6 +198,9 @@ RewriteResult rewritePlainClass( source, collectionFields: collectionNames, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ), ); } @@ -238,6 +244,9 @@ RewriteResult rewritePlainClass( classRegistry: classRegistry, collectionFields: collectionNames, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); pieces[userCtorSlots[i]] = applyEditsToRange( mergedHeaderAndBody, diff --git a/packages/solid_generator/lib/src/state_class_rewriter.dart b/packages/solid_generator/lib/src/state_class_rewriter.dart index a91a14b..5304193 100644 --- a/packages/solid_generator/lib/src/state_class_rewriter.dart +++ b/packages/solid_generator/lib/src/state_class_rewriter.dart @@ -43,8 +43,11 @@ RewriteResult rewriteStateClass( Map> classRegistry, Map> classCollectionFields, Map> classFieldTypes, - String source, -) { + String source, { + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, +}) { final className = classDecl.name.lexeme; // getter→Computed only ships for `StatelessWidget`. The in-place merge // logic this rewriter is built around does not yet handle the @@ -151,6 +154,9 @@ RewriteResult rewriteStateClass( environmentFields: environmentFields, collectionFields: collectionNames, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); pieces.add(buildRewrite.text); // `buildRewrite.emittedWrap` is sourced from `rewriteBuildMethod`'s @@ -197,6 +203,9 @@ RewriteResult rewriteStateClass( environmentFields: environmentFields, collectionFields: collectionNames, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ), ); } diff --git a/packages/solid_generator/lib/src/stateless_rewriter.dart b/packages/solid_generator/lib/src/stateless_rewriter.dart index 0111f17..d2cd974 100644 --- a/packages/solid_generator/lib/src/stateless_rewriter.dart +++ b/packages/solid_generator/lib/src/stateless_rewriter.dart @@ -30,8 +30,11 @@ RewriteResult rewriteStatelessWidget( Map> classRegistry, Map> classCollectionFields, Map> classFieldTypes, - String source, -) { + String source, { + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, +}) { final className = classDecl.name.lexeme; final stateClassName = '_${className}State'; final reactiveNames = { @@ -110,6 +113,9 @@ RewriteResult rewriteStatelessWidget( widgetBoundFields: widgetBoundForBuild, collectionFields: collectionNames, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); final buildMethodText = buildRewrite.text; @@ -161,6 +167,9 @@ RewriteResult rewriteStatelessWidget( environmentFields: environmentFieldsMap, collectionFields: collectionNames, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, )}', ) .join('\n\n'); diff --git a/packages/solid_generator/lib/src/value_rewriter.dart b/packages/solid_generator/lib/src/value_rewriter.dart index 27916db..a7a392c 100644 --- a/packages/solid_generator/lib/src/value_rewriter.dart +++ b/packages/solid_generator/lib/src/value_rewriter.dart @@ -156,6 +156,22 @@ bool _isOnPrefixedCallbackName(String name) { /// inside the body is detected as a self-cycle and surfaced via /// [ValueRewriteResult.selfCycleFound]. Pass `null` for non-query callers /// (state getters, effects, build bodies). +/// +/// [classRegistryOrigins] and [classCollectionFieldsOrigins] (issue #110) +/// are the per-name origin-qualified counterparts of [classRegistry] / +/// [classCollectionFields] — `name -> originUri -> fields` — populated by +/// `builder.dart` ONLY for names it could not resolve unambiguously: two or +/// more distinct cross-file classes sharing a simple name, or a cross-file +/// class whose name is ALSO declared locally in the consuming file. +/// [classRegistryShadowedNames] is the exact set of such flagged names. +/// Every name NOT in that set is served from the flat, name-keyed maps +/// exactly as before this issue existed; a flagged name rewrites only when +/// the receiver's resolved `staticType` (tier 1) points at a library URI +/// matching one of its recorded origins — see +/// [_ValueRewriteVisitor._fieldsForCrossClassName] for the full invariant. +/// Empty (the default) for every caller that has no +/// origin data to offer, which degrades this exactly to the pre-#110 +/// name-only behavior. ValueRewriteResult collectValueEdits( AstNode node, Set reactiveFields, @@ -167,6 +183,9 @@ ValueRewriteResult collectValueEdits( Set widgetBoundFields = const {}, Set collectionFields = const {}, Map> classCollectionFields = const {}, + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, }) { final visitor = _ValueRewriteVisitor( reactiveFields, @@ -177,6 +196,9 @@ ValueRewriteResult collectValueEdits( widgetBoundFields, collectionFields, classCollectionFields, + classRegistryOrigins, + classCollectionFieldsOrigins, + classRegistryShadowedNames, ); node.accept(visitor); return ValueRewriteResult( @@ -210,6 +232,9 @@ String rewriteUserMethod( Map environmentFields = const {}, Set collectionFields = const {}, Map> classCollectionFields = const {}, + Map>> classRegistryOrigins = const {}, + Map>> classCollectionFieldsOrigins = const {}, + Set classRegistryShadowedNames = const {}, }) { final result = collectValueEdits( method, @@ -219,6 +244,9 @@ String rewriteUserMethod( environmentFields: environmentFields, collectionFields: collectionFields, classCollectionFields: classCollectionFields, + classRegistryOrigins: classRegistryOrigins, + classCollectionFieldsOrigins: classCollectionFieldsOrigins, + classRegistryShadowedNames: classRegistryShadowedNames, ); return applyEditsToRange( source.substring(method.offset, method.end), @@ -273,6 +301,48 @@ const Set _signalApiGetters = {'value', 'hasValue', 'previousValue'}; /// tracked to keep the enclosing widget subtree reactive to signal updates. const Set _trackedSignalApiGetters = {'hasValue', 'previousValue'}; +/// Result of [_ValueRewriteVisitor._resolveReceiverType]: a cross-class +/// receiver's simple type `name`, plus the declaring library's `libraryUri` +/// — normalized via [_normalizeLibraryUri] — ONLY when a real resolved +/// `staticType` (tier 1) backed the answer. `libraryUri == null` means the +/// name came from an AST-only fallback (tiers 2-4) and therefore carries no +/// proof of which same-named class it refers to; see +/// [_ValueRewriteVisitor._fieldsForCrossClassName] (issue #110). +typedef _ReceiverType = ({String name, String? libraryUri}); + +/// Normalizes a resolved `LibraryElement.uri` into the same string form +/// `builder.dart`'s `_registerWantedClassesFrom` stores as a cross-class +/// registry entry's origin — `package:/`. +/// +/// The two sides start from different vocabularies (issue #110 design +/// note). A resolved element from another package (Flutter, +/// flutter_solidart, a published dependency) already reports a `package:` +/// URI, which matches the registry's format outright — returned unchanged. +/// A resolved element from THIS package's own `source/` tree — the shape +/// every cross-file `@SolidState` class this generator processes takes — +/// reports an `asset:/source/` URI instead: `build_resolvers` +/// resolves the file at its PRE-transformation `source/` location, and +/// `source/` is never a real `lib/` directory, so the analyzer can't mint a +/// `package:` URI for it (verified empirically against this repo's own +/// `testBuilder` harness). Both sides name the exact same file; this +/// function rewrites the `asset:` form into the `package:` form — stripping +/// the `source/` segment, mirroring `builder.dart`'s own +/// `_sourceToLibAsset` — so the comparison in [_ValueRewriteVisitor. +/// _fieldsForCrossClassName] is apples-to-apples. Returns `null` for any +/// other shape (`dart:`, `file:`, or an `asset:` URI outside `source/`, none +/// of which the registry ever produces an origin for) — a `null` result can +/// never equal a recorded origin key, so it safely falls through to "no +/// match" rather than guessing. +String? _normalizeLibraryUri(Uri uri) { + if (uri.scheme == 'package') return uri.toString(); + if (uri.scheme != 'asset') return null; + final segments = uri.pathSegments; + if (segments.length < 3 || segments[1] != 'source') return null; + final package = segments.first; + final relativePath = segments.skip(2).join('/'); + return 'package:$package/$relativePath'; +} + /// AST visitor that accumulates [ValueEdit]s for reactive-field identifiers. /// /// Scope tracking is name-based: a local variable, parameter, or function @@ -292,6 +362,9 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { this._widgetBoundFields, this._collectionFields, this._classCollectionFields, + this._classRegistryOrigins, + this._classCollectionFieldsOrigins, + this._classRegistryShadowedNames, ); final Set _reactiveFields; @@ -350,6 +423,29 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { /// must NOT receive a `.value` append between `todos` and `length`. final Map> _classCollectionFields; + /// Per-name origin-qualified counterpart of [_classRegistry] (issue #110) + /// — `name -> originUri -> reactive field/getter names` — populated by + /// `builder.dart` ONLY for a name it flagged in + /// [_classRegistryShadowedNames]. Consulted by [_fieldsForCrossClassName] + /// instead of [_classRegistry] for exactly those flagged names; empty + /// (the default) whenever the caller has no origin data to thread + /// through, in which case a flagged name simply never resolves (safe: + /// [_classRegistry] itself holds no entry for it either — see + /// `builder.dart::_populateCrossFileTypes`'s finalize pass). + final Map>> _classRegistryOrigins; + + /// Per-name origin-qualified counterpart of [_classCollectionFields], + /// parallel to [_classRegistryOrigins]. + final Map>> _classCollectionFieldsOrigins; + + /// Names `builder.dart` could not resolve unambiguously by simple name + /// alone (issue #110) — either two-plus distinct cross-file classes share + /// the name, or a cross-file class's name is ALSO declared locally in the + /// file being rewritten. [_fieldsForCrossClassName] routes these names + /// through [_classRegistryOrigins] / [_classCollectionFieldsOrigins] with + /// a mandatory tier-1 library-URI match instead of the flat maps. + final Set _classRegistryShadowedNames; + final List edits = []; /// Tracked-read offsets keyed by signal name — drives the placement @@ -512,11 +608,17 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { // property=c), property=d); `getController().field` parses as // PropertyAccess(target=MethodInvocation, property=field). Both shapes // are caught here by resolving the receiver's `staticType` via - // [_resolveReceiverTypeName] and looking up the property name in + // [_resolveReceiverType] and looking up the property name in // [_classRegistry]. PrefixedIdentifier (the single-level `a.b` shape) // is handled by [_maybeRewriteCrossClass] above so the two paths don't // overlap. - if (_classRegistry.isNotEmpty && target != null) { + // A name issue #110 flagged as ambiguous is stripped from `_classRegistry` + // (see `builder.dart::_populateCrossFileTypes`'s finalize pass) — so an + // otherwise-`_classRegistry`-empty file whose ONLY cross-class candidate + // is such a flagged name would wrongly skip this branch entirely without + // also checking `_classRegistryShadowedNames`. + if ((_classRegistry.isNotEmpty || _classRegistryShadowedNames.isNotEmpty) && + target != null) { _maybeRewriteCrossClassPropertyAccess(node); } super.visitPropertyAccess(node); @@ -552,12 +654,16 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { } return; } - final declaredTypeName = _resolveReceiverTypeName(target); - if (declaredTypeName == null) return; - final fieldsOfType = _classRegistry[declaredTypeName]; - if (fieldsOfType == null) return; + final receiverType = _resolveReceiverType(target); + if (receiverType == null) return; + final resolved = _fieldsForCrossClassName( + receiverType.name, + receiverType.libraryUri, + ); + if (resolved == null) return; + final fieldsOfType = resolved.fields; if (!fieldsOfType.contains(node.propertyName.name)) return; - final collectionFieldsOfType = _classCollectionFields[declaredTypeName]; + final collectionFieldsOfType = resolved.collectionFields; final isCollection = collectionFieldsOfType != null && collectionFieldsOfType.contains(node.propertyName.name); @@ -598,7 +704,10 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { // still requires the resolved-AST migration. We keep this branch // conservative — only single `.` shapes — so the // existing same-class goldens stay byte-identical. - if (_classRegistry.isNotEmpty) { + // See the matching comment in [visitPropertyAccess]: a name issue #110 + // flagged as ambiguous is stripped from `_classRegistry`, so this guard + // must also open the door via `_classRegistryShadowedNames`. + if (_classRegistry.isNotEmpty || _classRegistryShadowedNames.isNotEmpty) { _maybeRewriteCrossClass(node); } super.visitPrefixedIdentifier(node); @@ -606,7 +715,7 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { /// Single-level `.` cross-class rewrite — the /// shipped slice of the chain-aware rule. The receiver type resolves via - /// [_resolveReceiverTypeName] (parameter / local / property-of-resolved-type) + /// [_resolveReceiverType] (parameter / local / property-of-resolved-type) /// then [_environmentFields] (`@SolidEnvironment` host-class field); /// parameter wins because method parameters shadow host-class fields in /// Dart and are not tracked by [_isShadowed] (which covers only @@ -652,14 +761,18 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { } return; } + final receiverType = _resolveReceiverType(node.prefix); final declaredTypeName = - _resolveReceiverTypeName(node.prefix) ?? - _environmentFields[node.prefix.name]; + receiverType?.name ?? _environmentFields[node.prefix.name]; if (declaredTypeName == null) return; - final fieldsOfType = _classRegistry[declaredTypeName]; - if (fieldsOfType == null) return; + final resolved = _fieldsForCrossClassName( + declaredTypeName, + receiverType?.libraryUri, + ); + if (resolved == null) return; + final fieldsOfType = resolved.fields; if (!fieldsOfType.contains(node.identifier.name)) return; - final collectionFieldsOfType = _classCollectionFields[declaredTypeName]; + final collectionFieldsOfType = resolved.collectionFields; final isCollection = collectionFieldsOfType != null && collectionFieldsOfType.contains(node.identifier.name); @@ -712,13 +825,20 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { bool _isCrossClassChainPrefix(PrefixedIdentifier node) => _isAnyChainTarget(node); - /// Returns the simple type name of [receiver], using four-tier resolution: + /// Returns the simple type name of [receiver] — using the same four-tier + /// resolution [_resolveReceiverType] documents — paired with the + /// declaring library's origin URI ONLY when tier 1 answered (see + /// [_ReceiverType]). /// /// 1. **Element-based.** When [Expression.staticType] is a resolved - /// [InterfaceType], return its element name. Catches locals - /// (`var c = controller; c.field`), method-call receivers - /// (`getController().field`), and parameters identically — `staticType` - /// is populated for every expression in resolved AST. + /// [InterfaceType], return its element name AND (issue #110) the + /// origin URI of the class's declaring library, normalized via + /// [_normalizeLibraryUri] into the same form `builder.dart`'s registry + /// origins use. Catches locals (`var c = controller; c.field`), + /// method-call receivers (`getController().field`), and parameters + /// identically — `staticType` is populated for every expression in + /// resolved AST. This is the ONLY tier that can serve a name flagged + /// in [_classRegistryShadowedNames] — see [_fieldsForCrossClassName]. /// 2. **AST fallback (parameters).** When the resolver hasn't run /// (parsed-AST fallback or test sandbox without the necessary SDK), /// resolve [receiver] as a method/function parameter declared with a @@ -727,7 +847,8 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { /// return `null` in this tier — a parameter name match with no /// resolvable type still counts as "matched" and short-circuits tier 3 /// (a parameter shadows a same-named field; falling through to the - /// field would rewrite the wrong receiver). + /// field would rewrite the wrong receiver). No library URI is ever + /// available here — this tier never has a resolved element to ask. /// 3. **AST fallback (instance fields).** When [receiver] is not a /// parameter at all (matched or not), resolve it as a non-static field /// of the enclosing [ClassDeclaration] declared with a [NamedType] — @@ -745,28 +866,82 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { /// names a field; that is its purpose, and it must resolve to the /// field even when a same-named parameter is in scope (unlike a bare /// reference, which the parameter legitimately shadows). - String? _resolveReceiverTypeName(Expression receiver) { + _ReceiverType? _resolveReceiverType(Expression receiver) { final type = receiver.staticType; - if (type is InterfaceType) return type.element.name; + if (type is InterfaceType) { + final elementName = type.element.name; + if (elementName == null) return null; + return ( + name: elementName, + libraryUri: _normalizeLibraryUri(type.element.library.uri), + ); + } if (receiver is SimpleIdentifier) { - if (_isParameterName(receiver)) { - return _resolveParameterTypeNameFromAst(receiver); - } - return _resolveInstanceFieldTypeNameFromAst(receiver); + final name = _isParameterName(receiver) + ? _resolveParameterTypeNameFromAst(receiver) + : _resolveInstanceFieldTypeNameFromAst(receiver); + return name == null ? null : (name: name, libraryUri: null); } if (receiver is PropertyAccess && receiver.target is ThisExpression) { - return _resolveInstanceFieldTypeNameByName( + final name = _resolveInstanceFieldTypeNameByName( receiver.propertyName.name, receiver, ); + return name == null ? null : (name: name, libraryUri: null); } return null; } + /// Field-set resolver for a `.` (or longer chain) + /// cross-class candidate whose receiver's declared type resolved to + /// [declaredTypeName], given the receiver's [libraryUri] — non-null ONLY + /// when [_resolveReceiverType]'s tier 1 (a real resolved `staticType`) + /// answered the question. + /// + /// SAFETY INVARIANT (issue #110): [_classRegistryShadowedNames] contains + /// EXACTLY the names `builder.dart` could not resolve unambiguously by + /// simple name alone — two or more distinct cross-file classes sharing + /// the name, or a cross-file class whose name is ALSO declared locally in + /// the file being rewritten (the shadowing scenario the issue exists to + /// fix — previously dropped from the registry entirely, silently losing + /// the foreign class's reactivity). A name NOT in that set is served + /// straight from the flat [_classRegistry] / [_classCollectionFields] — + /// byte-identical to every rewrite this generator already performed + /// before issue #110, no URI check involved. A FLAGGED name can only + /// resolve when [libraryUri] is non-null (tier 1) AND matches one of the + /// origins [_classRegistryOrigins] recorded for [declaredTypeName]: an + /// AST-only receiver (tiers 2-4, [libraryUri] `null`) or a resolved + /// receiver whose library matches none of the recorded origins returns + /// `null` — no rewrite, the same conservative outcome the pre-#110 + /// registry produced for this name (it held no entry for it at all). + /// Net effect: every rewrite that fired before issue #110 still fires + /// unchanged; a previously-blocked foreign read can newly fire, but ONLY + /// through a tier-1 library-URI match — no name-based guess is ever + /// upgraded into a rewrite without one. + ({Set fields, Set? collectionFields})? + _fieldsForCrossClassName(String declaredTypeName, String? libraryUri) { + if (!_classRegistryShadowedNames.contains(declaredTypeName)) { + final fields = _classRegistry[declaredTypeName]; + if (fields == null) return null; + return ( + fields: fields, + collectionFields: _classCollectionFields[declaredTypeName], + ); + } + if (libraryUri == null) return null; + final fields = _classRegistryOrigins[declaredTypeName]?[libraryUri]; + if (fields == null) return null; + return ( + fields: fields, + collectionFields: + _classCollectionFieldsOrigins[declaredTypeName]?[libraryUri], + ); + } + /// True if [prefix] names a parameter of the nearest enclosing /// [MethodDeclaration] / [FunctionExpression], regardless of whether that /// parameter's type is resolvable. Gates the tier-3 instance-field fallback - /// in [_resolveReceiverTypeName]: Dart scoping always prefers a parameter + /// in [_resolveReceiverType]: Dart scoping always prefers a parameter /// over a same-named field, so an unresolvable parameter type must produce /// "no rewrite" rather than silently resolving through the shadowed field. bool _isParameterName(SimpleIdentifier prefix) { @@ -787,7 +962,7 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { return false; } - /// AST-only parameter resolver — tier 2 of [_resolveReceiverTypeName]. + /// AST-only parameter resolver — tier 2 of [_resolveReceiverType]. /// Walks the prefix's enclosing [MethodDeclaration] / [FunctionExpression] /// for a matching [SimpleFormalParameter] and returns the declared /// [NamedType]'s lexeme. @@ -814,7 +989,7 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { return null; } - /// AST-only instance-field resolver — tier 3 of [_resolveReceiverTypeName]. + /// AST-only instance-field resolver — tier 3 of [_resolveReceiverType]. /// Covers the most common Flutter DI shape — a constructor-injected field /// (`final AuthRepository _authRepository;`) — that has no parameter /// counterpart. Callers gate this tier on [_isParameterName] returning @@ -823,7 +998,7 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { _resolveInstanceFieldTypeNameByName(prefix.name, prefix); /// Shared implementation of [_resolveInstanceFieldTypeNameFromAst] (tier 3) - /// and the `this.` branch (tier 4) of [_resolveReceiverTypeName]. + /// and the `this.` branch (tier 4) of [_resolveReceiverType]. /// Walks [anchor]'s enclosing [ClassDeclaration] for a matching non-static /// [FieldDeclaration] named [name] and returns the declared [NamedType]'s /// lexeme. Returns `null` when no field on the enclosing class matches diff --git a/packages/solid_generator/test/golden/inputs/cross_file_local_shadowing_decoy/main.dart b/packages/solid_generator/test/golden/inputs/cross_file_local_shadowing_decoy/main.dart index 9d57e6e..51068d3 100644 --- a/packages/solid_generator/test/golden/inputs/cross_file_local_shadowing_decoy/main.dart +++ b/packages/solid_generator/test/golden/inputs/cross_file_local_shadowing_decoy/main.dart @@ -16,6 +16,22 @@ // `remote_address.dart`'s reactive `Address.line1` to that name and // populated `classRegistry['Address']`, producing a non-compiling // `address.line1.value` rewrite against the local plain field. +// +// Issue #110 changed HOW this stays fixed, not WHETHER it does: the +// registry no longer drops the foreign `Address` — it records it QUALIFIED +// by origin (`classRegistryOrigins['Address'][''] +// = {line1}`) and flags the name in `classRegistryShadowedNames` because +// this file ALSO declares its own local `Address`. At rewrite time, +// `address`'s resolved `staticType` (this file is `@SolidState`-annotated, +// so `Shipping.describe()` runs on a fully resolved `CompilationUnit`) +// points at THIS file's own `Address` — a library URI that matches neither +// `remote_address.dart`'s recorded origin nor anything else — so +// `_ValueRewriteVisitor._fieldsForCrossClassName` finds no match and +// `address.line1` correctly stays un-rewritten. Verified empirically +// (temporary instrumentation, issue #110 implementation): the flagged-name +// path is what actually fires here — this is no longer "the registry never +// heard of `Address`" but "the registry heard of it and precisely ruled +// out this receiver." // ignore_for_file: unused_import import 'package:solid_annotations/solid_annotations.dart'; diff --git a/packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/foo_a.dart b/packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/foo_a.dart new file mode 100644 index 0000000..753168c --- /dev/null +++ b/packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/foo_a.dart @@ -0,0 +1,8 @@ +// One of two DIFFERENT `Foo` classes sharing a simple name across files +// (issue #110's "ultimate disambiguation proof" — see `main.dart`). +import 'package:solid_annotations/solid_annotations.dart'; + +class Foo { + @SolidState() + String? label; +} diff --git a/packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/foo_b.dart b/packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/foo_b.dart new file mode 100644 index 0000000..fe4b6df --- /dev/null +++ b/packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/foo_b.dart @@ -0,0 +1,8 @@ +// The OTHER `Foo` — same simple name as `foo_a.dart`'s, an unrelated class +// with an unrelated reactive field. See `main.dart`. +import 'package:solid_annotations/solid_annotations.dart'; + +class Foo { + @SolidState() + int count = 0; +} diff --git a/packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/main.dart b/packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/main.dart new file mode 100644 index 0000000..504b327 --- /dev/null +++ b/packages/solid_generator/test/golden/inputs/cross_file_qualified_registry_disambiguation/main.dart @@ -0,0 +1,38 @@ +// New fixture (issue #110): two DIFFERENT @SolidState classes, both named +// `Foo`, declared in DIFFERENT files (`foo_a.dart`, `foo_b.dart`), both +// reached from this PURE CONSUMER file via ordinary constructor injection — +// `foo_b.dart`'s `Foo` imported under a prefix so both bare and prefixed +// references coexist without a compile-time ambiguous-import error. +// +// `_populateCrossFileTypes`'s cross-file walk seeds a SINGLE 'Foo' entry +// into `wantedTypes` (the map is name-keyed, prefix-blind) from `Holder`'s +// two typed fields, then finds BOTH classes across this file's two imports +// — a genuine same-simple-name collision the registry could never safely +// resolve before issue #110 (SPEC §4.9's documented residual risk). Each +// match is recorded QUALIFIED by its own origin library into +// `classRegistryOrigins['Foo']`, and the name is flagged ambiguous in +// `classRegistryShadowedNames` — two distinct origins, no local shadow +// needed to trigger it this time. +// +// At rewrite time, `a`'s resolved `staticType` points at `foo_a.dart`'s +// `Foo`; `b`'s points at `foo_b.dart`'s `Foo` (the `foo_b.` prefix is a +// purely syntactic import-scoping device, invisible to the resolved +// element's OWN library URI). Each receiver's tier-1 URI matches ONLY its +// own class's recorded origin, so each read lowers against its own class's +// fields — the ultimate proof this is genuine per-origin disambiguation, +// not a lucky single-candidate default: `a.label` -> `a.label.value` +// (foo_a.dart's field), `b.count` -> `b.count.value` (foo_b.dart's field) — +// never the other way around. +import 'foo_a.dart'; +import 'foo_b.dart' as foo_b; + +class Holder { + Holder(this.a, this.b); + + final Foo a; + final foo_b.Foo b; + + String? readA() => a.label; + + int readB() => b.count; +} diff --git a/packages/solid_generator/test/golden/inputs/cross_file_super_param_one_hop_local_shadow/consumer.dart b/packages/solid_generator/test/golden/inputs/cross_file_super_param_one_hop_local_shadow/consumer.dart index 51ab04a..c11edf4 100644 --- a/packages/solid_generator/test/golden/inputs/cross_file_super_param_one_hop_local_shadow/consumer.dart +++ b/packages/solid_generator/test/golden/inputs/cross_file_super_param_one_hop_local_shadow/consumer.dart @@ -1,32 +1,43 @@ -// PURE CONSUMER regression fixture (issue #108 fix review, finding 1): -// this file declares its OWN plain `class Foo` (zero `@SolidState` -// members) — held by `Holder` through ordinary constructor injection — and -// ALSO extends `Base` (declared in `base.dart`) through a bare `super.x` -// constructor parameter. `Base` itself is reached directly (main walk), but -// `Base`'s OWN field `thing` is typed with a DIFFERENT, `@SolidState`- -// bearing `Foo` declared in `foreign_foo.dart` — a file this file never -// imports at all; only `base.dart` does. Locating that declaration needs -// the fix review finding 1 one-hop extension. +// PURE CONSUMER regression fixture (issue #108 fix review, finding 1; +// promoted to the PRIMARY fixture for issue #110): this file declares its +// OWN plain `class Foo` (zero `@SolidState` members) — held by `Holder` +// through ordinary constructor injection — and ALSO extends `Base` (declared +// in `base.dart`) through a bare `super.x` constructor parameter. `Base` +// itself is reached directly (main walk), but `Base`'s OWN field `thing` is +// typed with a DIFFERENT, `@SolidState`-bearing `Foo` declared in +// `foreign_foo.dart` — a file this file never imports at all; only +// `base.dart` does. Locating that declaration needs the fix review finding 1 +// one-hop extension. // -// Before the fix: the one-hop walk registered the FOREIGN `Foo`'s reactive -// `label` field into `classRegistry['Foo']` before this file's own local -// `Foo` had a chance to shadow it, so `Holder.f.label` — a read against the -// LOCAL plain class — was wrongly rewritten to `f.label.value` (BLOCKER: -// non-compiling output, `.value` on a plain `String?` field). +// Pre-#108-fix-review: the one-hop walk registered the FOREIGN `Foo`'s +// reactive `label` field into `classRegistry['Foo']` before this file's own +// local `Foo` had a chance to shadow it, so `Holder.f.label` — a read +// against the LOCAL plain class — was wrongly rewritten to `f.label.value` +// (BLOCKER: non-compiling output, `.value` on a plain `String?` field). // -// After the fix: the one-hop registration is gated against THIS file's own -// declared type names, same discipline as the main walk (issue #104) — so -// `classRegistry` never gains a `'Foo'` entry at all once this file -// declares its own local `Foo`, name-based collision with no library -// qualification being the design's documented residual risk (SPEC §4.9). -// `f.label` correctly stays un-rewritten (the BLOCKER is closed); `Bar`'s -// `thing.label` — genuinely reactive through the foreign `Foo` — ALSO stays -// un-rewritten as the accepted, conservative cost of a safe fix: this file -// already has a local `Foo` claim on that simple name, so the registry -// cannot safely attribute `label` to `thing` either, exactly the same -// trade-off `cross_file_local_shadowing_decoy` (issue #104) already -// accepts. Favoring "no wrong rewrite" over "no missed rewrite" is -// consistent with every other ambiguity-handling rule in this file. +// Pre-#110 (the fix that closed the BLOCKER above): the one-hop +// registration was gated against this file's own declared type names, so +// `classRegistry` never gained a `'Foo'` entry AT ALL once this file +// declared its own local `Foo` — `f.label` correctly stayed un-rewritten, +// but `Bar`'s `thing.label` — genuinely reactive through the foreign `Foo` +// — ALSO stayed un-rewritten, as the accepted conservative cost of a safe +// fix: the registry could not tell `f`'s `Foo` (this file) from `thing`'s +// `Foo` (foreign_foo.dart) by simple name alone. +// +// After issue #110: the registry stops dropping the foreign entry and +// instead records it QUALIFIED by origin library +// (`builder.dart::_registerWantedClassesFrom` → `classRegistryOrigins`). +// Both `f` and `thing` resolve through the RESOLVED path here (this is the +// no-annotation pure-consumer branch, which still runs against a +// `buildStep.resolver`-backed `CompilationUnit` — see `builder.dart:: +// _resolveUnit`), so `value_rewriter.dart`'s tier 1 sees each receiver's +// real `staticType`: `f`'s element library is THIS file (`consumer.dart` +// itself), `thing`'s element library is `foreign_foo.dart`. Comparing each +// against `Foo`'s recorded origin (`foreign_foo.dart` only) now +// DISAMBIGUATES precisely: `f.label` still never rewrites (`f`'s library +// doesn't match), and `thing.label` — previously blocked — now correctly +// lowers to `thing.label.value`. See `_ValueRewriteVisitor. +// _fieldsForCrossClassName` for the full safety invariant this depends on. import 'base.dart'; class Foo { diff --git a/packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/foo_a.g.dart b/packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/foo_a.g.dart new file mode 100644 index 0000000..1cb90e6 --- /dev/null +++ b/packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/foo_a.g.dart @@ -0,0 +1,11 @@ +import 'package:flutter_solidart/flutter_solidart.dart'; +import 'package:solid_annotations/solid_annotations.dart'; + +class Foo implements Disposable { + final label = Signal(null, name: 'label'); + + @override + void dispose() { + label.dispose(); + } +} diff --git a/packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/foo_b.g.dart b/packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/foo_b.g.dart new file mode 100644 index 0000000..60dd50b --- /dev/null +++ b/packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/foo_b.g.dart @@ -0,0 +1,11 @@ +import 'package:flutter_solidart/flutter_solidart.dart'; +import 'package:solid_annotations/solid_annotations.dart'; + +class Foo implements Disposable { + final count = Signal(0, name: 'count'); + + @override + void dispose() { + count.dispose(); + } +} diff --git a/packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/main.g.dart b/packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/main.g.dart new file mode 100644 index 0000000..0a02028 --- /dev/null +++ b/packages/solid_generator/test/golden/outputs/cross_file_qualified_registry_disambiguation/main.g.dart @@ -0,0 +1,38 @@ +// New fixture (issue #110): two DIFFERENT @SolidState classes, both named +// `Foo`, declared in DIFFERENT files (`foo_a.dart`, `foo_b.dart`), both +// reached from this PURE CONSUMER file via ordinary constructor injection — +// `foo_b.dart`'s `Foo` imported under a prefix so both bare and prefixed +// references coexist without a compile-time ambiguous-import error. +// +// `_populateCrossFileTypes`'s cross-file walk seeds a SINGLE 'Foo' entry +// into `wantedTypes` (the map is name-keyed, prefix-blind) from `Holder`'s +// two typed fields, then finds BOTH classes across this file's two imports +// — a genuine same-simple-name collision the registry could never safely +// resolve before issue #110 (SPEC §4.9's documented residual risk). Each +// match is recorded QUALIFIED by its own origin library into +// `classRegistryOrigins['Foo']`, and the name is flagged ambiguous in +// `classRegistryShadowedNames` — two distinct origins, no local shadow +// needed to trigger it this time. +// +// At rewrite time, `a`'s resolved `staticType` points at `foo_a.dart`'s +// `Foo`; `b`'s points at `foo_b.dart`'s `Foo` (the `foo_b.` prefix is a +// purely syntactic import-scoping device, invisible to the resolved +// element's OWN library URI). Each receiver's tier-1 URI matches ONLY its +// own class's recorded origin, so each read lowers against its own class's +// fields — the ultimate proof this is genuine per-origin disambiguation, +// not a lucky single-candidate default: `a.label` -> `a.label.value` +// (foo_a.dart's field), `b.count` -> `b.count.value` (foo_b.dart's field) — +// never the other way around. +import 'foo_a.dart'; +import 'foo_b.dart' as foo_b; + +class Holder { + Holder(this.a, this.b); + + final Foo a; + final foo_b.Foo b; + + String? readA() => a.label.value; + + int readB() => b.count.value; +} diff --git a/packages/solid_generator/test/golden/outputs/cross_file_super_param_one_hop_local_shadow/consumer.g.dart b/packages/solid_generator/test/golden/outputs/cross_file_super_param_one_hop_local_shadow/consumer.g.dart index 51ab04a..a38b7d6 100644 --- a/packages/solid_generator/test/golden/outputs/cross_file_super_param_one_hop_local_shadow/consumer.g.dart +++ b/packages/solid_generator/test/golden/outputs/cross_file_super_param_one_hop_local_shadow/consumer.g.dart @@ -1,32 +1,43 @@ -// PURE CONSUMER regression fixture (issue #108 fix review, finding 1): -// this file declares its OWN plain `class Foo` (zero `@SolidState` -// members) — held by `Holder` through ordinary constructor injection — and -// ALSO extends `Base` (declared in `base.dart`) through a bare `super.x` -// constructor parameter. `Base` itself is reached directly (main walk), but -// `Base`'s OWN field `thing` is typed with a DIFFERENT, `@SolidState`- -// bearing `Foo` declared in `foreign_foo.dart` — a file this file never -// imports at all; only `base.dart` does. Locating that declaration needs -// the fix review finding 1 one-hop extension. +// PURE CONSUMER regression fixture (issue #108 fix review, finding 1; +// promoted to the PRIMARY fixture for issue #110): this file declares its +// OWN plain `class Foo` (zero `@SolidState` members) — held by `Holder` +// through ordinary constructor injection — and ALSO extends `Base` (declared +// in `base.dart`) through a bare `super.x` constructor parameter. `Base` +// itself is reached directly (main walk), but `Base`'s OWN field `thing` is +// typed with a DIFFERENT, `@SolidState`-bearing `Foo` declared in +// `foreign_foo.dart` — a file this file never imports at all; only +// `base.dart` does. Locating that declaration needs the fix review finding 1 +// one-hop extension. // -// Before the fix: the one-hop walk registered the FOREIGN `Foo`'s reactive -// `label` field into `classRegistry['Foo']` before this file's own local -// `Foo` had a chance to shadow it, so `Holder.f.label` — a read against the -// LOCAL plain class — was wrongly rewritten to `f.label.value` (BLOCKER: -// non-compiling output, `.value` on a plain `String?` field). +// Pre-#108-fix-review: the one-hop walk registered the FOREIGN `Foo`'s +// reactive `label` field into `classRegistry['Foo']` before this file's own +// local `Foo` had a chance to shadow it, so `Holder.f.label` — a read +// against the LOCAL plain class — was wrongly rewritten to `f.label.value` +// (BLOCKER: non-compiling output, `.value` on a plain `String?` field). // -// After the fix: the one-hop registration is gated against THIS file's own -// declared type names, same discipline as the main walk (issue #104) — so -// `classRegistry` never gains a `'Foo'` entry at all once this file -// declares its own local `Foo`, name-based collision with no library -// qualification being the design's documented residual risk (SPEC §4.9). -// `f.label` correctly stays un-rewritten (the BLOCKER is closed); `Bar`'s -// `thing.label` — genuinely reactive through the foreign `Foo` — ALSO stays -// un-rewritten as the accepted, conservative cost of a safe fix: this file -// already has a local `Foo` claim on that simple name, so the registry -// cannot safely attribute `label` to `thing` either, exactly the same -// trade-off `cross_file_local_shadowing_decoy` (issue #104) already -// accepts. Favoring "no wrong rewrite" over "no missed rewrite" is -// consistent with every other ambiguity-handling rule in this file. +// Pre-#110 (the fix that closed the BLOCKER above): the one-hop +// registration was gated against this file's own declared type names, so +// `classRegistry` never gained a `'Foo'` entry AT ALL once this file +// declared its own local `Foo` — `f.label` correctly stayed un-rewritten, +// but `Bar`'s `thing.label` — genuinely reactive through the foreign `Foo` +// — ALSO stayed un-rewritten, as the accepted conservative cost of a safe +// fix: the registry could not tell `f`'s `Foo` (this file) from `thing`'s +// `Foo` (foreign_foo.dart) by simple name alone. +// +// After issue #110: the registry stops dropping the foreign entry and +// instead records it QUALIFIED by origin library +// (`builder.dart::_registerWantedClassesFrom` → `classRegistryOrigins`). +// Both `f` and `thing` resolve through the RESOLVED path here (this is the +// no-annotation pure-consumer branch, which still runs against a +// `buildStep.resolver`-backed `CompilationUnit` — see `builder.dart:: +// _resolveUnit`), so `value_rewriter.dart`'s tier 1 sees each receiver's +// real `staticType`: `f`'s element library is THIS file (`consumer.dart` +// itself), `thing`'s element library is `foreign_foo.dart`. Comparing each +// against `Foo`'s recorded origin (`foreign_foo.dart` only) now +// DISAMBIGUATES precisely: `f.label` still never rewrites (`f`'s library +// doesn't match), and `thing.label` — previously blocked — now correctly +// lowers to `thing.label.value`. See `_ValueRewriteVisitor. +// _fieldsForCrossClassName` for the full safety invariant this depends on. import 'base.dart'; class Foo { @@ -44,5 +55,5 @@ class Holder { class Bar extends Base { Bar(super.thing); - String? readThing() => thing.label; + String? readThing() => thing.label.value; } diff --git a/packages/solid_generator/test/integration/golden_helpers.dart b/packages/solid_generator/test/integration/golden_helpers.dart index 70a36f4..e0ce7bd 100644 --- a/packages/solid_generator/test/integration/golden_helpers.dart +++ b/packages/solid_generator/test/integration/golden_helpers.dart @@ -160,6 +160,7 @@ const List goldenNames = [ 'cross_file_super_param_bare_chain', 'cross_file_super_param_one_hop_local_shadow', 'cross_file_super_param_generic_base', + 'cross_file_qualified_registry_disambiguation', ]; /// Memoized golden directory resolution. Resolved relative to the package diff --git a/packages/solid_generator/test/value_rewriter_test.dart b/packages/solid_generator/test/value_rewriter_test.dart index affd209..2dfee97 100644 --- a/packages/solid_generator/test/value_rewriter_test.dart +++ b/packages/solid_generator/test/value_rewriter_test.dart @@ -125,4 +125,57 @@ class SessionGuard { }, ); }); + + group('cross-class name-collision guard (unresolved AST) — issue #110', () { + test( + 'a name builder.dart flagged ambiguous never rewrites through an ' + 'AST-only receiver, even when a matching qualified origin exists', + () { + const source = ''' +class Holder { + Holder(this.repo); + + final AuthRepository repo; + + bool hasSession() { + return repo.session != null; + } +} +'''; + final method = _method(source, 'Holder', 'hasSession'); + // `classRegistry` is empty here on purpose — mirrors what + // `builder.dart::_populateCrossFileTypes`'s finalize pass actually + // does to a flagged name (strips the flat entry; see that + // function's doc comment). `classRegistryOrigins` DOES carry a + // real, matching entry — `session` is exactly the field this body + // reads — so the only thing standing between this receiver and a + // (wrong, unproven) rewrite is the tier-1-URI-match requirement. + final result = collectValueEdits( + method, + const {}, + source, + classRegistryOrigins: const { + 'AuthRepository': { + 'package:app/auth_repository.dart': {'session'}, + }, + }, + classRegistryShadowedNames: const {'AuthRepository'}, + ); + + // `repo`'s type resolves via AST-only tier 2 (the parameter/field + // NamedType text) since this is an unresolved AST — no + // `Expression.staticType`, hence no library URI ever reaches + // `_fieldsForCrossClassName`. Per issue #110's conservative- + // fallback invariant, a FLAGGED name can only resolve through a + // tier-1 URI match; an AST-only tier must refuse regardless of how + // plausible the qualified entry looks. The golden harness cannot + // exercise this: `testBuilder`'s resolver always supplies a real + // `staticType` for a `final AuthRepository repo;` field, so tier 1 + // would answer for real and this specific guard would never be the + // reason a golden passes or fails — same "golden can't reach it" + // reasoning as this file's header comment. + expect(result.edits, isEmpty); + }, + ); + }); } From 605ab3fec10a63d3e6b61b567f967265cabcb129 Mon Sep 17 00:00:00 2001 From: Alexandru Mariuti Date: Wed, 26 Aug 2026 19:53:09 +0700 Subject: [PATCH 2/2] docs: condense changelog entries --- packages/solid_annotations/CHANGELOG.md | 2 +- packages/solid_generator/CHANGELOG.md | 19 ++++++------------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/packages/solid_annotations/CHANGELOG.md b/packages/solid_annotations/CHANGELOG.md index 534f32a..f99beef 100644 --- a/packages/solid_annotations/CHANGELOG.md +++ b/packages/solid_annotations/CHANGELOG.md @@ -1,6 +1,6 @@ ## 3.0.0-dev.2 -- **DOCS**: Update `WidgetEnvironment.environment()` doc comment — `solid_generator` now only auto-injects `dispose:` when the created type is recognized as needing it (has a provable `dispose()`, or is `@Solid*`-annotated — same-file or cross-file); omitting `dispose:` on a type provably without one injects nothing instead of a call that previously failed source-layer typechecking with a compile-time `undefined_method` error on `.dispose()`. +- **DOCS**: Update `WidgetEnvironment.environment()` doc comment for `solid_generator`'s type-aware `dispose:` auto-injection. ## 3.0.0-dev.1 diff --git a/packages/solid_generator/CHANGELOG.md b/packages/solid_generator/CHANGELOG.md index f81cbeb..c4def6a 100644 --- a/packages/solid_generator/CHANGELOG.md +++ b/packages/solid_generator/CHANGELOG.md @@ -1,27 +1,20 @@ ## 3.0.0-dev.5 -- **FIX**: The cross-file registry's name-keyed same-simple-name collision — documented as a residual risk in §4.9 and reproduced as an accepted trade-off in the `cross_file_super_param_one_hop_local_shadow` fixture (dev.4/#108) — is now resolved wherever real type resolution reaches the receiver (#110). Previously, when a file locally declared a plain class under the same simple name as a foreign, `@SolidState`-bearing class reached via constructor injection, the two shadow rules added for #104/#105/#108 (`wantedTypes.removeAll(declaredInUnit)` in the main walk, the mirrored `declaredInConsumingUnit` guard in the one-hop extension) dropped the foreign class from the registry ENTIRELY before it was ever looked up — correctly preventing the wrong rewrite on the local class, but at the cost of also losing the genuinely reactive foreign read, which stayed silently un-lowered with no error. `_populateCrossFileTypes`'s cross-file walk (`_registerWantedClassesFrom`, shared by the main walk and the one-hop extension) no longer removes a wanted name from consideration on either front: a match under a locally-shadowed name, or a second DISTINCT match for a name already resolved once, is now recorded QUALIFIED by origin library into two new side-maps, `classRegistryOrigins` / `classCollectionFieldsOrigins` (`name -> originUri -> fields`, the origin URI computed the same `_sourceToLibAsset(...).uri` way `crossClassFieldTypeOriginUris` already does). A finalize pass at the end of `_populateCrossFileTypes` then flags any name with two-plus recorded origins, or with a local declaration of that name in the current file, into a new `classRegistryShadowedNames` set — and, for exactly those flagged names, strips the flat `classRegistry` / `classCollectionFields` entry back out, so every OTHER existing consumer of those two maps (dispose auto-injection, the tier 2-4 AST-only rewrite paths) sees precisely the same flat map they always did for an unflagged name. `value_rewriter.dart`'s cross-class rewrite (`_ValueRewriteVisitor._fieldsForCrossClassName`) is the one new consumer of the qualified side-maps: an unflagged name resolves exactly as before (no URI check, byte-identical); a FLAGGED name resolves only when the receiver's tier-1 `Expression.staticType` (real semantic resolution — never available on the AST-only tiers 2-4) is present AND its declaring library's URI, normalized from the resolver's `asset:/source/` form into the registry's own `package:/` form (`_normalizeLibraryUri`), matches one of the flagged name's recorded origins. This closes the `cross_file_super_param_one_hop_local_shadow` gap exactly (`thing.label` now lowers to `thing.label.value` while `f.label` correctly stays untouched — both classes share the simple name `Foo`) and, more generally, lets two unrelated `@SolidState` classes sharing a simple name across different files each resolve correctly through their own constructor-injected receiver in the same consuming file (see the new `cross_file_qualified_registry_disambiguation` fixture). An AST-only receiver, or a flagged name whose resolved library matches none of its recorded origins, still never rewrites — the pre-#110 conservative default for anything the registry cannot prove. One deliberate side-effect on dispose auto-injection: a flagged name's flat `classRegistry` entry is stripped, so a class whose simple name has two-plus distinct cross-file origins (no local shadow needed) is no longer recognized as `@Solid*`-annotated by the dispose rewriter and gets NO auto-injected `dispose:` (the visible-plain clause applies) — previously it non-deterministically fired for whichever origin an import walk registered first. Pass `dispose: (context, provider) => provider.dispose()` explicitly when providing such a same-named class. - -- **FIX**: The dev.4 residual gap — a PURE CONSUMER whose ONLY link to a cross-file `@SolidState`-bearing class is a bare `super.x` constructor parameter (no type written at that position) — is now closed (#108), including issue #108's own primary example: a consumer that imports only its immediate superclass's file, never the file that actually declares the injected class. Detecting a pure consumer at all still requires the no-annotation fast path's UNRESOLVED syntactic probe, and a bare `super.x` still carries no type text at that position, but `_populateCrossFileTypes`'s seeding loop now resolves it from the AST alone: it reads the subclass's own `extends` clause for the superclass's simple name, locates that class's declaration (checking the current unit's own top-level declarations first — skipped when the `extends` clause is import-prefixed, since a prefixed reference can never resolve to a local declaration — then walking imports with the same shadowing/combinator/skip-set discipline the rest of the function already applies, PLUS two more: same-package only, since this package's `@SolidState` classes can never live in a cross-package file — critical in practice, since this same code path fires for `{super.key}` on every widget, and without the guard would otherwise chase `StatefulWidget`/`State` into `package:flutter`; and alias-aware, since an unprefixed name can only come from an unprefixed import and a prefixed name only from the import carrying that exact prefix — `_importExposesName`'s show/hide check alone can't tell a same-named decoy behind a DIFFERENT prefix from the real, unprefixed source), finds the super-constructor the subclass's initializer list targets (`super.named(...)` for a named constructor, else the implicit call to the unnamed one), and matches the bare parameter against that constructor's corresponding parameter — positionally among its other `super.` siblings (never ambiguous for code that compiles, since Dart itself rejects mixing a positional `super.x` with an explicit positional argument in the same invocation) or by name. When the matched parameter is itself an untyped `this.x` field formal, the same-named instance field's declared type is used instead; when it is ITSELF a further bare `super.x` (a chain of plain relaying subclasses forwarding the same parameter), the search recurses up the inheritance chain, up to five superclasses deep, before giving up. The syntactic walk is skipped entirely once the RESOLVED fallback (`_seedFromResolvedSuperFormal`, dev.3) already answered the question — running the full import-and-parse walk on top of a successful resolution was pure waste, and, unconditional, was the other half of the `{super.key}`-on-every-widget cost. Once the wanted class's simple name is known, locating ITS OWN declaration gets exactly one further hop beyond the import walk just described — through the file that hosts the superclass's own imports, same-package-only, never a general transitive walk: this is what closes issue #108's own primary example, where the injected class is declared in a file the pure consumer never imports at all, only its superclass's file does. A class reachable only through a LONGER chain of imports beyond that one extra hop remains a documented residual, not recognized. When the matched parameter's declared type is itself one of the superclass's OWN type parameters (`class Base { Base(this.repo); final T repo; }`), the wanted name is mapped BY INDEX to the subclass's own `extends Base` type argument, recursing through nested generics, instead of seeding the placeholder name verbatim — a bare type-parameter reference is a syntactically ordinary type name, so without this mapping it silently seeded a name (`T`) that matches no class anywhere and the real dependency (`AuthRepository`) was never sought at all (fix review addendum finding 2). When the superclass can't be located, the targeted constructor can't be pinned down, the matched parameter resolves to no usable type, or the type-parameter mapping has no type argument to resolve against, every candidate name from the located superclass (every constructor parameter, every instance field) is seeded rather than guessing wrong or skipping silently — over-seeding a name is harmless, since the registry only fills from classes that actually carry `@SolidState` members, though it does further widen the pre-existing same-simple-name registry-collision surface (§4.9) by proposing names that would otherwise never have been candidates for the file at all. The fix lives in two shared helpers, `_seedFromSyntacticSuperFormal` (the chain search) and `_populateCrossFileTypesOneHop` (the one-hop class-location extension, sharing its registration rule with the main cross-file walk via `_registerWantedClassesFrom`), both reached from `_populateCrossFileTypes`'s single seeding loop, so the probe and the main pipeline pick them up identically. +- **FIX**: Bare `super.x` constructor parameters now seed the cross-file registry: the superclass is located syntactically (alias-aware, same-package imports only), the matched constructor parameter's type is resolved (recursing through super-formal chains, mapping generic type parameters to the extends-clause type arguments), and the registry walk also searches the located superclass file's own imports one hop further (#108). +- **FIX**: Registry entries under a shadowed or multi-origin simple name are qualified by origin library and resolve only on a receiver's resolved-library match, so a local plain class and a foreign `@SolidState` class sharing a name each behave correctly; ambiguous names never rewrite through AST-only resolution (#110). Side-effect: dispose auto-injection no longer recognizes a name with multiple cross-file origins — pass `dispose:` explicitly for such classes. ## 3.0.0-dev.4 -- **FIX**: The builder's fast bailout for files with no `@Solid*` annotation and no `Provider(...)`/`.environment()` call site now runs the same cheap cross-file registry seeding `_populateCrossFileTypes` already implements (#104/#105) against the file's UNRESOLVED parsed unit before giving up. A PURE CONSUMER — a file that holds a `@SolidState`-bearing class through plain constructor injection or an instance field, but carries no `@Solid*` annotation and no provider call site of its own — previously matched neither hint text, so the file was never even parsed and the cross-file seeding never ran for it: every read through the injected field (`_authRepository.session`) stayed silently un-lowered. Reproduced in a real app as a generated authentication bypass, where `dart fix`'s `dead_code`/`unnecessary_null_comparison` passes collapsed a router guard's null check because the un-lowered field looked always-non-null (#106). Honest cost accounting: a hint-free file now always pays the syntactic parse (previously skipped entirely for such files); only a file with no custom-typed fields/params at all (just core-SDK/self-declared types) avoids the import walk and any resolver call, and that narrower path is what stays zero-cost — not "no work at all". A file whose field/param DOES name an externally-declared type pays a bounded import walk (bounded by the file's own import count), and a same-named-but-non-reactive match along the way (the `cross_file_constructor_injected_no_state` shape) does not stop that walk early — the probe's registries are threaded into the pipeline's own cross-file pass so the walk still only runs once per file. Lowering itself is applied by a new, narrower pass (`collectPureConsumerCrossFileEdits`) that only rewrites `.value` reads on plain-class members — it does not add `implements Disposable` or synthesize `dispose()`, since a pure consumer owns no reactive member of its own to dispose. The builder runs this lowering BEFORE the `Provider`/`.environment` auto-dispose pass, not after: lowering depends on `value_rewriter.dart`'s tier-1 `Expression.staticType` resolution to type receivers the AST-only tiers can't (e.g. a for-in loop variable), so it needs the still-valid resolved unit the dispose pass's text edits would otherwise invalidate; the dispose pass, by contrast, degrades gracefully to its AST-only tiers on a re-parsed, unresolved unit. Both shapes originally named here as residual gaps are now closed. A pure-consumer `StatelessWidget`/`State` gets the same `.value` lowering plus reactive rebuild through a sibling pass, `collectPureConsumerWidgetEdits`: it reuses `rewriteBuildMethod` verbatim on the class's `build()` method — the exact SignalBuilder-wrap-placement machinery an `@SolidEnvironment`-consuming widget's `build()` already goes through — with no widget-class Stateless→Stateful lift, since a plain constructor-injected field needs no `BuildContext` (unlike `@SolidEnvironment`, which needs `context.read()`). The plain-class pass and the widget pass are unified by `lowerPureConsumers`: both collect their edits against the SAME pristine source text and resolved unit, and the two edit lists are merged and applied together in a single text transformation — safe because the two passes visit disjoint class-kind sets (`plainClass` vs `statelessWidget`/`stateClass`), so their edit ranges never overlap. (An earlier draft of this fix ran the two passes sequentially, re-parsing the widget pass's OUTPUT for the plain-class pass whenever the widget pass had edited anything — which silently dropped resolved-unit-dependent receiver resolution, e.g. a static-holder read, for a plain-class guard sharing a file with a widget pure consumer; fixed before release by collecting both edit sets up front instead of reordering.) Static-field-mediated DI (`static final AuthRepository instance = …;`, consumed elsewhere as `Holder.instance.session`) is also now recognized: the `_populateCrossFileTypes` DI-seeding loop's `if (member.isStatic) continue;` guard carried no stated rationale anywhere in the #104/#105 history — it simply mirrored `@SolidState`'s own unrelated instance-only restriction — and is removed. Receiver resolution needed no new code at all: `Holder.instance.session` parses as a `PropertyAccess` whose target (`Holder.instance`) already resolves through the existing tier-1 `Expression.staticType` check to `AuthRepository` once the type is seeded, verified empirically against `package:analyzer` directly. The `flutter_solidart` import needed for a widget pure consumer's `SignalBuilder` wrap is spliced in — or, if the file already imports `flutter_solidart` with a `show`/`hide` combinator that doesn't expose `SignalBuilder` (e.g. a pre-existing `show Signal` for an unrelated hand-rolled signal), REPAIRED in place — based on the lowering pass's own report of whether it emitted a wrap, not a substring scan for `SignalBuilder(` in the assembled output (which both risked a false positive on a preserved source comment and couldn't distinguish an import that exposes the name from one that doesn't). - -Two narrower, adjacent gaps close partially in the same pass. The bare-`super.x` gap from dev.3 (below) is now closed on the RESOLVED path only: a file that already enters the main lowering pipeline for another reason (its own `@Solid*` annotation, or a `Provider`/`.environment()` call site) consults `SuperFormalParameter.declaredFragment.element.type` when the AST carries no explicit type annotation, seeding `wantedTypes` the same as if the type had been written out — verified empirically that the resolved element's type IS populated even without a source-level annotation. This is moot for a PURE consumer whose only link to the cross-file class is a bare `super.x`: such a file carries no annotation and no provider hint, so it takes the no-annotation fast path's UNRESOLVED syntactic probe first, finds nothing to seed, and short-circuits to a verbatim copy before any resolved unit is ever requested — that narrower shape remains a known, accepted gap. Separately, `value_rewriter.dart`'s cross-class receiver resolution gained a fourth fallback tier: when an instance field is declared with no type annotation at all (`final _service;` — infers `dynamic`, since a same-named constructor field-formal parameter's explicit type does NOT propagate back onto the field's inferred type, confirmed empirically against a real resolved unit), the resolver now falls back to a matching `FieldFormalParameter`'s explicit type, or an initializer-list-assigned `SimpleFormalParameter`'s explicit type; multiple constructors supplying conflicting types bail to unresolved rather than guess. +- **FIX**: Files with no `@Solid*` annotation and no provider call site now enter the pipeline when they read cross-file `@SolidState` state — previously they passed through verbatim, leaving reads un-lowered (and `dart fix` could collapse the resulting always-non-null guards into dead code) (#106). Pure-consumer plain classes get `.value` lowering with no `Disposable` synthesis; pure-consumer widgets get the same `SignalBuilder`-wrapped `build()` as `@SolidEnvironment` widgets, with imports repaired combinator-aware. Also covered: static-field holders, untyped fields typed via constructor parameters, and collection receivers (`.first`, for-in). `part` files remain outside the pipeline. ## 3.0.0-dev.3 -- **FIX**: The cross-file class registry (`_populateCrossFileTypes`) now seeds `wantedTypes` from the declared type names of every class's instance fields and constructor parameters, not just `@SolidEnvironment` field types and `Provider(...)`/`.environment()` call sites. A file that only *constructor-receives* a `@SolidState`-bearing class — the plain DI shape, e.g. `CustomersRepository({required AuthRepository authRepository})` storing `final AuthRepository _authRepository;`, with no `@SolidEnvironment` field and no same-file `.environment()`/`Provider()` call site — previously left `classRegistry` empty for that type, so cross-class `.value` reads through it (`_authRepository.session`) were silently un-lowered: no compile error, just an always-non-null `Signal` object that `dart fix`'s `unnecessary_null_comparison` could collapse into dead code (#104). -- **FIX**: The seeding above (and, defensively, every other seeding path into `wantedTypes`) now mirrors Dart's own name-resolution rules instead of blindly matching on simple class name. A simple name that the CURRENT file itself declares as a class/enum/mixin is dropped from the wanted set before the cross-file import walk starts — a local top-level declaration always shadows a same-name import, so attributing an unrelated imported class's reactive members to it was provably wrong (e.g. a file with its own plain `class Address` plus an unrelated, unconnected `@SolidState`-annotated `class Address` imported from elsewhere previously emitted a non-compiling `address.line1.value` against the local class). Each import's `show`/`hide` combinators are also now honored: an import that hides the wanted name, or `show`s a list that excludes it, can no longer be credited as that name's source, closing a second, narrower collision window. -- **FIX**: Constructor parameters using the explicit-typed `super.` shorthand (`Foo(AuthRepository super.repo)`) now seed `wantedTypes` the same as a plain or field-formal parameter. The far more common *bare* `super.repo` (no type written) is a known, accepted gap: the type isn't present in the source at that position at all — recovering it would require resolving `repo` against the superclass's matching field/parameter, which this syntactic AST walk does not do — so a bare-shorthand `super.` parameter still isn't a seeding source. -- **FIX**: A field or constructor parameter declared with a generic collection type (`final List repos;`) now seeds `wantedTypes` from every type argument, at every nesting level, not just the outer container name (`List`). This lets the existing resolved-static-type rewrite tiers recognize collection-derived receivers whose element type is Solid-lowered — covered by golden fixtures for a `for (final r in repos) { r.field }` loop variable and a `repos.first.field` receiver (same resolved-type mechanism); deeper nesting (`Map>` and beyond) is mechanically identical but likewise untested. Container names themselves (`List`, `Map`, `String`, and other `dart:core`/`dart:async` SDK types) are now filtered out of every seed in the new constructor-injection/field loop before being added, which also fixes a performance regression where annotation-blind seeding of primitive types on nearly every field defeated the `wantedTypes.isEmpty` fast-path. +- **FIX**: The cross-file registry is seeded from instance-field and constructor-parameter declared types (generic type arguments included), so `@SolidState` reads through plain constructor-injected references lower across files (#104). Locally declared names shadow imports, `show`/`hide` combinators are honored, and `dart:core`/`dart:async` names are skipped. ## 3.0.0-dev.2 -- **FIX**: Cross-class `.value` rewrite now resolves constructor-injected instance fields (`final AuthRepository _authRepository;`), not just method/function parameters and `@SolidEnvironment` fields. Previously a bare instance field receiver silently kept its unlowered form, producing always-true null checks and compile errors against the unboxed `Signal` payload. Also covers a `this.`-prefixed receiver (`this._authRepository.session`) — `this.` parses as a distinct AST shape from the bare `_authRepository.session` form and previously fell through unrewritten. -- **FIX**: `.environment()` / `Provider(...)` dispose auto-injection is now type-aware — `dispose: (context, provider) => provider.dispose()` is injected per a four-tier decision: (1) the created type provably has `dispose()` (own declaration, or inherited — including transitively through a same-file base-class chain); (2) the created type is `@Solid*`-annotated, same-file OR cross-file (every Solid-lowered class synthesizes `dispose()`); (3) the type's declaration is visible and shows neither → skip, no injection; (4) the declaration isn't visible anywhere this check looked (typically a cross-file, non-`@Solid*` type) → inject anyway, preserving the pre-type-aware default so a wrong guess fails loudly at compile time rather than silently leaking a resource, with `dispose: null` as the explicit opt-out. Previously the injection was unconditional and made source-layer typechecking fail with a compile-time `undefined_method` error on `.dispose()` for any type with no `dispose()` method; omitting `dispose:` for such a type now injects nothing (same as an explicit `dispose: null`) instead of a load-bearing `dispose: null` workaround. This changes generated output for existing call sites that omit `dispose:` on a dispose-less type — the compile error goes away. -- **FIX**: The dispose auto-injection above now also recognizes a cross-file `@Solid*`-annotated type provided via `.environment()` / `Provider(...)` even when nothing in the providing file consumes `T` through an `@SolidEnvironment` field. Previously such a controller — the dominant real-world shape, e.g. a top-level `main()` that provides a controller it never itself consumes — got NO dispose injection at all: its synthesized `dispose()` only exists after lowering, which was invisible to every check this rewriter had. This was a silent resource leak, not a compile error, because the call site was already valid Dart (`dispose:` simply absent). +- **FIX**: Cross-class `.value` rewriting resolves constructor-injected instance fields (including `this.`-prefixed receivers), not just method parameters and `@SolidEnvironment` fields. +- **FIX**: `.environment()`/`Provider(...)` dispose auto-injection is type-aware: injected when the created type provably has `dispose()` or is `@Solid*`-annotated (same- or cross-file), skipped for provably plain types, and kept as the loud compile-time default for unresolvable types (`dispose: null` opts out). ## 3.0.0-dev.1