refactor(egfx)!: mark consumer-facing types as #[non_exhaustive] for pre-publish API stability

[Greg Lamberson (glamberson)]

Closes the API-stability item of #1229 (egfx pre-publish hardening). Marks consumer-facing aggregates and the top-level dispatch enum so future field/variant additions don't constitute breaking changes for downstream crates.

Marked

• Consumer-facing state with pub fields: Surface (client + server), BitmapUpdate, DecodedFrame, CodecCapabilities (client + server), FrameInfo, QoeMetrics, QoeSnapshot.
• Errors: DecoderError.
• Dispatch enum: GfxPdu (consumers must add _ => arms).

Intentionally not marked

• Spec-defined wire PDUs: shape is fixed by MS-RDPEGFX; spec growth comes via new GfxPdu variants, not field-adds to existing PDUs.
• Spec primitives (Timestamp, Point, Color, QuantQuality, Avc420Region, CacheEntryMetadata, AVC bitmap streams): shape fixed by spec.
• Spec-discriminant enums (CapabilitySet, Codec1Type, Codec2Type, PixelFormat, QueueDepth): to be addressed separately via the wrapper-struct-around-integer pattern documented in crates/ironrdp-pdu/README.md (cf. ironrdp_graphics::rle::Code, ironrdp_cliprdr::pdu::format_list::ClipboardFormatId).
• Tuple-struct PDU wrappers: same logic as wire PDUs.
• All-private-field aggregates (Surfaces, FrameTracker, GraphicsPipelineClient, GraphicsPipelineServer): #[non_exhaustive] has no mechanical effect when fields are private.

DecodedFrame::new

Adds debug_assert_eq! on the RGBA8888 size invariant (data.len() == width * height * 4) so contract-violating decoder impls surface in debug builds.

Breaking change

External crates can no longer construct the marked structs via struct-literal syntax; exhaustive match on GfxPdu now requires a _ => arm. Marked with ! in the subject. Internal-to-workspace construction is unaffected.

Local validation

cargo xtask check fmt / lints / tests / typos / locks — all good.

[Copilot]

Pull request overview

This PR hardens ironrdp-egfx’s public API ahead of its planned crates.io publish by marking protocol/spec-extensible public types as #[non_exhaustive], and adding constructors where #[non_exhaustive] would otherwise break existing workspace tests that relied on struct literals.

Changes:

• Mark a broad set of EGFX public structs/enums (PDUs, metadata/value types, client/server state, errors) as #[non_exhaustive] to allow future field/variant additions without SemVer-breaking changes.
• Add new(...) constructors for several PDUs / wrapper types and update tests to use constructors instead of struct literals.
• Update testsuite EGFX client/server integration tests to reflect the new construction pattern.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/ironrdp-testsuite-core/tests/egfx/server.rs	Update EGFX server tests to use new constructors for #[non_exhaustive] PDUs/wrappers.
crates/ironrdp-testsuite-core/tests/egfx/client.rs	Update EGFX client tests to use new constructors (DecodedFrame::new, PDU ::new).
crates/ironrdp-egfx/src/server.rs	Mark consumer-facing server state types as #[non_exhaustive].
crates/ironrdp-egfx/src/pdu/common.rs	Mark common value types (Point, Color, PixelFormat) as #[non_exhaustive].
crates/ironrdp-egfx/src/pdu/cmd.rs	Mark many command/negotiation PDUs as #[non_exhaustive] and add constructors for test/consumer construction.
crates/ironrdp-egfx/src/pdu/avc.rs	Mark AVC-related stream/value types as #[non_exhaustive].
crates/ironrdp-egfx/src/decode.rs	Mark DecodedFrame/DecoderError as #[non_exhaustive] and add DecodedFrame::new.
crates/ironrdp-egfx/src/client.rs	Mark consumer-facing client state types as #[non_exhaustive].

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Benoît Cortier (CBenoit)]

Thanks for the thorough write-up. I'm not convinced this should land as-is; I think the blanket application of #[non_exhaustive] makes the API meaningfully worse in exchange for fairly thin SemVer payoff, and the new() shims actively work against the stated goal.

First, where #[non_exhaustive] doesn't earn its cost:

• Wire PDU structs — WireToSurface1Pdu, WireToSurface2Pdu, SolidFillPdu, SurfaceToSurfacePdu, SurfaceToCachePdu, CacheToSurfacePdu, EvictCacheEntryPdu, CreateSurfacePdu, DeleteSurfacePdu, StartFramePdu, EndFramePdu, FrameAcknowledgePdu, QoeFrameAcknowledgePdu, DeleteEncodingContextPdu, the MapSurfaceTo* family, CacheImportOfferPdu, CacheImportReplyPdu, ResetGraphicsPdu. Their shape is fully determined by MS-RDPEGFX. You don't get to grow CreateSurfacePdu with a new field — if MS extends the wire format, the result is a new spec revision producing a different PDU (or a new variant of GfxPdu), not a silently-added struct field. Marking these #[non_exhaustive] denies callers struct-literal construction in exchange for forward-compat flexibility the wire format doesn't actually grant us.

• Spec-defined value primitives — Timestamp, Point, Color, QuantQuality, Avc420Region, CacheEntryMetadata. Same argument, more starkly: these are immutable spec primitives. Timestamp::new(hours, minutes, seconds, milliseconds) is literally the four fields the spec defines; they're not going to grow. The cost (no FRU, awkward pattern-matching) is real and the benefit is essentially zero.

• Enums whose variants encode spec-defined discriminants — CapabilitySet, Codec1Type, Codec2Type, PixelFormat. These currently use the Unknown(value) catch-all pattern, which crates/ironrdp-pdu/README.md discusses at length and explicitly flags as a forward-compat hazard (silent behavior change when a new variant is later added, PartialEq foot-gun, etc.). The README's recommended remedy is the wrapper-struct-around-integer pattern (cf. ironrdp_graphics::rle::Code, ironrdp_cliprdr::pdu::format_list::ClipboardFormatId), not #[non_exhaustive]. Adding #[non_exhaustive] here doesn't address that hazard at all — it's a library-internal silent-construction issue, not a consumer exhaustive-match issue — while still degrading match ergonomics for the named variants. If pre-publish stability for these enums is a real concern, the right move is the wrapper-struct refactor the README recommends, not slapping #[non_exhaustive] on the existing layout.

Also, the new() constructors are the bigger concern. Adding ~12 positional pub fn new(a, b, c, d, e) constructors is exactly the anti-pattern #[non_exhaustive] exists to prevent: it freezes field order and count into a new positional API. The day a field is added, new() either breaks (defeating the entire reason for #[non_exhaustive]) or sprouts new_with_X variants. They also run against STYLE.md's "no single-use helper" guidance — these exist solely to keep mechanical test-site edits compiling.

If a type genuinely is expected to evolve, the right tool is a builder, not a positional new. If it isn't expected to evolve, it doesn't need #[non_exhaustive] in the first place.

All that said, I'd keep it on the consumer-facing aggregates that genuinely grow with the implementation:

• Surface (client + server), BitmapUpdate, DecodedFrame, CodecCapabilities (client + server), FrameInfo, QoeMetrics, QoeSnapshot, Surfaces, FrameTracker, GraphicsPipelineClient, GraphicsPipelineServer.
• Error types: DecoderError.
• Arguably GfxPdu itself (new PDU variants are a real scenario), accepting the _ => cost on consumers.

My suggestion is to narrow the scope to the handler-facing aggregates + errors (+ optionally GfxPdu), drop the wire-PDU and spec-primitive markings, remove the corresponding new() shims, and address spec-discriminant enum stability via the wrapper-struct pattern documented in crates/ironrdp-pdu/README.md (in a separate PR) rather than via #[non_exhaustive].

Generally speaking, pre-v1 we can absolutely make breaking changes, which is not a reason to not use #[non_exhaustive] at all, but we can be a bit more conservative and deliberate.

[Benoît Cortier (CBenoit)]

I also have no idea what is "F16". Is it an audit artifact you are storing locally?

[Greg Lamberson (glamberson)]

Thanks for the careful read. I misread the common practice on #[non_exhaustive] and went a little crazy on the first pass; you're right on all four points. Hopefully this is a little better.

The narrowed shape keeps #[non_exhaustive] on the consumer-facing aggregates that genuinely grow with implementation (Surface client+server, Surfaces, BitmapUpdate, DecodedFrame, CodecCapabilities client+server, FrameInfo, QoeMetrics, QoeSnapshot, FrameTracker, GraphicsPipelineClient, GraphicsPipelineServer), DecoderError, and GfxPdu (accepting the _ => cost on consumers). Everything else reverts: wire PDUs go back to bare struct-literal construction, spec primitives (Timestamp, Point, Color, QuantQuality, Avc420Region, CacheEntryMetadata) too, and the eleven positional new() shims drop along with them. Test sites revert to struct-literal construction; the one surviving constructor (DecodedFrame::new) stays because the type is consumer-facing and still #[non_exhaustive] from outside the crate.

For the spec-discriminant enums (CapabilitySet, Codec1Type, Codec2Type, PixelFormat): I'll file the wrapper-struct refactor separately, following the pattern documented in `crates/ironrdp-pdu/README.md` and the existing precedents in `ironrdp_graphics::rle::Code` and `ironrdp_cliprdr::pdu::format_list::ClipboardFormatId`. That belongs as its own change, not folded in here.

Force-pushing the narrowed branch shortly.

[Greg Lamberson (glamberson)]

F16 is the egfx pre-publish hardening finding from a workspace API hygiene audit I keep in a private tracking repo. Internal shorthand that slipped into the PR body; won't surface in upstream prose again.