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() {}