From 3ef05ffb36cba7bf2d6b2a395abfe466195b6fbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20Houl=C3=A9?= <13155277+tomhoule@users.noreply.github.com> Date: Thu, 14 Sep 2023 13:24:08 +0200 Subject: [PATCH] psl: simplify attribute list validation (#4237) Since type aliases are not a thing, `AttributesValidationState.attributes` does not need to be a Vec anymore (we were accumulating attributes defined on the model and on aliases), it can be an Option. This is simpler to reason about. --- psl/parser-database/src/context.rs | 24 +++++++++---------- psl/parser-database/src/context/attributes.rs | 7 +++--- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/psl/parser-database/src/context.rs b/psl/parser-database/src/context.rs index 54c06ddd9a42..450146953024 100644 --- a/psl/parser-database/src/context.rs +++ b/psl/parser-database/src/context.rs @@ -117,7 +117,7 @@ impl<'db> Context<'db> { /// - When you are done validating an attribute set, you must call /// `validate_visited_attributes()`. Otherwise, Context will helpfully panic. pub(super) fn visit_attributes(&mut self, ast_attributes: ast::AttributeContainer) { - if !self.attributes.attributes.is_empty() || !self.attributes.unused_attributes.is_empty() { + if self.attributes.attributes.is_some() || !self.attributes.unused_attributes.is_empty() { panic!( "`ctx.visit_attributes() called with {:?} while the Context is still validating previous attribute set on {:?}`", ast_attributes, @@ -125,9 +125,7 @@ impl<'db> Context<'db> { ); } - self.attributes.attributes.clear(); - self.attributes.unused_attributes.clear(); - self.attributes.extend_attributes(ast_attributes, self.ast); + self.attributes.set_attributes(ast_attributes, self.ast); } /// Look for an optional attribute with a name of the form @@ -139,8 +137,8 @@ impl<'db> Context<'db> { /// with a default that can be first, but with native types, arguments are /// purely positional. pub(crate) fn visit_datasource_scoped(&mut self) -> Option<(StringId, StringId, ast::AttributeId)> { - let attrs = - iter_attributes(&self.attributes.attributes, self.ast).filter(|(_, attr)| attr.name.name.contains('.')); + let attrs = iter_attributes(self.attributes.attributes.as_ref(), self.ast) + .filter(|(_, attr)| attr.name.name.contains('.')); let mut native_type_attr = None; let diagnostics = &mut self.diagnostics; @@ -173,7 +171,8 @@ impl<'db> Context<'db> { /// is defined. #[must_use] pub(crate) fn visit_optional_single_attr(&mut self, name: &'static str) -> bool { - let mut attrs = iter_attributes(&self.attributes.attributes, self.ast).filter(|(_, a)| a.name.name == name); + let mut attrs = + iter_attributes(self.attributes.attributes.as_ref(), self.ast).filter(|(_, a)| a.name.name == name); let (first_idx, first) = match attrs.next() { Some(first) => first, None => return false, @@ -182,7 +181,7 @@ impl<'db> Context<'db> { if attrs.next().is_some() { for (idx, attr) in - iter_attributes(&self.attributes.attributes, self.ast).filter(|(_, a)| a.name.name == name) + iter_attributes(self.attributes.attributes.as_ref(), self.ast).filter(|(_, a)| a.name.name == name) { diagnostics.push_error(DatamodelError::new_duplicate_attribute_error( &attr.name.name, @@ -206,7 +205,7 @@ impl<'db> Context<'db> { let mut has_valid_attribute = false; while !has_valid_attribute { - let first_attr = iter_attributes(&self.attributes.attributes, self.ast) + let first_attr = iter_attributes(self.attributes.attributes.as_ref(), self.ast) .filter(|(_, attr)| attr.name.name == name) .find(|(attr_id, _)| self.attributes.unused_attributes.contains(attr_id)); let (attr_id, attr) = if let Some(first_attr) = first_attr { @@ -297,7 +296,8 @@ impl<'db> Context<'db> { attribute.span, )) } - self.attributes.attributes.clear(); + + self.attributes.attributes = None; self.attributes.unused_attributes.clear(); } @@ -430,11 +430,11 @@ impl<'db> Context<'db> { // Implementation detail. Used for arguments validation. fn iter_attributes<'a, 'ast: 'a>( - attrs: &'a [ast::AttributeContainer], + attrs: Option<&'a ast::AttributeContainer>, ast: &'ast ast::SchemaAst, ) -> impl Iterator + 'a { attrs - .iter() + .into_iter() .flat_map(move |container| ast[*container].iter().enumerate().map(|a| (a, *container))) .map(|((idx, attr), container)| (ast::AttributeId::new_in_container(container, idx), attr)) } diff --git a/psl/parser-database/src/context/attributes.rs b/psl/parser-database/src/context/attributes.rs index 39655decf8b4..9f35f5cc3644 100644 --- a/psl/parser-database/src/context/attributes.rs +++ b/psl/parser-database/src/context/attributes.rs @@ -4,7 +4,7 @@ use crate::interner::StringId; #[derive(Default, Debug)] pub(super) struct AttributesValidationState { /// The attributes list being validated. - pub(super) attributes: Vec, + pub(super) attributes: Option, pub(super) unused_attributes: HashSet, // the _remaining_ attributes /// The attribute being validated. @@ -13,10 +13,11 @@ pub(super) struct AttributesValidationState { } impl AttributesValidationState { - pub(super) fn extend_attributes(&mut self, attributes: ast::AttributeContainer, ast: &ast::SchemaAst) { + pub(super) fn set_attributes(&mut self, attributes: ast::AttributeContainer, ast: &ast::SchemaAst) { let attribute_ids = (0..ast[attributes].len()).map(|idx| ast::AttributeId::new_in_container(attributes, idx)); + self.unused_attributes.clear(); self.unused_attributes.extend(attribute_ids); - self.attributes.push(attributes); + self.attributes = Some(attributes); } }