From 71a9dc8304b73e30bed97bf96010f08f86f9ba85 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Mon, 26 Feb 2024 14:49:46 +0900 Subject: [PATCH] templater: add similarity hint to no such method/keyword errors The translation from method error to keyword error can go wrong if the context object had n-ary methods (n > 0), which isn't the case as of now. For simplicity, arguments error is mapped to "self.(..)" suggestion. Local variables and "self" could be merged without using extra method, but we'll need extend_*_candidates() to merge in symbol/function aliases anyway. --- Cargo.lock | 1 + cli/Cargo.toml | 1 + cli/src/cli_util.rs | 16 +++++-- cli/src/template_builder.rs | 36 +++++++++++---- cli/src/template_parser.rs | 89 ++++++++++++++++++++++++++++++------- cli/tests/test_templater.rs | 58 ++++++++++++++++++++++++ 6 files changed, 175 insertions(+), 26 deletions(-) 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]