From 76d4663de1916bb6d1ee575b2224d848923a00f4 Mon Sep 17 00:00:00 2001 From: Antoine Beyeler <49431240+abey79@users.noreply.github.com> Date: Thu, 5 Dec 2024 16:54:24 +0100 Subject: [PATCH] Fail codegen when archetype fields don't have one of the required attributes (#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. --- .../re_types_builder/src/codegen/rust/api.rs | 16 +++++----- crates/build/re_types_builder/src/objects.rs | 31 ++++++++++++++++++- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/crates/build/re_types_builder/src/codegen/rust/api.rs b/crates/build/re_types_builder/src/codegen/rust/api.rs index 3c10c6260b76..90b01d69e1a2 100644 --- a/crates/build/re_types_builder/src/codegen/rust/api.rs +++ b/crates/build/re_types_builder/src/codegen/rust/api.rs @@ -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; @@ -1090,7 +1090,7 @@ fn quote_trait_impls_for_archetype(obj: &Object) -> TokenStream { .collect::>(); 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); @@ -1104,7 +1104,7 @@ fn quote_trait_impls_for_archetype(obj: &Object) -> TokenStream { if is_plural { // Always log Option> as Vec, 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) @@ -1118,20 +1118,20 @@ fn quote_trait_impls_for_archetype(obj: &Object) -> TokenStream { } } else { // Always log Option, 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> - 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 - 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 or C - quote!{ Some((&self.#field_name as &dyn ComponentBatch).into()) } + quote! { Some((&self.#field_name as &dyn ComponentBatch).into()) } } })) }; @@ -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 = diff --git a/crates/build/re_types_builder/src/objects.rs b/crates/build/re_types_builder/src/objects.rs index 87512f2e0463..8a71b0d0cfb1 100644 --- a/crates/build/re_types_builder/src/objects.rs +++ b/crates/build/re_types_builder/src/objects.rs @@ -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, }; // --- @@ -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 { @@ -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::(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)