From e11684008e758424d9fbef2eae35c1c0be4962d7 Mon Sep 17 00:00:00 2001 From: Markus Date: Wed, 15 May 2024 03:39:42 +0200 Subject: [PATCH] Add 'allow-duplicate-variables' setting (#1922) --- GRAMMAR.md | 1 + README.md | 25 +++++++++++- src/analyzer.rs | 32 +++++++++------ src/binding.rs | 2 + src/keyword.rs | 1 + src/node.rs | 1 + src/parser.rs | 10 +++++ src/scope.rs | 1 + src/setting.rs | 2 + src/settings.rs | 4 ++ tests/allow_duplicate_variables.rs | 21 ++++++++++ tests/imports.rs | 32 +++++++++++++-- tests/json.rs | 62 ++++++++++++++++++++++++++++++ tests/lib.rs | 1 + tests/modules.rs | 20 +++++++--- 15 files changed, 195 insertions(+), 20 deletions(-) create mode 100644 tests/allow_duplicate_variables.rs diff --git a/GRAMMAR.md b/GRAMMAR.md index 5c23cc2..181f723 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 29a7550..7df04bd 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 19f36e1..7a144cf 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 73b64c2..6a8c0c7 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 830207e..225f267 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 1d98b99..33e6a0a 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 ba006ac..749e5bb 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 428b677..f13279c 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 0256ff9..635850e 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 f72cb54..dbabe4c 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, pub(crate) dotenv_load: Option, pub(crate) dotenv_path: Option, @@ -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); } diff --git a/tests/allow_duplicate_variables.rs b/tests/allow_duplicate_variables.rs new file mode 100644 index 0000000..56ea0a4 --- /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 fef87a1..0d32fad 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 4c71b43..92839a5 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 bcd1b1a..fa0df58 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 75aaf7f..4c10b41 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: │ ^^^ ", )