From c030b216c7a12a33c7cecf2bb5cc4a205e0cee96 Mon Sep 17 00:00:00 2001
From: Casey Rodarmor <casey@rodarmor.com>
Date: Fri, 3 Jan 2025 13:06:25 -0800
Subject: [PATCH 1/2] Check attributes in parser instead of analyzer

---
 src/analyzer.rs      | 34 ++--------------------------------
 src/attribute_set.rs | 10 ++++++++++
 src/item.rs          |  4 ++--
 src/parser.rs        | 35 ++++++++++++++++++++++++++++++-----
 4 files changed, 44 insertions(+), 39 deletions(-)

diff --git a/src/analyzer.rs b/src/analyzer.rs
index e160d8bcb4..1f9dd885b7 100644
--- 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) => {
@@ -67,34 +66,14 @@ impl<'run, 'src> Analyzer<'run, 'src> {
             absolute,
             name,
             doc,
-            attributes,
+            groups,
             ..
           } => {
-            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
index 49d10d2e82..a40b001d1a 100644
--- 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
index 60e2831461..e6036e7ab7 100644
--- 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
index 2258c13b3b..eea3e6c78e 100644
--- 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! {

From 42bfec2ec5669c2cf6b2af699dfd88afd91788ba Mon Sep 17 00:00:00 2001
From: Casey Rodarmor <casey@rodarmor.com>
Date: Fri, 3 Jan 2025 13:09:07 -0800
Subject: [PATCH 2/2] Modify

---
 src/analyzer.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/analyzer.rs b/src/analyzer.rs
index 1f9dd885b7..c2c3da67ec 100644
--- a/src/analyzer.rs
+++ b/src/analyzer.rs
@@ -64,9 +64,9 @@ impl<'run, 'src> Analyzer<'run, 'src> {
           }
           Item::Module {
             absolute,
-            name,
             doc,
             groups,
+            name,
             ..
           } => {
             if let Some(absolute) = absolute {