fix(error): box diagnostic metadata to shrink Error<Kind> size#1269
Conversation
Move context, location, and source into a heap-allocated ErrorMeta struct so that Error<Kind> holds only kind on the stack. This eliminates the size cascade that caused ConnectorError to exceed clippy's 128-byte threshold (it was raised to 152 to accommodate the Location field). Error construction is already #[cold], so the single Box allocation per error is acceptable.
There was a problem hiding this comment.
Pull request overview
This PR restructures ironrdp_error::Error<Kind> so that diagnostic metadata (context, call-site location, and optional source error) is stored in a heap-allocated ErrorMeta, keeping Error<Kind> itself small and avoiding large-error-size cascades in downstream error enums.
Changes:
- Introduces an
ErrorMetastruct (behindBox) and moves diagnostic fields into it whenallocis enabled. - Updates
Error<Kind>construction,Debug/Display, and source chaining to use the boxed metadata. - Removes the custom
clippy.tomllarge-error-thresholdoverride now that the error size is expected to be below the default limit.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/ironrdp-error/src/lib.rs | Boxes diagnostic metadata to reduce Error<Kind> stack size; adjusts formatting and source chaining accordingly. |
| clippy.toml | Drops the previously raised large-error-threshold configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I saw this come in this morning. The Box layout is the right move. I'll rebase #1266 onto this. The Option I proposed there fits as a fourth field on ErrorMeta, which dissolves the threshold bump that was the awkward part of that PR. I'll mark #1266 as depending on this once I push. If you don't want position metadata at all, withdrawal is fine. |
…ind> Adds two composable builder methods to ironrdp-error::Error<Kind> that record optional positional metadata: Error::at_offset records a byte offset in the input stream, and Error::in_field records a symbolic field or sub-PDU name. Both are optional, both compose, and both are surfaced in Display output when present. Position lives as a fourth field on ErrorMeta (alongside context, location, source) so it carries zero size cost on Result-Ok paths and is available only when the alloc feature is enabled. Refs Devolutions#1257, Devolutions#1120. Depends on Devolutions#1269.
…ind> Adds two composable builder methods to ironrdp-error::Error<Kind> that record optional positional metadata: Error::at_offset records a byte offset in the input stream, and Error::in_field records a symbolic field or sub-PDU name. Both are optional, both compose, and both are surfaced in Display output when present. Position lives as a fourth field on ErrorMeta (alongside context, location, source) so it carries zero size cost on Result-Ok paths and is available only when the alloc feature is enabled. Refs Devolutions#1257, Devolutions#1120. Depends on Devolutions#1269.
Hey! Thank you; yes that’s exactly what I had in mind. I saw that we were growing the size of the main error type a bit too much, but I think this new approach will allow us to store more metadata without feeling bad about it 🙂 |
Move context, location, and source into a heap-allocated ErrorMeta struct so that Error holds only kind on the stack. This eliminates the size cascade that caused ConnectorError to exceed clippy's 128-byte threshold (it was raised to 152 to accommodate the Location field).
Error construction is already #[cold], so the single Box allocation per error is acceptable.