From ddeb645d7f8c122d2e030be519901c4803ae55ea Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 23 Mar 2023 18:03:01 +0900 Subject: [PATCH] cli: provide hint for typo of revset function name This is similar to what Mercurial does. The similarity threshold is copied from clap, but we might want to adjust it later. --- Cargo.lock | 1 + Cargo.toml | 1 + lib/src/revset.rs | 23 +++++++++++++++++++--- src/cli_util.rs | 37 +++++++++++++++++++++++++++++++++-- tests/test_revset_output.rs | 39 +++++++++++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 00a1777fe..79332b36d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -825,6 +825,7 @@ dependencies = [ "rpassword", "serde", "slab", + "strsim", "tempfile", "testutils", "textwrap 0.16.0", diff --git a/Cargo.toml b/Cargo.toml index a9245ca02..3daaee33e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -55,6 +55,7 @@ regex = "1.7.2" rpassword = "7.2.0" serde = { version = "1.0", features = ["derive"] } slab = "0.4.8" +strsim = "0.10.0" tempfile = "3.4.0" textwrap = "0.16.0" timeago = { version = "0.4.1", default-features = false } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 4b02b3833..a3dfc57b3 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -74,8 +74,11 @@ pub enum RevsetParseErrorKind { similar_op: String, description: String, }, - #[error("Revset function \"{0}\" doesn't exist")] - NoSuchFunction(String), + #[error(r#"Revset function "{name}" doesn't exist"#)] + NoSuchFunction { + name: String, + candidates: Vec, + }, #[error("Invalid arguments to revset function \"{name}\": {message}")] InvalidFunctionArguments { name: String, message: String }, #[error("Invalid file pattern: {0}")] @@ -715,12 +718,26 @@ fn parse_function_expression( func(name, arguments_pair, state) } else { Err(RevsetParseError::with_span( - RevsetParseErrorKind::NoSuchFunction(name.to_owned()), + RevsetParseErrorKind::NoSuchFunction { + name: name.to_owned(), + candidates: collect_function_names(state.aliases_map), + }, name_pair.as_span(), )) } } +fn collect_function_names(aliases_map: &RevsetAliasesMap) -> Vec { + let mut names = BUILTIN_FUNCTION_MAP + .keys() + .map(|&n| n.to_owned()) + .collect_vec(); + names.extend(aliases_map.function_aliases.keys().map(|n| n.to_owned())); + names.sort_unstable(); + names.dedup(); + names +} + type RevsetFunction = fn(&str, Pair, ParseState) -> Result, RevsetParseError>; diff --git a/src/cli_util.rs b/src/cli_util.rs index 749e9ace7..f008dc601 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -45,7 +45,7 @@ use jujutsu_lib::repo::{ use jujutsu_lib::repo_path::{FsPathParseError, RepoPath}; use jujutsu_lib::revset::{ resolve_symbols, Revset, RevsetAliasesMap, RevsetError, RevsetExpression, RevsetIteratorExt, - RevsetParseError, RevsetWorkspaceContext, + RevsetParseError, RevsetParseErrorKind, RevsetWorkspaceContext, }; use jujutsu_lib::settings::UserSettings; use jujutsu_lib::transaction::Transaction; @@ -97,6 +97,29 @@ pub fn user_error_with_hint(message: impl Into, hint: impl Into) } } +fn collect_similar<'a, S: AsRef>(name: &str, candidates: &'a [S]) -> Vec<&'a S> { + candidates + .iter() + .filter_map(|cand| { + // The parameter is borrowed from clap f5540d26 + (strsim::jaro(name, cand.as_ref()) > 0.7).then_some(cand) + }) + .collect_vec() +} + +fn format_similarity_hint>(candidates: &[S]) -> Option { + match candidates { + [] => None, + names => { + let quoted_names = names + .iter() + .map(|s| format!(r#""{}""#, s.as_ref())) + .join(", "); + Some(format!("Did you mean {quoted_names}?")) + } + } +} + impl From for CommandError { fn from(err: std::io::Error) -> Self { if err.kind() == std::io::ErrorKind::BrokenPipe { @@ -229,7 +252,17 @@ impl From for CommandError { impl From for CommandError { fn from(err: RevsetParseError) -> Self { let message = iter::successors(Some(&err), |e| e.origin()).join("\n"); - user_error(format!("Failed to parse revset: {message}")) + // Only for the top-level error as we can't attach hint to inner errors + let hint = match err.kind() { + RevsetParseErrorKind::NoSuchFunction { name, candidates } => { + format_similarity_hint(&collect_similar(name, candidates)) + } + _ => None, + }; + CommandError::UserError { + message: format!("Failed to parse revset: {message}"), + hint, + } } } diff --git a/tests/test_revset_output.rs b/tests/test_revset_output.rs index 631cfe150..828c32232 100644 --- a/tests/test_revset_output.rs +++ b/tests/test_revset_output.rs @@ -184,6 +184,45 @@ fn test_bad_function_call() { "###); } +#[test] +fn test_function_name_hint() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + let evaluate_err = |expr| test_env.jj_cmd_failure(&repo_path, &["log", "-r", expr]); + + test_env.add_config( + r###" + [revset-aliases] + 'branches(x)' = 'x' # override builtin function + 'my_author(x)' = 'author(x)' # similar name to builtin function + 'author_sym' = 'x' # not a function alias + "###, + ); + + // The suggestion "branches" shouldn't be duplicated + insta::assert_snapshot!(evaluate_err("branch()"), @r###" + Error: Failed to parse revset: --> 1:1 + | + 1 | branch() + | ^----^ + | + = Revset function "branch" doesn't exist + Hint: Did you mean "branches"? + "###); + + // Both builtin function and function alias should be suggested + insta::assert_snapshot!(evaluate_err("author_()"), @r###" + Error: Failed to parse revset: --> 1:1 + | + 1 | author_() + | ^-----^ + | + = Revset function "author_" doesn't exist + Hint: Did you mean "author", "my_author"? + "###); +} + #[test] fn test_alias() { let test_env = TestEnvironment::default();