Skip to content

Commit

Permalink
Fail codegen when archetype fields don't have one of the required att…
Browse files Browse the repository at this point in the history
…ributes (#8326)

### What

All of our archetypes' fields must have one of the
`attr.rerun.component_{required|recommended|optional}` attribute. Let's
fail codegen when they are missing, so we stop falling into that
pitfall.
  • Loading branch information
abey79 authored Dec 5, 2024
1 parent 83f82ef commit 76d4663
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 9 deletions.
16 changes: 8 additions & 8 deletions crates/build/re_types_builder/src/codegen/rust/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1078,7 +1078,7 @@ fn quote_trait_impls_for_archetype(obj: &Object) -> TokenStream {
compute_components(obj, ATTR_RERUN_COMPONENT_RECOMMENDED, [indicator_fqname]);
let (num_optional, optional) = compute_components(obj, ATTR_RERUN_COMPONENT_OPTIONAL, []);

let num_components_docstring = quote_doc_line(&format!(
let num_components_docstring = quote_doc_line(&format!(
"The total number of components in the archetype: {num_required} required, {num_recommended} recommended, {num_optional} optional"
));
let num_all = num_required + num_recommended + num_optional;
Expand All @@ -1090,7 +1090,7 @@ fn quote_trait_impls_for_archetype(obj: &Object) -> TokenStream {
.collect::<Vec<_>>();

let all_component_batches = {
std::iter::once(quote!{
std::iter::once(quote! {
Some(Self::indicator())
}).chain(obj.fields.iter().map(|obj_field| {
let field_name = format_ident!("{}", obj_field.name);
Expand All @@ -1104,7 +1104,7 @@ fn quote_trait_impls_for_archetype(obj: &Object) -> TokenStream {
if is_plural {
// Always log Option<Vec<C>> as Vec<V>, mapping None to empty batch
let component_type = quote_field_type_from_typ(&obj_field.typ, false).0;
quote!{
quote! {
Some((
if let Some(comp_batch) = &self.#field_name {
(comp_batch as &dyn ComponentBatch)
Expand All @@ -1118,20 +1118,20 @@ fn quote_trait_impls_for_archetype(obj: &Object) -> TokenStream {
}
} else {
// Always log Option<C>, mapping None to empty batch
quote!{ Some((&self.#field_name as &dyn ComponentBatch).into()) }
quote! { Some((&self.#field_name as &dyn ComponentBatch).into()) }
}
} else {
if is_plural {
// Maybe logging an Option<Vec<C>>
quote!{ self.#field_name.as_ref().map(|comp_batch| (comp_batch as &dyn ComponentBatch).into()) }
quote! { self.#field_name.as_ref().map(|comp_batch| (comp_batch as &dyn ComponentBatch).into()) }
} else {
// Maybe logging an Option<C>
quote!{ self.#field_name.as_ref().map(|comp| (comp as &dyn ComponentBatch).into()) }
quote! { self.#field_name.as_ref().map(|comp| (comp as &dyn ComponentBatch).into()) }
}
}
} else {
// Always logging a Vec<C> or C
quote!{ Some((&self.#field_name as &dyn ComponentBatch).into()) }
quote! { Some((&self.#field_name as &dyn ComponentBatch).into()) }
}
}))
};
Expand Down Expand Up @@ -1168,7 +1168,7 @@ fn quote_trait_impls_for_archetype(obj: &Object) -> TokenStream {

// NOTE: An archetype cannot have overlapped component types by definition, so use the
// component's fqname to do the mapping.
let quoted_deser = if is_nullable && !is_plural{
let quoted_deser = if is_nullable && !is_plural {
// For a nullable mono-component, it's valid for data to be missing
// after a clear.
let quoted_collection =
Expand Down
31 changes: 30 additions & 1 deletion crates/build/re_types_builder/src/objects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ use itertools::Itertools;

use crate::{
root_as_schema, Docs, FbsBaseType, FbsEnum, FbsEnumVal, FbsField, FbsKeyValue, FbsObject,
FbsSchema, FbsType, Reporter, ATTR_RERUN_OVERRIDE_TYPE,
FbsSchema, FbsType, Reporter, ATTR_RERUN_COMPONENT_OPTIONAL, ATTR_RERUN_COMPONENT_RECOMMENDED,
ATTR_RERUN_COMPONENT_REQUIRED, ATTR_RERUN_OVERRIDE_TYPE,
};

// ---
Expand Down Expand Up @@ -91,6 +92,8 @@ impl Objects {
if field_obj.kind != ObjectKind::Component {
reporter.error(virtpath, field_type_fqname, "Is part of an archetypes but is not a component. Only components are allowed as fields on an archetype.");
}

validate_archetype_field_attributes(reporter, obj);
}
ObjectKind::View => {
if field_obj.kind != ObjectKind::Archetype {
Expand Down Expand Up @@ -174,6 +177,32 @@ impl Objects {
}
}

/// Ensure that each field of an archetype has exactly one of the
/// `attr.rerun.component_{required|recommended|optional}` attributes.
fn validate_archetype_field_attributes(reporter: &Reporter, obj: &Object) {
assert_eq!(obj.kind, ObjectKind::Archetype);

for field in &obj.fields {
if [
ATTR_RERUN_COMPONENT_OPTIONAL,
ATTR_RERUN_COMPONENT_RECOMMENDED,
ATTR_RERUN_COMPONENT_REQUIRED,
]
.iter()
.filter(|attr| field.try_get_attr::<String>(attr).is_some())
.count()
!= 1
{
reporter.error(
&field.virtpath,
&field.fqname,
"field must have exactly one of the \"attr.rerun.component_{{required|recommended|\
optional}}\" attributes",
);
}
}
}

impl Objects {
pub fn get(&self, fqname: &str) -> Option<&Object> {
self.objects.get(fqname)
Expand Down

0 comments on commit 76d4663

Please sign in to comment.