From bfdaaa42573fa7dbde77502b4e3be31c59fc46b9 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 12 Feb 2023 18:41:53 +0900 Subject: [PATCH] templater: implement symbol/function alias expansion Test vectors are mainly copied from revset.rs and adapted to the template syntax. Closes #1190 --- CHANGELOG.md | 4 + src/template_parser.rs | 329 +++++++++++++++++++++++++++++++++++++++- tests/test_templater.rs | 110 +++++++++++++- 3 files changed, 434 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9639322eb..5be3b0369 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -148,6 +148,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * It is now possible to change the author format of `jj log` with the new `ui.log-author-format` option. +* Added support for template aliases. New symbols and functions can be + configured by `template-aliases. = `. Be aware that + the template syntax isn't documented yet and is likely to change. + ### Fixed bugs * When sharing the working copy with a Git repo, we used to forget to export diff --git a/src/template_parser.rs b/src/template_parser.rs index 2b9845117..ff5e10ed9 100644 --- a/src/template_parser.rs +++ b/src/template_parser.rs @@ -72,6 +72,10 @@ pub enum TemplateParseErrorKind { InvalidArgumentType(String), #[error("Redefinition of function parameter")] RedefinedFunctionParameter, + #[error(r#"Alias "{0}" cannot be expanded"#)] + BadAliasExpansion(String), + #[error(r#"Alias "{0}" expanded recursively"#)] + RecursiveAlias(String), } impl TemplateParseError { @@ -89,7 +93,6 @@ impl TemplateParseError { } } - #[allow(unused)] // TODO: remove fn with_span_and_origin( kind: TemplateParseErrorKind, span: pest::Span<'_>, @@ -157,6 +160,14 @@ impl TemplateParseError { ) } + fn within_alias_expansion(self, id: TemplateAliasId<'_>, span: pest::Span<'_>) -> Self { + TemplateParseError::with_span_and_origin( + TemplateParseErrorKind::BadAliasExpansion(id.to_string()), + span, + self, + ) + } + /// Original parsing error which typically occurred in an alias expression. pub fn origin(&self) -> Option<&Self> { self.origin.as_deref() @@ -363,14 +374,12 @@ impl TemplateAliasesMap { Ok(()) } - #[cfg(test)] // TODO: remove fn get_symbol(&self, name: &str) -> Option<(TemplateAliasId<'_>, &str)> { self.symbol_aliases .get_key_value(name) .map(|(name, defn)| (TemplateAliasId::Symbol(name), defn.as_ref())) } - #[cfg(test)] // TODO: remove fn get_function(&self, name: &str) -> Option<(TemplateAliasId<'_>, &[String], &str)> { self.function_aliases .get_key_value(name) @@ -427,14 +436,12 @@ impl TemplateAliasDeclaration { } /// Borrowed reference to identify alias expression. -#[cfg(test)] // TODO: remove #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum TemplateAliasId<'a> { Symbol(&'a str), Function(&'a str), } -#[cfg(test)] // TODO: remove impl fmt::Display for TemplateAliasId<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { @@ -444,6 +451,125 @@ impl fmt::Display for TemplateAliasId<'_> { } } +/// Expand aliases recursively. +fn expand_aliases<'i>( + node: ExpressionNode<'i>, + aliases_map: &'i TemplateAliasesMap, +) -> TemplateParseResult> { + #[derive(Clone, Copy, Debug)] + struct State<'a, 'i> { + aliases_map: &'i TemplateAliasesMap, + aliases_expanding: &'a [TemplateAliasId<'a>], + locals: &'a HashMap<&'a str, ExpressionNode<'i>>, + } + + fn expand_defn<'i>( + id: TemplateAliasId<'_>, + defn: &'i str, + locals: &HashMap<&str, ExpressionNode<'i>>, + span: pest::Span<'_>, + state: State<'_, 'i>, + ) -> TemplateParseResult> { + // The stack should be short, so let's simply do linear search and duplicate. + if state.aliases_expanding.contains(&id) { + return Err(TemplateParseError::with_span( + TemplateParseErrorKind::RecursiveAlias(id.to_string()), + span, + )); + } + let mut aliases_expanding = state.aliases_expanding.to_vec(); + aliases_expanding.push(id); + let expanding_state = State { + aliases_map: state.aliases_map, + aliases_expanding: &aliases_expanding, + locals, + }; + // Parsed defn could be cached if needed. + parse_template(defn) + .and_then(|node| expand_node(node, expanding_state)) + .map_err(|e| e.within_alias_expansion(id, span)) + } + + fn expand_list<'i>( + nodes: Vec>, + state: State<'_, 'i>, + ) -> TemplateParseResult>> { + nodes + .into_iter() + .map(|node| expand_node(node, state)) + .try_collect() + } + + fn expand_function_call<'i>( + function: FunctionCallNode<'i>, + state: State<'_, 'i>, + ) -> TemplateParseResult> { + Ok(FunctionCallNode { + name: function.name, + name_span: function.name_span, + args: expand_list(function.args, state)?, + args_span: function.args_span, + }) + } + + fn expand_node<'i>( + mut node: ExpressionNode<'i>, + state: State<'_, 'i>, + ) -> TemplateParseResult> { + match node.kind { + ExpressionKind::Identifier(name) => { + if let Some(node) = state.locals.get(name) { + Ok(node.clone()) + } else if let Some((id, defn)) = state.aliases_map.get_symbol(name) { + let locals = HashMap::new(); // Don't spill out the current scope + expand_defn(id, defn, &locals, node.span, state) + } else { + Ok(node) + } + } + ExpressionKind::Integer(_) => Ok(node), + ExpressionKind::String(_) => Ok(node), + ExpressionKind::List(nodes) => { + node.kind = ExpressionKind::List(expand_list(nodes, state)?); + Ok(node) + } + ExpressionKind::FunctionCall(function) => { + if let Some((id, params, defn)) = state.aliases_map.get_function(function.name) { + if function.args.len() != params.len() { + return Err(TemplateParseError::invalid_argument_count_exact( + params.len(), + function.args_span, + )); + } + // Resolve arguments in the current scope, and pass them in to the alias + // expansion scope. + let args = expand_list(function.args, state)?; + let locals = params.iter().map(|s| s.as_str()).zip(args).collect(); + expand_defn(id, defn, &locals, node.span, state) + } else { + node.kind = + ExpressionKind::FunctionCall(expand_function_call(function, state)?); + Ok(node) + } + } + ExpressionKind::MethodCall(method) => { + node.kind = ExpressionKind::MethodCall(MethodCallNode { + object: Box::new(expand_node(*method.object, state)?), + function: expand_function_call(method.function, state)?, + }); + Ok(node) + } + } + } + + let state = State { + aliases_map, + aliases_expanding: &[], + locals: &HashMap::new(), + }; + expand_node(node, state) +} + enum Property<'a, I> { String(Box + 'a>), Boolean(Box + 'a>), @@ -971,9 +1097,10 @@ pub fn parse_commit_template<'a>( repo: RepoRef<'a>, workspace_id: &WorkspaceId, template_text: &str, - _aliases_map: &TemplateAliasesMap, + aliases_map: &TemplateAliasesMap, ) -> TemplateParseResult + 'a>> { let node = parse_template(template_text)?; + let node = expand_aliases(node, aliases_map)?; let expression = build_expression(&node, &|name, span| { build_commit_keyword(repo, workspace_id, name, span) })?; @@ -984,6 +1111,33 @@ pub fn parse_commit_template<'a>( mod tests { use super::*; + #[derive(Debug)] + struct WithTemplateAliasesMap(TemplateAliasesMap); + + impl WithTemplateAliasesMap { + fn parse<'i>(&'i self, template_text: &'i str) -> TemplateParseResult> { + let node = parse_template(template_text)?; + expand_aliases(node, &self.0) + } + + fn parse_normalized<'i>( + &'i self, + template_text: &'i str, + ) -> TemplateParseResult> { + self.parse(template_text).map(normalize_tree) + } + } + + fn with_aliases( + aliases: impl IntoIterator, impl Into)>, + ) -> WithTemplateAliasesMap { + let mut aliases_map = TemplateAliasesMap::new(); + for (decl, defn) in aliases { + aliases_map.insert(decl, defn).unwrap(); + } + WithTemplateAliasesMap(aliases_map) + } + fn parse(template_text: &str) -> TemplateParseResult> { let node = parse_template(template_text)?; build_expression(&node, &|name, span| { @@ -991,6 +1145,10 @@ mod tests { }) } + fn parse_normalized(template_text: &str) -> TemplateParseResult { + parse_template(template_text).map(normalize_tree) + } + /// Drops auxiliary data of AST so it can be compared with other node. fn normalize_tree(node: ExpressionNode) -> ExpressionNode { fn empty_span() -> pest::Span<'static> { @@ -1108,4 +1266,163 @@ mod tests { assert!(aliases_map.insert("l(a,b,)", r#"""#).is_ok()); assert!(aliases_map.insert("m(a,,b)", r#"""#).is_err()); } + + #[test] + fn test_expand_symbol_alias() { + assert_eq!( + with_aliases([("AB", "a b")]) + .parse_normalized("AB c") + .unwrap(), + parse_normalized("(a b) c").unwrap(), + ); + assert_eq!( + with_aliases([("AB", "a b")]) + .parse_normalized("if(AB, label(c, AB))") + .unwrap(), + parse_normalized("if((a b), label(c, (a b)))").unwrap(), + ); + + // Multi-level substitution. + assert_eq!( + with_aliases([("A", "BC"), ("BC", "b C"), ("C", "c")]) + .parse_normalized("A") + .unwrap(), + parse_normalized("b c").unwrap(), + ); + + // Method receiver and arguments should be expanded. + assert_eq!( + with_aliases([("A", "a")]) + .parse_normalized("A.f()") + .unwrap(), + parse_normalized("a.f()").unwrap(), + ); + assert_eq!( + with_aliases([("A", "a"), ("B", "b")]) + .parse_normalized("x.f(A, B)") + .unwrap(), + parse_normalized("x.f(a, b)").unwrap(), + ); + + // Infinite recursion, where the top-level error isn't of RecursiveAlias kind. + assert_eq!( + with_aliases([("A", "A")]).parse("A").unwrap_err().kind, + TemplateParseErrorKind::BadAliasExpansion("A".to_owned()), + ); + assert_eq!( + with_aliases([("A", "B"), ("B", "b C"), ("C", "c B")]) + .parse("A") + .unwrap_err() + .kind, + TemplateParseErrorKind::BadAliasExpansion("A".to_owned()), + ); + + // Error in alias definition. + assert_eq!( + with_aliases([("A", "a(")]).parse("A").unwrap_err().kind, + TemplateParseErrorKind::BadAliasExpansion("A".to_owned()), + ); + } + + #[test] + fn test_expand_function_alias() { + assert_eq!( + with_aliases([("F( )", "a")]) + .parse_normalized("F()") + .unwrap(), + parse_normalized("a").unwrap(), + ); + assert_eq!( + with_aliases([("F( x )", "x")]) + .parse_normalized("F(a)") + .unwrap(), + parse_normalized("a").unwrap(), + ); + assert_eq!( + with_aliases([("F( x, y )", "x y")]) + .parse_normalized("F(a, b)") + .unwrap(), + parse_normalized("a b").unwrap(), + ); + + // Arguments should be resolved in the current scope. + assert_eq!( + with_aliases([("F(x,y)", "if(x, y)")]) + .parse_normalized("F(a y, b x)") + .unwrap(), + parse_normalized("if((a y), (b x))").unwrap(), + ); + // F(a) -> if(G(a), y) -> if((x a), y) + assert_eq!( + with_aliases([("F(x)", "if(G(x), y)"), ("G(y)", "x y")]) + .parse_normalized("F(a)") + .unwrap(), + parse_normalized("if((x a), y)").unwrap(), + ); + // F(G(a)) -> F(x a) -> if(G(x a), y) -> if((x (x a)), y) + assert_eq!( + with_aliases([("F(x)", "if(G(x), y)"), ("G(y)", "x y")]) + .parse_normalized("F(G(a))") + .unwrap(), + parse_normalized("if((x (x a)), y)").unwrap(), + ); + + // Function parameter should precede the symbol alias. + assert_eq!( + with_aliases([("F(X)", "X"), ("X", "x")]) + .parse_normalized("F(a) X") + .unwrap(), + parse_normalized("a x").unwrap(), + ); + + // Function parameter shouldn't be expanded in symbol alias. + assert_eq!( + with_aliases([("F(x)", "x A"), ("A", "x")]) + .parse_normalized("F(a)") + .unwrap(), + parse_normalized("a x").unwrap(), + ); + + // Function and symbol aliases reside in separate namespaces. + assert_eq!( + with_aliases([("A()", "A"), ("A", "a")]) + .parse_normalized("A()") + .unwrap(), + parse_normalized("a").unwrap(), + ); + + // Method call shouldn't be substituted by function alias. + assert_eq!( + with_aliases([("F()", "f()")]) + .parse_normalized("x.F()") + .unwrap(), + parse_normalized("x.F()").unwrap(), + ); + + // Invalid number of arguments. + assert_eq!( + with_aliases([("F()", "x")]).parse("F(a)").unwrap_err().kind, + TemplateParseErrorKind::InvalidArgumentCountExact(0), + ); + assert_eq!( + with_aliases([("F(x)", "x")]).parse("F()").unwrap_err().kind, + TemplateParseErrorKind::InvalidArgumentCountExact(1), + ); + assert_eq!( + with_aliases([("F(x,y)", "x y")]) + .parse("F(a,b,c)") + .unwrap_err() + .kind, + TemplateParseErrorKind::InvalidArgumentCountExact(2), + ); + + // Infinite recursion, where the top-level error isn't of RecursiveAlias kind. + assert_eq!( + with_aliases([("F(x)", "G(x)"), ("G(x)", "H(x)"), ("H(x)", "F(x)")]) + .parse("F(a)") + .unwrap_err() + .kind, + TemplateParseErrorKind::BadAliasExpansion("F()".to_owned()), + ); + } } diff --git a/tests/test_templater.rs b/tests/test_templater.rs index ab21ea5d4..b3dad46e7 100644 --- a/tests/test_templater.rs +++ b/tests/test_templater.rs @@ -388,26 +388,130 @@ fn test_templater_separate_function() { render(r#"separate(author, "X", "Y", "Z")"#), @"X <>Y <>Z"); } +#[test] +fn test_templater_alias() { + 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 render = |template| get_template_output(&test_env, &repo_path, "@-", template); + let render_err = |template| test_env.jj_cmd_failure(&repo_path, &["log", "-T", template]); + + test_env.add_config( + r###" + [template-aliases] + 'my_commit_id' = 'commit_id.short()' + 'syntax_error' = 'foo.' + 'name_error' = 'unknown_id' + 'recurse' = 'recurse1' + 'recurse1' = 'recurse2()' + 'recurse2()' = 'recurse' + 'identity(x)' = 'x' + 'coalesce(x, y)' = 'if(x, x, y)' + "###, + ); + + insta::assert_snapshot!(render("my_commit_id"), @"000000000000"); + insta::assert_snapshot!(render("identity(my_commit_id)"), @"000000000000"); + + insta::assert_snapshot!(render_err("commit_id syntax_error"), @r###" + Error: Failed to parse template: --> 1:11 + | + 1 | commit_id syntax_error + | ^----------^ + | + = Alias "syntax_error" cannot be expanded + --> 1:5 + | + 1 | foo. + | ^--- + | + = expected identifier + "###); + + // TODO: outer template substitution should be reported too + insta::assert_snapshot!(render_err("commit_id name_error"), @r###" + Error: Failed to parse template: --> 1:1 + | + 1 | unknown_id + | ^--------^ + | + = Keyword "unknown_id" doesn't exist + "###); + + insta::assert_snapshot!(render_err("commit_id recurse"), @r###" + Error: Failed to parse template: --> 1:11 + | + 1 | commit_id recurse + | ^-----^ + | + = Alias "recurse" cannot be expanded + --> 1:1 + | + 1 | recurse1 + | ^------^ + | + = Alias "recurse1" cannot be expanded + --> 1:1 + | + 1 | recurse2() + | ^--------^ + | + = Alias "recurse2()" cannot be expanded + --> 1:1 + | + 1 | recurse + | ^-----^ + | + = Alias "recurse" expanded recursively + "###); + + insta::assert_snapshot!(render_err("identity()"), @r###" + Error: Failed to parse template: --> 1:10 + | + 1 | identity() + | ^ + | + = Expected 1 arguments + "###); + insta::assert_snapshot!(render_err("identity(commit_id, commit_id)"), @r###" + Error: Failed to parse template: --> 1:10 + | + 1 | identity(commit_id, commit_id) + | ^------------------^ + | + = Expected 1 arguments + "###); + + insta::assert_snapshot!(render_err(r#"coalesce(label("x", "not boolean"), "")"#), @r###" + Error: Failed to parse template: --> 1:10 + | + 1 | coalesce(label("x", "not boolean"), "") + | ^-----------------------^ + | + = Expected argument of type "Boolean" + "###); +} + #[test] fn test_templater_bad_alias_decl() { 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"); - // TODO: test alias substitution of parsable one test_env.add_config( r###" [template-aliases] 'badfn(a, a)' = 'a' + 'my_commit_id' = 'commit_id.short()' "###, ); // Invalid declaration should be warned and ignored. let assert = test_env - .jj_cmd(&repo_path, &["log", "--no-graph", "-r@-", "-Tcommit_id"]) + .jj_cmd(&repo_path, &["log", "--no-graph", "-r@-", "-Tmy_commit_id"]) .assert() .success(); - insta::assert_snapshot!(get_stdout_string(&assert), @"0000000000000000000000000000000000000000"); + insta::assert_snapshot!(get_stdout_string(&assert), @"000000000000"); insta::assert_snapshot!(get_stderr_string(&assert), @r###" Failed to load "template-aliases.badfn(a, a)": --> 1:7 |