Check attributes in parser instead of analyzer (#2560)

casey · Jan 3, 2025 · 1f7df4a · 1f7df4a
1 parent 74dabba
commit 1f7df4a
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 40 deletions.
diff --git a/src/analyzer.rs b/src/analyzer.rs
@@ -49,7 +49,6 @@ impl<'run, 'src> Analyzer<'run, 'src> {
         match item {
           Item::Alias(alias) => {
             Self::define(&mut definitions, alias.name, "alias", false)?;
-            Self::analyze_alias(alias)?;
             self.aliases.insert(alias.clone());
           }
           Item::Assignment(assignment) => {
@@ -65,36 +64,16 @@ impl<'run, 'src> Analyzer<'run, 'src> {
           }
           Item::Module {
             absolute,
-            name,
             doc,
-            attributes,
+            groups,
+            name,
             ..
           } => {
-            let mut doc_attr: Option<&str> = None;
-            let mut groups = Vec::new();
-            attributes.ensure_valid_attributes(
-              "Module",
-              **name,
-              &[AttributeDiscriminant::Doc, AttributeDiscriminant::Group],
-            )?;
-
-            for attribute in attributes {
-              match attribute {
-                Attribute::Doc(ref doc) => {
-                  doc_attr = Some(doc.as_ref().map(|s| s.cooked.as_ref()).unwrap_or_default());
-                }
-                Attribute::Group(ref group) => {
-                  groups.push(group.cooked.clone());
-                }
-                _ => unreachable!(),
-              }
-            }
-
             if let Some(absolute) = absolute {
               Self::define(&mut definitions, *name, "module", false)?;
               self.modules.insert(Self::analyze(
                 asts,
-                doc_attr.or(*doc).map(ToOwned::to_owned),
+                doc.clone(),
                 groups.as_slice(),
                 loaded,
                 Some(*name),
@@ -299,15 +278,6 @@ impl<'run, 'src> Analyzer<'run, 'src> {
     Ok(())
   }
 
-  fn analyze_alias(alias: &Alias<'src, Name<'src>>) -> CompileResult<'src> {
-    alias.attributes.ensure_valid_attributes(
-      "Alias",
-      *alias.name,
-      &[AttributeDiscriminant::Private],
-    )?;
-    Ok(())
-  }
-
   fn analyze_set(&self, set: &Set<'src>) -> CompileResult<'src> {
     if let Some(original) = self.sets.get(set.name.lexeme()) {
       return Err(set.name.error(DuplicateSet {

diff --git a/src/attribute_set.rs b/src/attribute_set.rs
@@ -58,3 +58,13 @@ impl<'src, 'a> IntoIterator for &'a AttributeSet<'src> {
     self.0.iter()
   }
 }
+
+impl<'src> IntoIterator for AttributeSet<'src> {
+  type Item = Attribute<'src>;
+
+  type IntoIter = collections::btree_set::IntoIter<Attribute<'src>>;
+
+  fn into_iter(self) -> Self::IntoIter {
+    self.0.into_iter()
+  }
+}
diff --git a/src/item.rs b/src/item.rs
@@ -13,9 +13,9 @@ pub(crate) enum Item<'src> {
     relative: StringLiteral<'src>,
   },
   Module {
-    attributes: AttributeSet<'src>,
     absolute: Option<PathBuf>,
-    doc: Option<&'src str>,
+    doc: Option<String>,
+    groups: Vec<String>,
     name: Name<'src>,
     optional: bool,
     relative: Option<StringLiteral<'src>>,

diff --git a/src/parser.rs b/src/parser.rs
@@ -393,8 +393,30 @@ impl<'run, 'src> Parser<'run, 'src> {
               None
             };
 
+            let attributes = take_attributes();
+
+            attributes.ensure_valid_attributes(
+              "Module",
+              *name,
+              &[AttributeDiscriminant::Doc, AttributeDiscriminant::Group],
+            )?;
+
+            let doc = match attributes.get(AttributeDiscriminant::Doc) {
+              Some(Attribute::Doc(Some(doc))) => Some(doc.cooked.clone()),
+              Some(Attribute::Doc(None)) => None,
+              None => doc.map(ToOwned::to_owned),
+              _ => unreachable!(),
+            };
+
+            let mut groups = Vec::new();
+            for attribute in attributes {
+              if let Attribute::Group(group) = attribute {
+                groups.push(group.cooked);
+              }
+            }
+
             items.push(Item::Module {
-              attributes: take_attributes(),
+              groups,
               absolute: None,
               doc,
               name,
@@ -469,6 +491,9 @@ impl<'run, 'src> Parser<'run, 'src> {
     self.presume_any(&[Equals, ColonEquals])?;
     let target = self.parse_name()?;
     self.expect_eol()?;
+
+    attributes.ensure_valid_attributes("Alias", *name, &[AttributeDiscriminant::Private])?;
+
     Ok(Alias {
       attributes,
       name,
@@ -1311,14 +1336,14 @@ mod tests {
 
   test! {
     name: single_argument_attribute_shorthand,
-    text: "[group: 'some-group']\nalias t := test",
-    tree: (justfile (alias t test)),
+    text: "[group: 'foo']\nbar:",
+    tree: (justfile (recipe bar)),
   }
 
   test! {
     name: single_argument_attribute_shorthand_multiple_same_line,
-    text: "[group: 'some-group', group: 'some-other-group']\nalias t := test",
-    tree: (justfile (alias t test)),
+    text: "[group: 'foo', group: 'bar']\nbaz:",
+    tree: (justfile (recipe baz)),
   }
 
   test! {