Skip to content

fix(shadows): support opaque newtype variants in adjacently-tagged enums#124

Merged
KennethKnudsen97 merged 11 commits into
masterfrom
fix/primitive_types_in_adj_tagged_enum
May 6, 2026
Merged

fix(shadows): support opaque newtype variants in adjacently-tagged enums#124
KennethKnudsen97 merged 11 commits into
masterfrom
fix/primitive_types_in_adj_tagged_enum

Conversation

@KennethKnudsen97
Copy link
Copy Markdown
Contributor

Summary

  • Adjacently-tagged shadow enums with a #[shadow_attr(opaque)] newtype variant (e.g. Manual(heapless::String<62>)) now compile and round-trip cleanly to a flat wire shape {"mode":"manual","apn":"onomondo"}.
  • Skips the From<<T as ShadowNode>::Reported> for Reported<Enum> impls (and the matching Delta) when the variant field is opaque — for leaf types where Delta = Reported = Self the projection collapses to From<X> for X and collides with impl<T> From<T> for T (E0119).
  • Emits a scalar serialize arm (map.serialize_entry(content_key, content)) for opaque variants instead of the ContentWrapper nested-map approach, and drops opaque variants from the inactive-variant null-padding list so sibling unit variants don't emit empty content maps.
  • Same opaque gating is applied to the externally-tagged enum codegen for symmetry.

The existing struct-inner-type path (PortMode::Sio(SioConfig), WifiAuth::Wpa2(WpaConfig)) is unchanged and still produces {"mode":"sio","config":{…}}.

Stacked on feat/subscription-disconnect-aware. Targeting that branch — change base after it lands.

Test plan

  • cargo test --lib --features "std,log" → 164 passed (7 new opaque tests).
  • cargo test -p rustot-derive → 10 passed.
  • cargo build --no-default-features → clean.
  • cargo build --no-default-features --features "shadows_kv_persist" → clean.
  • All 24 pre-existing adjacently_tagged tests still pass (PortMode struct-inner-type path unchanged).

Kenneth Sylvest Knudsen and others added 5 commits May 1, 2026 10:13
…opics

Bump the QoS on the request/response topic pairs from AtMostOnce to
AtLeastOnce so a transient drop between SUBSCRIBE and the broker's reply
doesn't silently lose the response and park the caller forever.
Watch the broker connection state from each Subscription so an
ungraceful drop wakes parked next_message() callers with None instead
of hanging forever. rumqttc impl uses tokio::sync::watch.

Builds on #122. Issue #121.
Only invalidate Subscriptions when the broker actually drops our subs
(CONNACK with session_present=false). Transient disconnects and
session-resume reconnects keep subs valid.

- mqttrust backend: bump rev to pick up FactbirdHQ/mqttrust#XX, which
  surfaces the same signal via clean_session_count.
- rumqttc backend: track a watch::Sender<u8> bumped only on
  session_present=false; subscriptions race recv against changed().
  Replaces the over-aggressive "any state change → None" behavior.
- Trait doc clarified.

Refs #121
…y-tagged enums

Marking a newtype enum variant with `#[shadow_attr(opaque)]` previously
failed to compile (E0119: conflicting From impls) and, even with that
worked around, the generated Reported serializer wrote the content as an
empty nested map instead of the scalar value.

- Skip the `From<<T as ShadowNode>::Reported> for Reported<Enum>` and
  matching Delta impls when the variant field is opaque. For leaf types
  where Delta = Reported = Self, the projection collapses to
  `From<X> for X` and collides with `impl<T> From<T> for T`.
- Emit a flat serialize arm for opaque variants that writes the inner
  value directly under the content key, producing
  `{"mode":"manual","apn":"onomondo"}` instead of `{"…":"…","apn":{}}`.
- Drop opaque variants from the inactive-variant null-padding list so
  unit variants no longer emit an empty content map alongside opaque
  siblings.
- Apply the same opaque gating to the externally-tagged enum codegen
  for symmetry.

Tests cover flat opaque serialize, unit-variant-only serialize, parse +
apply round-trip, mixed opaque/struct enums, and a compile-only check
that From impls remain for non-opaque siblings.
Comment thread rustot_derive/src/codegen/adjacently_tagged.rs Outdated
Base automatically changed from feat/subscription-disconnect-aware to master May 5, 2026 11:12
Kenneth Sylvest Knudsen and others added 4 commits May 5, 2026 13:30
Replace the codegen-time `#[shadow_attr(opaque)]` gate on adjacently- and
externally-tagged enum newtype variants with a const-time dispatch on
`<T as ReportedFields>::FIELD_NAMES`. Any inner type whose ShadowNode
impl sets `Reported = Self` (e.g. anything reachable via `impl_opaque!`
or the leaf impls under `src/shadows/impls/`) now works without
annotation.

- Serializer dispatches scalar vs nested map per-variant via
  `FIELD_NAMES.is_empty()`. Unit variants additionally const-fold over
  every data sibling to decide whether to emit a (possibly null-only)
  content map at all — all-leaf siblings → no content key.
- Drop the From-impl gate. The earlier collision claim was wrong:
  `<T as ShadowNode>::Reported` does not normalize during coherence, so
  the projection-typed impl never overlaps with `impl<T> From<T> for T`
  even when `Reported = Self`. Leaf variants now also gain
  `#[builder(into)]` ergonomics.
- Stop tracking `is_opaque` in `variants_with_data`; everything the flag
  used to gate is now driven by the trait projections at the use site.

Tests reuse generic fixtures (`LeafEnum`, `MixedEnum`, `IntEnum`,
`ToggleConfig`) instead of project-specific fields, and exercise the
transparent path with bare `heapless::String<N>` and `u32` newtype
variants — no `#[shadow_attr(opaque)]`.
Match the existing terminology used by `#[shadow_attr(opaque)]`,
`impl_opaque!`, and `src/shadows/impls/opaque.rs` — there is no reason
to introduce a parallel "leaf" vocabulary in the codegen comments and
test names. The structural signal is still
`<T as ReportedFields>::FIELD_NAMES.is_empty()`; only the wording
changes.
Opaque newtype variants caused E0119 in external crates because the
projection <T as ShadowNode>::Reported collapses to T, making the
emitted impl overlap core's blanket From<T> for T. Mark the variant
field #[shadow_attr(opaque)] to emit From<#inner_ty> directly.
Copy link
Copy Markdown
Member

@MathiasKoch MathiasKoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation: require #[shadow_attr(opaque)] and drop the trait-based detection

The two codegen paths in this PR use different dispatch criteria:

  • From impls (adjacently_tagged.rs:233, enum_codegen.rs mirror) dispatch on FieldAttrs::is_opaque() — i.e. the attribute.
  • Serialize arm (adjacently_tagged.rs:591-614) dispatches at const-eval on <<T as ShadowNode>::Reported as ReportedFields>::FIELD_NAMES.is_empty() — i.e. the trait.

This split has a sharp edge: an external consumer who writes Mode(heapless::String<32>) without #[shadow_attr(opaque)] gets correct serialization but a compile failure on the generated From impl, because rustc only normalizes <T as ShadowNode>::Reported through local impls inside the defining crate. The PR description acknowledges this. The 7 added tests cannot regress it because they live in-crate.

Proposal

Require #[shadow_attr(opaque)] on the variant field and drop the const-eval detection. Both codegen paths then dispatch on the attribute at macro-expansion time.

Concrete consequences in adjacently_tagged.rs:

  1. The __INNER_IS_OPAQUE const and the surrounding if/else collapse. The opaque newtype arm becomes:
    Self::#variant_ident(ref content) => {
        map.serialize_entry(#tag_key, #serde_name)?;
        map.serialize_entry(#content_key, content)?;
    }
    The non-opaque arm keeps the existing ContentWrapper path. No more dead ContentWrapper definition emitted in opaque arms.
  2. inactive_variant_field_nulls only collects non-opaque siblings, filtered at codegen. has_non_opaque_sibling reduces to a plain bool known at codegen, dropping the __HAS_NON_OPAQUE_SIBLING const-expression-of-projections.
  3. Failure mode for a missing attribute becomes consistent: serialize tries the ContentWrapper path on a leaf type and trips a first-round-trip test, instead of silently going one way and breaking only at a downstream consumer.

Trade-off: users must remember the attribute. That's already true for struct-field opaqueness — the burden becomes consistent, not new.

Secondary

  • Add an out-of-crate compile test for the E0119 fix. A trybuild test under rustot_derive/tests/ consuming an opaque newtype variant would actually pin down the bug this PR fixes. The 7 in-crate tests prove the new serialize/parse paths work, but cannot regress the original collision.
  • Update OpaqueSpec docs in rustot_derive/src/attr/field_attr.rs. Today they describe only the struct-field meaning. After this PR the attribute on a variant field controls (a) flat wire format and (b) suppression of the projected From impl. Document both, and confirm whether MaxSize is still required for the variant-field use.
  • Confirm GH base is master before merge. PR description references feat/subscription-disconnect-aware (landed as #123). Diff stat against master is clean, so the rebase looks done — just flip the base in the GH UI.

Comment thread src/shadows/shadow/tests.rs Outdated
Comment thread rustot_derive/src/codegen/adjacently_tagged.rs Outdated
Drop the const-eval trait-based detection of opaque inner types in the
serialize arm. Both the From-impl gating and the serialize arm now
dispatch on `#[shadow_attr(opaque)]` at macro-expansion time, removing
the sharp edge where in-crate consumers worked but external consumers
hit E0119 because rustc only normalizes the projection through local
trait impls.

- Collapse `__INNER_IS_OPAQUE` const-eval into a codegen-time if/else.
- Collapse `__HAS_NON_OPAQUE_SIBLING` const-eval into a plain bool;
  filter `inactive_variant_field_nulls` to non-opaque siblings up front.
- Drop the dead ContentWrapper definition from opaque arms.
- Annotate the affected test fixtures (Label, Named, Number) with
  `#[shadow_attr(opaque)]` to exercise the opaque path explicitly.
- Document the variant-field meaning of `#[shadow_attr(opaque)]` on
  OpaqueSpec.
@KennethKnudsen97
Copy link
Copy Markdown
Contributor Author

Addressed in defe840. Took the recommendation: dropped the const-eval trait-based dispatch and made both code paths (From impls + serialize arm) read #[shadow_attr(opaque)] at macro-expansion time.

Codegen (adjacently_tagged.rs)

  • __INNER_IS_OPAQUE and the surrounding if/else collapsed — opaque newtype variants now emit just map.serialize_entry(content_key, content). Non-opaque keep the ContentWrapper path. No more dead ContentWrapper definition in opaque arms.
  • __HAS_NON_OPAQUE_SIBLING is now a plain codegen-time bool (!inactive_variant_field_nulls.is_empty()); inactive_variant_field_nulls is filtered to non-opaque siblings up front.
  • Unit variant with no non-opaque siblings emits just the tag (no empty content map).

Tests (tests.rs)

  • Label(heapless::String<16>)Label(#[shadow_attr(opaque)] heapless::String<16>). Same for Named (in OpaqueEnum) and Number(u32) (in IntEnum).
  • Reworded the comment on test_from_impls_emitted_for_all_data_variants so it accurately describes which arm each variant exercises (Toggle → projection arm, Label → direct arm).

Docs (field_attr.rs)

  • OpaqueSpec and the opaque field doc-comment now describe the variant-field meaning (flat wire shape + From-impl gating to dodge E0119) alongside the existing struct-field meaning.

Verification

  • cargo test --lib --features "std,log" → 173 passed.
  • cargo test -p rustot-derive → 10 passed.
  • cargo build --no-default-features and --features "shadows_kv_persist" → clean.

Not done (from the secondary list)

  • Out-of-crate trybuild test for the E0119 fix — would add new test infrastructure for one assertion. Happy to do it as a follow-up if you want it pinned down.
  • GH base flip — diff against master is clean, so just a UI action whenever you're ready.

@KennethKnudsen97 KennethKnudsen97 merged commit 805c3d9 into master May 6, 2026
5 checks passed
@KennethKnudsen97 KennethKnudsen97 deleted the fix/primitive_types_in_adj_tagged_enum branch May 6, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants