Repository: casey/just
Pull request #1095 (2022-02-10 02:05:53)

Allow duplicate recipes
Hey there @casey first off just wanted to thank you for an awesome tool. I personally love using it in all my projects.

This is meant to address the allowing duplicate recipes via a setting. This should help partially resolve [the conversation in how to override recipes](https://github.com/casey/just/issues/383#issuecomment-882002853) when using `justprep`.

I know this conversation was a while ago, and your reasoning may have changed (I really want modules or recipes that can have prereqs from other files), but `justprep` is the closest we can get to having modular "dependencies" at the moment, and this is sort of the last piece that would help our workflows.

Why I chose to use a setting rather than a cli flag: if there are duplicate recipes in the justfiles, they will not parse, rather than pass around the config flags everywhere it seemed logical to have that be a setting in the file (that if used in an older version will properly barf not knowing about the `allow-duplicates` setting.

Some tradeoffs made: settings are parsed in order and duplicates are only allowed after the setting is used. I know this breaks the nice unordered property of justfiles generally, but the compromise seemed fair while I was there.

It looks like this now:

```
set allow-duplicates

# default that might come from `justprep`
foo:
  @echo "basic recipe"

foo:
  @echo "more specific overridden recipe"
```

```shell
$ just
more specific overridden recipe
```

and without the `set allow-duplicates` line will error as normal:
```
error: Recipe `foo` first defined on line 3 is redefined on line 6
  |
6 | @foo:
  |  ^^^
```


Open to any feedback (both on approach and code) -- or if you want to go a different direction entirely at this point it would be understandable too.

I can also write up the docs as needed, but didn't want to go overboard until got your general approval to refine more

casey: Thanks for the PR and the thoughtful comments! 

I'm not opposed to a setting. A few comments:

- I think it should be called `allow-duplicate-recipes`, for clarity. `allow-duplicates` makes me think that maybe it also allows duplicate variables. (Which might be a desirable thing to add later, since `justprep` might want to also allow overriding variables.)

- The setting should be able to appear anywhere in the `justfile` and have the same effect. Otherwise, the docs have to say "All the settings are order independent, except for `allow-duplicate-recipes`…". I haven't thought about it in depth, but `Analyzer::recipes` can probably be turned into a `Table<'src, Vec<UnresolvedRecipe<'src>>>`, and the check for duplicates can be deferred until after all the recipes are analyzed, and depend on whether `allow-duplicate-recipes` was set or not.

- There should be an integration test that actually runs a recipe that was overridden.
lutostag: Thanks for the quick response and feedback!

Went through and made the suggested changes.

Was able to implement the duplicate overriding from within the analyzer relatively cleanly (after reading the settings).

Not sure I added the integration test to the right spot, but put one into `tests/misc.rs`

Also added quick documentation to the Readme (but unsure if I missed other spots).

Any feedback/suggestions are welcome (even small nits). Thanks!
lutostag: I think I addressed your comments. As an extra thought however just noting that `allow-duplicate-recipes' I believe would be the first justfile codification of the word "recipe" to it. It lives throughout the documentation/code and in the unstable json dump area but would not be easy to change in a backwards compatible manner. Looking at https://github.com/casey/just/issues/166 particularly. It might actually make sense if we only enable `allow-duplicate-recipes` setting if we enable the `unstable` flag in the CLI.

I know that would be more work (and go back to pulling in config options into the analyzer which I tried to avoid... but would keep it behind the feature flag). Also because the only other code effected would be in the json dump code that is also unstable, we wouldn't need to pass the settings around everywhere. (Perhaps the tricky part would be documenting it clearly in the README).

@casey I don't mean to delay a feature I asked for I guess only want to make sure we bolt on new features wisely -- especially with 1.0 around the corner.

I'm fine either merging or doing the work to only allow this when `--unstable` is passed up to you entirely.
casey: > I think I addressed your comments. As an extra thought however just noting that `allow-duplicate-recipes` I believe would be the first justfile codification of the word "recipe" to it. It lives throughout the documentation/code and in the unstable json dump area but would not be easy to change in a backwards compatible manner. Looking at #166 particularly.

Good thought! I'm actually not too worried. It wouldn't be hard to just soft-deprecate the current setting name, at the same time as we introduce an alias. I'm also on the fence about #166 anyways.

Regarding 1.0, I actually think that anything that we want to change in the future can be introduced in a backwards compatible way with settings. And, if eventually there are a bunch of settings that we want to make the default, we can do so with something like Rust's edition system, where you would put something like `set version := 2` at the top of the justfile, which would opt you into a new set of defaults.

So I'm fine including this as is and worrying about it later if we want to change it.
casey: There was a clippy error, but it was simple so I went ahead and fixed it. Looks good!

Patch:
diff --git a/README.md b/README.md
index 0a59cf68..e9c7ecd9 100644
--- a/README.md
+++ b/README.md
@@ -521,13 +521,14 @@ foo:
 
 #### Table of Settings
 
-| Name                   | Value              | Description                                                    |
-| ---------------------- | ------------------ | -------------------------------------------------------------- |
-| `dotenv-load`          | boolean            | Load a `.env` file, if present.                                |
-| `export`               | boolean            | Export all variables as environment variables.                 |
-| `positional-arguments` | boolean            | Pass positional arguments.                                     |
-| `shell`                | `[COMMAND, ARGS…]` | Set the command used to invoke recipes and evaluate backticks. |
-| `windows-powershell`   | boolean            | Use PowerShell on Windows as default shell.                    |
+| Name                      | Value              | Description                                                                                   |
+| ------------------------- | ------------------ | --------------------------------------------------------------------------------------------- |
+| `allow-duplicate-recipes` | boolean            | Allow recipes appearing later in a `justfile` to override earlier recipes with the same name. |
+| `dotenv-load`             | boolean            | Load a `.env` file, if present.                                                               |
+| `export`                  | boolean            | Export all variables as environment variables.                                                |
+| `positional-arguments`    | boolean            | Pass positional arguments.                                                                    |
+| `shell`                   | `[COMMAND, ARGS…]` | Set the command used to invoke recipes and evaluate backticks.                                |
+| `windows-powershell`      | boolean            | Use PowerShell on Windows as default shell.                                                   |
 
 Boolean settings can be written as:
 
@@ -541,6 +542,25 @@ Which is equivalent to:
 set NAME := true
 ```
 
+#### Allow Duplicate Recipes
+
+If `allow-duplicate-recipes` is set to `true`, defining multiple recipes with the same name is not an error and the last definition is used. Defaults to `false`.
+
+```make
+set allow-duplicate-recipes
+
+@foo:
+  echo foo
+
+@foo:
+  echo bar
+```
+
+```sh
+$ just foo
+bar
+```
+
 #### Dotenv Load
 
 If `dotenv-load` is `true`, a `.env` file will be loaded if present. Defaults to `true`.
diff --git a/src/analyzer.rs b/src/analyzer.rs
index c8bfe3df..8ded49b7 100644
--- a/src/analyzer.rs
+++ b/src/analyzer.rs
@@ -16,6 +16,8 @@ impl<'src> Analyzer<'src> {
   }
 
   pub(crate) fn justfile(mut self, ast: Ast<'src>) -> CompileResult<'src, Justfile<'src>> {
+    let mut recipes = Vec::new();
+
     for item in ast.items {
       match item {
         Item::Alias(alias) => {
@@ -28,8 +30,8 @@ impl<'src> Analyzer<'src> {
         }
         Item::Comment(_) => (),
         Item::Recipe(recipe) => {
-          self.analyze_recipe(&recipe)?;
-          self.recipes.insert(recipe);
+          Self::analyze_recipe(&recipe)?;
+          recipes.push(recipe);
         }
         Item::Set(set) => {
           self.analyze_set(&set)?;
@@ -38,31 +40,13 @@ impl<'src> Analyzer<'src> {
       }
     }
 
-    let assignments = self.assignments;
-
-    AssignmentResolver::resolve_assignments(&assignments)?;
-
-    let recipes = RecipeResolver::resolve_recipes(self.recipes, &assignments)?;
-
-    for recipe in recipes.values() {
-      for parameter in &recipe.parameters {
-        if assignments.contains_key(parameter.name.lexeme()) {
-          return Err(parameter.name.token().error(ParameterShadowsVariable {
-            parameter: parameter.name.lexeme(),
-          }));
-        }
-      }
-    }
-
-    let mut aliases = Table::new();
-    while let Some(alias) = self.aliases.pop() {
-      aliases.insert(Self::resolve_alias(&recipes, alias)?);
-    }
-
     let mut settings = Settings::new();
 
     for (_, set) in self.sets {
       match set.value {
+        Setting::AllowDuplicateRecipes(allow_duplicate_recipes) => {
+          settings.allow_duplicate_recipes = allow_duplicate_recipes;
+        }
         Setting::DotenvLoad(dotenv_load) => {
           settings.dotenv_load = Some(dotenv_load);
         }
@@ -82,6 +66,39 @@ impl<'src> Analyzer<'src> {
       }
     }
 
+    let assignments = self.assignments;
+
+    AssignmentResolver::resolve_assignments(&assignments)?;
+
+    for recipe in recipes {
+      if let Some(original) = self.recipes.get(recipe.name.lexeme()) {
+        if !settings.allow_duplicate_recipes {
+          return Err(recipe.name.token().error(DuplicateRecipe {
+            recipe: original.name(),
+            first: original.line_number(),
+          }));
+        }
+      }
+      self.recipes.insert(recipe);
+    }
+
+    let recipes = RecipeResolver::resolve_recipes(self.recipes, &assignments)?;
+
+    for recipe in recipes.values() {
+      for parameter in &recipe.parameters {
+        if assignments.contains_key(parameter.name.lexeme()) {
+          return Err(parameter.name.token().error(ParameterShadowsVariable {
+            parameter: parameter.name.lexeme(),
+          }));
+        }
+      }
+    }
+
+    let mut aliases = Table::new();
+    while let Some(alias) = self.aliases.pop() {
+      aliases.insert(Self::resolve_alias(&recipes, alias)?);
+    }
+
     Ok(Justfile {
       warnings: ast.warnings,
       first: recipes
@@ -101,14 +118,7 @@ impl<'src> Analyzer<'src> {
     })
   }
 
-  fn analyze_recipe(&self, recipe: &UnresolvedRecipe<'src>) -> CompileResult<'src, ()> {
-    if let Some(original) = self.recipes.get(recipe.name.lexeme()) {
-      return Err(recipe.name.token().error(DuplicateRecipe {
-        recipe: original.name(),
-        first: original.line_number(),
-      }));
-    }
-
+  fn analyze_recipe(recipe: &UnresolvedRecipe<'src>) -> CompileResult<'src, ()> {
     let mut parameters = BTreeSet::new();
     let mut passed_default = false;
 
diff --git a/src/keyword.rs b/src/keyword.rs
index 2ed06d8b..0f3f6fc7 100644
--- a/src/keyword.rs
+++ b/src/keyword.rs
@@ -4,6 +4,7 @@ use crate::common::*;
 #[strum(serialize_all = "kebab_case")]
 pub(crate) enum Keyword {
   Alias,
+  AllowDuplicateRecipes,
   DotenvLoad,
   Else,
   Export,
diff --git a/src/node.rs b/src/node.rs
index 270d6bb2..adba4020 100644
--- a/src/node.rs
+++ b/src/node.rs
@@ -221,7 +221,11 @@ impl<'src> Node<'src> for Set<'src> {
     set.push_mut(self.name.lexeme().replace('-', "_"));
 
     match &self.value {
-      DotenvLoad(value) | Export(value) | PositionalArguments(value) | WindowsPowerShell(value) => {
+      AllowDuplicateRecipes(value)
+      | DotenvLoad(value)
+      | Export(value)
+      | PositionalArguments(value)
+      | WindowsPowerShell(value) => {
         set.push_mut(value.to_string());
       }
       Shell(setting::Shell { command, arguments }) => {
diff --git a/src/parser.rs b/src/parser.rs
index 996c07a4..cbefc1ad 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -729,7 +729,13 @@ impl<'tokens, 'src> Parser<'tokens, 'src> {
     let name = Name::from_identifier(self.presume(Identifier)?);
     let lexeme = name.lexeme();
 
-    if Keyword::DotenvLoad == lexeme {
+    if Keyword::AllowDuplicateRecipes == lexeme {
+      let value = self.parse_set_bool()?;
+      return Ok(Set {
+        value: Setting::AllowDuplicateRecipes(value),
+        name,
+      });
+    } else if Keyword::DotenvLoad == lexeme {
       let value = self.parse_set_bool()?;
       return Ok(Set {
         value: Setting::DotenvLoad(value),
@@ -1714,6 +1720,12 @@ mod tests {
     tree: (justfile (set dotenv_load true)),
   }
 
+  test! {
+    name: set_allow_duplicate_recipes_implicit,
+    text: "set allow-duplicate-recipes",
+    tree: (justfile (set allow_duplicate_recipes true)),
+  }
+
   test! {
     name: set_dotenv_load_true,
     text: "set dotenv-load := true",
diff --git a/src/setting.rs b/src/setting.rs
index 39c9ecad..220ce9dc 100644
--- a/src/setting.rs
+++ b/src/setting.rs
@@ -2,6 +2,7 @@ use crate::common::*;
 
 #[derive(Debug, Clone)]
 pub(crate) enum Setting<'src> {
+  AllowDuplicateRecipes(bool),
   DotenvLoad(bool),
   Export(bool),
   PositionalArguments(bool),
@@ -18,7 +19,8 @@ pub(crate) struct Shell<'src> {
 impl<'src> Display for Setting<'src> {
   fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> {
     match self {
-      Setting::DotenvLoad(value)
+      Setting::AllowDuplicateRecipes(value)
+      | Setting::DotenvLoad(value)
       | Setting::Export(value)
       | Setting::PositionalArguments(value)
       | Setting::WindowsPowerShell(value) => write!(f, "{}", value),
diff --git a/src/settings.rs b/src/settings.rs
index 8078ebc5..9ba196d7 100644
--- a/src/settings.rs
+++ b/src/settings.rs
@@ -7,6 +7,7 @@ pub(crate) const WINDOWS_POWERSHELL_ARGS: &[&str] = &["-NoLogo", "-Command"];
 
 #[derive(Debug, PartialEq, Serialize)]
 pub(crate) struct Settings<'src> {
+  pub(crate) allow_duplicate_recipes: bool,
   pub(crate) dotenv_load: Option<bool>,
   pub(crate) export: bool,
   pub(crate) positional_arguments: bool,
@@ -17,6 +18,7 @@ pub(crate) struct Settings<'src> {
 impl<'src> Settings<'src> {
   pub(crate) fn new() -> Settings<'src> {
     Settings {
+      allow_duplicate_recipes: false,
       dotenv_load: None,
       export: false,
       positional_arguments: false,


Test patch:
diff --git a/tests/allow_duplicate_recipes.rs b/tests/allow_duplicate_recipes.rs
new file mode 100644
index 00000000..d597ce0c
--- /dev/null
+++ b/tests/allow_duplicate_recipes.rs
@@ -0,0 +1,38 @@
+use crate::common::*;
+
+#[test]
+fn allow_duplicate_recipes() {
+  Test::new()
+    .justfile(
+      "
+      b:
+        echo foo
+      b:
+        echo bar
+
+      set allow-duplicate-recipes
+    ",
+    )
+    .stdout("bar\n")
+    .stderr("echo bar\n")
+    .run();
+}
+
+#[test]
+fn allow_duplicate_recipes_with_args() {
+  Test::new()
+    .justfile(
+      "
+      b a:
+        echo foo
+      b c d:
+        echo bar {{c}} {{d}}
+
+      set allow-duplicate-recipes
+    ",
+    )
+    .args(&["b", "one", "two"])
+    .stdout("bar one two\n")
+    .stderr("echo bar one two\n")
+    .run();
+}
diff --git a/tests/json.rs b/tests/json.rs
index 2604f6f3..345ca2ae 100644
--- a/tests/json.rs
+++ b/tests/json.rs
@@ -39,6 +39,7 @@ fn alias() {
         }
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -66,6 +67,7 @@ fn assignment() {
       "first": null,
       "recipes": {},
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -106,6 +108,7 @@ fn body() {
         }
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -156,6 +159,7 @@ fn dependencies() {
         }
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -243,6 +247,59 @@ fn dependency_argument() {
         }
       },
       "settings": {
+        "allow_duplicate_recipes": false,
+        "dotenv_load": null,
+        "export": false,
+        "positional_arguments": false,
+        "shell": null,
+        "windows_powershell": false,
+      },
+      "warnings": [],
+    }),
+  );
+}
+
+#[test]
+fn duplicate_recipes() {
+  test(
+    "
+      set allow-duplicate-recipes
+      alias f := foo
+
+      foo:
+      foo bar:
+    ",
+    json!({
+      "first": "foo",
+      "aliases": {
+        "f": {
+          "name": "f",
+          "target": "foo",
+        }
+      },
+      "assignments": {},
+      "recipes": {
+        "foo": {
+          "body": [],
+          "dependencies": [],
+          "doc": null,
+          "name": "foo",
+          "parameters": [
+            {
+              "name": "bar",
+              "export": false,
+              "default": null,
+              "kind": "singular",
+            },
+          ],
+          "priors": 0,
+          "private": false,
+          "quiet": false,
+          "shebang": false,
+        }
+      },
+      "settings": {
+        "allow_duplicate_recipes": true,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -276,6 +333,7 @@ fn doc_comment() {
         }
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -297,6 +355,7 @@ fn empty_justfile() {
       "first": null,
       "recipes": {},
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -427,6 +486,7 @@ fn parameters() {
         },
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -496,6 +556,7 @@ fn priors() {
         },
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -529,6 +590,7 @@ fn private() {
         }
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -562,6 +624,7 @@ fn quiet() {
         }
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -613,6 +676,7 @@ fn settings() {
         }
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": true,
         "export": true,
         "positional_arguments": true,
@@ -652,6 +716,7 @@ fn shebang() {
         }
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
@@ -685,6 +750,7 @@ fn simple() {
         }
       },
       "settings": {
+        "allow_duplicate_recipes": false,
         "dotenv_load": null,
         "export": false,
         "positional_arguments": false,
diff --git a/tests/lib.rs b/tests/lib.rs
index 7492e418..87394157 100644
--- a/tests/lib.rs
+++ b/tests/lib.rs
@@ -1,6 +1,7 @@
 #[macro_use]
 mod test;
 
+mod allow_duplicate_recipes;
 mod assert_stdout;
 mod assert_success;
 mod byte_order_mark;