ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

templater: add similarity hint to no such method/keyword errors

Browse source 
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.<name>(..)" 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.
This commit is contained in:
Yuya Nishihara 2024-02-26 14:49:46 +09:00
parent de1e4a39f4
commit 71a9dc8304
6 changed files with 175 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
Cargo.lock generated
View file

@ -1670,6 +1670,7 @@ dependencies = [
"scm-record",
"serde",
"slab",
"strsim",
"tempfile",
"test-case",
"testutils",

1
cli/Cargo.toml
View file

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

16
cli/src/cli_util.rs
View file

@ -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<RevsetResolutionError> for CommandError {
impl From<TemplateParseError> 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)
}
}

36
cli/src/template_builder.rs
View file

@ -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) => {

89
cli/src/template_parser.rs
View file

@ -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<String>,
},
#[error(r#"Function "{name}" doesn't exist"#)]
NoSuchFunction {
name: String,
candidates: Vec<String>,
},
#[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<String>,
},
#[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<String>, 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<String>, function: &FunctionCallNode) -> Self {
// TODO: migrate all callers to table-based lookup_method()
pub(crate) fn no_such_method(
type_name: impl Into<String>,
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<I>(mut self, other_keywords: I) -> Self
where
I: IntoIterator,
I::Item: AsRef<str>,
{
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<I>(name: &str, candidates: I) -> Vec<String>
where
I: IntoIterator,
I::Item: AsRef<str>,
{
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;

58
cli/tests/test_templater.rs
View file

@ -31,6 +31,64 @@ fn test_templater_parse_error() {
|
= expected <EOI>, `++`, `||`, 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]