refactor(pdu): hand-roll Display/Error impls and remove thiserror dependency#1264
Merged
Benoît Cortier (CBenoit) merged 3 commits intoMay 13, 2026
Conversation
…endency
The `ironrdp-pdu` crate is Core Tier per ARCHITECTURE.md, and Core Tier
crates carry an architectural invariant forbidding proc-macro dependencies
(`syn`) so compilation stays parallel and fast for downstream consumers
that build only the protocol-decode tree (the fuzzing infrastructure, web
client, no_std users).
`thiserror` was carried in the legacy block of `ironrdp-pdu/Cargo.toml`
under the existing `# TODO: get rid of these dependencies` comment. This
PR delivers on that TODO by replacing every `#[derive(thiserror::Error)]`
in the crate with hand-written `impl Display`, `impl core::error::Error`,
and `impl From<X>` blocks that preserve the exact `Display` output
byte-for-byte and the exact `Error::source()` semantics that the derives
generated.
Changes per error enum (13 enums across 13 files):
- `CoreDataError`, `GccError`, `NetworkDataError`, `ClusterDataError`,
`SecurityDataError` (gcc family)
- `RdpError`, `CapabilitySetsError`, `ChannelError`, `SessionError`,
`ServerLicenseError`, `ClientInfoError` (rdp family)
- `InputEventError`
- `McsError` (legacy submodule)
Each enum keeps its `#[derive(Debug)]`. The hand-written `Display`
preserves every `#[error("...")]` format string verbatim, including
`{0}`/`{0:?}` interpolation and the named-field interpolation in
`ChannelError::InvalidDvcTotalMessageSize`. `core::error::Error::source`
preserves the `#[from]`-driven source chain plus the named `source:`
field on `ServerLicenseError::InvalidX509Certificate`. The existing
manual `From<PduError>`, `From<RdpError> for io::Error`,
`From<McsError> for io::Error`, `From<ChannelError> for io::Error`, and
`From<LicensingErrorMessage> for ServerLicenseError` impls are preserved.
Verification:
- `cargo expand` against the post-change tree shows `Display` and `source`
impls that are functionally equivalent to the thiserror-generated ones
for every variant.
- `cargo check --workspace` clean.
- `cargo test --package ironrdp-pdu` 850 tests pass, 0 failures.
- `cargo clippy --package ironrdp-pdu --all-targets -- -D warnings` clean.
- `cargo xtask check fmt` clean.
- `cargo xtask check typos` clean.
- Only external consumers of these types in the workspace
(`ironrdp-connector` for `ServerLicenseError`, `ffi` and `ironrdp-session`
for `RdpError` / `SessionError`) treat them as opaque error types; no
variant-level pattern matching exists outside the crate, so the
preserved `From` and `Display` semantics fully cover downstream
expectations.
Diff: 15 files changed, +748 / -216.
Related: Devolutions#1257.
This was referenced May 12, 2026
Benoît Cortier (CBenoit)
approved these changes
May 13, 2026
There was a problem hiding this comment.
Pull request overview
This PR removes the thiserror proc-macro dependency from the Core Tier ironrdp-pdu crate by replacing all #[derive(thiserror::Error)] usages with hand-written Display, core::error::Error, and From implementations, keeping error formatting/source chains consistent with prior behavior.
Changes:
- Removed
thiserror = "2.0"fromcrates/ironrdp-pdu/Cargo.tomland updated lockfiles accordingly. - Replaced
thiserrorderives across theironrdp-pduerror enums with explicitfmt::Display,core::error::Error::source, andFromimpls. - Preserved existing
From<…> for io::Errorconversions while keeping error messages/source semantics stable.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| fuzz/Cargo.lock | Removes thiserror from the fuzz lockfile dependency graph. |
| Cargo.lock | Removes thiserror from the workspace lockfile dependency graph. |
| crates/ironrdp-pdu/Cargo.toml | Drops the direct thiserror dependency from ironrdp-pdu. |
| crates/ironrdp-pdu/src/rdp/vc/mod.rs | Hand-rolls ChannelError Display/Error/From impls. |
| crates/ironrdp-pdu/src/rdp/session_info/mod.rs | Hand-rolls SessionError Display/Error/From impls. |
| crates/ironrdp-pdu/src/rdp/server_license/mod.rs | Hand-rolls ServerLicenseError Display/Error/From impls (incl. source: field handling). |
| crates/ironrdp-pdu/src/rdp/mod.rs | Hand-rolls RdpError Display/Error/From impls and preserves conversion behavior. |
| crates/ironrdp-pdu/src/rdp/client_info.rs | Hand-rolls ClientInfoError Display/Error/From impls. |
| crates/ironrdp-pdu/src/rdp/capability_sets/mod.rs | Hand-rolls CapabilitySetsError Display/Error/From impls. |
| crates/ironrdp-pdu/src/mcs.rs | Hand-rolls legacy McsError Display/Error/From impls. |
| crates/ironrdp-pdu/src/input/mod.rs | Hand-rolls InputEventError Display/Error/From impls. |
| crates/ironrdp-pdu/src/gcc/security_data.rs | Hand-rolls SecurityDataError Display/Error/From impls. |
| crates/ironrdp-pdu/src/gcc/network_data.rs | Hand-rolls NetworkDataError Display/Error/From impls. |
| crates/ironrdp-pdu/src/gcc/mod.rs | Hand-rolls GccError Display/Error/From impls (and nested error sources). |
| crates/ironrdp-pdu/src/gcc/core_data/mod.rs | Hand-rolls CoreDataError Display/Error/From impls. |
| crates/ironrdp-pdu/src/gcc/cluster_data.rs | Hand-rolls ClusterDataError Display/Error/From impls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Benoît Cortier (CBenoit)
merged commit 14c5502
into
Devolutions:master
10 checks passed
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
May 18, 2026
Brings ironrdp-rdpsnd-native into line with the workspace convention from ARCHITECTURE.md that libraries provide concrete error types rather than anyhow::Result. Defines RdpsndNativeErrorKind with four variants covering the failure modes in DecodeStream::new (UnsupportedFormat, OpusInit, AudioDevice, StreamBuild). Display and core::error::Error impls are hand-rolled per the post-Devolutions#1264 pattern in ironrdp-pdu. DecodeStream::new now returns RdpsndNativeResult<Self> instead of anyhow::Result<Self>. Foreign error sources (opus2::Error, cpal::DefaultStreamConfigError, cpal::BuildStreamError) are attached via Error::with_source so the cause chain remains reachable through core::error::Error::source(). anyhow moves from [dependencies] to [dev-dependencies] for the cpal example binary. The internal log of construction failures switches from "{e:#}" to e.report() to preserve cause-chain output now that anyhow's alternate-format chain printing is no longer involved. Signed-off-by: Greg Lamberson <greg@lamco.io> Assisted-by: Claude Code (Anthropic, Opus)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the proc-macro architectural-invariant violation that issue #1257 surfaced and that I committed to addressing in #1257 (comment).
What changes
ironrdp-pduis Core Tier perARCHITECTURE.md. Core Tier carries the architectural invariant that no proc-macro dependency (syn) is allowed, so compilation stays fast for downstream consumers that build only the protocol-decode tree (the fuzzing infrastructure, the web client,no_stdusers).thiserror = "2.0"was carried in the legacy block ofironrdp-pdu/Cargo.tomlunder the existing# TODO: get rid of these dependenciescomment. This PR delivers on that TODO. Every#[derive(thiserror::Error)]in the crate is replaced with hand-writtenimpl Display,impl core::error::Error, andimpl From<X>blocks that preserve the exactDisplayoutput byte-for-byte and the exactError::source()semantics that the derives generated.Enums converted (13 total across 13 files)
GCC family:
CoreDataError,GccError,NetworkDataError,ClusterDataError,SecurityDataErrorRDP family:
RdpError,CapabilitySetsError,ChannelError,SessionError,ServerLicenseError,ClientInfoErrorOther:
InputEventErrorMcsError(legacy submodule inmcs.rs)Each enum keeps
#[derive(Debug)]. The hand-writtenDisplaypreserves every#[error("...")]format string verbatim, including{0}and{0:?}interpolations and the named-field interpolation inChannelError::InvalidDvcTotalMessageSize. TheError::source()impl preserves the#[from]-driven source chain plus the namedsource:field onServerLicenseError::InvalidX509Certificate. The existing manualFrom<PduError>,From<RdpError> for io::Error,From<McsError> for io::Error,From<ChannelError> for io::Error, andFrom<LicensingErrorMessage> for ServerLicenseErrorimpls are preserved unchanged.Verification
cargo expandagainst the post-change tree showsDisplayandsourceimpls that are functionally equivalent to the thiserror-generated versions for every variant.cargo check --workspaceclean.cargo test --package ironrdp-pdu850 tests pass, 0 failures.cargo clippy --package ironrdp-pdu --all-targets -- -D warningsclean.cargo xtask check fmtclean.cargo xtask check typosclean.Downstream impact
External consumers of these types in the workspace (
ironrdp-connectorforServerLicenseError,ffiandironrdp-sessionforRdpErrorandSessionError) treat them as opaque error types. A workspace-wide grep finds no variant-level pattern matching on these enums outsideironrdp-pduitself, so the preservedFromconversions andDisplayoutput fully cover downstream expectations.Diff shape
15 files changed, +748 / -216 (net +532). Most of the added lines are the explicit
Displaymatch arms and the per-variantFromandsource()arms that the proc-macro previously generated at compile time.Related: #1257.