diff --git a/SPEC.md b/SPEC.md index 5104a03..6662dc9 100644 --- a/SPEC.md +++ b/SPEC.md @@ -1109,7 +1109,7 @@ When a generated piece of code (anything under `lib/`) references a reactive val ### 5.1 Identifier rewrite -The rewrite is **type-driven and chain-aware**: any expression position whose resolved static type is `SignalBase` or a subtype (`Signal`, `Computed`, `ReadSignal`) from `package:flutter_solidart` receives a `.value` append at that position. The rule applies uniformly to: +The rewrite is **type-driven and chain-aware**: any expression position whose resolved static type is `SignalBase` or a subtype (`Signal`, `Computed`, `ReadSignal`) receives a `.value` append at that position. `SignalBase` and its subtypes are declared in `package:solidart`; `package:flutter_solidart` re-exports them, and the analyzer reports the declaring library regardless of which package the source imports from. The rule applies uniformly to: - Bare `SimpleIdentifier`s — same-class same-class `@SolidState` reads (M1 case). - `PrefixedIdentifier` and `PropertyAccess` chains of arbitrary depth — cross-class reactive reads (M6 case, e.g. an `@SolidEnvironment` field whose type carries `@SolidState` declarations). diff --git a/packages/integration_tests/lib/foreign_signal_app.dart b/packages/integration_tests/lib/foreign_signal_app.dart new file mode 100644 index 0000000..82cd10e --- /dev/null +++ b/packages/integration_tests/lib/foreign_signal_app.dart @@ -0,0 +1,52 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_solidart/flutter_solidart.dart'; +import 'package:provider/provider.dart'; + +class Gate { + /// Tri-state: `null` means "not known yet". + final Signal complete = Signal(null); + + // `complete` is hand-written, so the generator synthesizes no disposal for + // it — the owner disposes it, and the provider scope's `dispose:` callback + // calls through to here. + void dispose() => complete.dispose(); +} + +class GateView extends StatefulWidget { + const GateView({super.key}); + + @override + State createState() => _GateViewState(); +} + +class _GateViewState extends State { + late final gate = context.read(); + late final ready = Computed( + () => gate.complete.value ?? false, + name: 'ready', + ); + + @override + void dispose() { + ready.dispose(); + super.dispose(); + } + + @override + Widget build(BuildContext context) { + return Column( + children: [ + SignalBuilder( + builder: (context, child) { + return Text('raw: ${gate.complete.value}'); + }, + ), + SignalBuilder( + builder: (context, child) { + return Text('ready: ${ready.value}'); + }, + ), + ], + ); + } +} diff --git a/packages/integration_tests/source/foreign_signal_app.dart b/packages/integration_tests/source/foreign_signal_app.dart new file mode 100644 index 0000000..b17273f --- /dev/null +++ b/packages/integration_tests/source/foreign_signal_app.dart @@ -0,0 +1,45 @@ +// End-to-end source for the foreign-signal reactivity test: a `Gate` holding +// a hand-written `flutter_solidart` `Signal` (NOT `@SolidState` — the +// generator does not manage it) reached through an `@SolidEnvironment` +// receiver. +// +// Both read sites are covered: `build()` reads `gate.complete.value` +// directly and must get a `SignalBuilder` wrap, and the `@SolidState` getter +// `ready` derives from the same foreign signal and must lower to a +// `Computed`. Unlike the golden fixture of the same shape, `flutter_solidart` +// resolves here, so this exercises the resolved-`staticType` tier and the +// multi-level `..value` receiver the AST fallback cannot reach. + +import 'package:flutter/material.dart'; +import 'package:flutter_solidart/flutter_solidart.dart'; +import 'package:solid_annotations/solid_annotations.dart'; + +class Gate { + /// Tri-state: `null` means "not known yet". + final Signal complete = Signal(null); + + // `complete` is hand-written, so the generator synthesizes no disposal for + // it — the owner disposes it, and the provider scope's `dispose:` callback + // calls through to here. + void dispose() => complete.dispose(); +} + +class GateView extends StatelessWidget { + GateView({super.key}); + + @SolidEnvironment() + late Gate gate; + + @SolidState() + bool get ready => gate.complete.value ?? false; + + @override + Widget build(BuildContext context) { + return Column( + children: [ + Text('raw: ${gate.complete.value}'), + Text('ready: $ready'), + ], + ); + } +} diff --git a/packages/integration_tests/test/foreign_signal_test.dart b/packages/integration_tests/test/foreign_signal_test.dart new file mode 100644 index 0000000..46000e8 --- /dev/null +++ b/packages/integration_tests/test/foreign_signal_test.dart @@ -0,0 +1,60 @@ +// Runtime fence for reads of a FOREIGN solidart signal — a hand-written +// `Signal` the generator does not manage, reached through an +// `@SolidEnvironment` receiver (`gate.complete.value`). +// +// Before the type-aware reactivity rule the generator counted only its own +// `@SolidState` / `@SolidQuery` members as reactive dependencies, so this +// shape produced a `build()` with NO `SignalBuilder` — the widget read the +// signal outside any tracking context and silently never rebuilt — and a +// `@SolidState` getter deriving from the same signal was rejected outright +// with "has no reactive dependencies". +// +// `Gate` and `GateView` are imported from the generator-lowered +// `package:integration_tests/foreign_signal_app.dart`, so the widget under +// test is the real production output of +// `packages/integration_tests/source/foreign_signal_app.dart`. Unlike the +// paired golden of the same shape, `flutter_solidart` resolves here, which +// makes this the coverage for the resolved-`staticType` tier and for the +// multi-level receiver the AST fallback cannot reach. + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:integration_tests/foreign_signal_app.dart'; +import 'package:solid_annotations/solid_annotations.dart'; + +void main() { + testWidgets('mutating a foreign Signal rebuilds both the direct build() ' + 'read and the @SolidState getter derived from it', (tester) async { + final gate = Gate(); + + await tester.pumpWidget( + MaterialApp( + home: const GateView().environment( + (_) => gate, + dispose: (_, g) => g.dispose(), + ), + ), + ); + + expect(find.text('raw: null'), findsOneWidget); + expect(find.text('ready: false'), findsOneWidget); + + gate.complete.value = true; + await tester.pump(); + + expect( + find.text('raw: true'), + findsOneWidget, + reason: + 'the direct `gate.complete.value` read in build() must be ' + 'wrapped in a SignalBuilder', + ); + expect( + find.text('ready: true'), + findsOneWidget, + reason: + 'the @SolidState getter deriving from the foreign signal must ' + 'lower to a Computed that re-runs', + ); + }); +} diff --git a/packages/solid_generator/CHANGELOG.md b/packages/solid_generator/CHANGELOG.md index 9194f0b..67bc580 100644 --- a/packages/solid_generator/CHANGELOG.md +++ b/packages/solid_generator/CHANGELOG.md @@ -1,3 +1,9 @@ +## 3.0.0-dev.11 + +- **FIX**: A read of a signal the generator does NOT manage — a hand-written `Signal` / `Computed` / `Resource` held in a plain field or reached through an `@SolidEnvironment` receiver, e.g. `gate.complete.value` — now counts as a reactive dependency. Reactivity analysis previously recognized only the generator's own `@SolidState` / `@SolidQuery` members, so a `build()` reading a foreign signal got NO `SignalBuilder` and silently never rebuilt, and a `@SolidState` getter deriving from one was rejected with "has no reactive dependencies" instead of lowering to a `Computed`. +- Detection mirrors the `@SolidEnvironment` validator's two tiers: the receiver's resolved `staticType` reaching solidart's `SignalBase`, falling back to the declared type's lexeme when the resolver hasn't run. Every read form is covered — `.value`, `.hasValue`, `.previousValue`, `.hasPreviousValue`, the callable form (`sig()` / `gate.complete()`), and `Resource.state` (the non-deprecated read; upstream `Resource.value` is deprecated). A non-signal receiver (`controller.value`) stays untracked, `.value` on a generator-managed member keeps its existing opt-out meaning, and the documented opt-outs — `on*` callbacks, `untracked(…)`, and `.untrackedValue` — are unchanged. On a foreign signal the `.untracked` getter opts out of the `SignalBuilder` wrap in `build()`, but it is not rewritten to `.untrackedValue`, so a `.untracked` read inside a `Computed` / `@SolidEffect` body still subscribes at runtime. +- Scope: a file the generator never touches at all (no `solid_annotations` import, no provider call site, no cross-file connection to a registered class) still passes through verbatim — this fix only reaches foreign-signal reads inside files the generator already processes for some other reason. `@SolidQuery` also stays asymmetric: a query body reading a foreign signal gets the same tracked-read/correctness treatment as any other reactive body, but — unlike a same-class or cross-class `@SolidState` dependency — the generator does not synthesize a `Resource.source:` from it. + ## 3.0.0-dev.10 - **FIX**: A pure-consumer class that reaches a registered class's reactive members through a plain instance/constructor field (the common Flutter DI shape) now lowers even when it is co-located with annotated classes in the same file. Whole-file pure-consumer lowering only ran when NO class in the file was annotated; in a mixed file the consumer was passed through verbatim, so a cross-instance `@SolidState` write (`vm.field = x`) hit `assignment_to_final` on the generated `final Signal`, and a read stayed non-reactive. The no-annotation branch now applies the same per-class `.value` lowering + `SignalBuilder` wrap. diff --git a/packages/solid_generator/lib/src/annotation_reader.dart b/packages/solid_generator/lib/src/annotation_reader.dart index ee93705..5a158cb 100644 --- a/packages/solid_generator/lib/src/annotation_reader.dart +++ b/packages/solid_generator/lib/src/annotation_reader.dart @@ -35,22 +35,6 @@ const String solidQueryName = 'SolidQuery'; /// as [solidStateName]. const String solidEnvironmentName = 'SolidEnvironment'; -/// Lexemes of the `flutter_solidart` types whose runtime classes extend -/// `SignalBase`. Matched against the type-annotation lexeme as the -/// unresolved-AST fallback in -/// `target_validator._isSignalBaseTyped`; the Element-based primary path -/// walks `InterfaceType.allSupertypes` when the resolver populated the -/// type. Consumed by the target validator (rejecting -/// `@SolidEnvironment` fields typed as one of these) and by the -/// cross-class `.value` rewrite. Excludes `SignalBuilder` / -/// `SolidartConfig` (those are non-`SignalBase` solidart names). -const Set signalBaseTypeNames = { - 'Signal', - 'Computed', - 'Effect', - 'Resource', -}; - /// Lexeme of the `Future` return-type identifier on a `@SolidQuery` method. /// Matched textually on the return-type's [NamedType] lexeme — the user /// must import `dart:async` (or its re-export via `dart:core`) to write the diff --git a/packages/solid_generator/lib/src/element_utils.dart b/packages/solid_generator/lib/src/element_utils.dart index e09ba88..e8f4bc2 100644 --- a/packages/solid_generator/lib/src/element_utils.dart +++ b/packages/solid_generator/lib/src/element_utils.dart @@ -1,3 +1,4 @@ +import 'package:analyzer/dart/element/element.dart'; import 'package:analyzer/dart/element/type.dart'; /// `true` iff [uri] is a `package:/...` URI. @@ -31,3 +32,41 @@ bool supertypeChainContains( } return false; } + +/// Lexemes of the well-known `flutter_solidart` reactive-primitive types +/// rejected on an `@SolidEnvironment` field / matched as a foreign-signal +/// receiver. Most (`Signal`, `Computed`, `Resource`) extend `SignalBase`; +/// `Effect` does NOT (it `implements ReactionInterface`) but is included +/// here as the same kind of generator-adjacent primitive an env field +/// shouldn't hold directly. Matched against a type-annotation lexeme as the +/// unresolved-AST fallback in `target_validator._isSignalBaseTyped` and +/// `value_rewriter._isSolidartSignalReceiver`; the Element-based primary +/// path — [isSolidartSignalType], which genuinely checks the `SignalBase` +/// supertype chain — does not need this exception and correctly excludes +/// `Effect`. Excludes `SignalBuilder` / `SolidartConfig` (non-reactive +/// solidart names). +const Set signalBaseTypeNames = { + 'Signal', + 'Computed', + 'Effect', + 'Resource', +}; + +/// `true` iff [type] is `SignalBase` or a subtype of it. +/// +/// `SignalBase` and its subtypes are declared in `package:solidart`; +/// `package:flutter_solidart` re-exports them. The declaring library — not +/// the importing one — is what an Element reports, so both package names are +/// accepted. +bool isSolidartSignalType(InterfaceType type) { + if (_isSignalBaseElement(type.element)) return true; + for (final supertype in type.allSupertypes) { + if (_isSignalBaseElement(supertype.element)) return true; + } + return false; +} + +bool _isSignalBaseElement(InterfaceElement element) => + element.name == 'SignalBase' && + (isFromPackage(element.library.uri, 'solidart') || + isFromPackage(element.library.uri, 'flutter_solidart')); diff --git a/packages/solid_generator/lib/src/target_validator.dart b/packages/solid_generator/lib/src/target_validator.dart index 0aac28b..c0b7350 100644 --- a/packages/solid_generator/lib/src/target_validator.dart +++ b/packages/solid_generator/lib/src/target_validator.dart @@ -355,9 +355,8 @@ void _validateEnvironmentField(FieldDeclaration field, String className) { } // `SignalBase`-typed environment field detection, two-tier: // 1. Element-based: when the resolver populated the type, walk - // `allSupertypes` for a `SignalBase` class anchored to the - // `package:flutter_solidart/` library URI. Catches aliased imports - // and user-defined subclasses of `SignalBase`. + // `allSupertypes` for solidart's `SignalBase` class. Catches aliased + // imports and user-defined subclasses of `SignalBase`. // 2. Textual fallback: when the resolver hasn't run (parsed-AST // fallback or test sandboxes), match the type name's lexeme against // [signalBaseTypeNames]. The user-defined subclass case is missed @@ -368,23 +367,12 @@ void _validateEnvironmentField(FieldDeclaration field, String className) { } } -/// `true` iff [type]'s resolved supertype chain contains a `SignalBase` -/// declared in `package:flutter_solidart/`, or — when the resolver hasn't -/// produced a type — the type's lexeme is in [signalBaseTypeNames]. +/// `true` iff [type] resolves to solidart's `SignalBase` or a subtype of it +/// ([isSolidartSignalType]), or — when the resolver hasn't produced a type — +/// the type's lexeme is in [signalBaseTypeNames]. bool _isSignalBaseTyped(NamedType type) { final resolved = type.type; - if (resolved is InterfaceType) { - final element = resolved.element; - if (element.name == 'SignalBase' && - isFromPackage(element.library.uri, 'flutter_solidart')) { - return true; - } - return supertypeChainContains( - resolved.allSupertypes, - 'SignalBase', - packageName: 'flutter_solidart', - ); - } + if (resolved is InterfaceType) return isSolidartSignalType(resolved); // Type unresolved (parsed-AST fallback / test sandbox). Fall back to // lexeme match against the well-known set. return signalBaseTypeNames.contains(type.name.lexeme); diff --git a/packages/solid_generator/lib/src/value_rewriter.dart b/packages/solid_generator/lib/src/value_rewriter.dart index c41edab..0f175f3 100644 --- a/packages/solid_generator/lib/src/value_rewriter.dart +++ b/packages/solid_generator/lib/src/value_rewriter.dart @@ -2,6 +2,7 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/element/type.dart'; import 'package:solid_generator/src/ast_compat.dart'; +import 'package:solid_generator/src/element_utils.dart'; import 'package:solid_generator/src/query_model.dart'; /// A single value-level edit emitted by [collectValueEdits]. @@ -303,6 +304,33 @@ String applyEditsToRange(String text, List edits, int baseOffset) { /// (`untracked(() => ...)`). Must stay in sync with those declarations. const String _untrackedName = 'untracked'; +/// Namespace prefix for a foreign-signal read key in +/// [ValueRewriteResult.trackedReadNamesByOffset] — see [_foreignReadKey]. +/// +/// A generator-managed key is a `@SolidState` / `@SolidQuery` member name, +/// i.e. a bare Dart identifier, which can never contain `:`. Prefixing the +/// foreign keys therefore keeps the two name-spaces disjoint, which is what +/// keeps Section 7.6 ("different signals always keep both wraps") true when +/// a foreign signal's source text happens to spell a managed member's name. +const String _foreignReadKeyPrefix = 'foreign:'; + +/// Canonical [ValueRewriteResult.trackedReadNamesByOffset] key for a read of +/// the foreign signal [receiver]: the receiver's source text, with any +/// leading `this.` stripped (`this.gate` and `gate` name one signal, and the +/// placement pass's same-signal collapse must see them as one), under the +/// [_foreignReadKeyPrefix] namespace. +String _foreignReadKey(Expression receiver) { + final source = receiver.toSource(); + const thisPrefix = 'this.'; + final canonical = source.startsWith(thisPrefix) + ? source.substring(thisPrefix.length) + : source; + return '$_foreignReadKeyPrefix$canonical'; +} + +/// True if [name] is a [_foreignReadKey] rather than a managed member name. +bool _isForeignReadKey(String name) => name.startsWith(_foreignReadKeyPrefix); + /// Name of the runtime opt-out getter on `ReadableSignal` from /// `flutter_solidart`. Reading via this getter never subscribes the /// surrounding reactive context. @@ -335,13 +363,50 @@ const Set _queryRetainedStateGetterNames = { /// bare tracked-field access followed by any of them must skip the `.value` /// append. A type-driven rewriter would derive this from the resolved /// `staticType` of the access; in name-set mode we enumerate. +/// +/// Deliberately NARROWER than [_foreignSignalApiGetters]. This set gates the +/// no-double-append guard on a generator-MANAGED member, whose receiver is +/// typed as the user's own payload type, not as a signal — so every name +/// added here also stops being reachable as a plain property of that payload +/// type. `state` is the concrete casualty: `@SolidState MyMachine machine;` +/// plus `machine.state` must still lower to `machine.value.state`. const Set _signalApiGetters = {'value', 'hasValue', 'previousValue'}; +/// Signal-API getter names counted as an observing read on a FOREIGN signal +/// — one the generator does not manage, so the receiver's own static type is +/// a `SignalBase` and no payload-type member can collide. Consulted ONLY by +/// [_ValueRewriteVisitor._maybeRecordForeignSignalRead], which checks the +/// receiver's type first. +/// +/// Every name is observation-causing: `hasPreviousValue` and `previousValue` +/// evaluate `value` internally before answering, and `Resource.state` reads +/// the underlying signal's value (it is also the non-deprecated spelling of +/// a Resource read — `Resource.value` is deprecated upstream). +const Set _foreignSignalApiGetters = { + 'value', + 'hasValue', + 'previousValue', + 'hasPreviousValue', + 'state', +}; + /// Subset of [_signalApiGetters] whose access counts as a tracked read for -/// SignalBuilder placement. `.value` is excluded — by convention, an -/// explicit `.value` read is the user opting out of the auto-tracking flow, -/// while `.hasValue` / `.previousValue` have no bare equivalent and must be -/// tracked to keep the enclosing widget subtree reactive to signal updates. +/// SignalBuilder placement **on a generator-managed member** (a same-class +/// `@SolidState` field/getter, or a cross-class one reached through the +/// registry). `.value` is excluded there — the bare identifier IS the tracked +/// read for those members, so an explicit `.value` on top of one is the user +/// opting out of the auto-tracking flow, while `.hasValue` / `.previousValue` +/// have no bare equivalent and must be tracked to keep the enclosing widget +/// subtree reactive. +/// +/// This exclusion does NOT extend to a signal the generator does not manage +/// (a hand-written `Signal` field, injected or held directly): there `.value` +/// is the ONLY way to read it, so treating it as an opt-out would leave every +/// such read silently non-reactive. Those reads are tracked by +/// [_ValueRewriteVisitor._maybeRecordForeignSignalRead] over the full +/// [_foreignSignalApiGetters] set; the documented opt-outs +/// (`untracked(() => …)`, `.untracked`, `on*` callbacks, and solidart's own +/// `.untrackedValue`) still apply. const Set _trackedSignalApiGetters = {'hasValue', 'previousValue'}; /// Result of [_ValueRewriteVisitor._resolveReceiverType]: a cross-class @@ -640,6 +705,22 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { super.visitMethodInvocation(node); } + @override + void visitFunctionExpressionInvocation(FunctionExpressionInvocation node) { + // Callable read of a foreign signal: `SignalBase.call() => value`, so + // `sig()` / `gate.complete()` observes exactly like `sig.value`. The + // analyzer's resolver rewrites ANY zero-arg call of a non-method element + // (a field, parameter, or local variable — never a real method, which + // stays a [MethodInvocation]) into this node, `function` holding the + // callee expression whose `staticType` is the signal — true uniformly + // for both the bare (`sig()`) and property-chain (`gate.complete()`) + // shapes, so no separate [visitMethodInvocation] handling is needed. + if (node.argumentList.arguments.isEmpty) { + _recordForeignSignalRead(node.offset, node.function); + } + super.visitFunctionExpressionInvocation(node); + } + /// True if [node] is a cross-instance `.()` call: a /// zero-arg `MethodInvocation` with a target (never bare — that shape is /// [_isQueryShape]'s same-class branch above), not shadowed when the @@ -756,6 +837,14 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { target != null) { _maybeRewriteCrossClassPropertyAccess(node); } + // `.value` where `` is a foreign solidart signal — + // the multi-level shape (`..value`). Runs after + // the registry-backed branch above, which owns the offset when the + // receiver is a registered `@SolidState` holder rather than a signal. + // The single-level shape is handled in [visitPrefixedIdentifier]. + if (target != null) { + _maybeRecordForeignSignalRead(node, target, node.propertyName); + } super.visitPropertyAccess(node); } @@ -886,9 +975,104 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { if (_classRegistry.isNotEmpty || _classRegistryShadowedNames.isNotEmpty) { _maybeRewriteCrossClass(node); } + // `.value` where `` is a foreign solidart signal held + // directly (a field, parameter, or `this.`) — the single-level + // counterpart of the [visitPropertyAccess] branch. + _maybeRecordForeignSignalRead(node, node.prefix, node.identifier); super.visitPrefixedIdentifier(node); } + /// Records a tracked read for `.` (any name in + /// [_foreignSignalApiGetters]) when [receiver] is statically a signal the + /// generator does NOT manage — a hand-written `Signal` / `Computed` / `Resource` held in a + /// plain field, a parameter, or reached through an `@SolidEnvironment` + /// receiver. + /// + /// No source edit is emitted: the user already wrote the `.value` the + /// lowered code needs. Only the offset matters, and it matters twice — + /// `build()` needs it for `SignalBuilder` placement (without a wrap the + /// read happens outside any tracking context, so the widget silently never + /// rebuilds), and `annotation_reader._readReactiveBody` needs it so a + /// `@SolidState` getter / `@SolidEffect` whose only dependency is a foreign + /// signal is not rejected as dependency-free. + /// + /// Deliberately NOT recorded in [trackedReadNames] / + /// [trackedCrossClassReadNames]: those drive `@SolidQuery`'s + /// `Resource.source:` synthesis, which needs the dep's declared element + /// type from the class-field-type registry — and that registry only records + /// `@SolidState` members. A `@SolidQuery` body reading a foreign signal + /// keeps its current behaviour (no synthesized `source:`). + /// + /// The recorded name is [_foreignReadKey] of the receiver, so the placement + /// pass's same-signal nested-wrap collapse still groups repeat reads of one + /// signal while keeping distinct receivers — and every managed member name + /// — distinct. + void _maybeRecordForeignSignalRead( + Expression node, + Expression receiver, + SimpleIdentifier property, + ) { + if (!_foreignSignalApiGetters.contains(property.name)) return; + // Writes (plain and compound) never subscribe — Section 6.0. + if (property.inSetterContext()) return; + _recordForeignSignalRead(node.offset, receiver); + } + + /// Shared tail of the foreign-signal read paths (property reads via + /// [_maybeRecordForeignSignalRead], callable reads via + /// [visitFunctionExpressionInvocation]). [receiver] is the expression + /// whose static type must be the signal. + void _recordForeignSignalRead(int offset, Expression receiver) { + if (_untrackedDepth != 0) return; + // Section 6.4 opt-out: `.untracked` is an identity getter, so the + // receiver still types as the signal — only the spelling says "do not + // subscribe". + if (_isUntrackedOptOut(receiver)) return; + // Another branch already claimed this offset (a registry-backed + // cross-class read); do not overwrite its name. + if (trackedReadNamesByOffset.containsKey(offset)) return; + if (!_isSolidartSignalReceiver(receiver)) return; + _recordTrackedRead(offset, _foreignReadKey(receiver)); + } + + /// True if [receiver] is a `<…>.untracked` chain — the Section 6.4 opt-out + /// spelling. `UntrackedExtension` is an identity on `T`, so the chain's + /// static type is still the signal's; only the trailing property name + /// distinguishes it. + bool _isUntrackedOptOut(Expression receiver) { + if (receiver is PropertyAccess) { + return receiver.propertyName.name == _untrackedName; + } + if (receiver is PrefixedIdentifier) { + return receiver.identifier.name == _untrackedName; + } + return false; + } + + /// True if [receiver]'s static type is a solidart `SignalBase` subtype. + /// + /// Two tiers, mirroring `target_validator._isSignalBaseTyped`: the resolved + /// [InterfaceType] is authoritative when the resolver populated it (so a + /// non-signal receiver like `TextEditingController` is rejected outright, + /// keeping `controller.value` untracked, and a local alias of a signal — + /// `final alias = bag.sig;` — is correctly recognized); otherwise the + /// declared type name [_resolveReceiverType] recovers from the AST is + /// matched against [signalBaseTypeNames]. + /// + /// The [_isShadowed] guard applies to the lexeme tier ONLY: there the name + /// is all we have, so a local of the same name must suppress the match. A + /// resolved `staticType` already accounts for shadowing — it IS the local's + /// type — so consulting the shadow set there would drop legitimate aliases. + bool _isSolidartSignalReceiver(Expression receiver) { + final type = receiver.staticType; + if (type is InterfaceType) return isSolidartSignalType(type); + if (receiver is SimpleIdentifier && _isShadowed(receiver.name)) { + return false; + } + final name = _resolveReceiverType(receiver)?.name; + return name != null && signalBaseTypeNames.contains(name); + } + /// Single-level `.` cross-class rewrite — the /// shipped slice of the chain-aware rule. The receiver type resolves via /// [_resolveReceiverType] (parameter / local / property-of-resolved-type) @@ -1403,7 +1587,16 @@ class _ValueRewriteVisitor extends RecursiveAstVisitor { /// `trackedReadNamesByOffset.keys` in the same order as a parallel list /// would emit. Every caller is in a branch that already verified /// `_untrackedDepth == 0`. + /// + /// A [_foreignReadKey] already recorded at [offset] is never overwritten: + /// the registry-backed branches can reach the SAME offset as a foreign read + /// (`env.sig.hasValue` starts at `env` on both paths) and would replace the + /// collision-proof key with a bare member name, re-opening the Section 7.6 + /// mis-collapse the namespace exists to prevent. First foreign write wins; + /// the symmetric guard lives in [_recordForeignSignalRead]. void _recordTrackedRead(int offset, String name) { + final existing = trackedReadNamesByOffset[offset]; + if (existing != null && _isForeignReadKey(existing)) return; trackedReadNamesByOffset[offset] = name; } diff --git a/packages/solid_generator/pubspec.yaml b/packages/solid_generator/pubspec.yaml index b528bea..2923fa0 100644 --- a/packages/solid_generator/pubspec.yaml +++ b/packages/solid_generator/pubspec.yaml @@ -1,6 +1,6 @@ name: solid_generator description: Solid source-to-lib code generator for Flutter reactive state. -version: 3.0.0-dev.10 +version: 3.0.0-dev.11 homepage: https://solid.mariuti.com repository: https://github.com/nank1ro/solid issue_tracker: https://github.com/nank1ro/solid/issues diff --git a/packages/solid_generator/test/golden/inputs/foreign_signal_name_collision.dart b/packages/solid_generator/test/golden/inputs/foreign_signal_name_collision.dart new file mode 100644 index 0000000..729c1dd --- /dev/null +++ b/packages/solid_generator/test/golden/inputs/foreign_signal_name_collision.dart @@ -0,0 +1,56 @@ +// A foreign `flutter_solidart` signal whose SOURCE TEXT collides with the +// name of a generator-managed `@SolidState` member read by the SAME build +// method. `Collide.gate` is a hand-written `Signal`; `Model.gate` is an +// `@SolidState` field reached cross-instance. Both reads are tracked, the +// managed one anchors the outer `Tooltip` wrap and the foreign one the inner +// `Text` wrap — so unless the two occupy disjoint key name-spaces, the +// Section 7.5 same-signal collapse sees the inner wrap's name-set as a +// subset of the outer's and prunes it, leaving the foreign signal with no +// tracking context at all (silently no rebuild). +// +// `NoCollide` is the control: identical shape, foreign signal renamed, and +// its inner wrap must survive either way. Section 7.6 — different signals +// always keep both wraps — is what both classes assert. +// +// `flutter_solidart` is not a dependency of the generator package, so +// `Signal` is unresolved here and the rewriter matches the declared type's +// lexeme (the AST-fallback tier). +// ignore_for_file: undefined_class, undefined_identifier + +import 'package:flutter/material.dart'; +import 'package:solid_annotations/solid_annotations.dart'; + +class Model { + @SolidState() + int gate = 0; +} + +class Collide extends StatelessWidget { + const Collide({required this.model, required this.gate, super.key}); + + final Model model; + final Signal gate; + + @override + Widget build(BuildContext context) { + return Tooltip( + message: 'managed: ${model.gate}', + child: Text('foreign: ${gate.value}'), + ); + } +} + +class NoCollide extends StatelessWidget { + const NoCollide({required this.model, required this.other, super.key}); + + final Model model; + final Signal other; + + @override + Widget build(BuildContext context) { + return Tooltip( + message: 'managed: ${model.gate}', + child: Text('foreign: ${other.value}'), + ); + } +} diff --git a/packages/solid_generator/test/golden/inputs/foreign_signal_value_read.dart b/packages/solid_generator/test/golden/inputs/foreign_signal_value_read.dart new file mode 100644 index 0000000..45c7c66 --- /dev/null +++ b/packages/solid_generator/test/golden/inputs/foreign_signal_value_read.dart @@ -0,0 +1,112 @@ +// Reads of a FOREIGN `flutter_solidart` signal — a hand-written `Signal` +// the generator does not manage, injected rather than declared +// `@SolidState`. Both read sites must count as reactive dependencies: +// +// * `GateModel.ready` lowers to a `Computed`. Its only dependency is the +// injected signal, so before the type-aware rule it was rejected with +// "getter 'ready' has no reactive dependencies". +// * `GateBanner.build` gets a `SignalBuilder` wrap. Without it the widget +// silently never rebuilds — the `.value` read subscribes nothing because +// no tracking context is open around it. +// +// The remaining classes cover the other read FORMS of the same signal: +// +// * `GateCall` — the callable form. `SignalBase.call() => value`, so +// `complete()` observes exactly like `complete.value` and is the only +// read in that build; it must still produce a wrap. `GateModel +// .calledReady` reads the same callable form from a `Computed` body +// instead of `build()` — see its own comment. +// * `GateProbe` — `hasPreviousValue` and `Resource.state`, both of which +// evaluate the underlying value internally and therefore observe. +// * `GateOptOut` — the Section 6.4 `.untracked` spelling, which must NOT +// be wrapped. (The read is left verbatim: rewriting a foreign +// `.untracked` to solidart's `.untrackedValue` primitive is not +// implemented, so the opt-out holds in `build()` — no tracking context is +// opened around it — but not inside a `Computed` / `Effect` body.) +// +// `flutter_solidart` is not a dependency of the generator package, so +// `Signal` is unresolved in the golden sandbox and the rewriter falls back +// to matching the declared type's lexeme — the same two-tier rule +// `signalbase_typed.dart` exercises on the validator side. The resolved +// (`staticType`) tier and the multi-level `..value` receiver +// shape are fenced end-to-end by +// `packages/integration_tests/test/foreign_signal_test.dart`. +// ignore_for_file: undefined_class, undefined_identifier + +import 'package:flutter/material.dart'; +import 'package:solid_annotations/solid_annotations.dart'; + +class GateModel { + GateModel(this.complete); + + final Signal complete; + + @SolidState() + bool get ready => complete.value == true; + + // The callable form read from a `Computed` body (as opposed to `GateCall` + // below, the same form read from `build()`): both must count as tracked + // reads, driving different mechanisms (`Resource.source:`-style dep wiring + // here vs. `SignalBuilder` placement there). + @SolidState() + bool get calledReady => complete() == true; +} + +class GateBanner extends StatelessWidget { + const GateBanner({required this.complete, super.key}); + + final Signal complete; + + @override + Widget build(BuildContext context) { + return Column( + children: [ + const Text('gate'), + Text('raw: ${complete.value}'), + ], + ); + } +} + +class GateCall extends StatelessWidget { + const GateCall({required this.complete, super.key}); + + final Signal complete; + + @override + Widget build(BuildContext context) { + return Text('call: ${complete()}'); + } +} + +class GateProbe extends StatelessWidget { + const GateProbe({ + required this.complete, + required this.resource, + super.key, + }); + + final Signal complete; + final Resource resource; + + @override + Widget build(BuildContext context) { + return Column( + children: [ + Text('hadPrevious: ${complete.hasPreviousValue}'), + Text('state: ${resource.state}'), + ], + ); + } +} + +class GateOptOut extends StatelessWidget { + const GateOptOut({required this.complete, super.key}); + + final Signal complete; + + @override + Widget build(BuildContext context) { + return Text('opt-out: ${complete.untracked.value}'); + } +} diff --git a/packages/solid_generator/test/golden/inputs/two_effects_ordering.dart b/packages/solid_generator/test/golden/inputs/two_effects_ordering.dart index cea57e0..e6b06c2 100644 --- a/packages/solid_generator/test/golden/inputs/two_effects_ordering.dart +++ b/packages/solid_generator/test/golden/inputs/two_effects_ordering.dart @@ -1,3 +1,7 @@ +// The effect bodies below use `print` to make ordering observable in the +// lowered output; that's the point of this fixture, not a mistake. +// ignore_for_file: avoid_print + import 'package:solid_annotations/solid_annotations.dart'; // Two `@SolidEffect` methods on one class: locks in that both the field diff --git a/packages/solid_generator/test/golden/outputs/foreign_signal_name_collision.g.dart b/packages/solid_generator/test/golden/outputs/foreign_signal_name_collision.g.dart new file mode 100644 index 0000000..7f55ef2 --- /dev/null +++ b/packages/solid_generator/test/golden/outputs/foreign_signal_name_collision.g.dart @@ -0,0 +1,58 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_solidart/flutter_solidart.dart'; +import 'package:solid_annotations/solid_annotations.dart'; + +class Model implements Disposable { + final gate = Signal(0, name: 'gate'); + + @override + void dispose() { + gate.dispose(); + } +} + +class Collide extends StatelessWidget { + const Collide({required this.model, required this.gate, super.key}); + + final Model model; + final Signal gate; + + @override + Widget build(BuildContext context) { + return SignalBuilder( + builder: (context, child) { + return Tooltip( + message: 'managed: ${model.gate.value}', + child: SignalBuilder( + builder: (context, child) { + return Text('foreign: ${gate.value}'); + }, + ), + ); + }, + ); + } +} + +class NoCollide extends StatelessWidget { + const NoCollide({required this.model, required this.other, super.key}); + + final Model model; + final Signal other; + + @override + Widget build(BuildContext context) { + return SignalBuilder( + builder: (context, child) { + return Tooltip( + message: 'managed: ${model.gate.value}', + child: SignalBuilder( + builder: (context, child) { + return Text('foreign: ${other.value}'); + }, + ), + ); + }, + ); + } +} diff --git a/packages/solid_generator/test/golden/outputs/foreign_signal_value_read.g.dart b/packages/solid_generator/test/golden/outputs/foreign_signal_value_read.g.dart new file mode 100644 index 0000000..52dc1dd --- /dev/null +++ b/packages/solid_generator/test/golden/outputs/foreign_signal_value_read.g.dart @@ -0,0 +1,96 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_solidart/flutter_solidart.dart'; +import 'package:solid_annotations/solid_annotations.dart'; + +class GateModel implements Disposable { + GateModel(this.complete); + + final Signal complete; + + late final ready = Computed( + () => complete.value == true, + name: 'ready', + ); + + late final calledReady = Computed( + () => complete() == true, + name: 'calledReady', + ); + + @override + void dispose() { + calledReady.dispose(); + ready.dispose(); + } +} + +class GateBanner extends StatelessWidget { + const GateBanner({required this.complete, super.key}); + + final Signal complete; + + @override + Widget build(BuildContext context) { + return Column( + children: [ + const Text('gate'), + SignalBuilder( + builder: (context, child) { + return Text('raw: ${complete.value}'); + }, + ), + ], + ); + } +} + +class GateCall extends StatelessWidget { + const GateCall({required this.complete, super.key}); + + final Signal complete; + + @override + Widget build(BuildContext context) { + return SignalBuilder( + builder: (context, child) { + return Text('call: ${complete()}'); + }, + ); + } +} + +class GateProbe extends StatelessWidget { + const GateProbe({required this.complete, required this.resource, super.key}); + + final Signal complete; + final Resource resource; + + @override + Widget build(BuildContext context) { + return Column( + children: [ + SignalBuilder( + builder: (context, child) { + return Text('hadPrevious: ${complete.hasPreviousValue}'); + }, + ), + SignalBuilder( + builder: (context, child) { + return Text('state: ${resource.state}'); + }, + ), + ], + ); + } +} + +class GateOptOut extends StatelessWidget { + const GateOptOut({required this.complete, super.key}); + + final Signal complete; + + @override + Widget build(BuildContext context) { + return Text('opt-out: ${complete.untracked.value}'); + } +} diff --git a/packages/solid_generator/test/integration/golden_helpers.dart b/packages/solid_generator/test/integration/golden_helpers.dart index 3e855c3..3e6c003 100644 --- a/packages/solid_generator/test/integration/golden_helpers.dart +++ b/packages/solid_generator/test/integration/golden_helpers.dart @@ -171,6 +171,8 @@ const List goldenNames = [ 'cross_file_super_param_one_hop_local_shadow', 'cross_file_super_param_generic_base', 'cross_file_qualified_registry_disambiguation', + 'foreign_signal_value_read', + 'foreign_signal_name_collision', ]; /// Memoized golden directory resolution. Resolved relative to the package diff --git a/packages/solid_generator/test/resolved/signal_detection_fixture.dart b/packages/solid_generator/test/resolved/signal_detection_fixture.dart new file mode 100644 index 0000000..4308dce --- /dev/null +++ b/packages/solid_generator/test/resolved/signal_detection_fixture.dart @@ -0,0 +1,58 @@ +// Real (resolved) fixture for `resolved_signal_detection_test.dart`. +// +// Lives on disk, not as an inline string, because both scenarios below need +// a GENUINE `package:flutter_solidart` resolution — `resolveFile` only gets +// that from a path inside the pub workspace's shared `package_config.json`. +// `solid_generator`'s own `pubspec.yaml` does not depend on +// `flutter_solidart`, but the workspace (`resolution: workspace` at the repo +// root) resolves one shared `package_config.json` covering every member — +// `integration_tests` depends on `flutter_solidart` for real, so a file +// physically under this package's own `test/` directory resolves it too. +// +// * `ResolvedEnvironmentHost.counter` — a `@SolidEnvironment` field typed +// `Signal` must be rejected. `Signal`'s resolved supertype chain +// reaches `SignalBase`, whose DECLARING library is `package:solidart` +// (`flutter_solidart` only re-exports it) — the case a check anchored to +// `packageName: 'flutter_solidart'` can never match in a real (resolved) +// build, only in this package's own unresolved test sandbox where the +// lexeme fallback papers over it. +// * `readAlias` — `final alias = bag.sig; return alias.value;` reads a +// foreign signal through a LOCAL ALIAS. The resolved `staticType` of +// `alias` is `Signal` regardless of the local's own name, so this +// read must be tracked even though `alias` is itself declared in the +// enclosing scope (and would trip a shadow guard that ran before +// consulting the resolved type). +// * `readOptOut` — `bag.sig.untracked.value` (the Section 6.4 opt-out +// spelling) must NOT be tracked. `solid_annotations`'s +// `UntrackedExtension on T` is an identity getter generic over every +// type, so `bag.sig.untracked`'s resolved `staticType` is still +// `Signal` — only the trailing `.untracked` property name says "do +// not subscribe". This receiver shape (a chain through a generic +// extension getter) is also NOT reachable via the golden fixture's +// unresolved-AST fallback tier, unlike `readAlias`. +// +// `flutter_solidart` is deliberately not a declared dependency of this +// package (see above) — that's the whole point of the fixture, so the lint +// is expected here. +// ignore_for_file: depend_on_referenced_packages + +import 'package:flutter_solidart/flutter_solidart.dart'; +import 'package:solid_annotations/solid_annotations.dart'; + +class ResolvedEnvironmentHost { + @SolidEnvironment() + late Signal counter; +} + +class SignalBag { + final Signal sig = Signal(0); +} + +int readAlias(SignalBag bag) { + final alias = bag.sig; + return alias.value; +} + +int readOptOut(SignalBag bag) { + return bag.sig.untracked.value; +} diff --git a/packages/solid_generator/test/resolved_signal_detection_test.dart b/packages/solid_generator/test/resolved_signal_detection_test.dart new file mode 100644 index 0000000..b16229f --- /dev/null +++ b/packages/solid_generator/test/resolved_signal_detection_test.dart @@ -0,0 +1,141 @@ +// Resolved-`staticType`-tier coverage for two SignalBase-detection fixes +// that solid_generator's own `testBuilder` sandbox (`golden_test.dart`, +// `value_rewriter_test.dart`'s unresolved suite) cannot isolate, because +// both bugs live entirely inside the `resolved is InterfaceType` branch of +// their respective functions: +// +// * `target_validator._isSignalBaseTyped` used to anchor its resolved-tier +// `SignalBase` match to `packageName: 'flutter_solidart'`, but +// `SignalBase`'s declaring library is always `package:solidart` (which +// `flutter_solidart` only re-exports) — so in a REAL resolved build the +// resolved tier could never match, silently letting +// `@SolidEnvironment late Signal x;` through unrejected. Fixed by +// routing through `element_utils.isSolidartSignalType`, which checks +// both package names. +// * `value_rewriter._isSolidartSignalReceiver`'s shadow guard used to run +// BEFORE the resolved-type check, and a local variable's own name is +// always present in its own declaration scope — so `final alias = +// bag.sig; alias.value` was always treated as "shadowed" and never +// tracked as a reactive read, even though the resolved type proves +// `alias` is a signal. Fixed by moving the resolved-type check first and +// scoping the shadow guard to the lexeme-fallback tier only. +// +// `resolved/signal_detection_fixture.dart` is a real file on disk (not an +// inline string) so `resolveFile` resolves `package:flutter_solidart` for +// real, through the pub workspace's shared `package_config.json` — see that +// file's header comment for why a physical path is required. + +import 'dart:io'; +import 'dart:isolate'; + +import 'package:analyzer/dart/analysis/results.dart'; +import 'package:analyzer/dart/analysis/utilities.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:solid_generator/src/target_validator.dart'; +import 'package:solid_generator/src/transformation_error.dart'; +import 'package:solid_generator/src/value_rewriter.dart'; +import 'package:test/test.dart'; + +/// Resolved relative to the package root via `Isolate.resolvePackageUri` — +/// the same CWD-independent mechanism as `integration/golden_helpers.dart`. +/// CI invokes `dart test packages/solid_generator/` from the REPO ROOT, so a +/// `Directory.current`-relative path points at a file that does not exist, +/// and `resolveFile` answers with an EMPTY resolved unit rather than an +/// error — every assertion below would then pass or fail vacuously. Hence +/// the explicit existence check. +Future _fixturePath() async { + final libUri = await Isolate.resolvePackageUri( + Uri.parse('package:solid_generator/builder.dart'), + ); + final path = File.fromUri( + libUri!.resolve('../test/resolved/signal_detection_fixture.dart'), + ).path; + if (!File(path).existsSync()) fail('missing resolved fixture: $path'); + return path; +} + +/// The fixture's resolved unit, alongside its source text (taken from the +/// resolution result, so the two can never drift). +Future<({CompilationUnit unit, String source})> _resolvedFixture() async { + final result = await resolveFile(path: await _fixturePath()); + if (result is! ResolvedUnitResult) { + fail('fixture did not resolve as expected: $result'); + } + return (unit: result.unit, source: result.content); +} + +void main() { + test( + '@SolidEnvironment rejects a resolved Signal-typed field ' + '(SignalBase declares in package:solidart, not flutter_solidart)', + () async { + final fixture = await _resolvedFixture(); + expect( + () => validateSolidEnvironmentTargets(fixture.unit), + throwsA( + isA().having( + (e) => e.message, + 'message', + contains( + '@SolidEnvironment cannot be applied to a SignalBase-typed ' + 'field', + ), + ), + ), + ); + }, + ); + + test( + 'a local alias of a foreign signal is tracked even though its own name ' + 'is in scope (resolved staticType tier runs before the shadow guard)', + () async { + final fixture = await _resolvedFixture(); + final result = collectValueEdits( + _functionBody(fixture.unit, 'readAlias'), + const {}, + fixture.source, + ); + + expect( + result.trackedReadNamesByOffset, + isNotEmpty, + reason: + '`alias.value` must be recorded as a tracked read; the shadow ' + 'guard used to suppress it before the resolved type was ever ' + 'consulted', + ); + }, + ); + + test( + '`bag.sig.untracked.value` (Section 6.4 opt-out through a resolved ' + 'generic extension chain) is never tracked', + () async { + final fixture = await _resolvedFixture(); + final result = collectValueEdits( + _functionBody(fixture.unit, 'readOptOut'), + const {}, + fixture.source, + ); + + expect( + result.trackedReadNamesByOffset, + isEmpty, + reason: + 'the `.untracked` opt-out must suppress tracking even though ' + 'the resolved receiver type is still the signal', + ); + }, + ); +} + +/// The body of the top-level function named [name] in [unit]. +FunctionBody _functionBody(CompilationUnit unit, String name) { + for (final decl in unit.declarations) { + if (decl is FunctionDeclaration && decl.name.lexeme == name) { + return decl.functionExpression.body; + } + } + fail('function $name not found in fixture'); +} diff --git a/packages/solid_generator/test/value_rewriter_test.dart b/packages/solid_generator/test/value_rewriter_test.dart index 2dfee97..28a9556 100644 --- a/packages/solid_generator/test/value_rewriter_test.dart +++ b/packages/solid_generator/test/value_rewriter_test.dart @@ -126,6 +126,45 @@ class SessionGuard { ); }); + group('managed-field no-double-append guard is not widened by the ' + 'foreign-signal getter set', () { + // The foreign-signal read path recognizes a wider getter set than the + // managed-member guard (`state` / `hasPreviousValue` on top of `value` / + // `hasValue` / `previousValue`) because there the receiver is PROVEN to + // be a `SignalBase`. Sharing one set between the two regresses every + // `@SolidState` field whose own payload type happens to declare a member + // of that name: the guard would read `machine.state` as "already on the + // signal API" and skip the `.value` append, emitting `machine.state` + // against a `Signal` — which does not compile, and drops the + // reactive read. The foreign side is pinned by the `GateProbe` class in + // `test/golden/inputs/foreign_signal_value_read.dart`. + test( + '.state / .hasPreviousValue on a @SolidState field of a user type ' + 'still lower to .value.', + () { + const source = r''' +class Screen { + MyMachine machine; + + String describe() { + return '${machine.state} ${machine.hasPreviousValue}'; + } +} +'''; + final method = _method(source, 'Screen', 'describe'); + final result = collectValueEdits(method, const {'machine'}, source); + + final rewritten = applyEditsToRange( + source.substring(method.offset, method.end), + result.edits, + method.offset, + ); + expect(rewritten, contains(r'${machine.value.state}')); + expect(rewritten, contains(r'${machine.value.hasPreviousValue}')); + }, + ); + }); + group('cross-class name-collision guard (unresolved AST) — issue #110', () { test( 'a name builder.dart flagged ambiguous never rewrites through an '