Repository: casey/just
Pull request #1434 (2022-12-04 10:18:59)

Allow private attribute on aliases

neunenak: @casey could you take a look at this one when you have a chance?
casey: Yoo, sorry for slacking! This looks mostly good, but I believe the parser will incorrectly parse a recipe named `alias` with attributes:

```just
[private]
alias:
  echo foo
```

Just doesn't really have keywords, so `alias`, `let`, etc are all allowed to be identifiers. I took a look at the parsing code, but it isn't obvious to me how to fix this easily.
neunenak: @casey looks like I can use the same strategy the other alias-parsing code was using, namely explicitly looking ahead for a ColonEquals token. This is a bit messy, but not messier than other existing parsing code I think. 
casey: I noticed you just pushed some commits, haven't looked at them in detail, but the approach of checking the next few tokens seems reasonable.

One alternative is that we could parse attributes as items, and then let the analyzer figure out which attributes belonged on which items. This would allow attributes to be added to everything, without having to introduce special-purpose code to the parser.

Or we could take the approach used for doc-comments, where the parser basically stores pending doc-comments, and then adds them to recipes when it gets to them.
casey: What do you think? Since you got it working, putting off doing something more general is fine for now.
neunenak: In the short term, I'd like to get this merged and be able to close out https://github.com/casey/just/issues/1421 entirely. Longer-term, I think the current parser is a bit messy in a way that makes making changes to the grammar harder than it needs to be, and it's worth thinking about how to make that better in general rather than just trying to solve individual proposed changes to the grammar as they come up.

> One alternative is that we could parse attributes as items, and then let the analyzer figure out which attributes belonged on which items. This would allow attributes to be added to everything, without having to introduce special-purpose code to the parser.

I'm not a huge fan of this idea - this is basically introducing a new variant of `Item` whose sole purpose is to be merged into the existing nodes after the analyze step. Ideally the parser would be smart enough to figure out that aliases and recipes might or might not have annotations, and add those annotations directly to the AST node objects. It doesn't really make sense for the other sorts of item to have annotations anyway, I don't think.

> Or we could take the approach used for doc-comments, where the parser basically stores pending doc-comments, and then adds them to recipes when it gets to them.

This is actually kind of messy in the existing code, in that `pop_doc_comment` needs to be called in a few different places and the code surrounding it needs to keep track of the mutable `eol_since_last_comment` flag. Obviously this works, but ideally the parser would have some doc-comment-parsing code that only needs to be called once and could manage its own state internally.
casey: Sounds reasonable to me! Let's get this in.

Patch:
diff --git a/README.md b/README.md
index 8fdff68b..1810aac2 100644
--- a/README.md
+++ b/README.md
@@ -2007,12 +2007,15 @@ $ just --summary
 test
 ```
 
-The `[private]` attribute<sup>master</sup> may also be used to hide recipes without needing to change the name:
+The `[private]` attribute<sup>master</sup> may also be used to hide recipes or aliases without needing to change the name:
 
 ```just
 [private]
 foo:
 
+[private]
+alias b := bar
+
 bar:
 ```
 
diff --git a/src/alias.rs b/src/alias.rs
index f543abbb..0c19fe5a 100644
--- a/src/alias.rs
+++ b/src/alias.rs
@@ -3,6 +3,7 @@ use super::*;
 /// An alias, e.g. `name := target`
 #[derive(Debug, PartialEq, Clone, Serialize)]
 pub(crate) struct Alias<'src, T = Rc<Recipe<'src>>> {
+  pub(crate) attributes: BTreeSet<Attribute>,
   pub(crate) name: Name<'src>,
   #[serde(
     bound(serialize = "T: Keyed<'src>"),
@@ -20,6 +21,7 @@ impl<'src> Alias<'src, Name<'src>> {
     assert_eq!(self.target.lexeme(), target.name.lexeme());
 
     Alias {
+      attributes: self.attributes,
       name: self.name,
       target,
     }
@@ -28,7 +30,7 @@ impl<'src> Alias<'src, Name<'src>> {
 
 impl Alias<'_> {
   pub(crate) fn is_private(&self) -> bool {
-    self.name.lexeme().starts_with('_')
+    self.name.lexeme().starts_with('_') || self.attributes.contains(&Attribute::Private)
   }
 }
 
diff --git a/src/analyzer.rs b/src/analyzer.rs
index 237b6742..44efb43f 100644
--- a/src/analyzer.rs
+++ b/src/analyzer.rs
@@ -2,6 +2,8 @@ use super::*;
 
 use CompileErrorKind::*;
 
+const VALID_ALIAS_ATTRIBUTES: [Attribute; 1] = [Attribute::Private];
+
 #[derive(Default)]
 pub(crate) struct Analyzer<'src> {
   recipes: Table<'src, UnresolvedRecipe<'src>>,
@@ -195,6 +197,15 @@ impl<'src> Analyzer<'src> {
       }));
     }
 
+    for attr in &alias.attributes {
+      if !VALID_ALIAS_ATTRIBUTES.contains(attr) {
+        return Err(alias.name.token().error(AliasInvalidAttribute {
+          alias: name,
+          attr: *attr,
+        }));
+      }
+    }
+
     Ok(())
   }
 
diff --git a/src/compile_error.rs b/src/compile_error.rs
index 4464c618..c7dcbdf4 100644
--- a/src/compile_error.rs
+++ b/src/compile_error.rs
@@ -24,6 +24,14 @@ impl Display for CompileError<'_> {
     use CompileErrorKind::*;
 
     match &*self.kind {
+      AliasInvalidAttribute { alias, attr } => {
+        write!(
+          f,
+          "Alias {} has an invalid attribute `{}`",
+          alias,
+          attr.to_str()
+        )?;
+      }
       AliasShadowsRecipe { alias, recipe_line } => {
         write!(
           f,
diff --git a/src/compile_error_kind.rs b/src/compile_error_kind.rs
index a6cc5f00..8c1fcdfc 100644
--- a/src/compile_error_kind.rs
+++ b/src/compile_error_kind.rs
@@ -2,6 +2,10 @@ use super::*;
 
 #[derive(Debug, PartialEq)]
 pub(crate) enum CompileErrorKind<'src> {
+  AliasInvalidAttribute {
+    alias: &'src str,
+    attr: Attribute,
+  },
   AliasShadowsRecipe {
     alias: &'src str,
     recipe_line: usize,
diff --git a/src/parser.rs b/src/parser.rs
index 50082ba1..41ee8208 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -325,7 +325,7 @@ impl<'tokens, 'src> Parser<'tokens, 'src> {
       } else if self.next_is(Identifier) {
         match Keyword::from_lexeme(next.lexeme()) {
           Some(Keyword::Alias) if self.next_are(&[Identifier, Identifier, ColonEquals]) => {
-            items.push(Item::Alias(self.parse_alias()?));
+            items.push(Item::Alias(self.parse_alias(BTreeSet::new())?));
           }
           Some(Keyword::Export) if self.next_are(&[Identifier, Identifier, ColonEquals]) => {
             self.presume_keyword(Keyword::Export)?;
@@ -361,9 +361,17 @@ impl<'tokens, 'src> Parser<'tokens, 'src> {
           BTreeSet::new(),
         )?));
       } else if let Some(attributes) = self.parse_attributes()? {
-        let quiet = self.accepted(At)?;
-        let doc = pop_doc_comment(&mut items, eol_since_last_comment);
-        items.push(Item::Recipe(self.parse_recipe(doc, quiet, attributes)?));
+        let next_keyword = Keyword::from_lexeme(self.next()?.lexeme());
+        match next_keyword {
+          Some(Keyword::Alias) if self.next_are(&[Identifier, Identifier, ColonEquals]) => {
+            items.push(Item::Alias(self.parse_alias(attributes)?));
+          }
+          _ => {
+            let quiet = self.accepted(At)?;
+            let doc = pop_doc_comment(&mut items, eol_since_last_comment);
+            items.push(Item::Recipe(self.parse_recipe(doc, quiet, attributes)?));
+          }
+        }
       } else {
         return Err(self.unexpected_token()?);
       }
@@ -383,13 +391,20 @@ impl<'tokens, 'src> Parser<'tokens, 'src> {
   }
 
   /// Parse an alias, e.g `alias name := target`
-  fn parse_alias(&mut self) -> CompileResult<'src, Alias<'src, Name<'src>>> {
+  fn parse_alias(
+    &mut self,
+    attributes: BTreeSet<Attribute>,
+  ) -> CompileResult<'src, Alias<'src, Name<'src>>> {
     self.presume_keyword(Keyword::Alias)?;
     let name = self.parse_name()?;
     self.presume_any(&[Equals, ColonEquals])?;
     let target = self.parse_name()?;
     self.expect_eol()?;
-    Ok(Alias { name, target })
+    Ok(Alias {
+      attributes,
+      name,
+      target,
+    })
   }
 
   /// Parse an assignment, e.g. `foo := bar`
@@ -978,6 +993,12 @@ mod tests {
     tree: (justfile (alias t test)),
   }
 
+  test! {
+    name: alias_with_attributee,
+    text: "[private]\nalias t := test",
+    tree: (justfile (alias t test)),
+  }
+
   test! {
     name: aliases_multiple,
     text: "alias t := test\nalias b := build",
@@ -996,6 +1017,18 @@ mod tests {
     ),
   }
 
+  test! {
+      name: recipe_named_alias,
+      text: r#"
+      [private]
+      alias:
+        echo 'echoing alias'
+          "#,
+    tree: (justfile
+      (recipe alias (body ("echo 'echoing alias'")))
+    ),
+  }
+
   test! {
     name: export,
     text: r#"export x := "hello""#,


Test patch:
diff --git a/tests/error_messages.rs b/tests/error_messages.rs
index 54b93d03..d5c2300c 100644
--- a/tests/error_messages.rs
+++ b/tests/error_messages.rs
@@ -1,5 +1,17 @@
 use super::*;
 
+test! {
+    name: invalid_alias_attribute,
+    justfile: "[private]\n[linux]\nalias t := test\n\ntest:\n",
+    stderr: "
+      error: Alias t has an invalid attribute `linux`
+        |
+      3 | alias t := test
+        |       ^
+    ",
+    status: EXIT_FAILURE,
+}
+
 test! {
   name: expected_keyword,
   justfile: "foo := if '' == '' { '' } arlo { '' }",
diff --git a/tests/json.rs b/tests/json.rs
index 6f418cba..2927fb34 100644
--- a/tests/json.rs
+++ b/tests/json.rs
@@ -22,6 +22,7 @@ fn alias() {
         "f": {
           "name": "f",
           "target": "foo",
+          "attributes": [],
         }
       },
       "assignments": {},
@@ -299,6 +300,7 @@ fn duplicate_recipes() {
       "first": "foo",
       "aliases": {
         "f": {
+          "attributes": [],
           "name": "f",
           "target": "foo",
         }
diff --git a/tests/private.rs b/tests/private.rs
index 82e3d4f6..13dcacf5 100644
--- a/tests/private.rs
+++ b/tests/private.rs
@@ -1,7 +1,7 @@
 use super::*;
 
 #[test]
-fn attribute() {
+fn private_attribute_for_recipe() {
   Test::new()
     .justfile(
       "
@@ -17,3 +17,24 @@ fn attribute() {
     )
     .run();
 }
+
+#[test]
+fn private_attribute_for_alias() {
+  Test::new()
+    .justfile(
+      "
+      [private]
+      alias f := foo
+
+      foo:
+      ",
+    )
+    .args(&["--list"])
+    .stdout(
+      "
+      Available recipes:
+          foo
+      ",
+    )
+    .run();
+}