Skip to content

Derive Default on generated Rust types#1272

Merged
stephentoub merged 7 commits into
mainfrom
tclem/rust-skill-defaults-in-tests
May 14, 2026
Merged

Derive Default on generated Rust types#1272
stephentoub merged 7 commits into
mainfrom
tclem/rust-skill-defaults-in-tests

Conversation

@tclem
Copy link
Copy Markdown
Member

@tclem tclem commented May 13, 2026

When the @github/copilot schema gains a new optional field on a wire type, every Rust test fixture that constructs that type with a struct literal breaks until each call site is hand-edited. #1270 needed exactly that fix in 70eb60e2, and we'll hit it again on every schema bump. Test fixtures shouldn't have to acknowledge wire fields they don't care about.

Approach

Teach the Rust codegen to derive Default on the generated types so test fixtures can spell themselves with ..Default::default() and ride out additive schema changes:

  • Generated structs derive Default, with one carve-out: structs that have a required field of a type with no sensible default (currently only untagged enums) skip the derive. The codegen tracks this in a nonDefaultableTypes set and propagates transitively in a single depth-first pass.
  • Generated string enums (the open-world ones with #[serde(other)] Unknown) derive Default with #[default] on the Unknown variant.
  • Generated const string enums derive Default with #[default] on their single variant.
  • SessionEventType (hand-built in the codegen) gets the same treatment.
  • RequestId in rust/src/types.rs derives Default for parity with SessionId.

The three internal Model fixtures in rust/src/lib.rs and rust/tests/e2e/client.rs are converted to ..Default::default() to demonstrate and exercise the new derive.

Notes for reviewers

  • #[non_exhaustive] is intentionally not part of this change. It's a separate decision (it does nothing inside our own crate, so it doesn't fix the internal-fixture pain — it's purely a downstream-facing SemVer instrument). Worth a follow-up on the open-world types.
  • Production code stays explicit: the rust-coding skill already forbids ..Default::default() outside tests, and that rule is unchanged. The intent here is only to make wire-type fixtures resilient to additive schema changes.
  • I deliberately did not derive Default on SessionEventData (the adjacent-tagged event payload enum) or on TypedSessionEvent (its data field is non-default). Those are closed-set dispatch enums where consumers should get the compile-time signal when a new variant lands.
  • cargo check and cargo clippy pass with the workspace's --features test-support -D warnings -D clippy::unwrap_used -D clippy::disallowed_macros -D clippy::await_holding_invalid_type settings. cargo test --lib --features test-support passes (126/126). The full cargo test --features test-support run hit failures in the e2e harness that look unrelated to this change (worth confirming in CI).

When the @github/copilot schema gains an optional field on a wire type,
every Rust test fixture that constructs that type by struct literal
breaks until each one is hand-edited (cf.
70eb60e in #1270). Test fixtures
shouldn't have to acknowledge new wire fields they don't care about.

Teach the Rust codegen to derive `Default` on every generated struct
and string-style enum (with `#[default]` on the `Unknown` catch-all
where one exists), and propagate non-default-ness across structs that
have a required field of an untagged-enum type (which has no obvious
default variant). Also derive `Default` on `RequestId` for parity with
`SessionId`. With this in place, the three internal Model fixtures can
spell themselves with `..Default::default()` and survive future
additive schema changes without manual intervention.

Production code stays explicit: the rust-coding skill already forbids
`..Default::default()` outside tests, and that rule is unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@tclem tclem requested a review from a team as a code owner May 13, 2026 04:03
Copilot AI review requested due to automatic review settings May 13, 2026 04:03
main added explicit None initializers for the new model_picker_*
fields; the merge layered those on top of our ..Default::default()
making them redundant.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Rust code generator to derive Default on most generated wire-protocol types so Rust tests/fixtures can use ..Default::default() and remain resilient to additive schema changes (new optional fields), reducing churn on schema bumps.

Changes:

  • Updated Rust codegen to derive Default for generated structs and for generated string/const-string enums (defaulting to the forward-compat Unknown / sole variant), with an opt-out path for structs that require non-defaultable untagged-union types.
  • Regenerated Rust protocol types to include the new Default derives and #[default] enum markers.
  • Updated a few Rust fixtures to use ..Default::default() and derived Default for RequestId for parity with SessionId.
Show a summary per file
File Description
scripts/codegen/rust.ts Adds Default derivation logic for generated structs/enums and tracks non-defaultable types to avoid invalid derives for untagged unions.
rust/src/generated/api_types.rs Regenerated API wire types to derive Default across structs/enums.
rust/src/generated/session_events.rs Regenerated session event wire types to derive Default across structs/enums (and SessionEventType).
rust/src/types.rs Adds Default derive to RequestId.
rust/src/lib.rs Updates internal test fixtures to use ..Default::default() for Model.
rust/tests/e2e/client.rs Updates E2E test fixture Model construction to use ..Default::default().

Copilot's findings

  • Files reviewed: 4/6 changed files
  • Comments generated: 1

Comment thread scripts/codegen/rust.ts
tclem and others added 2 commits May 13, 2026 09:56
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@stephentoub
Copy link
Copy Markdown
Collaborator

That all looks good, thanks!

My one question was on this:
"I deliberately did not derive Default on SessionEventData (the adjacent-tagged event payload enum) or on TypedSessionEvent (its data field is non-default). Those are closed-set dispatch enums where consumers should get the compile-time signal when a new variant lands."

Can you elaborate on that further?

@tclem
Copy link
Copy Markdown
Member Author

tclem commented May 13, 2026

Worth spelling out the rationale for the two types in this PR that intentionally don't derive Default:

SessionEventData is an adjacent-tagged sum (#[serde(tag = "type", content = "data")]) where each variant carries a typed payload — it's the dispatch enum consumers match on to handle events. Deriving Default here would actively undermine the only useful thing exhaustive enums give us: when a schema bump lands a new event type, every downstream match becomes a compile error and the consumer is forced to decide what to do with the new shape. That's the behavior we want for event dispatch — new events should not silently fall through a handler chain. Picking any specific variant as the "default" would also be a lie (SessionEventData::default() doesn't mean "an idle event"). And nobody constructs these from scratch anyway — they come in from the CLI; the SDK reads them. There's no fixture-pain to solve here the way there was for Model.

This is also why I haven't reached for #[non_exhaustive] on it as a separate decision: that would convert the compile error into a silently-swallowed wildcard arm — exactly the opposite of what we want for closed-set dispatch enums. Same reason tonic doesn't mark service traits non-exhaustive.

TypedSessionEvent has a required payload: SessionEventData field, so the no-Default-on-payload poisons the wrapper transitively (this is the codegen's nonDefaultableTypes propagation). Even setting that aside, the struct also has required id: String and timestamp: String fields — a TypedSessionEvent::default() would produce an event with no identity and a fake type, which downstream code (subscribers index by ID, parent_id chains assume non-empty IDs) would mis-handle. Explicit construction is the only safe option, and it's what the rest of the codebase already does for these.

  Generated via Copilot (Claude Opus 4.7) on behalf of @tclem

tclem added 2 commits May 13, 2026 14:45
…ults-in-tests

# Conflicts:
#	rust/src/generated/api_types.rs
…ults-in-tests

# Conflicts:
#	rust/src/generated/api_types.rs
@stephentoub stephentoub added this pull request to the merge queue May 14, 2026
Merged via the queue into main with commit 0c45d17 May 14, 2026
23 checks passed
@stephentoub stephentoub deleted the tclem/rust-skill-defaults-in-tests branch May 14, 2026 01:42
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.

3 participants