fix: prelude-shadowing names (#90)

Author: dotkas

crates/oapi-codegen/src/console.rs

@@ -278,6 +278,7 @@ fn hints_for(err: &Error) -> Vec<String> {
         | Error::UnsupportedContentType { hint, .. }
         | Error::TypeNameCollision { hint, .. }
         | Error::DuplicateTypeName { hint, .. }
+        | Error::PreludeShadowing { hint, .. }
         | Error::OperationTypeCollision { hint, .. }
         | Error::SchemaNameCollision { hint, .. }
         | Error::RecursiveAlias { hint, .. }

crates/oapi-codegen/src/emit/mod.rs

@@ -89,15 +89,73 @@ pub fn emit_module(module: &Module, server_urls: Option<&ServerUrls>) -> Result<
 
 /// Which generator interfaces to emit alongside the shared per-operation types.
 ///
-/// At least one field is set whenever [`emit_flat`] is called.
-#[derive(Debug, Clone, Copy)]
+/// At least one field is set whenever [`emit_flat`] is called. The default, with
+/// no field set, is models-only generation.
+#[derive(Debug, Clone, Copy, Default)]
 pub struct Targets {
     /// Emit the axum server interface.
     pub server: bool,
     /// Emit the blocking `reqwest` client.
     pub client: bool,
 }
 
+/// A Rust prelude type that the emitted file names without a path, and what
+/// needs it.
+#[derive(Debug, Clone, Copy)]
+pub struct PreludeTypeName {
+    /// The prelude identifier, for example `Option`.
+    pub name: &'static str,
+    /// What generated code can name it for, for example `every optional field`,
+    /// used in the shadowing error.
+    pub used_for: &'static str,
+}
+
+/// The prelude types the requested `targets` name without a path.
+///
+/// A generated type of one of these names does not duplicate any item, so no
+/// collision check sees it. It shadows the prelude inside the file, and every
+/// use of the shadowed type stops compiling, so
+/// [`crate::lower::check_prelude_shadowing`] rejects it up front.
+///
+/// `Ok`, `Err`, `Some`, and `None` are absent, and belong in no list, but the
+/// reason is narrow. Those name values, and a *braced* `struct`, an `enum`, and
+/// an alias each take a type name only. A tuple or unit `struct` would take the
+/// value name too, and a model named `Ok` would then hide the prelude variant.
+/// The emitter writes `pub struct #name {..}` at every site, and
+/// `every_generated_struct_is_braced` holds it there. Fixture
+/// `combined_prelude_value_names` compiles the adversarial case: an operation
+/// references each of the four names, so none is pruned, and a server and a
+/// client then write `Ok(..)`, `Err(..)`, `Some(..)`, and `None` without a path
+/// beside models of those names.
+pub fn prelude_type_names(targets: Targets) -> Vec<PreludeTypeName> {
+    // Models carry the first four whichever target asks for them.
+    let mut names = vec![
+        PreludeTypeName {
+            name: "Option",
+            used_for: "every optional field",
+        },
+        PreludeTypeName {
+            name: "String",
+            used_for: "every string field",
+        },
+        PreludeTypeName {
+            name: "Vec",
+            used_for: "every array field",
+        },
+        PreludeTypeName {
+            name: "Box",
+            used_for: "the indirection a recursive schema takes",
+        },
+    ];
+    if targets.server || targets.client {
+        names.push(PreludeTypeName {
+            name: "Result",
+            used_for: "every generated method signature",
+        });
+    }
+    return names;
+}
+
 /// A fixed type name the generator emits at the crate root for a given target,
 /// which a component-schema model must not collide with.
 #[derive(Debug, Clone, Copy)]

crates/oapi-codegen/src/error.rs

@@ -220,6 +220,24 @@ pub enum Error {
         hint: String,
     },
 
+    /// A generated type took the name of a Rust prelude type that the emitted
+    /// code writes unqualified, such as `Option` or `Vec`.
+    ///
+    /// The name does not duplicate an emitted item, so no other collision check
+    /// sees it. It shadows the prelude inside the generated file instead, and
+    /// every use of the shadowed type there stops compiling.
+    PreludeShadowing {
+        /// The Rust type name that shadows the prelude.
+        name: String,
+        /// What generated code can name the shadowed type for, for example
+        /// `every optional field`. The check reads names, not uses, so the file
+        /// at hand does not have to hold one.
+        used_for: String,
+        /// How to resolve the clash. Rendered by the console as a hint, and not
+        /// by `Display`, so the console does not print it twice.
+        hint: String,
+    },
+
     /// Two emitted items took one Rust type name, and at least one of them came
     /// from an inline schema that lowering hoisted to the crate root.
     ///
@@ -438,6 +456,12 @@ impl std::fmt::Display for Error {
                     "generated {artifact} `{name}` collides with a component schema of the same name"
                 );
             }
+            Error::PreludeShadowing { name, used_for, .. } => {
+                return write!(
+                    f,
+                    "generated type `{name}` shadows the Rust prelude type of that name, which generated code can name without a path for {used_for}"
+                );
+            }
             Error::DuplicateTypeName { name, .. } => {
                 return write!(f, "two generated items both take the Rust type name `{name}`");
             }
@@ -511,6 +535,7 @@ impl std::error::Error for Error {
             | Error::SchemaDepthExceeded { .. }
             | Error::TypeNameCollision { .. }
             | Error::DuplicateTypeName { .. }
+            | Error::PreludeShadowing { .. }
             | Error::OperationTypeCollision { .. }
             | Error::SchemaNameCollision { .. }
             | Error::RecursiveAlias { .. }

crates/oapi-codegen/src/lib.rs

@@ -87,12 +87,14 @@ pub fn generate(spec_path: &Path, config: &Config) -> Result<String> {
             client: want_client,
         };
         lower::check_type_name_collisions(&service, &module, &emit::reserved_type_names(targets))?;
+        lower::check_prelude_shadowing(&module, targets)?;
         return emit::emit_flat(&module, &service, server_urls.as_ref(), targets);
     }
     // Models-only generation prunes nothing, so the module holds every schema and
     // every collision reports.
     names.check_emitted(&module)?;
     lower::check_duplicate_models(&module)?;
+    lower::check_prelude_shadowing(&module, emit::Targets::default())?;
     lower::box_recursive_types(&mut module)?;
     return emit::emit_module(&module, server_urls.as_ref());
 }
@@ -117,6 +119,7 @@ pub fn generate_models_string(spec_path: &Path) -> Result<String> {
     // Every schema becomes an item here, so every collision reaches the file.
     names.check_emitted(&module)?;
     lower::check_duplicate_models(&module)?;
+    lower::check_prelude_shadowing(&module, emit::Targets::default())?;
     lower::box_recursive_types(&mut module)?;
     let code = emit::emit_module(&module, None)?;
     return Ok(code);

crates/oapi-codegen/src/lower/mod.rs

@@ -21,6 +21,7 @@ pub use crate::lower::prune::prune_unused_models;
 pub use crate::lower::recurse::box_recursive_types;
 pub use crate::lower::rename::TypeNames;
 pub use crate::lower::rename::check_duplicate_models;
+pub use crate::lower::rename::check_prelude_shadowing;
 pub use crate::lower::rename::check_type_name_collisions;
 pub use crate::lower::rename::rewrite_service;
 pub use crate::lower::rename::type_renames;

crates/oapi-codegen/src/lower/rename.rs

@@ -24,6 +24,7 @@ use crate::config::OUTPUT_OPTIONS_KEY;
 use crate::config::RESPONSE_TYPE_SUFFIX_KEY;
 use crate::config::TYPE_NAME_SUFFIX_KEY;
 use crate::emit::ReservedTypeName;
+use crate::emit::Targets;
 use crate::error::Error;
 use crate::error::Result;
 use crate::ir::EnumKind;
@@ -422,6 +423,45 @@ pub fn check_duplicate_models(module: &Module) -> Result<()> {
     return diagnostics.into_result();
 }
 
+/// Fail generation if an emitted item takes the name of a prelude type that the
+/// file names without a path.
+///
+/// This is not a duplicate-name check. A schema named `Option` emits one item,
+/// so [`check_duplicate_models`] and [`check_type_name_collisions`] both pass.
+/// The item shadows `Option` for the whole file instead, and every `Option<T>`
+/// in it then reads as that struct.
+///
+/// Every generation mode calls this check. `targets` says which names to hold,
+/// because only a server or a client writes `Result`.
+///
+/// The check reads names, not uses, so it is wider than it has to be. A spec
+/// with a schema named `Box` and no recursion writes no `Box<T>`, and it would
+/// compile. It is still rejected. Two reasons keep it that way: a false
+/// rejection is loud and has a one-line remedy in the hint, while a missed use
+/// site emits code that does not compile, which is the failure this check
+/// exists to stop. The verdict also stays put. Adding a recursive schema later
+/// cannot turn an accepted name into a broken build.
+///
+/// # Errors
+///
+/// Returns one [`Error::PreludeShadowing`] for a single name, or an
+/// [`Error::Validation`] that holds all of them.
+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();
+}
+
 /// What claimed one crate-root type name.
 enum Claim {
     /// A component model, or an inline schema that lowering hoisted to the crate

crates/oapi-codegen/tests/coverage.rs

@@ -338,6 +338,21 @@ const TEST_TABLE: &[Feature] = &[
         status: Status::Supported,
         fixture: Some("type_name_collisions"),
     },
+    Feature {
+        element: "naming.prelude-value-names",
+        status: Status::Supported,
+        fixture: Some("prelude_value_names"),
+    },
+    Feature {
+        element: "naming.prelude-type-names",
+        status: Status::Unsupported,
+        fixture: Some("unsupported_prelude_shadowing"),
+    },
+    Feature {
+        element: "naming.prelude-type-names.target-scoped",
+        status: Status::Supported,
+        fixture: Some("prelude_result_name"),
+    },
     // Document-level (server/client generation). The axum server generator now
     // covers a slice of paths/parameters/requestBody/responses; the blocking
     // reqwest client generator additionally covers securitySchemes. Those slices
@@ -501,6 +516,7 @@ const CLIENT_UNSUPPORTED_FIXTURES: &[&str] = &[
 /// flat crate-root layout in which the server and client share one file and the
 /// same per-operation types alongside the component models.
 const COMBINED_FIXTURES: &[&str] = &[
+    "combined_prelude_value_names",
     "combined_server_client",
     "combined_response_name_collision",
     "combined_x_rust_derive",
@@ -630,6 +646,8 @@ generated_tests!(
     number_formats,
     object_additional_properties,
     object_deny_unknown_fields,
+    prelude_result_name,
+    prelude_value_names,
     schema_defaults,
     object_nested_inline,
     object_optional_required,
@@ -1157,6 +1175,7 @@ macro_rules! combined_generated_tests {
 }
 
 combined_generated_tests!(
+    combined_prelude_value_names,
     combined_server_client,
     combined_response_name_collision,
     combined_x_rust_derive,
@@ -1426,6 +1445,82 @@ fn operation_types_that_take_client_reserved_names_fail() {
     );
 }
 
+/// A model that takes the name of a prelude type the file writes without a path
+/// must stop generation. Such a model emits one item and duplicates nothing, so
+/// no other collision check sees it. It shadows the prelude instead, and the
+/// generated file stops compiling.
+#[test]
+fn a_model_that_shadows_a_prelude_type_fails() {
+    let fixture = tests_dir().join("fixtures").join("unsupported_prelude_shadowing.yaml");
+    let err = oapi_codegen::generate_models_string(&fixture).expect_err("a model named `Option` must stop generation");
+    let oapi_codegen::Error::Validation { problems } = &err else {
+        panic!("expected an aggregated Validation error, got: {err:?}");
+    };
+    // One run reports every shadowed name, so the author fixes them together.
+    assert_eq!(problems.len(), 4, "expected every shadowed name, got: {problems:?}");
+    let report = err.to_string();
+    for name in ["Option", "String", "Vec", "Box"] {
+        assert!(report.contains(name), "the report must name `{name}`, got: {report}");
+    }
+}
+
+/// The remedy must name a rename, because the prelude name is fixed.
+#[test]
+fn the_prelude_shadowing_hint_offers_a_rename() {
+    let fixture = tests_dir().join("fixtures").join("unsupported_prelude_shadowing.yaml");
+    let err = oapi_codegen::generate_models_string(&fixture).expect_err("a shadowing model must stop generation");
+    let oapi_codegen::Error::Validation { problems } = &err else {
+        panic!("expected an aggregated Validation error, got: {err:?}");
+    };
+    let first = problems.first().expect("at least one problem");
+    let oapi_codegen::Error::PreludeShadowing { hint, used_for, .. } = first else {
+        panic!("expected PreludeShadowing, got: {first:?}");
+    };
+    assert!(
+        hint.contains("x-rust-name"),
+        "the hint must offer a rename, got: {hint}"
+    );
+    assert!(!used_for.is_empty(), "the problem must say what needs the name");
+}
+
+/// A prelude name is only held when the run writes it. Models name no `Result`,
+/// so a model of that name generates. A server writes `Result` in every method
+/// signature, so the same document fails there.
+#[test]
+fn prelude_name_check_is_target_scoped() {
+    let fixture = tests_dir().join("fixtures").join("prelude_result_name.yaml");
+    oapi_codegen::generate_models_string(&fixture).expect("models alone name no `Result`");
+    let err = oapi_codegen::generate(&fixture, &server_config()).expect_err("a server writes `Result` in every method");
+    assert!(
+        matches!(err, oapi_codegen::Error::PreludeShadowing { .. }),
+        "expected PreludeShadowing, got: {err:?}",
+    );
+}
+
+/// A value name is not a type name. `Ok`, `Err`, `Some`, and `None` name values,
+/// and a *braced* `struct` takes a type name only, so those four must generate.
+/// The generated file under `tests/generated` compiles as proof.
+///
+/// This covers models-only output. `combined_prelude_value_names` covers the
+/// harder case, where a server and a client write `Ok(..)`, `Err(..)`, `Some(..)`,
+/// and `None` around models of those names. Both rest on
+/// `every_generated_struct_is_braced`.
+#[test]
+fn a_model_named_after_a_prelude_value_generates() {
+    let fixture = tests_dir().join("fixtures").join("prelude_value_names.yaml");
+    let code = oapi_codegen::generate_models_string(&fixture).expect("a prelude value name is free");
+    assert!(
+        code.contains("pub struct Ok") && code.contains("pub struct None"),
+        "the models must keep their names, got: {code}",
+    );
+    // The same file still writes `Option` for an optional field, which proves the
+    // four names left it alone.
+    assert!(
+        code.contains("pub maybe: Option<String>"),
+        "the file must still name the prelude `Option`, got: {code}",
+    );
+}
+
 /// A reserved name is only reserved when its target is requested. The server-only
 /// `Api` trait must not block an operation whose response enum takes that name in a
 /// client-only run.
@@ -2108,6 +2203,48 @@ fn fixtures_and_test_table_agree() {
     }
 }
 
+/// Every generated `struct` must be braced, never a tuple or a unit `struct`.
+///
+/// This is what keeps `Ok`, `Err`, `Some`, and `None` free as schema names. A
+/// braced `struct` takes a type name only. A tuple or unit `struct` takes the
+/// value name of that identifier too, so a model named `Ok` would then shadow the
+/// prelude variant, and every `Ok(..)` the generators write would stop compiling.
+///
+/// The invariant is implicit in the emitter, which writes `pub struct #name {..}`
+/// at every site. This test states it, so a newtype added later fails here rather
+/// than in a consumer's build.
+#[test]
+fn every_generated_struct_is_braced() {
+    let dir = tests_dir().join("generated");
+    let entries = std::fs::read_dir(&dir).expect("read generated dir");
+    let mut checked = 0_usize;
+    for entry in entries {
+        let path = entry.expect("dir entry").path();
+        if !path.extension().is_some_and(|ext| {
+            return ext == "rs";
+        }) {
+            continue;
+        }
+        let source = std::fs::read_to_string(&path).expect("read generated file");
+        for line in source.lines() {
+            let Some(rest) = line.trim_start().strip_prefix("pub struct ") else {
+                continue;
+            };
+            checked += 1;
+            // A braced struct opens its body, or its generics, before anything
+            // else. A tuple struct opens `(` and a unit struct ends at `;`.
+            let tail = rest.trim_end();
+            assert!(
+                tail.ends_with('{'),
+                "`{}` in {} is not a braced struct, which would take a prelude value name",
+                tail,
+                path.display(),
+            );
+        }
+    }
+    assert!(checked > 0, "no generated struct was checked, so the scan is broken");
+}
+
 /// The test table itself must be well-formed: unique elements, and every
 /// supported/unsupported row backed by a fixture.
 #[test]