fix: prelude-shadowing names - #90
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new validation to prevent generated model type names from shadowing unqualified Rust prelude type identifiers used in emitted code (e.g. Option, String, etc.), along with docs and fixtures/tests to cover the behavior (including target-scoping for Result).
Changes:
- Introduces
check_prelude_shadowingand wires it into generation flows (models-only and server/client). - Defines a target-scoped list of unqualified prelude type names used by emitted code (
prelude_type_names) and a newPreludeShadowingerror surfaced with console hints. - Adds fixtures + generated-code compile tests + coverage table entries documenting and exercising the new validation.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/design.md | Documents prelude-name shadowing behavior and target scoping. |
| crates/oapi-codegen/src/lower/rename.rs | Adds check_prelude_shadowing validation that aggregates multiple problems. |
| crates/oapi-codegen/src/lower/mod.rs | Re-exports the new validation helper. |
| crates/oapi-codegen/src/lib.rs | Calls the new validation in both flat (server/client) and models-only generation paths. |
| crates/oapi-codegen/src/error.rs | Adds Error::PreludeShadowing and its Display formatting. |
| crates/oapi-codegen/src/emit/mod.rs | Adds Targets: Default and the prelude_type_names(targets) list used by validation. |
| crates/oapi-codegen/src/console.rs | Ensures PreludeShadowing hints are printed. |
| crates/oapi-codegen/tests/generated.rs | Registers new generated-code modules for compilation. |
| crates/oapi-codegen/tests/coverage.rs | Adds feature entries and tests covering the new validation + target scoping. |
| crates/oapi-codegen/tests/fixtures/unsupported_prelude_shadowing.yaml | New fixture to assert shadowing prelude type names fails. |
| crates/oapi-codegen/tests/fixtures/prelude_result_name.yaml | New fixture to assert Result is only reserved when server/client targets are generated. |
| crates/oapi-codegen/tests/fixtures/prelude_value_names.yaml | New fixture asserting certain names can be generated in models-only output. |
| crates/oapi-codegen/tests/generated/prelude_result_name.rs | New generated-code baseline for compilation. |
| crates/oapi-codegen/tests/generated/prelude_value_names.rs | New generated-code baseline for compilation. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (4)
crates/oapi-codegen/tests/fixtures/combined_prelude_value_names.yaml:48
- The fixture comment claims every operation references
Ok,Err,Some, andNoneso pruning keeps them, but/probe’s204response has no schema, soNoneis currently not referenced by any operation and can be pruned. This makes the comment misleading and weakens the intended “all four names present” adversarial case.
# The adversarial case for `Ok`, `Err`, `Some`, and `None`. Every operation
# here references one, so pruning keeps them, and both generators emit
# `Ok(..)`, `Err(..)`, `Some(..)`, and `None` without a path around them. The
# response enums below even read `Ok(Ok)` and `Ok(Some)`.
docs/design.md:205
- This paragraph states that
combined_prelude_value_namescompiles “models namedOk,Err,Some, andNonebeside a server and a client”, but thecombined_prelude_value_namesfixture doesn’t reference theNoneschema from any operation, so the combined output can prune it. Consider updating the text (or the fixture) so the documented adversarial case matches what’s actually compiled.
states it. The `combined_prelude_value_names` fixture compiles the adversarial
case: models named `Ok`, `Err`, `Some`, and `None` beside a server and a client
that write all four unqualified, including response enums that read `Ok(Ok)` and
`Ok(Some)`.
crates/oapi-codegen/src/emit/mod.rs:128
prelude_type_namesdocs mentioncombined_prelude_value_namesas compiling a server+client that writesOk(..),Err(..),Some(..), andNonearound models of those names, but the fixture currently doesn’t reference theNoneschema from any operation, so combined output can prune it. Either referenceNonefrom an operation in the fixture, or adjust this doc comment to avoid claimingNoneis present in the compiled combined output.
/// `every_generated_struct_is_braced` holds it there. Fixture
/// `combined_prelude_value_names` compiles the adversarial case: a server and a
/// client that write `Ok(..)`, `Err(..)`, `Some(..)`, and `None` around models of
/// those names.
crates/oapi-codegen/src/lower/rename.rs:454
check_prelude_shadowingrejects any emitted item namedOption/String/Vec/Boxeven if the generated file for a given spec would never actually use that prelude identifier (e.g.,Boxis only emitted whenbox_recursive_typesintroducesRustType::Boxedfor recursive schemas). This is a behavior change that can unnecessarily fail otherwise-compilable specs that happen to define (but not trigger uses of) these names. Consider scoping the held-name set to the types actually used in the lowered module/service (similar to the target-scoping already done forResult).
pub fn check_prelude_shadowing(module: &Module, targets: Targets) -> Result<()> {
let mut diagnostics = crate::lower::validate::Diagnostics::new();
let prelude = crate::emit::prelude_type_names(targets);
for item in &module.items {
let Some(shadowed) = prelude.iter().find(|entry| return entry.name == item.name()) else {
continue;
};
diagnostics.push(Error::PreludeShadowing {
name: shadowed.name.to_owned(),
used_for: shadowed.used_for.to_owned(),
hint: format!("Rename the schema with `{X_RUST_NAME}`, or with `output-options.type-name-suffix`."),
});
}
return diagnostics.into_result();
|
🎉 This PR is included in version 1.0.0-dev.28 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
No description provided.