From 0981bb1f77302c31d3f243d244fbb02660fc441f Mon Sep 17 00:00:00 2001 From: James Gilles Date: Mon, 9 Sep 2024 12:37:55 -0400 Subject: [PATCH] Final comments addressed & copy editing --- crates/schema/src/def/validate/v9.rs | 2 +- crates/schema/src/type_for_generate.rs | 184 +++++++++++++++---------- 2 files changed, 112 insertions(+), 74 deletions(-) diff --git a/crates/schema/src/def/validate/v9.rs b/crates/schema/src/def/validate/v9.rs index 2a072b9935..c9dee0d9db 100644 --- a/crates/schema/src/def/validate/v9.rs +++ b/crates/schema/src/def/validate/v9.rs @@ -363,7 +363,7 @@ impl ModuleValidator<'_> { /// Validates that a type can be used to generate a client type use. fn validate_for_type_use(&mut self, location: &TypeLocation, ty: &AlgebraicType) -> Result { - self.typespace_for_generate.add_use(ty).map_err(|err| { + self.typespace_for_generate.parse_use(ty).map_err(|err| { ErrorStream::expect_nonempty(err.into_iter().map(|error| ValidationError::ClientCodegenError { location: location.clone().make_static(), error, diff --git a/crates/schema/src/type_for_generate.rs b/crates/schema/src/type_for_generate.rs index 72477b6e78..1ed56bdb5a 100644 --- a/crates/schema/src/type_for_generate.rs +++ b/crates/schema/src/type_for_generate.rs @@ -31,7 +31,7 @@ pub enum ClientCodegenError { NonDeclaredTypeDef { ref_: AlgebraicTypeRef }, #[error("internal codegen error: all type elements require names: {ty}")] - NamelessTypeDef { ty: PrettyAlgebraicType }, + NamelessTypeDefElement { ty: PrettyAlgebraicType }, #[error("internal codegen error: all reducer parameters require names")] NamelessReducerParam, @@ -152,6 +152,7 @@ pub enum AlgebraicTypeDef { } impl AlgebraicTypeDef { + /// Check if a def is recursive. pub fn is_recursive(&self) -> bool { match self { AlgebraicTypeDef::Product(ProductTypeDef { recursive, .. }) => *recursive, @@ -177,6 +178,8 @@ impl AlgebraicTypeDef { } } + /// Mark a def recursive. + /// Panics if the def is a `PlainEnum`, because how would that be recursive? fn mark_recursive(&mut self) { match self { AlgebraicTypeDef::Product(ProductTypeDef { recursive, .. }) => { @@ -185,7 +188,9 @@ impl AlgebraicTypeDef { AlgebraicTypeDef::Sum(SumTypeDef { recursive, .. }) => { *recursive = true; } - AlgebraicTypeDef::PlainEnum(_) => {} + AlgebraicTypeDef::PlainEnum(def) => { + panic!("mark_recursive called on a PlainEnumTypeDef: {def:?}"); + } } } } @@ -274,6 +279,10 @@ impl<'a> IntoIterator for &'a SumTypeDef { } /// A use of an algebraic type. +/// +/// This type uses `Arc`s to make cloning cheap. +/// These `Arc`s are interned/hash-consed in the `TypespaceForGenerateBuilder`. +/// They are not semantically meaningful and are guaranteed to be acyclic. #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub enum AlgebraicTypeUse { /// A type where the definition is given by the typing context (`Typespace`). @@ -283,13 +292,11 @@ pub enum AlgebraicTypeUse { /// The type of array values where elements are of a base type `elem_ty`. /// Values [`AlgebraicValue::Array(array)`](crate::AlgebraicValue::Array) will have this type. - /// Stores an `Arc` for compression, the `Arc` is not semantically meaningful. Array(Arc), /// The type of map values consisting of a key type `key_ty` and value `ty`. /// Values [`AlgebraicValue::Map(map)`](crate::AlgebraicValue::Map) will have this type. /// The order of entries in a map value is observable. - /// Stores `Arc`s for compression, the `Arc` is not semantically meaningful. Map { key: Arc, value: Arc, @@ -312,7 +319,7 @@ pub enum AlgebraicTypeUse { Unit, /// The never type (empty sum). - /// This is *distinct* from a use of a definition of a product type with no elements. + /// This is *distinct* from a use of a definition of a sum type with no variants. Never, /// The UTF-8 encoded `String` type. @@ -323,7 +330,7 @@ pub enum AlgebraicTypeUse { } impl AlgebraicTypeUse { - /// Extract all `AlgebraicTypeRef`s that are used in this type. + /// Extract all `AlgebraicTypeRef`s that are used in this type and add them to `buf`.` fn extract_refs(&self, buf: &mut HashSet) { match self { AlgebraicTypeUse::Ref(ref_) => { @@ -340,26 +347,6 @@ impl AlgebraicTypeUse { } } -impl AlgebraicTypeUse { - /// Recurses through this `AlgebraicTypeUse` tree, calling the function F for every `Ref` found. - pub fn for_each_ref(&self, mut f: F) { - self._for_each_ref(&mut f) - } - - fn _for_each_ref(&self, f: &mut F) { - match self { - AlgebraicTypeUse::Ref(r) => f(*r), - AlgebraicTypeUse::Array(elem) => elem._for_each_ref(f), - AlgebraicTypeUse::Map { key, value } => { - key._for_each_ref(f); - value._for_each_ref(f); - } - AlgebraicTypeUse::Option(t) => t._for_each_ref(f), - _ => {} - } - } -} - /// A builder for a `TypespaceForGenerate`. /// /// This is complicated by the fact that a typespace can store both *uses* and *definitions* of types. @@ -397,7 +384,8 @@ impl TypespaceForGenerateBuilder<'_> { for type_ in self.is_def.iter() { assert!( self.result.defs.contains_key(type_), - "internal codegen error: not all definitions were processed" + "internal codegen error: not all definitions were processed. + Did you call `add_definition` for all types in `is_def`?" ); } @@ -406,9 +394,9 @@ impl TypespaceForGenerateBuilder<'_> { self.result } - /// Add a *type use* to the typespace. - /// This is a *use* of a type, not a definition. - pub fn add_use(&mut self, ty: &AlgebraicType) -> Result { + /// Use the `TypespaceForGenerateBuilder` to validate an `AlgebraicTypeUse`. + /// Does not actually add anything to the `TypespaceForGenerate`. + pub fn parse_use(&mut self, ty: &AlgebraicType) -> Result { if ty.is_address() { Ok(AlgebraicTypeUse::Address) } else if ty.is_identity() { @@ -418,7 +406,7 @@ impl TypespaceForGenerateBuilder<'_> { } else if ty.is_never() { Ok(AlgebraicTypeUse::Never) } else if let Some(elem_ty) = ty.as_option() { - let elem_ty = self.add_use(elem_ty)?; + let elem_ty = self.parse_use(elem_ty)?; let interned = self.intern_use(elem_ty); Ok(AlgebraicTypeUse::Option(interned)) } else if ty.is_schedule_at() { @@ -427,16 +415,16 @@ impl TypespaceForGenerateBuilder<'_> { match ty { AlgebraicType::Ref(ref_) => { // Indirectly recurse. - self.add_ref(*ref_) + self.parse_ref(*ref_) } AlgebraicType::Array(ArrayType { elem_ty }) => { - let elem_ty = self.add_use(elem_ty)?; + let elem_ty = self.parse_use(elem_ty)?; let interned = self.intern_use(elem_ty); Ok(AlgebraicTypeUse::Array(interned)) } AlgebraicType::Map(map) => { - let key_ty = self.add_use(&map.key_ty); - let value_ty = self.add_use(&map.ty); + let key_ty = self.parse_use(&map.key_ty); + let value_ty = self.parse_use(&map.ty); let (key_ty, value_ty) = (key_ty, value_ty).combine_errors()?; let interned_key = self.intern_use(key_ty); let interned_value = self.intern_use(value_ty); @@ -471,10 +459,10 @@ impl TypespaceForGenerateBuilder<'_> { } } - /// This is the only seriously complicated case of `add_use`, which has to deal with cycle detection. + /// This is the only seriously complicated case of `parse_use`, which has to deal with cycle detection. /// So it gets its own function. - /// Mutually recursive with `add_use`. - fn add_ref(&mut self, ref_: AlgebraicTypeRef) -> Result { + /// Mutually recursive with `parse_use`. + fn parse_ref(&mut self, ref_: AlgebraicTypeRef) -> Result { if self.is_def.contains(&ref_) { // We know this type is going to be a definition. // So, we can just return a ref to it. @@ -505,7 +493,7 @@ impl TypespaceForGenerateBuilder<'_> { // Mark this ref. self.currently_touching.insert(ref_); // Recurse. - let result = self.add_use(def); + let result = self.parse_use(def); // Unmark this ref before dealing with possible errors. self.currently_touching.remove(&ref_); @@ -522,9 +510,12 @@ impl TypespaceForGenerateBuilder<'_> { /// Does not detect cycles, those are left for `mark_allowed_cycles`, which is called after all definitions are processed. /// /// Why not invoke this for all definitions ourselves, since we know which refs are definitions? - /// It's so that the caller can wrap definitions with better context and error information. + /// It's so that the caller can wrap errors with better context information. pub fn add_definition(&mut self, ref_: AlgebraicTypeRef) -> Result<()> { - assert!(self.is_def.contains(&ref_)); + assert!( + self.is_def.contains(&ref_), + "internal codegen error: any AlgebraicTypeRef passed to `add_definition` must refer to a declared definition, {ref_} does not" + ); let def = self .typespace @@ -580,11 +571,10 @@ impl TypespaceForGenerateBuilder<'_> { result } - /// Process an element of a product or sum type. + /// Process an element/variant of a product/sum type. /// /// `def` is the *containing* type that corresponds to a `Def`, - /// `algebraic_type` is the type of the element and corresponds to a `Use`. - /// (Or possibly an error in either case.) + /// `algebraic_type` is the type of the element/variant inside `def` and corresponds to a `Use`. fn process_element( &mut self, def: &AlgebraicType, @@ -593,7 +583,7 @@ impl TypespaceForGenerateBuilder<'_> { ) -> Result<(Identifier, AlgebraicTypeUse)> { let element_name = element_name .as_ref() - .ok_or_else(|| ErrorStream::from(ClientCodegenError::NamelessTypeDef { ty: def.clone().into() })) + .ok_or_else(|| ErrorStream::from(ClientCodegenError::NamelessTypeDefElement { ty: def.clone().into() })) .and_then(|element_name| { Identifier::new(element_name.clone()).map_err(|err| { ErrorStream::from(ClientCodegenError::NotValidIdentifier { @@ -602,7 +592,7 @@ impl TypespaceForGenerateBuilder<'_> { }) }) }); - let ty = self.add_use(element_type); + let ty = self.parse_use(element_type); (element_name, ty).combine_errors() } @@ -618,7 +608,7 @@ impl TypespaceForGenerateBuilder<'_> { } /// Cycles passing through definitions are allowed. - /// This function is called after all definitions and uses have been processed. + /// This function is called after all definitions have been processed. fn mark_allowed_cycles(&mut self) { let mut to_process = self.is_def.clone(); let mut scratch = HashSet::new(); @@ -647,11 +637,12 @@ impl TypespaceForGenerateBuilder<'_> { // Figure out who to look at. // Note: this skips over refs in the original typespace that - // didn't point to definitions; those have already been combined. + // didn't point to definitions; those have already been removed. scratch.clear(); let to_examine = scratch; self.result.defs[&def].extract_refs(to_examine); + // Update the parent chain with the current def, for passing to children. let chain = ParentChain { parent, ref_: def }; // First, check for finished cycles. @@ -665,7 +656,7 @@ impl TypespaceForGenerateBuilder<'_> { .get_mut(&parent_ref) .expect("all defs should have been processed by now") .mark_recursive(); - // It's tempting to also remove the result from "to_process" here, + // It's tempting to also remove `parent_ref` from `to_process` here, // but that's wrong, because it might participate in other cycles. // We want to mark the start of the cycle as recursive too. @@ -678,15 +669,13 @@ impl TypespaceForGenerateBuilder<'_> { } // Now that we've marked everything possible, we need to recurse. - // Need a buffer to iterate from. This will usually not allocate. - let mut to_recurse = SmallVec::<[AlgebraicTypeRef; 16]>::new(); - for element in to_examine.iter() { - let not_touching = !self.currently_touching.contains(element); // Skip elements from previous loop. - let not_processed = to_process.contains(element); - if not_touching && not_processed { - to_recurse.push(*element); - } - } + // Need a buffer to iterate from because we reuse `to_examine` in children. + // This will usually not allocate. Most defs have less than 16 refs. + let to_recurse = to_examine + .iter() + .cloned() + .filter(|element| to_process.contains(element) && !self.currently_touching.contains(element)) + .collect::>(); // Recurse. let scratch = to_examine; @@ -701,6 +690,7 @@ impl TypespaceForGenerateBuilder<'_> { correct, "mark_allowed_cycles_rec is finishing, we should be touching that ref." ); + // Only remove a def from `to_process` once we've explored all the paths leaving it. to_process.remove(&def); } } @@ -762,7 +752,7 @@ mod tests { if is_def.contains(&ref_) { builder.add_definition(ref_).unwrap(); } else { - builder.add_use(ty).unwrap(); + builder.parse_use(ty).unwrap(); } } } @@ -789,11 +779,11 @@ mod tests { let mut for_generate_forward = TypespaceForGenerate::builder(&t, [def]); for_generate_forward.add_definition(def).unwrap(); - let use0 = for_generate_forward.add_use(&ref0.into()).unwrap(); - let use1 = for_generate_forward.add_use(&ref1.into()).unwrap(); - let use2 = for_generate_forward.add_use(&ref2.into()).unwrap(); - let use3 = for_generate_forward.add_use(&ref3.into()).unwrap(); - let use4 = for_generate_forward.add_use(&ref4.into()).unwrap(); + let use0 = for_generate_forward.parse_use(&ref0.into()).unwrap(); + let use1 = for_generate_forward.parse_use(&ref1.into()).unwrap(); + let use2 = for_generate_forward.parse_use(&ref2.into()).unwrap(); + let use3 = for_generate_forward.parse_use(&ref3.into()).unwrap(); + let use4 = for_generate_forward.parse_use(&ref4.into()).unwrap(); assert_eq!(use0, expected_0); assert_eq!(use1, expected_1); @@ -802,11 +792,11 @@ mod tests { assert_eq!(use4, expected_4); let mut for_generate_backward = TypespaceForGenerate::builder(&t, [def]); - let use4 = for_generate_backward.add_use(&ref4.into()).unwrap(); - let use3 = for_generate_forward.add_use(&ref3.into()).unwrap(); - let use2 = for_generate_forward.add_use(&ref2.into()).unwrap(); - let use1 = for_generate_forward.add_use(&ref1.into()).unwrap(); - let use0 = for_generate_backward.add_use(&ref0.into()).unwrap(); + let use4 = for_generate_backward.parse_use(&ref4.into()).unwrap(); + let use3 = for_generate_forward.parse_use(&ref3.into()).unwrap(); + let use2 = for_generate_forward.parse_use(&ref2.into()).unwrap(); + let use1 = for_generate_forward.parse_use(&ref1.into()).unwrap(); + let use0 = for_generate_backward.parse_use(&ref0.into()).unwrap(); for_generate_backward.add_definition(def).unwrap(); assert_eq!(use0, expected_0); @@ -820,7 +810,7 @@ mod tests { fn test_detects_cycles() { let cyclic_1 = Typespace::new(vec![AlgebraicType::Ref(AlgebraicTypeRef(0))]); let mut for_generate = TypespaceForGenerate::builder(&cyclic_1, []); - let err1 = for_generate.add_use(&AlgebraicType::Ref(AlgebraicTypeRef(0))); + let err1 = for_generate.parse_use(&AlgebraicType::Ref(AlgebraicTypeRef(0))); expect_error_matching!( err1, @@ -832,7 +822,7 @@ mod tests { AlgebraicType::Ref(AlgebraicTypeRef(0)), ]); let mut for_generate = TypespaceForGenerate::builder(&cyclic_2, []); - let err2 = for_generate.add_use(&AlgebraicType::Ref(AlgebraicTypeRef(0))); + let err2 = for_generate.parse_use(&AlgebraicType::Ref(AlgebraicTypeRef(0))); expect_error_matching!( err2, @@ -845,7 +835,7 @@ mod tests { ]); let mut for_generate = TypespaceForGenerate::builder(&cyclic_3, [AlgebraicTypeRef(1)]); for_generate - .add_use(&AlgebraicType::Ref(AlgebraicTypeRef(0))) + .parse_use(&AlgebraicType::Ref(AlgebraicTypeRef(0))) .expect("should be allowed"); for_generate .add_definition(AlgebraicTypeRef(1)) @@ -875,7 +865,7 @@ mod tests { for i in 0..5 { for_generate - .add_use(&AlgebraicType::Ref(AlgebraicTypeRef(i))) + .parse_use(&AlgebraicType::Ref(AlgebraicTypeRef(i))) .expect("should be allowed"); for_generate .add_definition(AlgebraicTypeRef(i)) @@ -890,5 +880,53 @@ mod tests { !result[AlgebraicTypeRef(4)].is_recursive(), "recursion detected incorrectly" ); + + // Branching cycles. + let cyclic_5 = Typespace::new(vec![ + // cyclic component. + AlgebraicType::product([("field", AlgebraicTypeRef(1).into())]), + AlgebraicType::product([ + ("cyclic_1", AlgebraicTypeRef(2).into()), + ("cyclic_2", AlgebraicTypeRef(3).into()), + ("acyclic", AlgebraicTypeRef(5).into()), + ]), + AlgebraicType::product([("field", AlgebraicTypeRef(0).into())]), + AlgebraicType::product([("field", AlgebraicTypeRef(0).into())]), + // points into cyclic component, but is not cyclic. + AlgebraicType::product([("field", AlgebraicTypeRef(2).into())]), + // referred to by cyclic component, but is not cyclic. + AlgebraicType::product([("field", AlgebraicType::U32)]), + ]); + let mut for_generate = TypespaceForGenerate::builder( + &cyclic_5, + [ + AlgebraicTypeRef(0), + AlgebraicTypeRef(1), + AlgebraicTypeRef(2), + AlgebraicTypeRef(3), + AlgebraicTypeRef(4), + AlgebraicTypeRef(5), + ], + ); + + for i in 0..6 { + for_generate + .parse_use(&AlgebraicType::Ref(AlgebraicTypeRef(i))) + .expect("should be allowed"); + for_generate + .add_definition(AlgebraicTypeRef(i)) + .expect("should be allowed"); + } + let result = for_generate.finish(); + + for i in 0..4 { + assert!(result[AlgebraicTypeRef(i)].is_recursive(), "recursion not detected"); + } + for i in 4..6 { + assert!( + !result[AlgebraicTypeRef(i)].is_recursive(), + "recursion detected incorrectly" + ); + } } }