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

This commit is contained in:
Casey Rodarmor 2024-06-13 19:41:45 -07:00 committed by GitHub
parent e1b17fe9cf
commit 18ec9796b9
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 588 additions and 155 deletions

403
src/argument_parser.rs Normal file
View File

@ -0,0 +1,403 @@
use super::*;
#[allow(clippy::doc_markdown)]
/// The argument parser is responsible for grouping positional arguments into
/// argument groups, which consist of a path to a recipe and its arguments.
///
/// Argument parsing is substantially complicated by the fact that recipe paths
/// can be given on the command line as multiple arguments, i.e., "foo" "bar"
/// baz", or as a single "::"-separated argument.
///
/// Error messages produced by the argument parser should use the format of the
/// recipe path as passed on the command line.
///
/// Additionally, if a recipe is specified with a "::"-separated path, extra
/// components of that path after a valid recipe must not be used as arguments,
/// whereas arguments after multiple argument path may be used as arguments. As
/// an example, `foo bar baz` may refer to recipe `foo::bar` with argument
/// `baz`, but `foo::bar::baz` is an error, since `bar` is a recipe, not a
/// module.
pub(crate) struct ArgumentParser<'src: 'run, 'run> {
arguments: &'run [&'run str],
next: usize,
root: &'run Justfile<'src>,
}
#[derive(Debug, PartialEq)]
pub(crate) struct ArgumentGroup<'run> {
pub(crate) arguments: Vec<&'run str>,
pub(crate) path: Vec<String>,
}
impl<'src: 'run, 'run> ArgumentParser<'src, 'run> {
pub(crate) fn parse_arguments(
root: &'run Justfile<'src>,
arguments: &'run [&'run str],
) -> RunResult<'src, Vec<ArgumentGroup<'run>>> {
let mut groups = Vec::new();
let mut invocation_parser = Self {
arguments,
next: 0,
root,
};
loop {
groups.push(invocation_parser.parse_group()?);
if invocation_parser.next == arguments.len() {
break;
}
}
Ok(groups)
}
fn parse_group(&mut self) -> RunResult<'src, ArgumentGroup<'run>> {
let (recipe, path) = if let Some(next) = self.next() {
if next.contains(':') {
let module_path =
ModulePath::try_from([next].as_slice()).map_err(|()| Error::UnknownRecipe {
recipe: next.into(),
suggestion: None,
})?;
let (recipe, path, _) = self.resolve_recipe(true, &module_path.path)?;
self.next += 1;
(recipe, path)
} else {
let (recipe, path, consumed) = self.resolve_recipe(false, self.rest())?;
self.next += consumed;
(recipe, path)
}
} else {
let (recipe, path, consumed) = self.resolve_recipe(false, self.rest())?;
assert_eq!(consumed, 0);
(recipe, path)
};
let rest = self.rest();
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(),
});
}
let arguments = rest[..argument_count].to_vec();
self.next += argument_count;
Ok(ArgumentGroup { arguments, path })
}
fn resolve_recipe(
&self,
module_path: bool,
args: &[impl AsRef<str>],
) -> RunResult<'src, (&'run Recipe<'src>, Vec<String>, usize)> {
let mut current = self.root;
let mut path = Vec::new();
for (i, arg) in args.iter().enumerate() {
let arg = arg.as_ref();
path.push(arg.to_string());
if let Some(module) = current.modules.get(arg) {
current = module;
} else if let Some(recipe) = current.get_recipe(arg) {
if module_path && i + 1 < args.len() {
return Err(Error::ExpectedSubmoduleButFoundRecipe {
path: if module_path {
path.join("::")
} else {
path.join(" ")
},
});
}
return Ok((recipe, path, i + 1));
} else {
if module_path && i + 1 < args.len() {
return Err(Error::UnknownSubmodule {
path: path.join("::"),
});
}
return Err(Error::UnknownRecipe {
recipe: if module_path {
path.join("::")
} else {
path.join(" ")
},
suggestion: current.suggest_recipe(arg),
});
}
}
if let Some(recipe) = &current.default {
recipe.check_can_be_default_recipe()?;
path.push(recipe.name().into());
Ok((recipe, path, args.len()))
} else if current.recipes.is_empty() {
Err(Error::NoRecipes)
} else {
Err(Error::NoDefaultRecipe)
}
}
fn next(&self) -> Option<&'run str> {
self.arguments.get(self.next).copied()
}
fn rest(&self) -> &[&'run str] {
&self.arguments[self.next..]
}
}
#[cfg(test)]
mod tests {
use {super::*, tempfile::TempDir};
trait TempDirExt {
fn write(&self, path: &str, content: &str);
}
impl TempDirExt for TempDir {
fn write(&self, path: &str, content: &str) {
let path = self.path().join(path);
fs::create_dir_all(path.parent().unwrap()).unwrap();
fs::write(path, content).unwrap();
}
}
#[test]
fn single_no_arguments() {
let justfile = testing::compile("foo:");
assert_eq!(
ArgumentParser::parse_arguments(&justfile, &["foo"]).unwrap(),
vec![ArgumentGroup {
path: vec!["foo".into()],
arguments: Vec::new()
}],
);
}
#[test]
fn single_with_argument() {
let justfile = testing::compile("foo bar:");
assert_eq!(
ArgumentParser::parse_arguments(&justfile, &["foo", "baz"]).unwrap(),
vec![ArgumentGroup {
path: vec!["foo".into()],
arguments: vec!["baz"],
}],
);
}
#[test]
fn single_argument_count_mismatch() {
let justfile = testing::compile("foo bar:");
assert_matches!(
ArgumentParser::parse_arguments(&justfile, &["foo"]).unwrap_err(),
Error::ArgumentCountMismatch {
recipe: "foo",
found: 0,
min: 1,
max: 1,
..
},
);
}
#[test]
fn single_unknown() {
let justfile = testing::compile("foo:");
assert_matches!(
ArgumentParser::parse_arguments(&justfile, &["bar"]).unwrap_err(),
Error::UnknownRecipe {
recipe,
suggestion: None
} if recipe == "bar",
);
}
#[test]
fn multiple_unknown() {
let justfile = testing::compile("foo:");
assert_matches!(
ArgumentParser::parse_arguments(&justfile, &["bar", "baz"]).unwrap_err(),
Error::UnknownRecipe {
recipe,
suggestion: None
} if recipe == "bar",
);
}
#[test]
fn recipe_in_submodule() {
let loader = Loader::new();
let tempdir = tempfile::tempdir().unwrap();
let path = tempdir.path().join("justfile");
fs::write(&path, "mod foo").unwrap();
fs::create_dir(tempdir.path().join("foo")).unwrap();
fs::write(tempdir.path().join("foo/mod.just"), "bar:").unwrap();
let compilation = Compiler::compile(true, &loader, &path).unwrap();
assert_eq!(
ArgumentParser::parse_arguments(&compilation.justfile, &["foo", "bar"]).unwrap(),
vec![ArgumentGroup {
path: vec!["foo".into(), "bar".into()],
arguments: Vec::new()
}],
);
}
#[test]
fn recipe_in_submodule_unknown() {
let loader = Loader::new();
let tempdir = tempfile::tempdir().unwrap();
let path = tempdir.path().join("justfile");
fs::write(&path, "mod foo").unwrap();
fs::create_dir(tempdir.path().join("foo")).unwrap();
fs::write(tempdir.path().join("foo/mod.just"), "bar:").unwrap();
let compilation = Compiler::compile(true, &loader, &path).unwrap();
assert_matches!(
ArgumentParser::parse_arguments(&compilation.justfile, &["foo", "zzz"]).unwrap_err(),
Error::UnknownRecipe {
recipe,
suggestion: None
} if recipe == "foo zzz",
);
}
#[test]
fn recipe_in_submodule_path_unknown() {
let tempdir = tempfile::tempdir().unwrap();
tempdir.write("justfile", "mod foo");
tempdir.write("foo.just", "bar:");
let loader = Loader::new();
let compilation = Compiler::compile(true, &loader, &tempdir.path().join("justfile")).unwrap();
assert_matches!(
ArgumentParser::parse_arguments(&compilation.justfile, &["foo::zzz"]).unwrap_err(),
Error::UnknownRecipe {
recipe,
suggestion: None
} if recipe == "foo::zzz",
);
}
#[test]
fn module_path_not_consumed() {
let tempdir = tempfile::tempdir().unwrap();
tempdir.write("justfile", "mod foo");
tempdir.write("foo.just", "bar:");
let loader = Loader::new();
let compilation = Compiler::compile(true, &loader, &tempdir.path().join("justfile")).unwrap();
assert_matches!(
ArgumentParser::parse_arguments(&compilation.justfile, &["foo::bar::baz"]).unwrap_err(),
Error::ExpectedSubmoduleButFoundRecipe {
path,
} if path == "foo::bar",
);
}
#[test]
fn no_recipes() {
let tempdir = tempfile::tempdir().unwrap();
tempdir.write("justfile", "");
let loader = Loader::new();
let compilation = Compiler::compile(true, &loader, &tempdir.path().join("justfile")).unwrap();
assert_matches!(
ArgumentParser::parse_arguments(&compilation.justfile, &[]).unwrap_err(),
Error::NoRecipes,
);
}
#[test]
fn default_recipe_requires_arguments() {
let tempdir = tempfile::tempdir().unwrap();
tempdir.write("justfile", "foo bar:");
let loader = Loader::new();
let compilation = Compiler::compile(true, &loader, &tempdir.path().join("justfile")).unwrap();
assert_matches!(
ArgumentParser::parse_arguments(&compilation.justfile, &[]).unwrap_err(),
Error::DefaultRecipeRequiresArguments {
recipe: "foo",
min_arguments: 1,
},
);
}
#[test]
fn no_default_recipe() {
let tempdir = tempfile::tempdir().unwrap();
tempdir.write("justfile", "import 'foo.just'");
tempdir.write("foo.just", "bar:");
let loader = Loader::new();
let compilation = Compiler::compile(true, &loader, &tempdir.path().join("justfile")).unwrap();
assert_matches!(
ArgumentParser::parse_arguments(&compilation.justfile, &[]).unwrap_err(),
Error::NoDefaultRecipe,
);
}
#[test]
fn complex_grouping() {
let justfile = testing::compile(
"
FOO A B='blarg':
echo foo: {{A}} {{B}}
BAR X:
echo bar: {{X}}
BAZ +Z:
echo baz: {{Z}}
",
);
assert_eq!(
ArgumentParser::parse_arguments(
&justfile,
&["BAR", "0", "FOO", "1", "2", "BAZ", "3", "4", "5"]
)
.unwrap(),
vec![
ArgumentGroup {
path: vec!["BAR".into()],
arguments: vec!["0"],
},
ArgumentGroup {
path: vec!["FOO".into()],
arguments: vec!["1", "2"],
},
ArgumentGroup {
path: vec!["BAZ".into()],
arguments: vec!["3", "4", "5"],
},
],
);
}
}

View File

@ -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}")?;
}

View File

@ -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 groups = ArgumentParser::parse_arguments(self, &arguments)?;
let mut missing = Vec::new();
let mut invocations = Vec::new();
let mut scopes = BTreeMap::new();
let arena: Arena<Scope> = Arena::new();
let mut invocations = Vec::<Invocation>::new();
let mut scopes = BTreeMap::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 rest = &remaining[1..];
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,21 +436,38 @@ 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
@ -545,12 +475,12 @@ mod tests {
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"),

View File

@ -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;

View File

@ -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),
})
}

View File

@ -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!(

View File

@ -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,
}

View File

@ -115,7 +115,7 @@ fn missing_recipe_after_invalid_path() {
.test_round_trip(false)
.arg(":foo::foo")
.arg("bar")
.stderr("error: Justfile does not contain recipes `:foo::foo` or `bar`.\n")
.stderr("error: Justfile does not contain recipe `:foo::foo`.\n")
.status(EXIT_FAILURE)
.run();
}
@ -690,3 +690,94 @@ fn recipes_with_same_name_are_both_run() {
.stdout("MODULE\nROOT\n")
.run();
}
#[test]
fn submodule_recipe_not_found_error_message() {
Test::new()
.args(["--unstable", "foo::bar"])
.stderr("error: Justfile does not contain submodule `foo`\n")
.status(1)
.run();
}
#[test]
fn submodule_recipe_not_found_spaced_error_message() {
Test::new()
.write("foo.just", "bar:\n @echo MODULE")
.justfile(
"
mod foo
",
)
.test_round_trip(false)
.args(["--unstable", "foo", "baz"])
.stderr("error: Justfile does not contain recipe `foo baz`.\nDid you mean `bar`?\n")
.status(1)
.run();
}
#[test]
fn submodule_recipe_not_found_colon_separated_error_message() {
Test::new()
.write("foo.just", "bar:\n @echo MODULE")
.justfile(
"
mod foo
",
)
.test_round_trip(false)
.args(["--unstable", "foo::baz"])
.stderr("error: Justfile does not contain recipe `foo::baz`.\nDid you mean `bar`?\n")
.status(1)
.run();
}
#[test]
fn colon_separated_path_does_not_run_recipes() {
Test::new()
.justfile(
"
foo:
@echo FOO
bar:
@echo BAR
",
)
.args(["--unstable", "foo::bar"])
.stderr("error: Expected submodule at `foo` but found recipe.\n")
.status(1)
.run();
}
#[test]
fn expected_submodule_but_found_recipe_in_root_error() {
Test::new()
.justfile("foo:")
.arg("foo::baz")
.stderr("error: Expected submodule at `foo` but found recipe.\n")
.status(1)
.run();
}
#[test]
fn expected_submodule_but_found_recipe_in_submodule_error() {
Test::new()
.justfile("mod foo")
.write("foo.just", "bar:")
.test_round_trip(false)
.args(["--unstable", "foo::bar::baz"])
.stderr("error: Expected submodule at `foo::bar` but found recipe.\n")
.status(1)
.run();
}
#[test]
fn colon_separated_path_components_are_not_used_as_arguments() {
Test::new()
.justfile("foo bar:")
.args(["foo::bar"])
.stderr("error: Expected submodule at `foo` but found recipe.\n")
.status(1)
.run();
}