From 5d88a38f29aeddf7bf6c471bb0013a6546558962 Mon Sep 17 00:00:00 2001 From: Daniel Eades Date: Sun, 12 Jul 2026 12:32:02 +0100 Subject: [PATCH] Disambiguate duplicate enum variant names instead of panicking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When enum values sanitize to identical identifiers (e.g. "=", ">", "≥"), generation panicked. Instead, disambiguate deterministically: in variant order, the first variant with a given name keeps it and each subsequent duplicate gets the smallest numeric suffix (starting at 2) that doesn't collide with any other variant name. Serialized names are unaffected since serde renames and the Display/FromStr impls are generated from the raw variant names. Fixes #948 --- typify-impl/src/enums.rs | 41 ++++++ typify-impl/src/type_entry.rs | 52 ++++--- .../schemas/colliding-variant-names.json | 18 +++ .../tests/schemas/colliding-variant-names.rs | 135 ++++++++++++++++++ 4 files changed, 230 insertions(+), 16 deletions(-) create mode 100644 typify/tests/schemas/colliding-variant-names.json create mode 100644 typify/tests/schemas/colliding-variant-names.rs diff --git a/typify-impl/src/enums.rs b/typify-impl/src/enums.rs index 4be20fda..222732d9 100644 --- a/typify-impl/src/enums.rs +++ b/typify-impl/src/enums.rs @@ -1237,6 +1237,47 @@ mod tests { } } + // See https://github.com/oxidecomputer/typify/issues/948 + #[test] + fn test_variant_name_collisions() { + let schema_json = r##" + { + "type": "string", + "enum": ["=", ">", "<", "≥", ">=", "≤", "<=", "≠", "!="] + } + "##; + + let schema: schemars::schema::Schema = serde_json::from_str(schema_json).unwrap(); + + let mut type_space = TypeSpace::default(); + let (type_entry, _) = type_space + .convert_schema(Name::Required("ComparisonOperator".to_string()), &schema) + .unwrap(); + + let TypeEntryDetails::Enum(TypeEntryEnum { variants, .. }) = &type_entry.details else { + panic!("expected an enum"); + }; + + // The raw names--used for serialization--are preserved, in order. + let raw_names = variants + .iter() + .map(|variant| variant.raw_name.as_str()) + .collect::>(); + assert_eq!(raw_names, ["=", ">", "<", "≥", ">=", "≤", "<=", "≠", "!="]); + + // The identifier names are disambiguated with deterministic numeric + // suffixes: the first variant with a given name keeps it and each + // subsequent duplicate gets the next available suffix. + let ident_names = variants + .iter() + .map(|variant| variant.ident_name.as_deref().unwrap()) + .collect::>(); + assert_eq!( + ident_names, + ["X", "X2", "X3", "X4", "Xx", "X5", "Xx2", "X6", "Xx3"] + ); + } + #[test] fn test_maybe_option() { let subschemas = vec![ diff --git a/typify-impl/src/type_entry.rs b/typify-impl/src/type_entry.rs index 5f3b4897..b4ff0abe 100644 --- a/typify-impl/src/type_entry.rs +++ b/typify-impl/src/type_entry.rs @@ -1,6 +1,6 @@ // Copyright 2025 Oxide Computer Company -use std::collections::{BTreeMap, BTreeSet, HashMap}; +use std::collections::{BTreeMap, BTreeSet}; use proc_macro2::{Punct, Spacing, TokenStream, TokenTree}; use quote::{format_ident, quote, ToTokens}; @@ -268,24 +268,44 @@ impl TypeEntryEnum { }); } - // If variants still aren't unique, we fail: we'd rather not emit code - // that can't compile + // If variants still aren't unique, we disambiguate deterministically: + // proceeding in variant order, the first variant with a given name + // keeps it unchanged, and each subsequent duplicate gets the smallest + // numeric suffix (starting at 2) that doesn't collide with any other + // variant name. For example, three variants that all sanitize to `X` + // become `X`, `X2`, and `X3`. Serialized names are unaffected: serde + // renames and Display/FromStr impls are all generated from the + // variant's raw name. if !variants_unique(&variants) { - let mut counts = HashMap::new(); - variants.iter().for_each(|variant| { - counts - .entry(variant.ident_name.as_ref().unwrap()) - .and_modify(|xxx| *xxx += 1) - .or_insert(0); - }); - let dups = variants + // Names of all variants prior to disambiguation; a suffixed name + // must not collide with any of these (e.g. an actual `X2` + // variant that appears later in the list). + let original_names = variants .iter() - .filter(|variant| *counts.get(variant.ident_name.as_ref().unwrap()).unwrap() > 0) - .map(|variant| variant.raw_name.as_str()) - .collect::>() - .join(","); - panic!("Failed to make unique variant names for [{}]", dups); + .map(|variant| variant.ident_name.as_ref().unwrap().clone()) + .collect::>(); + + // Names assigned so far: first occurrences and suffixed names. + let mut assigned_names = BTreeSet::new(); + for variant in variants.iter_mut() { + let name = variant.ident_name.as_ref().unwrap().clone(); + + // The first variant with a given name keeps it. + if assigned_names.insert(name.clone()) { + continue; + } + + let new_name = (2..) + .map(|ii| format!("{}{}", name, ii)) + .find(|candidate| { + !original_names.contains(candidate) && !assigned_names.contains(candidate) + }) + .unwrap(); + assigned_names.insert(new_name.clone()); + variant.ident_name = Some(new_name); + } } + debug_assert!(variants_unique(&variants)); let name = get_type_name(&type_name, metadata).unwrap(); let rename = None; diff --git a/typify/tests/schemas/colliding-variant-names.json b/typify/tests/schemas/colliding-variant-names.json new file mode 100644 index 00000000..2dba7c41 --- /dev/null +++ b/typify/tests/schemas/colliding-variant-names.json @@ -0,0 +1,18 @@ +{ + "definitions": { + "comparison-operator": { + "type": "string", + "enum": [ + "=", + ">", + "<", + "≥", + ">=", + "≤", + "<=", + "≠", + "!=" + ] + } + } +} diff --git a/typify/tests/schemas/colliding-variant-names.rs b/typify/tests/schemas/colliding-variant-names.rs new file mode 100644 index 00000000..a6fa9ec9 --- /dev/null +++ b/typify/tests/schemas/colliding-variant-names.rs @@ -0,0 +1,135 @@ +#![deny(warnings)] +#[doc = r" Error types."] +pub mod error { + #[doc = r" Error from a `TryFrom` or `FromStr` implementation."] + pub struct ConversionError(::std::borrow::Cow<'static, str>); + impl ::std::error::Error for ConversionError {} + impl ::std::fmt::Display for ConversionError { + fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> Result<(), ::std::fmt::Error> { + ::std::fmt::Display::fmt(&self.0, f) + } + } + impl ::std::fmt::Debug for ConversionError { + fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> Result<(), ::std::fmt::Error> { + ::std::fmt::Debug::fmt(&self.0, f) + } + } + impl From<&'static str> for ConversionError { + fn from(value: &'static str) -> Self { + Self(value.into()) + } + } + impl From for ConversionError { + fn from(value: String) -> Self { + Self(value.into()) + } + } +} +#[doc = "`ComparisonOperator`"] +#[doc = r""] +#[doc = r"
JSON schema"] +#[doc = r""] +#[doc = r" ```json"] +#[doc = "{"] +#[doc = " \"type\": \"string\","] +#[doc = " \"enum\": ["] +#[doc = " \"=\","] +#[doc = " \">\","] +#[doc = " \"<\","] +#[doc = " \"≥\","] +#[doc = " \">=\","] +#[doc = " \"≤\","] +#[doc = " \"<=\","] +#[doc = " \"≠\","] +#[doc = " \"!=\""] +#[doc = " ]"] +#[doc = "}"] +#[doc = r" ```"] +#[doc = r"
"] +#[derive( + :: serde :: Deserialize, + :: serde :: Serialize, + Clone, + Copy, + Debug, + Eq, + Hash, + Ord, + PartialEq, + PartialOrd, +)] +pub enum ComparisonOperator { + #[serde(rename = "=")] + X, + #[serde(rename = ">")] + X2, + #[serde(rename = "<")] + X3, + #[serde(rename = "≥")] + X4, + #[serde(rename = ">=")] + Xx, + #[serde(rename = "≤")] + X5, + #[serde(rename = "<=")] + Xx2, + #[serde(rename = "≠")] + X6, + #[serde(rename = "!=")] + Xx3, +} +impl ::std::fmt::Display for ComparisonOperator { + fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result { + match *self { + Self::X => f.write_str("="), + Self::X2 => f.write_str(">"), + Self::X3 => f.write_str("<"), + Self::X4 => f.write_str("≥"), + Self::Xx => f.write_str(">="), + Self::X5 => f.write_str("≤"), + Self::Xx2 => f.write_str("<="), + Self::X6 => f.write_str("≠"), + Self::Xx3 => f.write_str("!="), + } + } +} +impl ::std::str::FromStr for ComparisonOperator { + type Err = self::error::ConversionError; + fn from_str(value: &str) -> ::std::result::Result { + match value { + "=" => Ok(Self::X), + ">" => Ok(Self::X2), + "<" => Ok(Self::X3), + "≥" => Ok(Self::X4), + ">=" => Ok(Self::Xx), + "≤" => Ok(Self::X5), + "<=" => Ok(Self::Xx2), + "≠" => Ok(Self::X6), + "!=" => Ok(Self::Xx3), + _ => Err("invalid value".into()), + } + } +} +impl ::std::convert::TryFrom<&str> for ComparisonOperator { + type Error = self::error::ConversionError; + fn try_from(value: &str) -> ::std::result::Result { + value.parse() + } +} +impl ::std::convert::TryFrom<&::std::string::String> for ComparisonOperator { + type Error = self::error::ConversionError; + fn try_from( + value: &::std::string::String, + ) -> ::std::result::Result { + value.parse() + } +} +impl ::std::convert::TryFrom<::std::string::String> for ComparisonOperator { + type Error = self::error::ConversionError; + fn try_from( + value: ::std::string::String, + ) -> ::std::result::Result { + value.parse() + } +} +fn main() {}