Skip to content

Commit

Permalink
Try harder to disambiguate enum variant identifiers (#762)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ahl
ahl authored Feb 3, 2025
1 parent 407e360 commit 54e7e3c
Show file tree
Hide file tree
Showing 10 changed files with 337 additions and 207 deletions.
18 changes: 8 additions & 10 deletions typify-impl/src/convert.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Oxide Computer Company
// Copyright 2025 Oxide Computer Company

use std::collections::BTreeSet;

Expand All @@ -7,7 +7,7 @@ use crate::type_entry::{
EnumTagType, TypeEntry, TypeEntryDetails, TypeEntryEnum, TypeEntryNewtype, TypeEntryStruct,
Variant, VariantDetails,
};
use crate::util::{all_mutually_exclusive, recase, ref_key, Case, StringValidator};
use crate::util::{all_mutually_exclusive, ref_key, StringValidator};
use log::{debug, info};
use schemars::schema::{
ArrayValidation, InstanceType, Metadata, ObjectValidation, Schema, SchemaObject, SingleOrVec,
Expand Down Expand Up @@ -918,14 +918,12 @@ impl TypeSpace {
has_null = true;
None
}
serde_json::Value::String(value) if validator.is_valid(value) => {
let (name, rename) = recase(value, Case::Pascal);
Some(Ok(Variant {
name,
rename,
description: None,
details: VariantDetails::Simple,
}))
serde_json::Value::String(variant_name) if validator.is_valid(variant_name) => {
Some(Ok(Variant::new(
variant_name.clone(),
None,
VariantDetails::Simple,
)))
}

// Ignore enum variants whose strings don't match the given
Expand Down
14 changes: 5 additions & 9 deletions typify-impl/src/defaults.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2022 Oxide Computer Company
// Copyright 2025 Oxide Computer Company

use std::collections::BTreeMap;

Expand Down Expand Up @@ -384,7 +384,7 @@ pub(crate) fn validate_default_for_external_enum(
if let Some(simple_name) = default.as_str() {
let variant = variants
.iter()
.find(|variant| simple_name == variant.rename.as_ref().unwrap_or(&variant.name))?;
.find(|variant| simple_name == variant.raw_name)?;
matches!(&variant.details, VariantDetails::Simple).then(|| ())?;

Some(DefaultKind::Specific)
Expand All @@ -396,9 +396,7 @@ pub(crate) fn validate_default_for_external_enum(

let (name, value) = map.iter().next()?;

let variant = variants
.iter()
.find(|variant| name == variant.rename.as_ref().unwrap_or(&variant.name))?;
let variant = variants.iter().find(|variant| name == &variant.raw_name)?;

match &variant.details {
VariantDetails::Simple => None,
Expand All @@ -419,9 +417,7 @@ pub(crate) fn validate_default_for_internal_enum(
) -> Option<DefaultKind> {
let map = default.as_object()?;
let name = map.get(tag).and_then(serde_json::Value::as_str)?;
let variant = variants
.iter()
.find(|variant| name == variant.rename.as_ref().unwrap_or(&variant.name))?;
let variant = variants.iter().find(|variant| name == variant.raw_name)?;

match &variant.details {
VariantDetails::Simple => Some(DefaultKind::Specific),
Expand Down Expand Up @@ -462,7 +458,7 @@ pub(crate) fn validate_default_for_adjacent_enum(

let variant = variants
.iter()
.find(|variant| tag_value == variant.rename.as_ref().unwrap_or(&variant.name))?;
.find(|variant| tag_value == variant.raw_name)?;

match (&variant.details, content_value) {
(VariantDetails::Simple, None) => Some(DefaultKind::Specific),
Expand Down
106 changes: 41 additions & 65 deletions typify-impl/src/enums.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Oxide Computer Company
// Copyright 2025 Oxide Computer Company

use std::collections::{BTreeMap, BTreeSet, HashSet};

Expand All @@ -18,7 +18,7 @@ use crate::{
},
util::{
constant_string_value, get_object, get_type_name, metadata_description,
metadata_title_and_description, recase, schema_is_named, Case,
metadata_title_and_description, schema_is_named,
},
Name, Result, TypeSpace,
};
Expand Down Expand Up @@ -211,15 +211,11 @@ impl TypeSpace {
ProtoVariant::Simple {
name: variant_name,
description,
} => {
let (name, rename) = recase(variant_name, Case::Pascal);
Some(Variant {
name,
rename,
description,
details: VariantDetails::Simple,
})
}
} => Some(Variant::new(
variant_name.to_string(),
description,
VariantDetails::Simple,
)),

ProtoVariant::Typed {
name: variant_name,
Expand All @@ -233,13 +229,7 @@ impl TypeSpace {
.ok()?;
deny_unknown_fields |= deny;

let (name, rename) = recase(variant_name, Case::Pascal);
Some(Variant {
name,
rename,
description,
details,
})
Some(Variant::new(variant_name.to_string(), description, details))
}
})
.collect::<Option<Vec<_>>>()?;
Expand Down Expand Up @@ -409,23 +399,19 @@ impl TypeSpace {
if validation.properties.len() == 1 {
let (tag_name, schema) = validation.properties.iter().next().unwrap();
let variant_name = constant_string_value(schema).unwrap();
let (name, rename) = recase(variant_name, Case::Pascal);

// The lone property must be our tag.
assert_eq!(tag_name, tag);
assert_eq!(validation.required.len(), 1);

let variant = Variant {
name,
rename,
description: None,
details: VariantDetails::Simple,
};
Ok(variant)
Ok(Variant::new(
variant_name.to_string(),
None,
VariantDetails::Simple,
))
} else {
let tag_schema = validation.properties.get(tag).unwrap();
let variant_name = constant_string_value(tag_schema).unwrap();
let (name, rename) = recase(variant_name, Case::Pascal);

// Make a new object validation that omits the tag.
let mut new_validation = validation.clone();
Expand All @@ -434,13 +420,11 @@ impl TypeSpace {

let (properties, _) =
self.struct_members(enum_type_name.into_option(), &new_validation)?;
let variant = Variant {
name,
rename,
description: metadata_title_and_description(metadata),
details: VariantDetails::Struct(properties),
};
Ok(variant)
Ok(Variant::new(
variant_name.to_string(),
metadata_title_and_description(metadata),
VariantDetails::Struct(properties),
))
}
}

Expand Down Expand Up @@ -543,23 +527,16 @@ impl TypeSpace {
if validation.properties.len() == 1 {
let (tag_name, schema) = validation.properties.iter().next().unwrap();
let variant_name = constant_string_value(schema).unwrap();
let (name, rename) = recase(variant_name, Case::Pascal);

// The lone property must be our tag.
assert_eq!(tag_name, tag);
assert_eq!(validation.required.len(), 1);

let variant = Variant {
name,
rename,
description: None,
details: VariantDetails::Simple,
};
let variant = Variant::new(variant_name.to_string(), None, VariantDetails::Simple);
Ok((variant, false))
} else {
let tag_schema = validation.properties.get(tag).unwrap();
let variant_name = constant_string_value(tag_schema).unwrap();
let (name, rename) = recase(variant_name, Case::Pascal);

let sub_type_name = match enum_type_name {
// If the type name is known (required) we append the name of
Expand All @@ -582,12 +559,11 @@ impl TypeSpace {
let content_schema = validation.properties.get(content).unwrap();
let (details, deny) = self.external_variant(sub_type_name, content_schema)?;

let variant = Variant {
name,
rename,
description: metadata_title_and_description(metadata),
let variant = Variant::new(
variant_name.to_string(),
metadata_title_and_description(metadata),
details,
};
);
Ok((variant, deny))
}
}
Expand Down Expand Up @@ -692,14 +668,9 @@ impl TypeSpace {

let variants = variant_details
.into_iter()
.map(|(details, name)| {
assert!(!name.is_empty());
Variant {
name,
rename: None,
description: None,
details,
}
.map(|(details, variant_name)| {
assert!(!variant_name.is_empty());
Variant::new(variant_name, None, details)
})
.collect();

Expand Down Expand Up @@ -738,18 +709,20 @@ pub(crate) fn output_variant(
output: &mut OutputSpace,
type_name: &str,
) -> TokenStream {
let name = format_ident!("{}", variant.name);
let ident_name = variant.ident_name.as_ref().unwrap();
let variant_name = format_ident!("{}", ident_name);
let doc = variant.description.as_ref().map(|s| {
quote! { #[doc = #s] }
});
let serde = variant.rename.as_ref().map(|s| {
let serde = (&variant.raw_name != ident_name).then(|| {
let s = &variant.raw_name;
quote! { #[serde(rename = #s)] }
});
match &variant.details {
VariantDetails::Simple => quote! {
#doc
#serde
#name,
#variant_name,
},
VariantDetails::Item(type_id) => {
let item_type_ident = type_space
Expand All @@ -761,7 +734,7 @@ pub(crate) fn output_variant(
quote! {
#doc
#serde
#name(#item_type_ident),
#variant_name(#item_type_ident),
}
}

Expand All @@ -778,7 +751,7 @@ pub(crate) fn output_variant(
quote! {
#doc
#serde
#name(#(#types),*),
#variant_name(#(#types),*),
}
} else {
// A tuple variant with a single element requires special
Expand All @@ -788,7 +761,7 @@ pub(crate) fn output_variant(
quote! {
#doc
#serde
#name((#(#types,)*)),
#variant_name((#(#types,)*)),
}
}
}
Expand All @@ -799,7 +772,7 @@ pub(crate) fn output_variant(

let prop_type_entry = type_space.id_to_entry.get(&prop.type_id).unwrap();
let (prop_serde, _) = generate_serde_attr(
&format!("{}{}", type_name, &variant.name),
&format!("{}{}", type_name, variant.ident_name.as_ref().unwrap()),
&prop.name,
&prop.rename,
&prop.state,
Expand All @@ -820,7 +793,7 @@ pub(crate) fn output_variant(
quote! {
#doc
#serde
#name {
#variant_name {
#(#prop_streams)*
},
}
Expand Down Expand Up @@ -1210,7 +1183,7 @@ mod tests {
{
let variant_names = variants
.iter()
.map(|variant| variant.name.clone())
.map(|variant| variant.ident_name.as_ref().unwrap().clone())
.collect::<HashSet<_>>();
assert_eq!(variant_names.len(), variants.len());
assert_eq!(tag_type, &EnumTagType::Untagged);
Expand Down Expand Up @@ -1328,7 +1301,10 @@ mod tests {
match &variant.details {
VariantDetails::Item(item) => {
let variant_type = type_space.id_to_entry.get(item).unwrap();
assert!(variant_type.name().unwrap().ends_with(&variant.name));
assert!(variant_type
.name()
.unwrap()
.ends_with(variant.ident_name.as_ref().unwrap()));
}
_ => panic!("{:#?}", type_entry),
}
Expand Down
8 changes: 4 additions & 4 deletions typify-impl/src/lib.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Oxide Computer Company
// Copyright 2025 Oxide Computer Company

//! typify backend implementation.
Expand Down Expand Up @@ -1158,7 +1158,7 @@ impl<'a> TypeEnum<'a> {
),
};
TypeEnumVariantInfo {
name: variant.name.as_str(),
name: variant.ident_name.as_ref().unwrap(),
description: variant.description.as_deref(),
details,
}
Expand Down Expand Up @@ -1345,7 +1345,7 @@ mod tests {
}
let var_names = variants
.iter()
.map(|variant| variant.name.clone())
.map(|variant| variant.ident_name.as_ref().unwrap().clone())
.collect::<HashSet<_>>();
assert_eq!(
var_names,
Expand Down Expand Up @@ -1391,7 +1391,7 @@ mod tests {
let variants = variants
.iter()
.map(|v| match v.details {
VariantDetails::Simple => v.name.clone(),
VariantDetails::Simple => v.ident_name.as_ref().unwrap().clone(),
_ => panic!("unexpected variant type"),
})
.collect::<HashSet<_>>();
Expand Down
Loading

0 comments on commit 54e7e3c

Please sign in to comment.