From ead6d6df82cba6dc226293013539af7a5ef109c0 Mon Sep 17 00:00:00 2001 From: Johan Peltenburg Date: Thu, 23 Jul 2026 14:47:32 +0200 Subject: [PATCH 1/2] Rework schema builders to defer validation --- crates/constraints/src/lib.rs | 2 +- crates/constraints/tests/validator.rs | 43 +-- crates/fsm/src/builder.rs | 30 +-- crates/fsm/tests/fsm-constraint.rs | 13 +- crates/instrumentation-build/src/any_event.rs | 16 +- crates/instrumentation-build/src/data_type.rs | 6 +- crates/instrumentation-build/src/events.rs | 21 +- .../instrumentation-build/src/runtime/mod.rs | 15 +- .../ref-target/tests/ref-target-constraint.rs | 6 +- crates/ref-tree/tests/ref-tree-constraint.rs | 15 +- crates/resource/src/builder.rs | 68 ++--- crates/resource/tests/resource-constraint.rs | 31 ++- crates/schema/src/builder/annotations.rs | 175 ++++--------- crates/schema/src/builder/entity.rs | 82 ++---- crates/schema/src/builder/event.rs | 99 ++----- crates/schema/src/builder/mod.rs | 160 ++++-------- crates/schema/src/builder/record.rs | 82 ++---- crates/schema/src/test_utils.rs | 18 +- crates/schema/src/visitor.rs | 6 +- crates/yaml/src/lib.rs | 10 +- crates/yaml/src/lower.rs | 245 +++++++++--------- crates/yaml/tests/diagnostics.rs | 2 +- 22 files changed, 413 insertions(+), 732 deletions(-) diff --git a/crates/constraints/src/lib.rs b/crates/constraints/src/lib.rs index 981644655..8ff8d6cfb 100644 --- a/crates/constraints/src/lib.rs +++ b/crates/constraints/src/lib.rs @@ -114,7 +114,7 @@ pub struct Report { /// # type ConstraintB = DocConstraint; /// # /// # let schema: Schema = -/// # SchemaBuilder::new(Identifier::try_new("MySchema").unwrap()).build(); +/// # SchemaBuilder::new(Identifier::try_new("MySchema").unwrap()).build().unwrap(); /// /// let report = validate::<(ConstraintA, ConstraintB)>(&schema); /// assert!(report.base_constraints.is_ok()); diff --git a/crates/constraints/tests/validator.rs b/crates/constraints/tests/validator.rs index b7c9c2825..d437d0204 100644 --- a/crates/constraints/tests/validator.rs +++ b/crates/constraints/tests/validator.rs @@ -12,9 +12,9 @@ use quent_schema::{ /// Annotations carrying the constraint `name` without data. fn constraint(name: &str) -> Annotations { AnnotationsBuilder::new() - .try_with_constraint(name, None) - .unwrap() + .with_constraint(name, None) .build() + .unwrap() } fn empty_schema() -> Schema { @@ -97,7 +97,8 @@ fn passing_constraint_on_empty_schema() { fn constraint_without_validator_is_unregistered() { let schema = SchemaBuilder::new(ident("TestSchema")) .with_annotations(constraint("unknown")) - .build(); + .build() + .unwrap(); let report = validate::<(NoopA,)>(&schema); assert_eq!(report.unregistered_constraints.len(), 1); assert!( @@ -114,11 +115,12 @@ fn metadata_is_never_validated() { let schema = SchemaBuilder::new(ident("TestSchema")) .with_annotations( AnnotationsBuilder::new() - .try_with_metadata("not_validated", None) - .unwrap() - .build(), + .with_metadata("not_validated", None) + .build() + .unwrap(), ) - .build(); + .build() + .unwrap(); let report = validate::<(NoopA,)>(&schema); assert!(report.unregistered_constraints.is_empty()); } @@ -128,29 +130,27 @@ fn unregistered_constraint_is_reported_once() { let unknown = || constraint("unknown"); let field = Field::new(ident("ef"), DataType::U64, unknown()); let event = EventBuilder::new(ident("Ev"), Cardinality::Once) - .try_with_field(field) - .unwrap() + .with_field(field) .with_annotations(unknown()) - .build(); + .build() + .unwrap(); let entity = EntityBuilder::new(ident("E")) - .try_with_event(event) - .unwrap() + .with_event(event) .with_annotations(unknown()) .build() .unwrap(); let record_field = Field::new(ident("rf"), DataType::U64, unknown()); let record = RecordBuilder::new(ident("R")) - .try_with_field(record_field) - .unwrap() + .with_field(record_field) .with_annotations(unknown()) - .build(); + .build() + .unwrap(); let schema = SchemaBuilder::new(ident("S")) - .try_with_entity(entity) - .unwrap() - .try_with_record(record) - .unwrap() + .with_entity(entity) + .with_record(record) .with_annotations(unknown()) - .build(); + .build() + .unwrap(); let report = validate::<(NoopA,)>(&schema); // The same name used at six sites is deduplicated to a single entry. assert_eq!( @@ -174,7 +174,8 @@ fn constraint_failure_is_reported_per_constraint() { fn unregistered_and_failure_aggregate() { let schema = SchemaBuilder::new(ident("TestSchema")) .with_annotations(constraint("unknown")) - .build(); + .build() + .unwrap(); let report = validate::<(Failing,)>(&schema); assert!( report diff --git a/crates/fsm/src/builder.rs b/crates/fsm/src/builder.rs index a2ffdf0d1..9a43769a2 100644 --- a/crates/fsm/src/builder.rs +++ b/crates/fsm/src/builder.rs @@ -46,7 +46,7 @@ pub enum FsmEntityBuilderError { NoExitState, #[error("duplicate state `{0}`")] DuplicateState(Identifier), - /// A duplicate attribute name within a state reached the schema builder. + /// The generated schema element is invalid. #[error(transparent)] Build(#[from] BuilderError), /// The FSM topology failed to serialize to its constraint payload. @@ -119,7 +119,7 @@ impl FsmEntityBuilder { .map(|s| s.name.clone()) .collect(); let initial = match initials.as_slice() { - [one] => one.clone(), + [initial] => initial.clone(), [] => return Err(FsmEntityBuilderError::NoInitialState), _ => return Err(FsmEntityBuilderError::MultipleInitialStates(initials)), }; @@ -132,8 +132,7 @@ impl FsmEntityBuilder { let Some((first_exit, other_exits)) = exits.split_first() else { return Err(FsmEntityBuilderError::NoExitState); }; - - let transitions: Vec = states + let transitions = states .iter() .flat_map(|state| { let source = state.name.clone(); @@ -155,22 +154,23 @@ impl FsmEntityBuilder { // reports it, so `Once` here is only a stand-in. let cardinality = fsm.cardinality(&state.name).unwrap_or(Cardinality::Once); let event = EventBuilder::new(state.name, cardinality) - .try_with_fields(state.attributes)? - .build(); - entity = entity.try_with_event(event)?; + .with_fields(state.attributes) + .build()?; + entity = entity.with_event(event); } - annotations.set_constraint(FsmConstraint::NAME, Some(fsm.constraint_data()?)); - let entity = entity.with_annotations(annotations.build()).build()?; + annotations = + annotations.with_constraint(FsmConstraint::NAME, Some(fsm.constraint_data()?)); + let entity = entity.with_annotations(annotations.build()?).build()?; // Validate the full topology now, the same checks the constraint runs // during schema validation, so a built entity is always valid. - let mut errors = Vec::new(); - check_entity(&entity, &fsm, &mut errors); - match errors.len() { - 0 => Ok(entity), - 1 => Err(FsmEntityBuilderError::Invalid(errors.pop().unwrap())), - _ => Err(FsmEntityBuilderError::Invalid(FsmError::Multiple(errors))), + let mut topology_errors = Vec::new(); + check_entity(&entity, &fsm, &mut topology_errors); + if let Some(error) = topology_errors.into_iter().next() { + Err(FsmEntityBuilderError::Invalid(error)) + } else { + Ok(entity) } } } diff --git a/crates/fsm/tests/fsm-constraint.rs b/crates/fsm/tests/fsm-constraint.rs index d82d0f264..051057426 100644 --- a/crates/fsm/tests/fsm-constraint.rs +++ b/crates/fsm/tests/fsm-constraint.rs @@ -41,15 +41,14 @@ fn fsm(initial: &str, transitions: &[(&str, &str)], exit: &[&str]) -> String { /// Annotations carrying the FSM constraint with `data`. fn fsm_annotations(data: Option) -> Annotations { AnnotationsBuilder::new() - .try_with_constraint(FsmConstraint::NAME, data) - .unwrap() + .with_constraint(FsmConstraint::NAME, data) .build() + .unwrap() } fn entity_with(name: &str, events: Vec, data: &str) -> Entity { EntityBuilder::new(ident(name)) - .try_with_events(events) - .unwrap() + .with_events(events) .with_annotations(fsm_annotations(Some(data.to_string()))) .build() .unwrap() @@ -96,8 +95,7 @@ fn single_state_fsm_passes() { #[test] fn missing_data_is_rejected() { let entity = EntityBuilder::new(ident("E")) - .try_with_event(event("a", Cardinality::Once)) - .unwrap() + .with_event(event("a", Cardinality::Once)) .with_annotations(fsm_annotations(None)) .build() .unwrap(); @@ -112,8 +110,7 @@ fn missing_data_is_rejected() { #[test] fn invalid_json_is_rejected() { let entity = EntityBuilder::new(ident("E")) - .try_with_event(event("a", Cardinality::Once)) - .unwrap() + .with_event(event("a", Cardinality::Once)) .with_annotations(fsm_annotations(Some("{ trash".to_string()))) .build() .unwrap(); diff --git a/crates/instrumentation-build/src/any_event.rs b/crates/instrumentation-build/src/any_event.rs index 95e36b370..559f64e0d 100644 --- a/crates/instrumentation-build/src/any_event.rs +++ b/crates/instrumentation-build/src/any_event.rs @@ -70,8 +70,11 @@ mod tests { fn entity(name: &str, event: &str) -> quent_schema::Entity { EntityBuilder::new(ident(name)) - .try_with_event(EventBuilder::new(ident(event), Cardinality::Once).build()) - .unwrap() + .with_event( + EventBuilder::new(ident(event), Cardinality::Once) + .build() + .unwrap(), + ) .build() .unwrap() } @@ -79,11 +82,10 @@ mod tests { #[test] fn emits_a_variant_and_arm_per_entity() { let schema = SchemaBuilder::new(ident("Demo")) - .try_with_entity(entity("Query", "submitted")) - .unwrap() - .try_with_entity(entity("Server", "booted")) - .unwrap() - .build(); + .with_entity(entity("Query", "submitted")) + .with_entity(entity("Server", "booted")) + .build() + .unwrap(); let opts = Options { event_derives: &["Debug"], ..Options::default() diff --git a/crates/instrumentation-build/src/data_type.rs b/crates/instrumentation-build/src/data_type.rs index d2a42146a..235286e23 100644 --- a/crates/instrumentation-build/src/data_type.rs +++ b/crates/instrumentation-build/src/data_type.rs @@ -104,11 +104,11 @@ mod tests { fn entity_ref_uses_its_ref_target_marker() { use quent_schema::builder::AnnotationsBuilder; - let mut annotations = AnnotationsBuilder::new(); - annotations.set_constraint(RefTargetConstraint::NAME, Some("Cluster".to_string())); + let annotations = AnnotationsBuilder::new() + .with_constraint(RefTargetConstraint::NAME, Some("Cluster".to_string())); let ty = DataType::EntityRef { data: Some(Box::new(DataType::U64)), - annotations: annotations.build(), + annotations: annotations.build().unwrap(), }; let tokens = map_data_type(&ty, 0).to_string(); assert!(tokens.contains("EntityRef < Cluster , u64 >"), "{tokens}"); diff --git a/crates/instrumentation-build/src/events.rs b/crates/instrumentation-build/src/events.rs index 87cd86701..4ff56fffa 100644 --- a/crates/instrumentation-build/src/events.rs +++ b/crates/instrumentation-build/src/events.rs @@ -135,27 +135,22 @@ mod tests { #[test] fn docs_annotations_become_doc_attributes() { - let docs = |text: &str| { - let mut builder = AnnotationsBuilder::new(); - builder.set_docs(text); - builder.build() - }; + let docs = |text: &str| AnnotationsBuilder::new().with_docs(text).build().unwrap(); let field_x = Field::new(ident("x"), DataType::U8, docs("field doc")); let ev = EventBuilder::new(ident("ev"), Cardinality::Once) - .try_with_field(field_x) - .unwrap() + .with_field(field_x) .with_annotations(docs("event doc")) - .build(); + .build() + .unwrap(); let en = EntityBuilder::new(ident("E")) - .try_with_event(ev) - .unwrap() + .with_event(ev) .with_annotations(docs("entity doc")) .build() .unwrap(); let s = SchemaBuilder::new(ident("M")) - .try_with_entity(en) - .unwrap() - .build(); + .with_entity(en) + .build() + .unwrap(); let expected = quote! { #[doc = "entity doc"] diff --git a/crates/instrumentation-build/src/runtime/mod.rs b/crates/instrumentation-build/src/runtime/mod.rs index e64cfd678..c0055a2f7 100644 --- a/crates/instrumentation-build/src/runtime/mod.rs +++ b/crates/instrumentation-build/src/runtime/mod.rs @@ -120,19 +120,18 @@ mod tests { #[test] fn generate_assembles_event_impl_observer_handle_and_context() { let connection = EntityBuilder::new(ident("Connection")) - .try_with_event( + .with_event( EventBuilder::new(ident("data"), Cardinality::Multi) - .try_with_field(field("bytes", DataType::U64)) - .unwrap() - .build(), + .with_field(field("bytes", DataType::U64)) + .build() + .unwrap(), ) - .unwrap() .build() .unwrap(); let s = SchemaBuilder::new(ident("Demo")) - .try_with_entity(connection) - .unwrap() - .build(); + .with_entity(connection) + .build() + .unwrap(); let src = pretty(generate_runtime_types(&s).unwrap()); assert!(src.contains("impl ::quent_instrumentation::EntityEvent for ConnectionEvent")); assert!(src.contains(r#"const NAME: &'static str = "connection""#)); diff --git a/crates/ref-target/tests/ref-target-constraint.rs b/crates/ref-target/tests/ref-target-constraint.rs index c0a9b616b..db52f3940 100644 --- a/crates/ref-target/tests/ref-target-constraint.rs +++ b/crates/ref-target/tests/ref-target-constraint.rs @@ -13,9 +13,9 @@ fn entity_ref(target: Option<&str>) -> DataType { DataType::EntityRef { data: None, annotations: AnnotationsBuilder::new() - .try_with_constraint(RefTargetConstraint::NAME, target.map(ToString::to_string)) - .unwrap() - .build(), + .with_constraint(RefTargetConstraint::NAME, target.map(ToString::to_string)) + .build() + .unwrap(), } } diff --git a/crates/ref-tree/tests/ref-tree-constraint.rs b/crates/ref-tree/tests/ref-tree-constraint.rs index 9b0f0d9ac..94478dd78 100644 --- a/crates/ref-tree/tests/ref-tree-constraint.rs +++ b/crates/ref-tree/tests/ref-tree-constraint.rs @@ -15,9 +15,9 @@ fn tree_ref() -> DataType { DataType::EntityRef { data: None, annotations: AnnotationsBuilder::new() - .try_with_constraint(RefTreeConstraint::NAME, None) - .unwrap() - .build(), + .with_constraint(RefTreeConstraint::NAME, None) + .build() + .unwrap(), } } @@ -26,11 +26,10 @@ fn tree_ref_to(target: &str) -> DataType { DataType::EntityRef { data: None, annotations: AnnotationsBuilder::new() - .try_with_constraint(RefTreeConstraint::NAME, None) - .unwrap() - .try_with_constraint(RefTargetConstraint::NAME, Some(ident(target).to_string())) - .unwrap() - .build(), + .with_constraint(RefTreeConstraint::NAME, None) + .with_constraint(RefTargetConstraint::NAME, Some(ident(target).to_string())) + .build() + .unwrap(), } } diff --git a/crates/resource/src/builder.rs b/crates/resource/src/builder.rs index f1e2f1d21..3c3b6f417 100644 --- a/crates/resource/src/builder.rs +++ b/crates/resource/src/builder.rs @@ -26,8 +26,7 @@ pub struct ResourceBuilder { name: Identifier, usage_record_name: Identifier, bounds_record_name: Identifier, - capacities: Capacities, - errors: Vec, + capacities: Vec<(Identifier, Capacity)>, } impl ResourceBuilder { @@ -60,28 +59,13 @@ impl ResourceBuilder { name, usage_record_name, bounds_record_name, - capacities: Capacities::default(), - errors: Vec::new(), + capacities: Vec::new(), } } - /// The capacity declared under `name`, if any. - pub fn capacity(&self, name: &Identifier) -> Option<&Capacity> { - self.capacities.get(name) - } - - /// Set a capacity, returning the replaced declaration, if any. - pub fn set_capacity(&mut self, name: Identifier, capacity: Capacity) -> Option { - self.capacities.insert(name, capacity) - } - /// Add a capacity, returning the builder for chaining. pub fn with_capacity(mut self, name: Identifier, capacity: Capacity) -> Self { - if self.capacities.contains_key(&name) { - self.errors.push(BuildError::DuplicateCapacity(name)); - } else { - self.capacities.insert(name, capacity); - } + self.capacities.push((name, capacity)); self } @@ -90,9 +74,7 @@ impl ResourceBuilder { mut self, capacities: impl IntoIterator, ) -> Self { - for (name, capacity) in capacities { - self = self.with_capacity(name, capacity); - } + self.capacities.extend(capacities); self } @@ -111,16 +93,22 @@ impl ResourceBuilder { usage_record_name, bounds_record_name, capacities, - mut errors, } = self; + let mut unique_capacities = Capacities::default(); + for (name, capacity) in capacities { + match unique_capacities.entry(name) { + indexmap::map::Entry::Occupied(entry) => { + return Err(BuildError::DuplicateCapacity(entry.key().clone())); + } + indexmap::map::Entry::Vacant(entry) => { + entry.insert(capacity); + } + } + } + let capacities = unique_capacities; let has_bounds = capacities.values().any(Capacity::is_bounded); if has_bounds && usage_record_name == bounds_record_name { - errors.push(BuildError::DuplicateRecordName(usage_record_name.clone())); - } - match errors.len() { - 0 => {} - 1 => return Err(errors.pop().unwrap()), - _ => return Err(BuildError::Multiple(errors)), + return Err(BuildError::DuplicateRecordName(usage_record_name)); } // Usages include every capacity. Bounds omit unbounded capacities @@ -165,17 +153,17 @@ fn build_resource_record<'a>( fields: impl Iterator, ) -> Result { let annotations = AnnotationsBuilder::new() - .try_with_constraint(Resource::NAME, Some(resource.constraint_data()?))? - .build(); + .with_constraint(Resource::NAME, Some(resource.constraint_data()?)) + .build()?; let mut builder = RecordBuilder::new(name).with_annotations(annotations); for field in fields { - builder = builder.try_with_field(Field::new( + builder = builder.with_field(Field::new( field.clone(), DataType::U64, Annotations::default(), - ))?; + )); } - Ok(builder.build()) + Ok(builder.build()?) } fn suffixed_identifier(resource: &Identifier, suffix: &str) -> Identifier { @@ -189,8 +177,6 @@ pub enum BuildError { DuplicateCapacity(Identifier), #[error("usage and bounds records have the same name \"{0}\"")] DuplicateRecordName(Identifier), - #[error("multiple resource builder errors: {0:?}")] - Multiple(Vec), #[error(transparent)] Schema(#[from] BuilderError), #[error("serializing resource data: {0}")] @@ -275,11 +261,9 @@ mod tests { .with_capacity(watts.clone(), Capacity::new(CapacityKind::Rate, false)) .with_capacity(watts, Capacity::new(CapacityKind::Rate, true)) .build(); - let Err(BuildError::Multiple(errors)) = result else { - panic!("expected multiple errors"); - }; - assert_eq!(errors.len(), 2); - assert!(matches!(&errors[0], BuildError::DuplicateCapacity(name) if name == &bytes)); - assert!(matches!(&errors[1], BuildError::DuplicateCapacity(_))); + assert!(matches!( + result, + Err(BuildError::DuplicateCapacity(name)) if name == bytes + )); } } diff --git a/crates/resource/tests/resource-constraint.rs b/crates/resource/tests/resource-constraint.rs index eebf92a94..73345952a 100644 --- a/crates/resource/tests/resource-constraint.rs +++ b/crates/resource/tests/resource-constraint.rs @@ -35,25 +35,25 @@ fn bounds_data(resource: &str) -> String { /// Create resource constraint annotations carrying `data`. fn resource_annotations(data: String) -> Annotations { AnnotationsBuilder::new() - .try_with_constraint(ResourceConstraint::NAME, Some(data)) - .unwrap() + .with_constraint(ResourceConstraint::NAME, Some(data)) .build() + .unwrap() } fn fsm_annotations() -> Annotations { AnnotationsBuilder::new() - .try_with_constraint(FsmConstraint::NAME, None) - .unwrap() + .with_constraint(FsmConstraint::NAME, None) .build() + .unwrap() } /// Create a record carrying resource `data` with a `U64` field for each name. fn resource_record(name: &str, data: String, fields: &[&str]) -> Record { let mut builder = RecordBuilder::new(ident(name)).with_annotations(resource_annotations(data)); for &f in fields { - builder = builder.try_with_field(field(f, DataType::U64)).unwrap(); + builder = builder.with_field(field(f, DataType::U64)); } - builder.build() + builder.build().unwrap() } fn usage_record(name: &str, resource: &str, claims: &[&str]) -> Record { @@ -78,7 +78,7 @@ fn resource_entity_with_bounds_type( let mut builder = EntityBuilder::new(ident(name)) .with_annotations(resource_annotations(definition_data(capacities))); let fields = bounds.map(|bounds| field("bounds", bounds)).into_iter(); - builder = builder.try_with_event(event("operating", fields)).unwrap(); + builder = builder.with_event(event("operating", fields)); builder.build().unwrap() } @@ -100,9 +100,8 @@ fn user_entity_with_data_type(name: &str, fsm: bool, data: DataType, on_ref: boo } else { data }; - let mut builder = EntityBuilder::new(ident(name)) - .try_with_event(event("using", [field("claim", ty)])) - .unwrap(); + let mut builder = + EntityBuilder::new(ident(name)).with_event(event("using", [field("claim", ty)])); if fsm { builder = builder.with_annotations(fsm_annotations()); } @@ -346,8 +345,7 @@ fn misplaced_resources_are_rejected() { let definition_on_record = schema("App", [], [bad_record]); let bad_entity = EntityBuilder::new(ident("Worker")) - .try_with_event(event("using", [])) - .unwrap() + .with_event(event("using", [])) .with_annotations(resource_annotations(usage_data("Memory"))) .build() .unwrap(); @@ -355,7 +353,8 @@ fn misplaced_resources_are_rejected() { let usage_on_schema = SchemaBuilder::new(ident("App")) .with_annotations(resource_annotations(usage_data("Memory"))) - .build(); + .build() + .unwrap(); let memory = resource_entity("Memory", &[("bytes", "occupancy", false)], None); let usage = usage_record("MemoryUsage", "Memory", &["bytes"]); @@ -364,9 +363,9 @@ fn misplaced_resources_are_rejected() { annotations: Annotations::default(), }; let wrapper = RecordBuilder::new(ident("Wrapper")) - .try_with_field(field("carried", carrier)) - .unwrap() - .build(); + .with_field(field("carried", carrier)) + .build() + .unwrap(); let usage_without_entity = schema("App", [memory], [usage, wrapper]); for (case, schema) in [ diff --git a/crates/schema/src/builder/annotations.rs b/crates/schema/src/builder/annotations.rs index 1da34acdf..27b8ed2e3 100644 --- a/crates/schema/src/builder/annotations.rs +++ b/crates/schema/src/builder/annotations.rs @@ -1,38 +1,16 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::builder::{BuilderError, insert_unique}; +use crate::builder::BuilderError; use crate::schema::Map; use crate::{Annotations, Constraint, Metadata}; -/// Builder for a map of named items. -struct OpaqueMapBuilder(Map); - -impl Default for OpaqueMapBuilder { - fn default() -> Self { - Self(Map::default()) - } -} - -impl OpaqueMapBuilder { - fn try_insert(&mut self, name: String, value: T) -> Result<(), BuilderError> { - if name.is_empty() { - return Err(BuilderError::EmptyName); - } - insert_unique(&mut self.0, name, value) - } - - fn set(&mut self, name: String, value: T) -> Option { - self.0.insert(name, value) - } -} - /// Builder for [`Annotations`]. #[derive(Default)] pub struct AnnotationsBuilder { docs: Option, - constraints: OpaqueMapBuilder, - metadata: OpaqueMapBuilder, + constraints: Vec, + metadata: Vec, } impl AnnotationsBuilder { @@ -45,125 +23,66 @@ impl AnnotationsBuilder { pub fn from_annotations(annotations: &Annotations) -> Self { Self { docs: annotations.docs().map(str::to_owned), - constraints: OpaqueMapBuilder( - annotations - .constraints() - .map(|c| (c.name().to_owned(), c.clone())) - .collect(), - ), - metadata: OpaqueMapBuilder( - annotations - .metadata_entries() - .map(|m| (m.name().to_owned(), m.clone())) - .collect(), - ), + constraints: annotations.constraints().cloned().collect(), + metadata: annotations.metadata_entries().cloned().collect(), } } - /// The documentation, if set. - pub fn docs(&self) -> Option<&str> { - self.docs.as_deref() - } - - /// Set the documentation string, returning the replaced one, if any. - pub fn set_docs(&mut self, docs: impl Into) -> Option { - self.docs.replace(docs.into()) - } - - /// The constraint declared under `name`, if any. - pub fn constraint(&self, name: &str) -> Option<&Constraint> { - self.constraints.0.get(name) - } - - /// Add a constraint named `name`. - /// - /// # Errors - /// - /// Errors if `name` is empty or already declared. - pub fn try_insert_constraint( - &mut self, - name: impl Into, - data: Option, - ) -> Result<&mut Self, BuilderError> { - let name = name.into(); - self.constraints - .try_insert(name.clone(), Constraint::from_parts(name, data))?; - Ok(self) + /// Set the documentation string. + pub fn with_docs(mut self, docs: impl Into) -> Self { + self.docs = Some(docs.into()); + self } /// Add a constraint named `name`, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors if `name` is empty or already declared. - pub fn try_with_constraint( - mut self, - name: impl Into, - data: Option, - ) -> Result { - self.try_insert_constraint(name, data)?; - Ok(self) - } - - /// Set the constraint named `name`, returning the replaced entry, if any. - pub fn set_constraint( - &mut self, - name: impl Into, - data: Option, - ) -> Option { + pub fn with_constraint(mut self, name: impl Into, data: Option) -> Self { let name = name.into(); - self.constraints - .set(name.clone(), Constraint::from_parts(name, data)) + self.constraints.push(Constraint::from_parts(name, data)); + self } - /// The metadata entry declared under `name`, if any. - pub fn metadata(&self, name: &str) -> Option<&Metadata> { - self.metadata.0.get(name) - } - - /// Add a metadata entry named `name`. - /// - /// # Errors - /// - /// Errors if `name` is empty or already declared. - pub fn try_insert_metadata( - &mut self, - name: impl Into, - data: Option, - ) -> Result<&mut Self, BuilderError> { + /// Add a metadata entry named `name`, returning the builder for chaining. + pub fn with_metadata(mut self, name: impl Into, data: Option) -> Self { let name = name.into(); - self.metadata - .try_insert(name.clone(), Metadata::from_parts(name, data))?; - Ok(self) + self.metadata.push(Metadata::from_parts(name, data)); + self } - /// Add a metadata entry named `name`, returning the builder for chaining. + /// Finish building the annotations. /// /// # Errors /// - /// Errors if `name` is empty or already declared. - pub fn try_with_metadata( - mut self, - name: impl Into, - data: Option, - ) -> Result { - self.try_insert_metadata(name, data)?; - Ok(self) - } - - /// Set the metadata entry named `name`, returning the replaced entry, if any. - pub fn set_metadata( - &mut self, - name: impl Into, - data: Option, - ) -> Option { - let name = name.into(); - self.metadata - .set(name.clone(), Metadata::from_parts(name, data)) + /// Errors if a name is empty or repeated. + pub fn build(self) -> Result { + let Self { + docs, + constraints, + metadata, + } = self; + let constraints = collect_named(constraints, Constraint::name)?; + let metadata = collect_named(metadata, Metadata::name)?; + Ok(Annotations::from_parts(docs, constraints, metadata)) } +} - /// Finish building the annotations. - pub fn build(self) -> Annotations { - Annotations::from_parts(self.docs, self.constraints.0, self.metadata.0) +fn collect_named( + values: impl IntoIterator, + mut name: impl FnMut(&T) -> &str, +) -> Result, BuilderError> { + let mut map = Map::default(); + for value in values { + let name = name(&value).to_owned(); + if name.is_empty() { + return Err(BuilderError::EmptyName); + } + match map.entry(name) { + indexmap::map::Entry::Occupied(entry) => { + return Err(BuilderError::DuplicateName(entry.key().clone())); + } + indexmap::map::Entry::Vacant(entry) => { + entry.insert(value); + } + } } + Ok(map) } diff --git a/crates/schema/src/builder/entity.rs b/crates/schema/src/builder/entity.rs index d73d5f3db..73a76445d 100644 --- a/crates/schema/src/builder/entity.rs +++ b/crates/schema/src/builder/entity.rs @@ -1,15 +1,14 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::builder::{AnnotationsBuilder, BuilderError, insert_unique}; -use crate::schema::Map; +use crate::builder::{AnnotationsBuilder, BuilderError, collect_unique}; use crate::schema::identifier::IdentifierError; use crate::{Annotations, Entity, Event, Identifier}; /// Builder for an [`Entity`]. pub struct EntityBuilder { name: Identifier, - events: Map, + events: Vec, annotations: AnnotationsBuilder, } @@ -18,7 +17,7 @@ impl EntityBuilder { pub fn new(name: Identifier) -> Self { Self { name, - events: Map::default(), + events: Vec::new(), annotations: AnnotationsBuilder::new(), } } @@ -34,64 +33,16 @@ impl EntityBuilder { Ok(Self::new(name.try_into()?)) } - /// The name of the entity. - pub fn name(&self) -> &Identifier { - &self.name - } - - /// The event declared under `name`, if any. - pub fn event(&self, name: &Identifier) -> Option<&Event> { - self.events.get(name) - } - - /// Set an event, returning the replaced one with the same name, if any. - pub fn set_event(&mut self, event: Event) -> Option { - self.events.insert(event.name().clone(), event) - } - - /// Add an event. - /// - /// # Errors - /// - /// Errors if its name is already declared. - pub fn try_insert_event(&mut self, event: Event) -> Result<&mut Self, BuilderError> { - insert_unique(&mut self.events, event.name().clone(), event)?; - Ok(self) - } - /// Add an event, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors if its name is already declared. - pub fn try_with_event(mut self, event: Event) -> Result { - self.try_insert_event(event)?; - Ok(self) + pub fn with_event(mut self, event: Event) -> Self { + self.events.push(event); + self } /// Add several events, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors on the first duplicate name. - pub fn try_with_events( - mut self, - events: impl IntoIterator, - ) -> Result { - for event in events { - self.try_insert_event(event)?; - } - Ok(self) - } - - /// The annotations of the entity. - pub fn annotations(&self) -> &AnnotationsBuilder { - &self.annotations - } - - /// The annotations of the entity. - pub fn annotations_mut(&mut self) -> &mut AnnotationsBuilder { - &mut self.annotations + pub fn with_events(mut self, events: impl IntoIterator) -> Self { + self.events.extend(events); + self } /// Set the entity's annotations, replacing any added so far, and return @@ -107,14 +58,17 @@ impl EntityBuilder { /// /// Errors if the entity declares no events. pub fn build(self) -> Result { - if self.events.is_empty() { + let Self { + name, + events, + annotations, + } = self; + if events.is_empty() { return Err(BuilderError::NoEvents); } - Ok(Entity::from_parts( - self.name, - self.events, - self.annotations.build(), - )) + let events = collect_unique(events, |event| event.name().clone())?; + let annotations = annotations.build()?; + Ok(Entity::from_parts(name, events, annotations)) } } diff --git a/crates/schema/src/builder/event.rs b/crates/schema/src/builder/event.rs index 42078514f..b0e489919 100644 --- a/crates/schema/src/builder/event.rs +++ b/crates/schema/src/builder/event.rs @@ -1,8 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::builder::{AnnotationsBuilder, BuilderError, insert_unique}; -use crate::schema::Map; +use crate::builder::{AnnotationsBuilder, BuilderError, collect_unique}; use crate::schema::identifier::IdentifierError; use crate::{Annotations, Cardinality, Event, Field, Identifier}; @@ -10,7 +9,7 @@ use crate::{Annotations, Cardinality, Event, Field, Identifier}; pub struct EventBuilder { name: Identifier, cardinality: Cardinality, - payload: Map, + payload: Vec, annotations: AnnotationsBuilder, } @@ -20,7 +19,7 @@ impl EventBuilder { Self { name, cardinality, - payload: Map::default(), + payload: Vec::new(), annotations: AnnotationsBuilder::new(), } } @@ -38,75 +37,16 @@ impl EventBuilder { Ok(Self::new(name.try_into()?, cardinality)) } - /// The name of the event. - pub fn name(&self) -> &Identifier { - &self.name - } - - /// The cardinality of the event. - pub fn cardinality(&self) -> Cardinality { - self.cardinality - } - - /// Set the cardinality of the event, returning the previous one. - pub fn set_cardinality(&mut self, cardinality: Cardinality) -> Cardinality { - std::mem::replace(&mut self.cardinality, cardinality) - } - - /// The payload field declared under `name`, if any. - pub fn field(&self, name: &Identifier) -> Option<&Field> { - self.payload.get(name) - } - - /// Set a payload field, returning the replaced one with the same name, if - /// any. - pub fn set_field(&mut self, field: Field) -> Option { - self.payload.insert(field.name().clone(), field) - } - - /// Add a payload field. - /// - /// # Errors - /// - /// Errors if its name is already declared. - pub fn try_insert_field(&mut self, field: Field) -> Result<&mut Self, BuilderError> { - insert_unique(&mut self.payload, field.name().clone(), field)?; - Ok(self) - } - /// Add a payload field, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors if its name is already declared. - pub fn try_with_field(mut self, field: Field) -> Result { - self.try_insert_field(field)?; - Ok(self) + pub fn with_field(mut self, field: Field) -> Self { + self.payload.push(field); + self } /// Add several payload fields, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors on the first duplicate name. - pub fn try_with_fields( - mut self, - fields: impl IntoIterator, - ) -> Result { - for field in fields { - self.try_insert_field(field)?; - } - Ok(self) - } - - /// The annotations of the event. - pub fn annotations(&self) -> &AnnotationsBuilder { - &self.annotations - } - - /// The annotations of the event. - pub fn annotations_mut(&mut self) -> &mut AnnotationsBuilder { - &mut self.annotations + pub fn with_fields(mut self, fields: impl IntoIterator) -> Self { + self.payload.extend(fields); + self } /// Set the event's annotations, replacing any added so far, and return the @@ -117,12 +57,19 @@ impl EventBuilder { } /// Finish building the event. - pub fn build(self) -> Event { - Event::from_parts( - self.name, - self.cardinality, - self.payload, - self.annotations.build(), - ) + /// + /// # Errors + /// + /// Errors if a field name is repeated or the annotations are invalid. + pub fn build(self) -> Result { + let Self { + name, + cardinality, + payload, + annotations, + } = self; + let payload = collect_unique(payload, |field| field.name().clone())?; + let annotations = annotations.build()?; + Ok(Event::from_parts(name, cardinality, payload, annotations)) } } diff --git a/crates/schema/src/builder/mod.rs b/crates/schema/src/builder/mod.rs index 778d6c942..91e9faa6e 100644 --- a/crates/schema/src/builder/mod.rs +++ b/crates/schema/src/builder/mod.rs @@ -25,37 +25,40 @@ pub use record::RecordBuilder; /// Error returned while assembling a schema element. #[derive(Debug, Clone, PartialEq, Eq, Error)] pub enum BuilderError { - /// A name was added more than once within the same collection. #[error("duplicate name \"{0}\"")] DuplicateName(String), - /// A name was empty. #[error("name must not be empty")] EmptyName, - /// No events were added to an entity. #[error("entity must declare at least one event")] NoEvents, } -pub(crate) fn insert_unique(map: &mut Map, key: K, value: V) -> Result<(), BuilderError> +pub(crate) fn collect_unique( + values: impl IntoIterator, + mut key: impl FnMut(&V) -> K, +) -> Result, BuilderError> where K: Eq + Hash + Display, { - match map.entry(key) { - indexmap::map::Entry::Occupied(entry) => { - Err(BuilderError::DuplicateName(entry.key().to_string())) - } - indexmap::map::Entry::Vacant(entry) => { - entry.insert(value); - Ok(()) + let mut map = Map::default(); + for value in values { + match map.entry(key(&value)) { + indexmap::map::Entry::Occupied(entry) => { + return Err(BuilderError::DuplicateName(entry.key().to_string())); + } + indexmap::map::Entry::Vacant(entry) => { + entry.insert(value); + } } } + Ok(map) } /// Builder for a [`Schema`]. pub struct SchemaBuilder { name: Identifier, - entities: Map, - records: Map, + entities: Vec, + records: Vec, annotations: AnnotationsBuilder, } @@ -64,8 +67,8 @@ impl SchemaBuilder { pub fn new(name: Identifier) -> Self { Self { name, - entities: Map::default(), - records: Map::default(), + entities: Vec::new(), + records: Vec::new(), annotations: AnnotationsBuilder::new(), } } @@ -81,109 +84,28 @@ impl SchemaBuilder { Ok(Self::new(name.try_into()?)) } - /// The name of the schema. - pub fn name(&self) -> &Identifier { - &self.name - } - - /// The entity declared under `name`, if any. - pub fn entity(&self, name: &Identifier) -> Option<&Entity> { - self.entities.get(name) - } - - /// Set an entity, returning the replaced one with the same name, if any. - pub fn set_entity(&mut self, entity: Entity) -> Option { - self.entities.insert(entity.name().clone(), entity) - } - - /// Add an entity. - /// - /// # Errors - /// - /// Errors if its name is already declared. - pub fn try_insert_entity(&mut self, entity: Entity) -> Result<&mut Self, BuilderError> { - insert_unique(&mut self.entities, entity.name().clone(), entity)?; - Ok(self) - } - /// Add an entity, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors if its name is already declared. - pub fn try_with_entity(mut self, entity: Entity) -> Result { - self.try_insert_entity(entity)?; - Ok(self) + pub fn with_entity(mut self, entity: Entity) -> Self { + self.entities.push(entity); + self } /// Add several entities, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors on the first duplicate name. - pub fn try_with_entities( - mut self, - entities: impl IntoIterator, - ) -> Result { - for entity in entities { - self.try_insert_entity(entity)?; - } - Ok(self) - } - - /// The record declared under `name`, if any. - pub fn record(&self, name: &Identifier) -> Option<&Record> { - self.records.get(name) - } - - /// Set a record, returning the replaced one with the same name, if any. - pub fn set_record(&mut self, record: Record) -> Option { - self.records.insert(record.name().clone(), record) - } - - /// Add a record. - /// - /// # Errors - /// - /// Errors if its name is already declared. - pub fn try_insert_record(&mut self, record: Record) -> Result<&mut Self, BuilderError> { - insert_unique(&mut self.records, record.name().clone(), record)?; - Ok(self) + pub fn with_entities(mut self, entities: impl IntoIterator) -> Self { + self.entities.extend(entities); + self } /// Add a record, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors if its name is already declared. - pub fn try_with_record(mut self, record: Record) -> Result { - self.try_insert_record(record)?; - Ok(self) + pub fn with_record(mut self, record: Record) -> Self { + self.records.push(record); + self } /// Add several records, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors on the first duplicate name. - pub fn try_with_records( - mut self, - records: impl IntoIterator, - ) -> Result { - for record in records { - self.try_insert_record(record)?; - } - Ok(self) - } - - /// The annotations of the schema. - pub fn annotations(&self) -> &AnnotationsBuilder { - &self.annotations - } - - /// The annotations of the schema. - pub fn annotations_mut(&mut self) -> &mut AnnotationsBuilder { - &mut self.annotations + pub fn with_records(mut self, records: impl IntoIterator) -> Self { + self.records.extend(records); + self } /// Set the schema's annotations, replacing any added so far, and return @@ -194,12 +116,20 @@ impl SchemaBuilder { } /// Finish building the schema. - pub fn build(self) -> Schema { - Schema::from_parts( - self.name, - self.entities, - self.records, - self.annotations.build(), - ) + /// + /// # Errors + /// + /// Errors if a child name is repeated or the annotations are invalid. + pub fn build(self) -> Result { + let Self { + name, + entities, + records, + annotations, + } = self; + let entities = collect_unique(entities, |entity| entity.name().clone())?; + let records = collect_unique(records, |record| record.name().clone())?; + let annotations = annotations.build()?; + Ok(Schema::from_parts(name, entities, records, annotations)) } } diff --git a/crates/schema/src/builder/record.rs b/crates/schema/src/builder/record.rs index a015866ad..8e1570e3f 100644 --- a/crates/schema/src/builder/record.rs +++ b/crates/schema/src/builder/record.rs @@ -1,15 +1,14 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::builder::{AnnotationsBuilder, BuilderError, insert_unique}; -use crate::schema::Map; +use crate::builder::{AnnotationsBuilder, BuilderError, collect_unique}; use crate::schema::identifier::IdentifierError; use crate::{Annotations, Field, Identifier, Record}; /// Builder for a [`Record`]. pub struct RecordBuilder { name: Identifier, - fields: Map, + fields: Vec, annotations: AnnotationsBuilder, } @@ -18,7 +17,7 @@ impl RecordBuilder { pub fn new(name: Identifier) -> Self { Self { name, - fields: Map::default(), + fields: Vec::new(), annotations: AnnotationsBuilder::new(), } } @@ -34,64 +33,16 @@ impl RecordBuilder { Ok(Self::new(name.try_into()?)) } - /// The name of the record. - pub fn name(&self) -> &Identifier { - &self.name - } - - /// The field declared under `name`, if any. - pub fn field(&self, name: &Identifier) -> Option<&Field> { - self.fields.get(name) - } - - /// Set a field, returning the replaced one with the same name, if any. - pub fn set_field(&mut self, field: Field) -> Option { - self.fields.insert(field.name().clone(), field) - } - - /// Add a field. - /// - /// # Errors - /// - /// Errors if its name is already declared. - pub fn try_insert_field(&mut self, field: Field) -> Result<&mut Self, BuilderError> { - insert_unique(&mut self.fields, field.name().clone(), field)?; - Ok(self) - } - /// Add a field, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors if its name is already declared. - pub fn try_with_field(mut self, field: Field) -> Result { - self.try_insert_field(field)?; - Ok(self) + pub fn with_field(mut self, field: Field) -> Self { + self.fields.push(field); + self } /// Add several fields, returning the builder for chaining. - /// - /// # Errors - /// - /// Errors on the first duplicate name. - pub fn try_with_fields( - mut self, - fields: impl IntoIterator, - ) -> Result { - for field in fields { - self.try_insert_field(field)?; - } - Ok(self) - } - - /// The annotations of the record. - pub fn annotations(&self) -> &AnnotationsBuilder { - &self.annotations - } - - /// The annotations of the record. - pub fn annotations_mut(&mut self) -> &mut AnnotationsBuilder { - &mut self.annotations + pub fn with_fields(mut self, fields: impl IntoIterator) -> Self { + self.fields.extend(fields); + self } /// Set the record's annotations, replacing any added so far, and return @@ -102,7 +53,18 @@ impl RecordBuilder { } /// Finish building the record. - pub fn build(self) -> Record { - Record::from_parts(self.name, self.fields, self.annotations.build()) + /// + /// # Errors + /// + /// Errors if a field name is repeated or the annotations are invalid. + pub fn build(self) -> Result { + let Self { + name, + fields, + annotations, + } = self; + let fields = collect_unique(fields, |field| field.name().clone())?; + let annotations = annotations.build()?; + Ok(Record::from_parts(name, fields, annotations)) } } diff --git a/crates/schema/src/test_utils.rs b/crates/schema/src/test_utils.rs index 8766f9cb4..ecac61ae9 100644 --- a/crates/schema/src/test_utils.rs +++ b/crates/schema/src/test_utils.rs @@ -27,14 +27,13 @@ pub fn event_with( payload: impl IntoIterator, ) -> Event { EventBuilder::new(ident(name), cardinality) - .try_with_fields(payload) - .unwrap() + .with_fields(payload) .build() + .unwrap() } pub fn entity(name: &str, events: impl IntoIterator) -> Entity { EntityBuilder::new(ident(name)) - .try_with_events(events) - .unwrap() + .with_events(events) .build() .unwrap() } @@ -43,9 +42,9 @@ pub fn eventless_entity(name: &str) -> Entity { } pub fn record(name: &str, fields: impl IntoIterator) -> Record { RecordBuilder::new(ident(name)) - .try_with_fields(fields) - .unwrap() + .with_fields(fields) .build() + .unwrap() } pub fn schema( name: &str, @@ -53,9 +52,8 @@ pub fn schema( records: impl IntoIterator, ) -> Schema { SchemaBuilder::new(ident(name)) - .try_with_entities(entities) - .unwrap() - .try_with_records(records) - .unwrap() + .with_entities(entities) + .with_records(records) .build() + .unwrap() } diff --git a/crates/schema/src/visitor.rs b/crates/schema/src/visitor.rs index ff62316b7..52455a2f6 100644 --- a/crates/schema/src/visitor.rs +++ b/crates/schema/src/visitor.rs @@ -337,9 +337,9 @@ mod test { let entity_ref = DataType::EntityRef { data: None, annotations: AnnotationsBuilder::new() - .try_with_constraint("quent.visitor-test.v0.1.0", None) - .unwrap() - .build(), + .with_constraint("quent.visitor-test.v0.1.0", None) + .build() + .unwrap(), }; let schema = schema( "S", diff --git a/crates/yaml/src/lib.rs b/crates/yaml/src/lib.rs index d32c9eefe..9ef1c7f5a 100644 --- a/crates/yaml/src/lib.rs +++ b/crates/yaml/src/lib.rs @@ -63,7 +63,15 @@ pub fn parse_from_str(src: impl AsRef, source: Option<&str>) -> Result schema, + None => { + if !sink.has_errors() { + sink.error("", "schema could not be built", None); + } + return Err(Error::Invalid(sink)); + } + }; if sink.has_errors() { return Err(Error::Invalid(sink)); } diff --git a/crates/yaml/src/lower.rs b/crates/yaml/src/lower.rs index 038e3f319..ab1a50b96 100644 --- a/crates/yaml/src/lower.rs +++ b/crates/yaml/src/lower.rs @@ -5,17 +5,17 @@ //! //! Lowering deliberately avoids the usual `Result`/`?` path, which would stop //! at the first error. Instead each function reports any problems into one -//! shared sink and carries on — returning what it built, or `None` (skip the -//! element) or a stand-in when a name is rejected — so a single run surfaces -//! every problem in the file, not just the first. Whether it succeeded is -//! decided by the caller from the sink, not from a `Result`. +//! shared sink and carries on, returning what it built or `None` to skip an +//! invalid element. A single run can therefore surface problems across +//! independent declarations. Whether it succeeded is decided by the caller +//! from the sink, not from a `Result`. //! //! Constraint and metadata payloads are opaque: attached as written, never //! interpreted. use indexmap::IndexMap; use quent_constraints::Constraint; -use quent_fsm::{FsmConstraint, FsmEntityBuilder, FsmEntityBuilderError, FsmError, StateDecl}; +use quent_fsm::{FsmConstraint, FsmEntityBuilder, FsmEntityBuilderError, StateDecl}; use quent_ref_target::RefTargetConstraint; use quent_ref_tree::RefTreeConstraint; use quent_resource::{Capacity, Resource, ResourceBuilder}; @@ -41,9 +41,9 @@ use crate::diag::Diagnostics; /// the schema. /// /// Recoverable errors do not stop later phases, allowing one run to report -/// multiple problems. The returned schema is meaningful only when `sink` -/// contains no errors. -pub(crate) fn lower(model: &Model, sink: &mut Diagnostics) -> Schema { +/// multiple problems. A returned schema is meaningful only when `sink` +/// contains no errors. Returns `None` when no schema can be built. +pub(crate) fn lower(model: &Model, sink: &mut Diagnostics) -> Option { // Validate model-level inputs and prepare resource name resolution. if model.quent != "alpha" { sink.error( @@ -53,11 +53,7 @@ pub(crate) fn lower(model: &Model, sink: &mut Diagnostics) -> Schema { ); } - // Keep going after a bad name so one run reports every problem, not just the - // first. The caller discards this schema whenever the sink holds errors, so - // building it with a valid stand-in name here is harmless. - let name = ident(&model.model, "model", sink) - .unwrap_or_else(|| Identifier::try_new("invalid").expect("stand-in is valid")); + let name = ident(&model.model, "model", sink); let resources = ResourceLowerer::new(model, sink); // Lower declared records. @@ -92,24 +88,9 @@ pub(crate) fn lower(model: &Model, sink: &mut Diagnostics) -> Schema { } } - // Assemble the schema and diagnose record-name collisions. - let mut schema = SchemaBuilder::new(name); - for record in records { - let record_name = record.name().clone(); - if let Err(error) = schema.try_insert_record(record) { - sink.error( - &format!("records.{record_name}"), - format!( - "record `{record_name}` conflicts with a generated resource record: {error}" - ), - None, - ); - } - } - - schema - .try_with_entities(entities) - .expect("entity names are unique (deserialized from a map)") + let schema = SchemaBuilder::new(name?) + .with_records(records) + .with_entities(entities) .with_annotations(annotations( &model.doc, &model.constraints, @@ -117,7 +98,25 @@ pub(crate) fn lower(model: &Model, sink: &mut Diagnostics) -> Schema { "", sink, )) - .build() + .build(); + match schema { + Ok(schema) => Some(schema), + Err(error) => { + schema_builder_diagnostics(&error, sink); + None + } + } +} + +fn schema_builder_diagnostics(error: &BuilderError, sink: &mut Diagnostics) { + match error { + BuilderError::DuplicateName(name) => sink.error( + &format!("records.{name}"), + format!("record `{name}` conflicts with a generated resource record: {error}"), + None, + ), + error => sink.error("", error.to_string(), None), + } } /// Lower one record, or `None` (after reporting) if its name is rejected. @@ -133,19 +132,17 @@ fn record_of( let path = format!("records.{name}"); let id = type_decl_ident(name, "records", sink); let fields = fields_of(&record.fields, &path, resources, sink); - Some( - RecordBuilder::new(id?) - .try_with_fields(fields) - .expect("field names are unique") - .with_annotations(annotations( - &record.doc, - &record.constraints, - &record.metadata, - &path, - sink, - )) - .build(), - ) + let record = RecordBuilder::new(id?) + .with_fields(fields) + .with_annotations(annotations( + &record.doc, + &record.constraints, + &record.metadata, + &path, + sink, + )) + .build(); + build_or_diagnose(record, &path, sink) } /// Lower one entity and any records generated by its resource declaration. @@ -166,10 +163,13 @@ fn entity_of( sink, ); - let (records, bounds_record) = match (id.clone(), &entity.resource) { - (Some(id), Some(resource)) => resources.lower(name, id, resource, &mut anns, &path, sink), - _ => (Vec::new(), None), + let (records, bounds_record, resource_constraint) = match (id.clone(), &entity.resource) { + (Some(id), Some(resource)) => resources.lower(name, id, resource, &path, sink), + _ => (Vec::new(), None, None), }; + if let Some(data) = resource_constraint { + anns = anns.with_constraint(Resource::NAME, Some(data)); + } let events: Vec<_> = entity .events @@ -186,9 +186,8 @@ fn entity_of( }) .collect(); match EntityBuilder::new(id?) - .try_with_events(events) - .expect("event names are unique") - .with_annotations(anns.build()) + .with_events(events) + .with_annotations(build_or_diagnose(anns.build(), &path, sink).unwrap_or_default()) .build() { Ok(entity) => Some((entity, records)), @@ -221,10 +220,13 @@ fn fsm_entity_of( let id = type_decl_ident(name, "fsms", sink); let mut anns = annotations_builder(&spec.doc, &spec.constraints, &spec.metadata, &path, sink); - let (records, bounds_record) = match (id.clone(), &spec.resource) { - (Some(id), Some(resource)) => resources.lower(name, id, resource, &mut anns, &path, sink), - _ => (Vec::new(), None), + let (records, bounds_record, resource_constraint) = match (id.clone(), &spec.resource) { + (Some(id), Some(resource)) => resources.lower(name, id, resource, &path, sink), + _ => (Vec::new(), None, None), }; + if let Some(data) = resource_constraint { + anns = anns.with_constraint(Resource::NAME, Some(data)); + } // Lower the states first, before bailing on a rejected entity name, so one // run still reports problems inside the states. @@ -270,7 +272,7 @@ fn fsm_entity_of( } let built = FsmEntityBuilder::new(id) - .with_annotations(anns.build()) + .with_annotations(build_or_diagnose(anns.build(), &path, sink).unwrap_or_default()) .with_states(states) .build(); match built { @@ -295,20 +297,8 @@ fn fsm_shape_error(error: &FsmEntityBuilderError, path: &str, sink: &mut Diagnos FsmEntityBuilderError::NoExitState => { sink.error(path, "no state transitions to `exit`", None); } - // Topology violations carry the constraint's errors; report one per - // violation, flattening `Multiple`. - FsmEntityBuilderError::Invalid(error) => fsm_error_diagnostics(error, path, sink), - other => sink.error(path, other.to_string(), None), - } -} - -/// Report one diagnostic per FSM topology violation, flattening `Multiple`. -fn fsm_error_diagnostics(error: &FsmError, path: &str, sink: &mut Diagnostics) { - match error { - FsmError::Multiple(errors) => { - for error in errors { - fsm_error_diagnostics(error, path, sink); - } + FsmEntityBuilderError::Invalid(error) => { + sink.error(path, error.to_string(), None); } other => sink.error(path, other.to_string(), None), } @@ -339,13 +329,11 @@ fn event_of( } else { Cardinality::Once }; - Some( - EventBuilder::new(id?, cardinality) - .try_with_fields(fields) - .expect("field names are unique") - .with_annotations(anns) - .build(), - ) + let event = EventBuilder::new(id?, cardinality) + .with_fields(fields) + .with_annotations(anns) + .build(); + build_or_diagnose(event, &path, sink) } /// Lower event attributes, including a resource bounds field. @@ -461,19 +449,25 @@ fn entity_ref( Some(expr) => Some(type_of(expr, path, resources, sink)?), None => None, }; - Some(entity_ref_type(target, data, tree)) + entity_ref_type(target, data, tree, path, sink) } -fn entity_ref_type(target: &str, data: Option, tree: bool) -> DataType { - let mut builder = AnnotationsBuilder::new(); - builder.set_constraint(RefTargetConstraint::NAME, Some(target.to_string())); +fn entity_ref_type( + target: &str, + data: Option, + tree: bool, + path: &str, + sink: &mut Diagnostics, +) -> Option { + let mut builder = AnnotationsBuilder::new() + .with_constraint(RefTargetConstraint::NAME, Some(target.to_string())); if tree { - builder.set_constraint(RefTreeConstraint::NAME, None); + builder = builder.with_constraint(RefTreeConstraint::NAME, None); } - DataType::EntityRef { + Some(DataType::EntityRef { data: data.map(Box::new), - annotations: builder.build(), - } + annotations: build_or_diagnose(builder.build(), path, sink).unwrap_or_default(), + }) } /// A bare name that is not a [`BuiltinType`], lowered as a record reference. @@ -527,36 +521,40 @@ fn annotations( path: &str, sink: &mut Diagnostics, ) -> Annotations { - annotations_builder(doc, constraints, metadata, path, sink).build() + build_or_diagnose( + annotations_builder(doc, constraints, metadata, path, sink).build(), + path, + sink, + ) + .unwrap_or_default() } -fn annotations_builder( - doc: &Option, - constraints: &AnnotationMap, - metadata: &AnnotationMap, +fn build_or_diagnose( + result: Result, path: &str, sink: &mut Diagnostics, -) -> AnnotationsBuilder { - let mut builder = AnnotationsBuilder::new(); - if let Some(doc) = doc { - builder.set_docs(doc.clone()); +) -> Option { + match result { + Ok(value) => Some(value), + Err(error) => { + sink.error(path, error.to_string(), None); + None + } } - add_annotations(&mut builder, constraints, metadata, path, sink); - builder } -fn add_annotations( - builder: &mut AnnotationsBuilder, +fn annotations_builder( + doc: &Option, constraints: &AnnotationMap, metadata: &AnnotationMap, path: &str, sink: &mut Diagnostics, -) { +) -> AnnotationsBuilder { + let mut builder = match doc { + Some(doc) => AnnotationsBuilder::new().with_docs(doc), + None => AnnotationsBuilder::new(), + }; for (name, value) in constraints { - if name.is_empty() { - sink.error(path, "constraint name must not be empty", None); - continue; - } // The FSM constraint is produced only by the builder, from an `fsms:` // block, never written by hand. if name == FsmConstraint::NAME { @@ -575,19 +573,12 @@ fn add_annotations( ); continue; } - if let Err(e) = builder.try_insert_constraint(name, value.clone()) { - sink.error(path, e.to_string(), None); - } + builder = builder.with_constraint(name, value.clone()); } for (name, value) in metadata { - if name.is_empty() { - sink.error(path, "metadata name must not be empty", None); - continue; - } - if let Err(e) = builder.try_insert_metadata(name, value.clone()) { - sink.error(path, e.to_string(), None); - } + builder = builder.with_metadata(name, value.clone()); } + builder } /// Validate a record/entity name, or `None` (after reporting) if it is invalid @@ -668,20 +659,19 @@ impl ResourceLowerer { owner: &str, id: Identifier, resource: &ast::ResourceDecl, - anns: &mut AnnotationsBuilder, path: &str, sink: &mut Diagnostics, - ) -> (Vec, Option) { + ) -> (Vec, Option, Option) { let resource_path = format!("{path}.resource"); let Some(usage_record_name) = self.usage_records.get(owner).cloned() else { - return (Vec::new(), None); + return (Vec::new(), None, None); }; let bounds_record_name = match resource.bounds_record() { Some(name) => ident(name, &format!("{resource_path}.bounds-record"), sink), None => Some(ResourceBuilder::default_bounds_record_name(&id)), }; let Some(bounds_record_name) = bounds_record_name else { - return (Vec::new(), None); + return (Vec::new(), None, None); }; let mut builder = @@ -694,7 +684,7 @@ impl ResourceLowerer { "write `resource: true` for a unit resource, or list capacities", None, ); - return (Vec::new(), None); + return (Vec::new(), None, None); } ast::ResourceDecl::Capacities(capacities) => Some(capacities), ast::ResourceDecl::Detailed(spec) => Some(&spec.capacities), @@ -710,20 +700,21 @@ impl ResourceLowerer { match builder.build() { Ok(parts) => { - match parts.definition.constraint_data() { - Ok(data) => { - anns.set_constraint(Resource::NAME, Some(data)); + let constraint = match parts.definition.constraint_data() { + Ok(data) => Some(data), + Err(error) => { + sink.error(&resource_path, error.to_string(), None); + None } - Err(error) => sink.error(&resource_path, error.to_string(), None), - } + }; let bounds = parts.bounds.as_ref().map(|record| record.name().clone()); let mut records = vec![parts.usage]; records.extend(parts.bounds); - (records, bounds) + (records, bounds, constraint) } Err(error) => { sink.error(&resource_path, error.to_string(), None); - (Vec::new(), None) + (Vec::new(), None, None) } } } @@ -779,9 +770,5 @@ fn usage_ref( sink: &mut Diagnostics, ) -> Option { let usage = resources.usage_record(target, path, sink)?; - Some(entity_ref_type( - target, - Some(DataType::Record(usage)), - false, - )) + entity_ref_type(target, Some(DataType::Record(usage)), false, path, sink) } diff --git a/crates/yaml/tests/diagnostics.rs b/crates/yaml/tests/diagnostics.rs index 001b8243d..f062767bc 100644 --- a/crates/yaml/tests/diagnostics.rs +++ b/crates/yaml/tests/diagnostics.rs @@ -154,7 +154,7 @@ fn empty_annotation_name() { constraints: '': x ", - &["constraint name must not be empty"], + &["name must not be empty"], ); } From 5f40c82d862db460d476ca87be0a09c20c1e7b8a Mon Sep 17 00:00:00 2001 From: Johan Peltenburg Date: Thu, 23 Jul 2026 19:53:35 +0200 Subject: [PATCH 2/2] docs(schema): document entity builder errors --- crates/schema/src/builder/entity.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/schema/src/builder/entity.rs b/crates/schema/src/builder/entity.rs index 73a76445d..b2405084c 100644 --- a/crates/schema/src/builder/entity.rs +++ b/crates/schema/src/builder/entity.rs @@ -56,7 +56,8 @@ impl EntityBuilder { /// /// # Errors /// - /// Errors if the entity declares no events. + /// Errors if the entity declares no events, an event name is repeated, or + /// the annotations are invalid. pub fn build(self) -> Result { let Self { name,