fix: Bound the size of a type named in a decoder diagnostic - #771
Merged
Merged
Conversation
What it costs to render a type follows that type's own width and depth. A type reaching the decoder is chosen by the sender, so a diagnostic that names one has to describe it without letting its shape decide the cost of the description. Render types through `elide_large`, which checks the existing `text_size` budget first and substitutes a placeholder when the type is over it, so nothing is built for an outsized type. Applied at every diagnostic that names a type: the argument, subtype and primitive-mismatch messages and the state dump in `de.rs`, and the subtyping messages in `subtype.rs`, which build their own text and were unguarded. Two of those diagnostics guarded the budget behind `full_error_message || text_size(..).is_ok()`, so asking for verbose errors skipped the budget entirely. The budget now always applies; verbose errors still carry the input and the decoder state, and an ordinary mismatch is unchanged and still names both types. `text_size` already existed for exactly this purpose and was already used at two sites. This extends it to the rest rather than introducing a new mechanism. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical unresolved issues remain in diagnostic rendering and test feature gating.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
Open (4)
What changed in this PR
This PR bounds rendering of sender-controlled Candid types in decoder and subtype diagnostics.
Changes:
- Adds budgeted type elision via
elide_large. - Applies bounded rendering across diagnostics.
- Adds regression tests and changelog documentation.
Unresolved critical findings remain around unbounded formatting, Class handling, eager subtype formatting, and test feature gating.
| File | Summary |
|---|---|
rust/candid/tests/type_diagnostics.rs |
Adds large-type diagnostic tests. |
rust/candid/src/types/subtype.rs |
Applies bounded rendering to subtype errors. |
rust/candid/src/types/internal.rs |
Adds diagnostic type-rendering limits. |
rust/candid/src/de.rs |
Uses bounded rendering in decoder diagnostics. |
CHANGELOG.md |
Documents the diagnostic size bound. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…rer total Review feedback. Three diagnostics still rendered a type directly, and a fourth path could ask the printer for a rendering it does not have. Route through the bounded renderer: - `deserialize_seq`'s "is not a tuple type", which names the wire type; - `equal_impl`'s "Field .. has different types" and "Method .. has different types" contexts; - `pp_args`, used by the "Mismatch in init args" context. Its two feature-split implementations collapse into one, since the bounded renderer already goes through `Display`, which picks the pretty printer or the fallback itself. The first was missed because the interpolation sits on a different line from the `format!`, which a line-oriented search does not see; the others because they name types as `f1.ty`, `m1.1` and `pp_args(..)` rather than through a variable a search would recognise. The audit is now structural: walk every format-producing call and look at its arguments. The renderer is also total now. It is reached for whatever type a diagnostic names, including from `subtype` and `equal`, and a service constructor has no rendering of its own -- the printer reaches an `unreachable!()` for one on master as well. Those are elided rather than rendered, so `equal` on a service constructor returns an error where it previously aborted. `text_size` is left as it was. `de.rs` drops its own copy of the budget for the one beside `text_size`, so the two cannot drift apart. The test target is gated on the `value` feature, following `tests/types.rs`. It does build without it today, because the `candid_parser` dev-dependency takes candid with `features = ["all"]` and Cargo unifies them, but that is incidental. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lists Review feedback, both of which my previous round only half-addressed. A service constructor was elided by testing for one at the top of the type, which left it reached through any type that merely contains one: a record, vector or option holding one still traversed into `text_size` and its `unreachable!()`. `text_size` now refuses a service constructor wherever it appears, so the bounded renderer is total for nested types as well without having to search for one itself. Turning the assertion into a refusal is what the previous round should have done; the top-level test treated the symptom. That also collapses the two placeholders into one. The distinction was only ever visible for a top-level service constructor, and a nested one would have been described as too large, which it need not be. A per-element budget also left a list of types unbounded, since the cost then grows with the number of elements rather than their size. `pp_args` and the type-table dump now stop once the list reaches its own budget and say how many entries were left out. The table is the one that matters: `max_type_len` allows ten thousand entries, so a per-entry bound alone still left the rendering growing with their number. Tests cover a service constructor at the top of a type, in a record, in a vector and under an option, and a long argument list staying bounded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate findings remain in decoder fallback handling and Class rendering, alongside documentation and placeholder inconsistencies.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (2)
lwshang
marked this pull request as ready for review
September 25, 2026 13:10
Contributor
|
✅ No security or compliance issues detected. Reviewed everything up to 738c0f1. Security OverviewDetected Code Changes
|
Review feedback from Marco's agent.
`text_size` subtracted its running total from the remaining budget after
every field, not the cost of the field it had just added, so the budget
ran out quadratically: an eleven-field record of the shape an ICRC-2
transfer takes renders to 287 characters and was already judged over a
budget of 500. That only showed in terse errors before, because verbose
ones skipped the check; this branch removed that skip, which would have
turned an old accounting bug into elided output for ordinary interfaces
exactly where a developer reads it. Each loop now spends what its own
item costs.
A hashed field id was charged 4 whatever its value, though it prints with
underscore separators and runs to 13 characters. Eight such fields render
to 194 characters and were priced at 89; they now price at 161.
ICRC-2-style, 11 named fields 287 chars over budget -> 252
8 hashed-id fields 194 chars 89 -> 161
The estimate still runs under the rendering, which only makes the budget
slightly permissive; it stays a bound, and an outsized type is refused as
before.
`pp_args` wrote a separator after every argument, so `(nat, text)` came
out as `(nat, text, )` in every build where the pretty printer had
produced the former. It now separates between items.
`describe_table` takes `&TypeEnv`, which is already in scope.
Left alone: `binary_parser` renders a raw table entry with `{t:?}` in
`Invalid table entry`. That is a parse struct rather than a type, its
Debug is flat, and its size follows the header, which carries its own
bound. The pull request wording no longer claims otherwise.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
marc0olo
approved these changes
Sep 25, 2026
marc0olo
left a comment
marc0olo
left a comment
Member
There was a problem hiding this comment.
LGTM. One optional nit: the Func arm of text_size doesn't count the , separators (or line breaks for long lists), so a many-arg func passes at 500 but renders ~1.5 KB. Still a constant bound, so not blocking. Either charge 2 per extra arg or phrase MAX_DIAGNOSTIC_TYPE_LEN's doc as an estimate.
Reviewer's nit. Every other arm of `text_size` covers its own separators through a per-item charge -- a record field pays 3 for the "; " and a little over -- but a function type charged nothing between arguments, so a many-argument function passed a budget of 500 while rendering about three times that. Charge 2 per argument after the first, in each of the argument and result lists. The worst case a function can render at falls from roughly 1.5 KB to under 900 bytes. What is left is the pretty printer wrapping a long list across lines and indenting it, which is a property of the layout rather than of the type. `MAX_DIAGNOSTIC_TYPE_LEN` is now described as an estimate for that reason: a type that passes can still render somewhat longer, and what the bound rests on is the estimate staying proportional to the rendering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 of 5 tasks
lwshang
added a commit
that referenced
this pull request
Sep 25, 2026
Patch release of `candid` / `candid_derive`, 0.10.36 → 0.10.37. No source changes — it dates the two decoder fixes already on `master`. ## Summary - Bump `candid` and `candid_derive` 0.10.36 → 0.10.37, and the `=` pin between them - Date the `Unreleased` changelog entries as `2026-09-25` / `Candid 0.10.37`, covering: + Bounding the byte length of the type-table header, 64 KiB by default and configurable with `set_max_header_len` (#770) + Bounding the size of a type named in a decoder diagnostic, so its rendering no longer follows the type's own width and depth (#771) - Refresh `Cargo.lock`, `rust/bench/Cargo.lock`, `tools/ui/Cargo.lock` and `rust/candid/fuzz/Cargo.lock` `candid_parser` is unchanged at 0.4.1; its dependency on `candid` is a `0.10.16` floor, so it needs no bump. The lockfile diffs are version bumps only — no dependency churn. ## All four lockfiles this time The 0.10.36 release noted that `rust/candid/fuzz/Cargo.lock` was stale at 0.10.34 and that all four lockfiles should move together as a release-checklist step. This one does that, so `fuzz` goes 0.10.34 → 0.10.37. `tools/ui` patches `candid` to the workspace path, so its lockfile pins the path crate's version and has to move with the bump. `candid-ui-release.yml` builds `didjs` there with `--locked` and triggers on the date tag, so a stale lock would only fail at tag time, after merge. ## Test plan - [x] `cargo check -p candid -p candid_derive -p candid_parser` - [x] `cargo test -p candid --features all` — all suites pass - [x] `cd tools/ui && cargo build --target wasm32-unknown-unknown --profile canister --package didjs --locked` — the release workflow's exact command, succeeds - [x] `cd rust/candid/fuzz && cargo check` - [ ] Reviewer to confirm release scope and changelog date ## After merge 1. Tag `2026-09-25` on `master` — this is what `candid-ui-release.yml` triggers on to publish the `candid_ui` canister wasm 2. Run the **Crates Publish** workflow (`workflow_dispatch`) with the `candid` input checked, which publishes `candid_derive` then `candid` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
What this changes
A diagnostic that names a type now renders it through
elide_large, which checks the existingtext_sizebudget first and substitutes(type elided)when the type is over it — so nothing is built for a type whose rendering cost would follow its own width and depth. Types reaching the decoder come from the wire, so the budget applies to every diagnostic, including the verbose formset_full_error_message(true)selects, which previously short-circuited it away.Two budgets:
MAX_DIAGNOSTIC_TYPE_LENfor one type,MAX_DIAGNOSTIC_LIST_LENfor a list of them, since a per-element bound leaves a list growing with its length and the type-table dump can holdmax_type_lenentries.This covers every type-naming diagnostic in
de.rsandsubtype.rs. The latter builds its own message text and had no budget at all, which is the larger half of the change.With #770's 64 KiB header bound in place this is defence in depth, mainly for callers raising
max_header_lenand for thecandid_parserpaths that have no header bound.Public behaviour change
text_sizeasserted a service constructor could not reach it. That held for its two original callers, butelide_largeis reached from the publicsubtypeandequal, including for aClassnested in a record, vector or option. It now returnsErr(())for one wherever it appears, and such types are elided rather than rendered — the pretty printer has no arm for one either.Two pre-existing bugs, surfaced by removing the skip
text_sizesubtracted its running total from the remaining budget after each field rather than that field's cost, so the budget drained quadratically and rejected types well under it: an eleven-field ICRC-2-shaped record renders to 287 characters and was judged over a budget of 500. Hashed field ids were also charged 4 however wide they print.Both only showed in terse errors before, because verbose mode skipped the check. Removing that skip would have turned them into elided output for ordinary interfaces, so both are fixed here.
Compatibility
Diagnostics only — no change to what decodes.
subtype.rsis shared withcandid_parser, so a.didtypechecking error naming a very large type elides it too.binary_parser'sInvalid table entrystill renders a raw parse struct with{t:?}: itsDebugis flat and its size follows the header, which carries its own bound.Tests
rust/candid/tests/type_diagnostics.rs, gated on thevaluefeature, decoding on a thread given little room: outsized types againstint,nat16and a function reference under bothfull_error_messagesettings; a service constructor at the top of a type, in a record, in a vector and under an option; a 20,000-element argument list; and, on the other side, an ordinary ICRC-2-shaped record still named in full.Release
No version bump; the changelog entry joins the existing
## Unreleasedsection.🤖 Generated with Claude Code