Repository: casey/just
Pull request #1922 (2024-02-21 20:03:25)

Implemented 'set allow-duplicate-variables'
I ran into the same issue as https://github.com/casey/just/issues/1827 and decided to make a quick draft for it.
It's my first rodeo with rust (and I am not a big fan so far, but it may grow onto me), so I appreciate feedback.

This issue allows us to overwrite assignments, even when they are imported from other justfiles. Previously it would have thrown an error. We now can use `set allow-duplicate-variables` to enable this behavior (in the same style as the existing `set allow-duplicate-recipes`).


# Example: Default behavior
```
a := "foo"
a := "bar" 
#  Example: Raises an exception
```

# Example: Using the flag to overwrite a variable
```
set allow-duplicate-variables

a := "foo"
a := "bar"
# a is now "bar"
```

# Example: Using the flag in an import
```
# imp.just
a := "foo"
```


```
# some.just
import "./imp.just"
set allow-duplicate-variables

a := "bar"
# a is now "bar"
```

## Feedback appreciated
I am not quite happy with the detour I had to take with `DepthAwareBinding`. I introduced it because the assignments need to be aware of their depth, otherwise imported modules would overwrite assignments that appear later in the justfile. As I did not use rust before, I do not know the best practices for this in this language and just wanted to get the draft done (for now). 


### Some other thing
I ran into `just` just (hehe) a few months ago - and fell in love. Thanks for this tool!

Mijago: @casey Hi! I do not want to come off rude or impatient - but is there anything I should change in order to progress this PR further?
casey: Sorry for letting this sit! This looks great. I made a couple changes:

- I removed `DepthAwareBinding` and just added a depth, which is set to 0 when not needed, to `Binding`.
- I removed the `modules_use_module_settings_variables` test. Since there is no code specific to different settings which implements this, I don't think a new test is needed for each setting.

Patch:
diff --git a/GRAMMAR.md b/GRAMMAR.md
index 5c23cc2f..181f723b 100644
--- a/GRAMMAR.md
+++ b/GRAMMAR.md
@@ -62,6 +62,7 @@ assignment    : NAME ':=' expression eol
 export        : 'export' assignment
 
 setting       : 'set' 'allow-duplicate-recipes' boolean?
+              | 'set' 'allow-duplicate-variables' boolean?
               | 'set' 'dotenv-filename' ':=' string
               | 'set' 'dotenv-load' boolean?
               | 'set' 'dotenv-path' ':=' string
diff --git a/README.md b/README.md
index 29a75503..7df04bd1 100644
--- a/README.md
+++ b/README.md
@@ -792,6 +792,7 @@ foo:
 | Name | Value | Default | Description |
 |------|-------|---------|-------------|
 | `allow-duplicate-recipes` | boolean | `false` | Allow recipes appearing later in a `justfile` to override earlier recipes with the same name. |
+| `allow-duplicate-variables` | boolean | `false` | Allow variables appearing later in a `justfile` to override earlier variables with the same name. |
 | `dotenv-filename` | string | - | Load a `.env` file with a custom name, if present. |
 | `dotenv-load` | boolean | `false` | Load a `.env` file, if present. |
 | `dotenv-path` | string | - | Load a `.env` file from a custom path, if present. Overrides `dotenv-filename`. |
@@ -837,6 +838,27 @@ $ just foo
 bar
 ```
 
+#### Allow Duplicate Variables
+
+If `allow-duplicate-variables` is set to `true`, defining multiple variables
+with the same name is not an error and the last definition is used. Defaults to
+`false`.
+
+```just
+set allow-duplicate-variables
+
+a := "foo"
+a := "bar"
+
+@foo:
+  echo $a
+```
+
+```sh
+$ just foo
+bar
+```
+
 #### Dotenv Settings
 
 If `dotenv-load`, `dotenv-filename` or `dotenv-path` is set, `just` will load
@@ -2738,7 +2760,8 @@ Imported files can themselves contain `import`s, which are processed
 recursively.
 
 When `allow-duplicate-recipes` is set, recipes in parent modules override
-recipes in imports.
+recipes in imports. In a similar manner, when `allow-duplicate-variables` is
+set, variables in parent modules override variables in imports.
 
 Imports may be made optional by putting a `?` after the `import` keyword:
 
diff --git a/src/analyzer.rs b/src/analyzer.rs
index 19f36e1d..7a144cfc 100644
--- a/src/analyzer.rs
+++ b/src/analyzer.rs
@@ -26,6 +26,8 @@ impl<'src> Analyzer<'src> {
   ) -> CompileResult<'src, Justfile<'src>> {
     let mut recipes = Vec::new();
 
+    let mut assignments = Vec::new();
+
     let mut stack = Vec::new();
     stack.push(asts.get(root).unwrap());
 
@@ -70,8 +72,7 @@ impl<'src> Analyzer<'src> {
             self.aliases.insert(alias.clone());
           }
           Item::Assignment(assignment) => {
-            self.analyze_assignment(assignment)?;
-            self.assignments.insert(assignment.clone());
+            assignments.push(assignment);
           }
           Item::Comment(_) => (),
           Item::Import { absolute, .. } => {
@@ -108,6 +109,24 @@ impl<'src> Analyzer<'src> {
 
     let mut recipe_table: Table<'src, UnresolvedRecipe<'src>> = Table::default();
 
+    for assignment in assignments {
+      if !settings.allow_duplicate_variables
+        && self.assignments.contains_key(assignment.name.lexeme())
+      {
+        return Err(assignment.name.token.error(DuplicateVariable {
+          variable: assignment.name.lexeme(),
+        }));
+      }
+
+      if self
+        .assignments
+        .get(assignment.name.lexeme())
+        .map_or(true, |original| assignment.depth <= original.depth)
+      {
+        self.assignments.insert(assignment.clone());
+      }
+    }
+
     AssignmentResolver::resolve_assignments(&self.assignments)?;
 
     for recipe in recipes {
@@ -199,15 +218,6 @@ impl<'src> Analyzer<'src> {
     Ok(())
   }
 
-  fn analyze_assignment(&self, assignment: &Assignment<'src>) -> CompileResult<'src> {
-    if self.assignments.contains_key(assignment.name.lexeme()) {
-      return Err(assignment.name.token.error(DuplicateVariable {
-        variable: assignment.name.lexeme(),
-      }));
-    }
-    Ok(())
-  }
-
   fn analyze_alias(alias: &Alias<'src, Name<'src>>) -> CompileResult<'src> {
     let name = alias.name.lexeme();
 
diff --git a/src/binding.rs b/src/binding.rs
index 73b64c21..6a8c0c70 100644
--- a/src/binding.rs
+++ b/src/binding.rs
@@ -3,6 +3,8 @@ use super::*;
 /// A binding of `name` to `value`
 #[derive(Debug, Clone, PartialEq, Serialize)]
 pub(crate) struct Binding<'src, V = String> {
+  /// Module depth where binding appears
+  pub(crate) depth: u32,
   /// Export binding as an environment variable to child processes
   pub(crate) export: bool,
   /// Binding name
diff --git a/src/keyword.rs b/src/keyword.rs
index 830207e5..225f267e 100644
--- a/src/keyword.rs
+++ b/src/keyword.rs
@@ -5,6 +5,7 @@ use super::*;
 pub(crate) enum Keyword {
   Alias,
   AllowDuplicateRecipes,
+  AllowDuplicateVariables,
   DotenvFilename,
   DotenvLoad,
   DotenvPath,
diff --git a/src/node.rs b/src/node.rs
index 1d98b991..33e6a0a8 100644
--- a/src/node.rs
+++ b/src/node.rs
@@ -265,6 +265,7 @@ impl<'src> Node<'src> for Set<'src> {
 
     match &self.value {
       Setting::AllowDuplicateRecipes(value)
+      | Setting::AllowDuplicateVariables(value)
       | Setting::DotenvLoad(value)
       | Setting::Export(value)
       | Setting::Fallback(value)
diff --git a/src/parser.rs b/src/parser.rs
index ba006ac5..749e5bb6 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -456,6 +456,7 @@ impl<'run, 'src> Parser<'run, 'src> {
     let value = self.parse_expression()?;
     self.expect_eol()?;
     Ok(Assignment {
+      depth: self.submodule_depth,
       export,
       name,
       value,
@@ -847,6 +848,9 @@ impl<'run, 'src> Parser<'run, 'src> {
       Keyword::AllowDuplicateRecipes => {
         Some(Setting::AllowDuplicateRecipes(self.parse_set_bool()?))
       }
+      Keyword::AllowDuplicateVariables => {
+        Some(Setting::AllowDuplicateVariables(self.parse_set_bool()?))
+      }
       Keyword::DotenvLoad => Some(Setting::DotenvLoad(self.parse_set_bool()?)),
       Keyword::Export => Some(Setting::Export(self.parse_set_bool()?)),
       Keyword::Fallback => Some(Setting::Fallback(self.parse_set_bool()?)),
@@ -1913,6 +1917,12 @@ mod tests {
     tree: (justfile (set allow_duplicate_recipes true)),
   }
 
+  test! {
+    name: set_allow_duplicate_variables_implicit,
+    text: "set allow-duplicate-variables",
+    tree: (justfile (set allow_duplicate_variables true)),
+  }
+
   test! {
     name: set_dotenv_load_true,
     text: "set dotenv-load := true",
diff --git a/src/scope.rs b/src/scope.rs
index 428b6770..f13279c3 100644
--- a/src/scope.rs
+++ b/src/scope.rs
@@ -23,6 +23,7 @@ impl<'src, 'run> Scope<'src, 'run> {
 
   pub(crate) fn bind(&mut self, export: bool, name: Name<'src>, value: String) {
     self.bindings.insert(Binding {
+      depth: 0,
       export,
       name,
       value,
diff --git a/src/setting.rs b/src/setting.rs
index 0256ff94..635850e8 100644
--- a/src/setting.rs
+++ b/src/setting.rs
@@ -3,6 +3,7 @@ use super::*;
 #[derive(Debug, Clone)]
 pub(crate) enum Setting<'src> {
   AllowDuplicateRecipes(bool),
+  AllowDuplicateVariables(bool),
   DotenvFilename(String),
   DotenvLoad(bool),
   DotenvPath(String),
@@ -21,6 +22,7 @@ impl<'src> Display for Setting<'src> {
   fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> {
     match self {
       Setting::AllowDuplicateRecipes(value)
+      | Setting::AllowDuplicateVariables(value)
       | Setting::DotenvLoad(value)
       | Setting::Export(value)
       | Setting::Fallback(value)
diff --git a/src/settings.rs b/src/settings.rs
index f72cb546..dbabe4c2 100644
--- a/src/settings.rs
+++ b/src/settings.rs
@@ -8,6 +8,7 @@ pub(crate) const WINDOWS_POWERSHELL_ARGS: &[&str] = &["-NoLogo", "-Command"];
 #[derive(Debug, PartialEq, Serialize, Default)]
 pub(crate) struct Settings<'src> {
   pub(crate) allow_duplicate_recipes: bool,
+  pub(crate) allow_duplicate_variables: bool,
   pub(crate) dotenv_filename: Option<String>,
   pub(crate) dotenv_load: Option<bool>,
   pub(crate) dotenv_path: Option<PathBuf>,
@@ -31,6 +32,9 @@ impl<'src> Settings<'src> {
         Setting::AllowDuplicateRecipes(allow_duplicate_recipes) => {
           settings.allow_duplicate_recipes = allow_duplicate_recipes;
         }
+        Setting::AllowDuplicateVariables(allow_duplicate_variables) => {
+          settings.allow_duplicate_variables = allow_duplicate_variables;
+        }
         Setting::DotenvFilename(filename) => {
           settings.dotenv_filename = Some(filename);
         }


Test patch:
diff --git a/tests/allow_duplicate_variables.rs b/tests/allow_duplicate_variables.rs
new file mode 100644
index 00000000..56ea0a4f
--- /dev/null
+++ b/tests/allow_duplicate_variables.rs
@@ -0,0 +1,21 @@
+use super::*;
+
+#[test]
+fn allow_duplicate_variables() {
+  Test::new()
+    .justfile(
+      "
+      a := 'foo'
+      a := 'bar'
+
+      set allow-duplicate-variables
+
+      b:
+        echo {{a}}
+      ",
+    )
+    .arg("b")
+    .stdout("bar\n")
+    .stderr("echo bar\n")
+    .run();
+}
diff --git a/tests/imports.rs b/tests/imports.rs
index fef87a12..0d32fadc 100644
--- a/tests/imports.rs
+++ b/tests/imports.rs
@@ -182,12 +182,12 @@ fn recipes_in_import_are_overridden_by_recipes_in_parent() {
     })
     .justfile(
       "
+        a:
+          @echo ROOT
+
         import './import.justfile'
 
         set allow-duplicate-recipes
-
-        a:
-          @echo ROOT
       ",
     )
     .test_round_trip(false)
@@ -196,6 +196,32 @@ fn recipes_in_import_are_overridden_by_recipes_in_parent() {
     .run();
 }
 
+#[test]
+fn variables_in_import_are_overridden_by_variables_in_parent() {
+  Test::new()
+    .tree(tree! {
+      "import.justfile": "
+    f := 'foo'
+    ",
+    })
+    .justfile(
+      "
+      f := 'bar'
+
+      import './import.justfile'
+
+      set allow-duplicate-variables
+
+      a:
+        @echo {{f}}
+    ",
+    )
+    .test_round_trip(false)
+    .arg("a")
+    .stdout("bar\n")
+    .run();
+}
+
 #[cfg(not(windows))]
 #[test]
 fn import_paths_beginning_with_tilde_are_expanded_to_homdir() {
diff --git a/tests/json.rs b/tests/json.rs
index 4c71b430..92839a52 100644
--- a/tests/json.rs
+++ b/tests/json.rs
@@ -44,6 +44,7 @@ fn alias() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -73,6 +74,7 @@ fn assignment() {
           "export": false,
           "name": "foo",
           "value": "bar",
+          "depth": 0,
         }
       },
       "first": null,
@@ -80,6 +82,7 @@ fn assignment() {
       "recipes": {},
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -131,6 +134,7 @@ fn body() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -194,6 +198,7 @@ fn dependencies() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -240,6 +245,7 @@ fn dependency_argument() {
           "export": false,
           "name": "x",
           "value": "foo",
+          "depth": 0,
         },
       },
       "modules": {},
@@ -294,6 +300,7 @@ fn dependency_argument() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -357,6 +364,49 @@ fn duplicate_recipes() {
       },
       "settings": {
         "allow_duplicate_recipes": true,
+        "allow_duplicate_variables": false,
+        "dotenv_filename": null,
+        "dotenv_load": null,
+        "dotenv_path": null,
+        "export": false,
+        "fallback": false,
+        "ignore_comments": false,
+        "positional_arguments": false,
+        "quiet": false,
+        "shell": null,
+        "tempdir" : null,
+        "windows_powershell": false,
+        "windows_shell": null,
+      },
+      "warnings": [],
+    }),
+  );
+}
+
+#[test]
+fn duplicate_variables() {
+  case(
+    "
+      set allow-duplicate-variables
+      x := 'foo'
+      x := 'bar'
+    ",
+    json!({
+      "aliases": {},
+      "assignments": {
+        "x": {
+          "export": false,
+          "name": "x",
+          "value": "bar",
+          "depth": 0,
+        }
+      },
+      "first": null,
+      "modules": {},
+      "recipes": {},
+      "settings": {
+        "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": true,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -401,6 +451,7 @@ fn doc_comment() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -431,6 +482,7 @@ fn empty_justfile() {
       "recipes": {},
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -582,6 +634,7 @@ fn parameters() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -666,6 +719,7 @@ fn priors() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -710,6 +764,7 @@ fn private() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -754,6 +809,7 @@ fn quiet() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -810,6 +866,7 @@ fn settings() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": "filename",
         "dotenv_load": true,
         "dotenv_path": "path",
@@ -860,6 +917,7 @@ fn shebang() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -904,6 +962,7 @@ fn simple() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -951,6 +1010,7 @@ fn attribute() {
       },
       "settings": {
         "allow_duplicate_recipes": false,
+        "allow_duplicate_variables": false,
         "dotenv_filename": null,
         "dotenv_load": null,
         "dotenv_path": null,
@@ -1011,6 +1071,7 @@ fn module() {
             },
             "settings": {
               "allow_duplicate_recipes": false,
+              "allow_duplicate_variables": false,
               "dotenv_filename": null,
               "dotenv_load": null,
               "dotenv_path": null,
@@ -1030,6 +1091,7 @@ fn module() {
         "recipes": {},
         "settings": {
           "allow_duplicate_recipes": false,
+          "allow_duplicate_variables": false,
           "dotenv_filename": null,
           "dotenv_load": null,
           "dotenv_path": null,
diff --git a/tests/lib.rs b/tests/lib.rs
index bcd1b1a6..fa0df588 100644
--- a/tests/lib.rs
+++ b/tests/lib.rs
@@ -33,6 +33,7 @@ pub(crate) use {
 mod test;
 
 mod allow_duplicate_recipes;
+mod allow_duplicate_variables;
 mod assert_stdout;
 mod assert_success;
 mod attributes;
diff --git a/tests/modules.rs b/tests/modules.rs
index 75aaf7fc..4c10b413 100644
--- a/tests/modules.rs
+++ b/tests/modules.rs
@@ -200,7 +200,11 @@ fn modules_use_module_settings() {
   Test::new()
     .write(
       "foo.just",
-      "set allow-duplicate-recipes\nfoo:\nfoo:\n @echo FOO\n",
+      "set allow-duplicate-recipes
+foo:
+foo:
+  @echo FOO
+",
     )
     .justfile(
       "
@@ -215,7 +219,13 @@ fn modules_use_module_settings() {
     .run();
 
   Test::new()
-    .write("foo.just", "\nfoo:\nfoo:\n @echo FOO\n")
+    .write(
+      "foo.just",
+      "foo:
+foo:
+  @echo FOO
+",
+    )
     .justfile(
       "
         mod foo
@@ -230,10 +240,10 @@ fn modules_use_module_settings() {
     .arg("foo")
     .stderr(
       "
-      error: Recipe `foo` first defined on line 2 is redefined on line 3
-       ——▶ foo.just:3:1
+      error: Recipe `foo` first defined on line 1 is redefined on line 2
+       ——▶ foo.just:2:1
         │
-      3 │ foo:
+      2 │ foo:
         │ ^^^
     ",
     )