Repository: casey/just
Pull request #2483 (2024-11-27 22:26:03)

Forbid duplicated non-repeatable attributes
Forbid duplicated non-repeatable attributes in the parser. Duplicate attributes with different values were previously accepted by the compiler, often to ill effect, like multiple `confirm` attributes being allowed on a recipe, but only one being respected.

This fixes that by disallowing all duplicate non-repeatable attributes, regardless of whether they have different arguments or not. The only repeatable attribute is the `[group]` attribute, since recipes can be in multiple groups.

This is a backwards compatibility break, but it is most definitely a bug, and backwards compatibility breaking bug fixes are allowed.


Patch:
diff --git a/src/attribute.rs b/src/attribute.rs
index ebdc2127..e6e9f52a 100644
--- a/src/attribute.rs
+++ b/src/attribute.rs
@@ -6,7 +6,7 @@ use super::*;
 #[strum(serialize_all = "kebab-case")]
 #[serde(rename_all = "kebab-case")]
 #[strum_discriminants(name(AttributeDiscriminant))]
-#[strum_discriminants(derive(EnumString))]
+#[strum_discriminants(derive(EnumString, Ord, PartialOrd))]
 #[strum_discriminants(strum(serialize_all = "kebab-case"))]
 pub(crate) enum Attribute<'src> {
   Confirm(Option<StringLiteral<'src>>),
@@ -96,9 +96,17 @@ impl<'src> Attribute<'src> {
     })
   }
 
+  pub(crate) fn discriminant(&self) -> AttributeDiscriminant {
+    self.into()
+  }
+
   pub(crate) fn name(&self) -> &'static str {
     self.into()
   }
+
+  pub(crate) fn repeatable(&self) -> bool {
+    matches!(self, Attribute::Group(_))
+  }
 }
 
 impl<'src> Display for Attribute<'src> {
diff --git a/src/parser.rs b/src/parser.rs
index 229f5aea..b5f2ed5f 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -1126,6 +1126,7 @@ impl<'run, 'src> Parser<'run, 'src> {
     &mut self,
   ) -> CompileResult<'src, Option<(Token<'src>, BTreeSet<Attribute<'src>>)>> {
     let mut attributes = BTreeMap::new();
+    let mut discriminants = BTreeMap::new();
 
     let mut token = None;
 
@@ -1152,13 +1153,23 @@ impl<'run, 'src> Parser<'run, 'src> {
 
         let attribute = Attribute::new(name, arguments)?;
 
-        if let Some(line) = attributes.get(&attribute) {
+        let first = attributes.get(&attribute).or_else(|| {
+          if attribute.repeatable() {
+            None
+          } else {
+            discriminants.get(&attribute.discriminant())
+          }
+        });
+
+        if let Some(&first) = first {
           return Err(name.error(CompileErrorKind::DuplicateAttribute {
             attribute: name.lexeme(),
-            first: *line,
+            first,
           }));
         }
 
+        discriminants.insert(attribute.discriminant(), name.line);
+
         attributes.insert(attribute, name.line);
 
         if !self.accepted(Comma)? {


Test patch:
diff --git a/tests/attributes.rs b/tests/attributes.rs
index 65c4d5ad..9f022025 100644
--- a/tests/attributes.rs
+++ b/tests/attributes.rs
@@ -230,3 +230,26 @@ fn extension_on_linewise_error() {
     .status(EXIT_FAILURE)
     .run();
 }
+
+#[test]
+fn duplicate_non_repeatable_attributes_are_forbidden() {
+  Test::new()
+    .justfile(
+      "
+        [confirm: 'yes']
+        [confirm: 'no']
+        baz:
+      ",
+    )
+    .stderr(
+      "
+  error: Recipe attribute `confirm` first used on line 1 is duplicated on line 2
+   ——▶ justfile:2:2
+    │
+  2 │ [confirm: 'no']
+    │  ^^^^^^^
+",
+    )
+    .status(EXIT_FAILURE)
+    .run();
+}