diff --git a/Cargo.lock b/Cargo.lock index 79306c840..0af36cb40 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1670,6 +1670,7 @@ dependencies = [ "scm-record", "serde", "slab", + "strsim", "tempfile", "test-case", "testutils", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index cb0e83e45..43b4ec237 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -63,6 +63,7 @@ rpassword = { workspace = true } scm-record = { workspace = true } serde = { workspace = true } slab = { workspace = true } +strsim = { workspace = true } tempfile = { workspace = true } textwrap = { workspace = true } thiserror = { workspace = true } diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 1512880de..920e2378d 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -86,7 +86,7 @@ use crate::git_util::{ is_colocated_git_workspace, print_failed_git_export, print_git_import_stats, }; use crate::merge_tools::{ConflictResolveError, DiffEditError, DiffGenerateError}; -use crate::template_parser::{TemplateAliasesMap, TemplateParseError}; +use crate::template_parser::{TemplateAliasesMap, TemplateParseError, TemplateParseErrorKind}; use crate::templater::Template; use crate::ui::{ColorChoice, Ui}; use crate::{commit_templater, text_util}; @@ -453,8 +453,18 @@ impl From for CommandError { impl From for CommandError { fn from(err: TemplateParseError) -> Self { - let message = iter::successors(Some(&err), |e| e.origin()).join("\n"); - user_error(format!("Failed to parse template: {message}")) + let err_chain = iter::successors(Some(&err), |e| e.origin()); + let message = err_chain.clone().join("\n"); + // Only for the bottom error, which is usually the root cause + let hint = match err_chain.last().unwrap().kind() { + TemplateParseErrorKind::NoSuchKeyword { candidates, .. } + | TemplateParseErrorKind::NoSuchFunction { candidates, .. } + | TemplateParseErrorKind::NoSuchMethod { candidates, .. } => { + format_similarity_hint(candidates) + } + _ => None, + }; + user_error_with_hint_opt(format!("Failed to parse template: {message}"), hint) } } diff --git a/cli/src/template_builder.rs b/cli/src/template_builder.rs index 809ba8a0c..84f93a7be 100644 --- a/cli/src/template_builder.rs +++ b/cli/src/template_builder.rs @@ -19,7 +19,7 @@ use jj_lib::backend::{Signature, Timestamp}; use crate::template_parser::{ self, BinaryOp, ExpressionKind, ExpressionNode, FunctionCallNode, MethodCallNode, - TemplateParseError, TemplateParseResult, UnaryOp, + TemplateParseError, TemplateParseErrorKind, TemplateParseResult, UnaryOp, }; use crate::templater::{ ConcatTemplate, ConditionalTemplate, IntoTemplate, LabelTemplate, ListPropertyTemplate, @@ -389,12 +389,27 @@ fn build_keyword<'a, L: TemplateLanguage<'a>>( args: vec![], args_span: name_span.end_pos().span(&name_span.end_pos()), }; - let property = language - .build_method(build_ctx, self_property, &function) - // Since keyword is a 0-ary method, any argument-related errors mean - // there's no such keyword. - .map_err(|_| TemplateParseError::no_such_keyword(name, name_span))?; - Ok(Expression::with_label(property, name)) + match language.build_method(build_ctx, self_property, &function) { + Ok(property) => Ok(Expression::with_label(property, name)), + Err(err) => { + let kind = if let TemplateParseErrorKind::NoSuchMethod { candidates, .. } = err.kind() { + TemplateParseErrorKind::NoSuchKeyword { + name: name.to_owned(), + // TODO: filter methods by arity? + candidates: candidates.clone(), + } + } else { + // Since keyword is a 0-ary method, any argument-related errors + // mean there's no such keyword. + TemplateParseErrorKind::NoSuchKeyword { + name: name.to_owned(), + // TODO: might be better to phrase the error differently + candidates: vec![format!("self.{name}(..)")], + } + }; + Err(TemplateParseError::with_span(kind, name_span)) + } + } } fn build_unary_operation<'a, L: TemplateLanguage<'a>>( @@ -923,7 +938,12 @@ pub fn build_expression<'a, L: TemplateLanguage<'a>>( // "self" is a special variable, so don't label it Ok(Expression::unlabeled(language.build_self())) } else { - build_keyword(language, build_ctx, name, node.span) + build_keyword(language, build_ctx, name, node.span).map_err(|err| { + err.extend_keyword_candidates(itertools::chain( + build_ctx.local_variables.keys().copied(), + ["self"], + )) + }) } } ExpressionKind::Boolean(value) => { diff --git a/cli/src/template_parser.rs b/cli/src/template_parser.rs index 39d04d2e7..0036027cf 100644 --- a/cli/src/template_parser.rs +++ b/cli/src/template_parser.rs @@ -14,7 +14,7 @@ use std::collections::HashMap; use std::num::ParseIntError; -use std::{error, fmt, iter}; +use std::{error, fmt, iter, mem}; use itertools::Itertools as _; use once_cell::sync::Lazy; @@ -77,12 +77,22 @@ pub enum TemplateParseErrorKind { SyntaxError, #[error("Invalid integer literal")] ParseIntError(#[source] ParseIntError), - #[error(r#"Keyword "{0}" doesn't exist"#)] - NoSuchKeyword(String), - #[error(r#"Function "{0}" doesn't exist"#)] - NoSuchFunction(String), + #[error(r#"Keyword "{name}" doesn't exist"#)] + NoSuchKeyword { + name: String, + candidates: Vec, + }, + #[error(r#"Function "{name}" doesn't exist"#)] + NoSuchFunction { + name: String, + candidates: Vec, + }, #[error(r#"Method "{name}" doesn't exist for type "{type_name}""#)] - NoSuchMethod { type_name: String, name: String }, + NoSuchMethod { + type_name: String, + name: String, + candidates: Vec, + }, #[error(r#"Function "{name}": {message}"#)] InvalidArguments { name: String, message: String }, #[error("Redefinition of function parameter")] @@ -126,22 +136,27 @@ impl TemplateParseError { } } - pub fn no_such_keyword(name: impl Into, span: pest::Span<'_>) -> Self { - TemplateParseError::with_span(TemplateParseErrorKind::NoSuchKeyword(name.into()), span) - } - - pub fn no_such_function(function: &FunctionCallNode) -> Self { + // TODO: migrate callers to something like lookup_method() + pub(crate) fn no_such_function(function: &FunctionCallNode) -> Self { TemplateParseError::with_span( - TemplateParseErrorKind::NoSuchFunction(function.name.to_owned()), + TemplateParseErrorKind::NoSuchFunction { + name: function.name.to_owned(), + candidates: vec![], + }, function.name_span, ) } - pub fn no_such_method(type_name: impl Into, function: &FunctionCallNode) -> Self { + // TODO: migrate all callers to table-based lookup_method() + pub(crate) fn no_such_method( + type_name: impl Into, + function: &FunctionCallNode, + ) -> Self { TemplateParseError::with_span( TemplateParseErrorKind::NoSuchMethod { type_name: type_name.into(), name: function.name.to_owned(), + candidates: vec![], }, function.name_span, ) @@ -177,6 +192,26 @@ impl TemplateParseError { ) } + /// If this is a `NoSuchKeyword` error, expands the candidates list with the + /// given `other_keywords`. + pub fn extend_keyword_candidates(mut self, other_keywords: I) -> Self + where + I: IntoIterator, + I::Item: AsRef, + { + if let TemplateParseErrorKind::NoSuchKeyword { name, candidates } = &mut self.kind { + let other_candidates = collect_similar(name, other_keywords); + *candidates = itertools::merge(mem::take(candidates), other_candidates) + .dedup() + .collect(); + } + self + } + + pub fn kind(&self) -> &TemplateParseErrorKind { + &self.kind + } + /// Original parsing error which typically occurred in an alias expression. pub fn origin(&self) -> Option<&Self> { self.origin.as_deref() @@ -864,11 +899,35 @@ pub fn lookup_method<'a, V>( if let Some(value) = table.get(function.name) { Ok(value) } else { - // TODO: provide typo hint - Err(TemplateParseError::no_such_method(type_name, function)) + let candidates = collect_similar(function.name, table.keys()); + Err(TemplateParseError::with_span( + TemplateParseErrorKind::NoSuchMethod { + type_name: type_name.into(), + name: function.name.to_owned(), + candidates, + }, + function.name_span, + )) } } +// TODO: merge with revset::collect_similar()? +fn collect_similar(name: &str, candidates: I) -> Vec +where + I: IntoIterator, + I::Item: AsRef, +{ + candidates + .into_iter() + .filter(|cand| { + // The parameter is borrowed from clap f5540d26 + strsim::jaro(name, cand.as_ref()) > 0.7 + }) + .map(|s| s.as_ref().to_owned()) + .sorted_unstable() + .collect() +} + #[cfg(test)] mod tests { use assert_matches::assert_matches; diff --git a/cli/tests/test_templater.rs b/cli/tests/test_templater.rs index 8d9090cf3..8a56d6d50 100644 --- a/cli/tests/test_templater.rs +++ b/cli/tests/test_templater.rs @@ -31,6 +31,64 @@ fn test_templater_parse_error() { | = expected , `++`, `||`, or `&&` "###); + + // Typo + test_env.add_config( + r###" + [template-aliases] + 'format_id(id)' = 'id.sort()' + "###, + ); + insta::assert_snapshot!(render_err(r#"conflicts"#), @r###" + Error: Failed to parse template: --> 1:1 + | + 1 | conflicts + | ^-------^ + | + = Keyword "conflicts" doesn't exist + Hint: Did you mean "conflict"? + "###); + insta::assert_snapshot!(render_err(r#"commit_id.shorter()"#), @r###" + Error: Failed to parse template: --> 1:11 + | + 1 | commit_id.shorter() + | ^-----^ + | + = Method "shorter" doesn't exist for type "CommitOrChangeId" + Hint: Did you mean "short", "shortest"? + "###); + insta::assert_snapshot!(render_err(r#"cat()"#), @r###" + Error: Failed to parse template: --> 1:1 + | + 1 | cat() + | ^-^ + | + = Function "cat" doesn't exist + "###); + insta::assert_snapshot!(render_err(r#""".lines().map(|s| se)"#), @r###" + Error: Failed to parse template: --> 1:20 + | + 1 | "".lines().map(|s| se) + | ^^ + | + = Keyword "se" doesn't exist + Hint: Did you mean "s", "self"? + "###); + insta::assert_snapshot!(render_err(r#"format_id(commit_id)"#), @r###" + Error: Failed to parse template: --> 1:1 + | + 1 | format_id(commit_id) + | ^------------------^ + | + = Alias "format_id()" cannot be expanded + --> 1:4 + | + 1 | id.sort() + | ^--^ + | + = Method "sort" doesn't exist for type "CommitOrChangeId" + Hint: Did you mean "short", "shortest"? + "###); } #[test]