Improve argument parsing and error handling for submodules (#2154)

Author: casey

src/argument_parser.rs

@@ -95,6 +95,9 @@ pub(crate) enum Error<'src> {
     variable: String,
     suggestion: Option<Suggestion<'src>>,
   },
+  ExpectedSubmoduleButFoundRecipe {
+    path: String,
+  },
   FormatCheckFoundDiff,
   FunctionCall {
     function: Name<'src>,
@@ -162,13 +165,13 @@ pub(crate) enum Error<'src> {
     line_number: Option<usize>,
   },
   UnknownSubmodule {
-    path: ModulePath,
+    path: String,
   },
   UnknownOverrides {
     overrides: Vec<String>,
   },
-  UnknownRecipes {
-    recipes: Vec<String>,
+  UnknownRecipe {
+    recipe: String,
     suggestion: Option<Suggestion<'src>>,
   },
   Unstable {
@@ -365,6 +368,9 @@ impl<'src> ColorDisplay for Error<'src> {
           write!(f, "\n{suggestion}")?;
         }
       }
+      ExpectedSubmoduleButFoundRecipe { path } => {
+        write!(f, "Expected submodule at `{path}` but found recipe.")?;
+      },
       FormatCheckFoundDiff => {
         write!(f, "Formatted justfile differs from original.")?;
       }
@@ -447,10 +453,8 @@ impl<'src> ColorDisplay for Error<'src> {
         let overrides = List::and_ticked(overrides);
         write!(f, "{count} {overrides} overridden on the command line but not present in justfile")?;
       }
-      UnknownRecipes { recipes, suggestion } => {
-        let count = Count("recipe", recipes.len());
-        let recipes = List::or_ticked(recipes);
-        write!(f, "Justfile does not contain {count} {recipes}.")?;
+      UnknownRecipe { recipe, suggestion } => {
+        write!(f, "Justfile does not contain recipe `{recipe}`.")?;
         if let Some(suggestion) = suggestion {
           write!(f, "\n{suggestion}")?;
         }

src/error.rs

@@ -173,66 +173,26 @@ impl<'src> Justfile<'src> {
       _ => {}
     }
 
-    let mut remaining: Vec<&str> = if !arguments.is_empty() {
-      arguments.iter().map(String::as_str).collect()
-    } else if let Some(recipe) = &self.default {
-      recipe.check_can_be_default_recipe()?;
-      vec![recipe.name()]
-    } else if self.recipes.is_empty() {
-      return Err(Error::NoRecipes);
-    } else {
-      return Err(Error::NoDefaultRecipe);
-    };
+    let arguments = arguments.iter().map(String::as_str).collect::<Vec<&str>>();
 
-    let mut missing = Vec::new();
-    let mut invocations = Vec::new();
-    let mut scopes = BTreeMap::new();
-    let arena: Arena<Scope> = Arena::new();
-
-    while let Some(first) = remaining.first().copied() {
-      if first.contains("::")
-        && !(first.starts_with(':') || first.ends_with(':') || first.contains(":::"))
-      {
-        remaining = first
-          .split("::")
-          .chain(remaining[1..].iter().copied())
-          .collect();
-
-        continue;
-      }
+    let groups = ArgumentParser::parse_arguments(self, &arguments)?;
 
-      let rest = &remaining[1..];
+    let arena: Arena<Scope> = Arena::new();
+    let mut invocations = Vec::<Invocation>::new();
+    let mut scopes = BTreeMap::new();
 
-      if let Some((invocation, consumed)) = self.invocation(
-        0,
-        &mut Vec::new(),
+    for group in &groups {
+      invocations.push(self.invocation(
         &arena,
-        &mut scopes,
+        &group.arguments,
         config,
         &dotenv,
-        search,
         &scope,
-        first,
-        rest,
-      )? {
-        remaining = rest[consumed..].to_vec();
-        invocations.push(invocation);
-      } else {
-        missing.push(first.to_string());
-        remaining = rest.to_vec();
-      }
-    }
-
-    if !missing.is_empty() {
-      let suggestion = if missing.len() == 1 {
-        self.suggest_recipe(missing.first().unwrap())
-      } else {
-        None
-      };
-      return Err(Error::UnknownRecipes {
-        recipes: missing,
-        suggestion,
-      });
+        &group.path,
+        0,
+        &mut scopes,
+        search,
+      )?);
     }
 
     let mut ran = Ran::default();
@@ -278,21 +238,29 @@ impl<'src> Justfile<'src> {
 
   fn invocation<'run>(
     &'run self,
-    depth: usize,
-    path: &mut Vec<&'run str>,
     arena: &'run Arena<Scope<'src, 'run>>,
-    scopes: &mut BTreeMap<Vec<&'run str>, &'run Scope<'src, 'run>>,
+    arguments: &[&'run str],
     config: &'run Config,
     dotenv: &'run BTreeMap<String, String>,
-    search: &'run Search,
     parent: &'run Scope<'src, 'run>,
-    first: &'run str,
-    rest: &[&'run str],
-  ) -> RunResult<'src, Option<(Invocation<'src, 'run>, usize)>> {
-    if let Some(module) = self.modules.get(first) {
-      path.push(first);
+    path: &'run [String],
+    position: usize,
+    scopes: &mut BTreeMap<&'run [String], &'run Scope<'src, 'run>>,
+    search: &'run Search,
+  ) -> RunResult<'src, Invocation<'src, 'run>> {
+    if position + 1 == path.len() {
+      let recipe = self.get_recipe(&path[position]).unwrap();
+      Ok(Invocation {
+        recipe,
+        module_source: &self.source,
+        arguments: arguments.into(),
+        settings: &self.settings,
+        scope: parent,
+      })
+    } else {
+      let module = self.modules.get(&path[position]).unwrap();
 
-      let scope = if let Some(scope) = scopes.get(path) {
+      let scope = if let Some(scope) = scopes.get(&path[..position]) {
         scope
       } else {
         let scope = Evaluator::evaluate_assignments(
@@ -304,76 +272,21 @@ impl<'src> Justfile<'src> {
           search,
         )?;
         let scope = arena.alloc(scope);
-        scopes.insert(path.clone(), scope);
+        scopes.insert(path, scope);
         scopes.get(path).unwrap()
       };
 
-      if rest.is_empty() {
-        if let Some(recipe) = &module.default {
-          recipe.check_can_be_default_recipe()?;
-          return Ok(Some((
-            Invocation {
-              settings: &module.settings,
-              recipe,
-              arguments: Vec::new(),
-              scope,
-              module_source: &self.source,
-            },
-            depth,
-          )));
-        }
-        Err(Error::NoDefaultRecipe)
-      } else {
-        module.invocation(
-          depth + 1,
-          path,
-          arena,
-          scopes,
-          config,
-          dotenv,
-          search,
-          scope,
-          rest[0],
-          &rest[1..],
-        )
-      }
-    } else if let Some(recipe) = self.get_recipe(first) {
-      if recipe.parameters.is_empty() {
-        Ok(Some((
-          Invocation {
-            arguments: Vec::new(),
-            recipe,
-            scope: parent,
-            settings: &self.settings,
-            module_source: &self.source,
-          },
-          depth,
-        )))
-      } else {
-        let argument_range = recipe.argument_range();
-        let argument_count = cmp::min(rest.len(), recipe.max_arguments());
-        if !argument_range.range_contains(&argument_count) {
-          return Err(Error::ArgumentCountMismatch {
-            recipe: recipe.name(),
-            parameters: recipe.parameters.clone(),
-            found: rest.len(),
-            min: recipe.min_arguments(),
-            max: recipe.max_arguments(),
-          });
-        }
-        Ok(Some((
-          Invocation {
-            arguments: rest[..argument_count].to_vec(),
-            recipe,
-            scope: parent,
-            settings: &self.settings,
-            module_source: &self.source,
-          },
-          depth + argument_count,
-        )))
-      }
-    } else {
-      Ok(None)
+      module.invocation(
+        arena,
+        arguments,
+        config,
+        dotenv,
+        scope,
+        path,
+        position + 1,
+        scopes,
+        search,
+      )
     }
   }
 
@@ -523,34 +436,51 @@ mod tests {
   use Error::*;
 
   run_error! {
-    name: unknown_recipes,
+    name: unknown_recipe_no_suggestion,
     src: "a:\nb:\nc:",
-    args: ["a", "x", "y", "z"],
-    error: UnknownRecipes {
-      recipes,
+    args: ["a", "xyz", "y", "z"],
+    error: UnknownRecipe {
+      recipe,
       suggestion,
     },
     check: {
-      assert_eq!(recipes, &["x", "y", "z"]);
+      assert_eq!(recipe, "xyz");
       assert_eq!(suggestion, None);
     }
   }
 
   run_error! {
-    name: unknown_recipes_show_alias_suggestion,
+    name: unknown_recipe_with_suggestion,
+    src: "a:\nb:\nc:",
+    args: ["a", "x", "y", "z"],
+    error: UnknownRecipe {
+      recipe,
+      suggestion,
+    },
+    check: {
+      assert_eq!(recipe, "x");
+      assert_eq!(suggestion, Some(Suggestion {
+        name: "a",
+        target: None,
+      }));
+    }
+  }
+
+  run_error! {
+    name: unknown_recipe_show_alias_suggestion,
     src: "
       foo:
         echo foo
 
       alias z := foo
     ",
     args: ["zz"],
-    error: UnknownRecipes {
-      recipes,
+    error: UnknownRecipe {
+      recipe,
       suggestion,
     },
     check: {
-      assert_eq!(recipes, &["zz"]);
+      assert_eq!(recipe, "zz");
       assert_eq!(suggestion, Some(Suggestion {
         name: "z",
         target: Some("foo"),

src/justfile.rs

@@ -15,7 +15,7 @@
 
 pub(crate) use {
   crate::{
-    alias::Alias, analyzer::Analyzer, assignment::Assignment,
+    alias::Alias, analyzer::Analyzer, argument_parser::ArgumentParser, assignment::Assignment,
     assignment_resolver::AssignmentResolver, ast::Ast, attribute::Attribute, binding::Binding,
     color::Color, color_display::ColorDisplay, command_ext::CommandExt, compilation::Compilation,
     compile_error::CompileError, compile_error_kind::CompileErrorKind, compiler::Compiler,
@@ -113,6 +113,7 @@ pub mod summary;
 
 mod alias;
 mod analyzer;
+mod argument_parser;
 mod assignment;
 mod assignment_resolver;
 mod ast;

src/lib.rs

@@ -150,7 +150,7 @@ impl Subcommand {
         };
 
         match Self::run_inner(config, loader, arguments, overrides, &search) {
-          Err((err @ Error::UnknownRecipes { .. }, true)) => {
+          Err((err @ Error::UnknownRecipe { .. }, true)) => {
             match search.justfile.parent().unwrap().parent() {
               Some(parent) => {
                 unknown_recipes_errors.get_or_insert(err);
@@ -428,7 +428,9 @@ impl Subcommand {
       module = module
         .modules
         .get(name)
-        .ok_or_else(|| Error::UnknownSubmodule { path: path.clone() })?;
+        .ok_or_else(|| Error::UnknownSubmodule {
+          path: path.to_string(),
+        })?;
     }
 
     Self::list_module(config, module, 0);
@@ -588,7 +590,9 @@ impl Subcommand {
       module = module
         .modules
         .get(name)
-        .ok_or_else(|| Error::UnknownSubmodule { path: path.clone() })?;
+        .ok_or_else(|| Error::UnknownSubmodule {
+          path: path.to_string(),
+        })?;
     }
 
     let name = path.path.last().unwrap();
@@ -602,8 +606,8 @@ impl Subcommand {
       println!("{}", recipe.color_display(config.color.stdout()));
       Ok(())
     } else {
-      Err(Error::UnknownRecipes {
-        recipes: vec![name.to_owned()],
+      Err(Error::UnknownRecipe {
+        recipe: name.to_owned(),
         suggestion: module.suggest_recipe(name),
       })
     }

src/subcommand.rs

@@ -131,7 +131,7 @@ macro_rules! run_error {
 }
 
 macro_rules! assert_matches {
-  ($expression:expr, $( $pattern:pat_param )|+ $( if $guard:expr )?) => {
+  ($expression:expr, $( $pattern:pat_param )|+ $( if $guard:expr )? $(,)?) => {
     match $expression {
       $( $pattern )|+ $( if $guard )? => {}
       left => panic!(

src/testing.rs

@@ -652,7 +652,7 @@ test! {
   justfile: "hello:",
   args:     ("foo", "bar"),
   stdout:   "",
-  stderr:   "error: Justfile does not contain recipes `foo` or `bar`.\n",
+  stderr:   "error: Justfile does not contain recipe `foo`.\n",
   status:   EXIT_FAILURE,
 }