From 3124444d24b3c16f4fbcead97c4c31284a716f7c Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 7 Mar 2023 19:55:28 +0900 Subject: [PATCH] templater: add list.map(|x| ...) operation This involves a little hack to insert a lambda parameter 'x' to be used at keyword position. If the template language were dynamically typed (and were interpreted), .map() implementation would be simpler. I considered that, but interpreter version has its own warts (late error reporting, uneasy to cache static object, etc.), and I don't think the current template engine is complex enough to rewrite from scratch. .map() returns template, which can't be join()-ed. This will be fixed later. --- docs/templates.md | 2 + src/commit_templater.rs | 4 +- src/template_builder.rs | 89 ++++++++++++++++++++++++++++++----- src/template_parser.rs | 22 +++++++++ src/templater.rs | 49 +++++++++++++++++++ tests/test_commit_template.rs | 11 +++++ tests/test_templater.rs | 75 +++++++++++++++++++++++++++++ 7 files changed, 240 insertions(+), 12 deletions(-) diff --git a/docs/templates.md b/docs/templates.md index e7ebc5a89..a90429558 100644 --- a/docs/templates.md +++ b/docs/templates.md @@ -92,6 +92,8 @@ The following methods are defined. * `.join(separator: Template) -> Template`: Concatenate elements with the given `separator`. +* `.map(|item| expression) -> Template`: Apply template `expression` + to each element. Example: `parent_commit_ids.map(|id| id.short())` ### OperationId type diff --git a/src/commit_templater.rs b/src/commit_templater.rs index 368480f50..656352c12 100644 --- a/src/commit_templater.rs +++ b/src/commit_templater.rs @@ -65,7 +65,9 @@ impl<'repo> TemplateLanguage<'repo> for CommitTemplateLanguage<'repo, '_> { build_commit_or_change_id_method(self, build_ctx, property, function) } CommitTemplatePropertyKind::CommitOrChangeIdList(property) => { - template_builder::build_list_method(self, build_ctx, property, function) + template_builder::build_list_method(self, build_ctx, property, function, |item| { + self.wrap_commit_or_change_id(item) + }) } CommitTemplatePropertyKind::ShortestIdPrefix(property) => { build_shortest_id_prefix_method(self, build_ctx, property, function) diff --git a/src/template_builder.rs b/src/template_builder.rs index 19bf9d1e8..7f69dad67 100644 --- a/src/template_builder.rs +++ b/src/template_builder.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::HashMap; + use itertools::Itertools as _; use jujutsu_lib::backend::{Signature, Timestamp}; @@ -21,8 +23,8 @@ use crate::template_parser::{ }; use crate::templater::{ ConcatTemplate, ConditionalTemplate, IntoTemplate, LabelTemplate, ListPropertyTemplate, - Literal, PlainTextFormattedProperty, ReformatTemplate, SeparateTemplate, Template, - TemplateFunction, TemplateProperty, TimestampRange, + Literal, PlainTextFormattedProperty, PropertyPlaceholder, ReformatTemplate, SeparateTemplate, + Template, TemplateFunction, TemplateProperty, TimestampRange, }; use crate::{text_util, time_util}; @@ -237,7 +239,8 @@ impl<'a, C: 'a, P: IntoTemplate<'a, C>> IntoTemplate<'a, C> for Expression

{ } pub struct BuildContext<'i, P> { - _phantom: std::marker::PhantomData<&'i P>, // TODO + /// Map of functions to create `L::Property`. + local_variables: HashMap<&'i str, &'i (dyn Fn() -> P)>, } fn build_method_call<'a, L: TemplateLanguage<'a>>( @@ -263,7 +266,9 @@ pub fn build_core_method<'a, L: TemplateLanguage<'a>>( build_string_method(language, build_ctx, property, function) } CoreTemplatePropertyKind::StringList(property) => { - build_list_method(language, build_ctx, property, function) + build_list_method(language, build_ctx, property, function, |item| { + language.wrap_string(item) + }) } CoreTemplatePropertyKind::Boolean(property) => { build_boolean_method(language, build_ctx, property, function) @@ -450,12 +455,20 @@ fn build_timestamp_range_method<'a, L: TemplateLanguage<'a>>( Ok(property) } -pub fn build_list_method<'a, L: TemplateLanguage<'a>, P: Template<()> + 'a>( +/// Builds method call expression for printable list property. +pub fn build_list_method<'a, L, O>( language: &L, build_ctx: &BuildContext, - self_property: impl TemplateProperty> + 'a, + self_property: impl TemplateProperty> + 'a, function: &FunctionCallNode, -) -> TemplateParseResult { + // TODO: Generic L: WrapProperty trait might be needed to support more + // list operations such as first()/slice(). For .map(), a simple callback works. + wrap_item: impl Fn(PropertyPlaceholder) -> L::Property, +) -> TemplateParseResult +where + L: TemplateLanguage<'a>, + O: Template<()> + Clone + 'a, +{ let property = match function.name { "join" => { let [separator_node] = template_parser::expect_exact_arguments(function)?; @@ -466,12 +479,61 @@ pub fn build_list_method<'a, L: TemplateLanguage<'a>, P: Template<()> + 'a>( }); language.wrap_template(template) } - // TODO: .map() + "map" => build_map_operation(language, build_ctx, self_property, function, wrap_item)?, _ => return Err(TemplateParseError::no_such_method("List", function)), }; Ok(property) } +/// Builds expression that extracts iterable property and applies template to +/// each item. +/// +/// `wrap_item()` is the function to wrap a list item of type `O` as a property. +fn build_map_operation<'a, L, O, P>( + language: &L, + build_ctx: &BuildContext, + self_property: P, + function: &FunctionCallNode, + wrap_item: impl Fn(PropertyPlaceholder) -> L::Property, +) -> TemplateParseResult +where + L: TemplateLanguage<'a>, + P: TemplateProperty + 'a, + P::Output: IntoIterator, + O: Clone + 'a, +{ + // Build an item template with placeholder property, then evaluate it + // for each item. + // + // It would be nice if we could build a template of (L::Context, O) + // input, but doing that for a generic item type wouldn't be easy. It's + // also invalid to convert &C to &(C, _). + let [lambda_node] = template_parser::expect_exact_arguments(function)?; + let item_placeholder = PropertyPlaceholder::new(); + let item_template = template_parser::expect_lambda_with(lambda_node, |lambda, _span| { + let item_fn = || wrap_item(item_placeholder.clone()); + let mut local_variables = build_ctx.local_variables.clone(); + if let [name] = lambda.params.as_slice() { + local_variables.insert(name, &item_fn); + } else { + return Err(TemplateParseError::unexpected_expression( + "Expected 1 lambda parameters", + lambda.params_span, + )); + } + let build_ctx = BuildContext { local_variables }; + Ok(build_expression(language, &build_ctx, &lambda.body)?.into_template()) + })?; + let list_template = ListPropertyTemplate::new( + self_property, + Literal(" "), // separator + move |context, formatter, item| { + item_placeholder.with_value(item, || item_template.format(context, formatter)) + }, + ); + Ok(language.wrap_template(list_template)) +} + fn build_global_function<'a, L: TemplateLanguage<'a>>( language: &L, build_ctx: &BuildContext, @@ -558,8 +620,13 @@ pub fn build_expression<'a, L: TemplateLanguage<'a>>( ) -> TemplateParseResult> { match &node.kind { ExpressionKind::Identifier(name) => { - let property = language.build_keyword(name, node.span)?; - Ok(Expression::with_label(property, *name)) + if let Some(make) = build_ctx.local_variables.get(name) { + // Don't label a local variable with its name + Ok(Expression::unlabeled(make())) + } else { + let property = language.build_keyword(name, node.span)?; + Ok(Expression::with_label(property, *name)) + } } ExpressionKind::Integer(value) => { let property = language.wrap_integer(Literal(*value)); @@ -596,7 +663,7 @@ pub fn build<'a, L: TemplateLanguage<'a>>( node: &ExpressionNode, ) -> TemplateParseResult + 'a>> { let build_ctx = BuildContext { - _phantom: std::marker::PhantomData, + local_variables: HashMap::new(), }; let expression = build_expression(language, &build_ctx, node)?; Ok(expression.into_template()) diff --git a/src/template_parser.rs b/src/template_parser.rs index cbe917acf..22aa669f2 100644 --- a/src/template_parser.rs +++ b/src/template_parser.rs @@ -689,6 +689,28 @@ pub fn expect_string_literal_with<'a, 'i, T>( } } +/// Applies the given function if the `node` is a lambda. +pub fn expect_lambda_with<'a, 'i, T>( + node: &'a ExpressionNode<'i>, + f: impl FnOnce(&'a LambdaNode<'i>, pest::Span<'i>) -> TemplateParseResult, +) -> TemplateParseResult { + match &node.kind { + ExpressionKind::Lambda(lambda) => f(lambda, node.span), + ExpressionKind::String(_) + | ExpressionKind::Identifier(_) + | ExpressionKind::Integer(_) + | ExpressionKind::Concat(_) + | ExpressionKind::FunctionCall(_) + | ExpressionKind::MethodCall(_) => Err(TemplateParseError::unexpected_expression( + "Expected lambda expression", + node.span, + )), + ExpressionKind::AliasExpanded(id, subst) => { + expect_lambda_with(subst, f).map_err(|e| e.within_alias_expansion(*id, node.span)) + } + } +} + #[cfg(test)] mod tests { use assert_matches::assert_matches; diff --git a/src/templater.rs b/src/templater.rs index 1d10727ab..e8f24e851 100644 --- a/src/templater.rs +++ b/src/templater.rs @@ -12,7 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::cell::RefCell; use std::io; +use std::rc::Rc; use jujutsu_lib::backend::{Signature, Timestamp}; @@ -477,6 +479,53 @@ where } } +/// Property which will be compiled into template once, and substituted later. +#[derive(Clone, Debug)] +pub struct PropertyPlaceholder { + value: Rc>>, +} + +impl PropertyPlaceholder { + pub fn new() -> Self { + PropertyPlaceholder { + value: Rc::new(RefCell::new(None)), + } + } + + pub fn set(&self, value: O) { + *self.value.borrow_mut() = Some(value); + } + + pub fn take(&self) -> Option { + self.value.borrow_mut().take() + } + + pub fn with_value(&self, value: O, f: impl FnOnce() -> R) -> R { + self.set(value); + let result = f(); + self.take(); + result + } +} + +impl Default for PropertyPlaceholder { + fn default() -> Self { + Self::new() + } +} + +impl TemplateProperty for PropertyPlaceholder { + type Output = O; + + fn extract(&self, _: &C) -> Self::Output { + self.value + .borrow() + .as_ref() + .expect("placeholder value must be set before evaluating template") + .clone() + } +} + pub fn format_joined( context: &C, formatter: &mut dyn Formatter, diff --git a/tests/test_commit_template.rs b/tests/test_commit_template.rs index e40c6215e..7b7b8cc07 100644 --- a/tests/test_commit_template.rs +++ b/tests/test_commit_template.rs @@ -39,6 +39,17 @@ fn test_log_parent_commit_ids() { ● 0000000000000000000000000000000000000000 P: "###); + + let template = r#"parent_commit_ids.map(|id| id.shortest(4))"#; + let stdout = test_env.jj_cmd_success( + &repo_path, + &["log", "-T", template, "-r@", "--color=always"], + ); + insta::assert_snapshot!(stdout, @r###" + @ 4db4 230d + │ + ~ + "###); } #[test] diff --git a/tests/test_templater.rs b/tests/test_templater.rs index f03fc5eb2..9f171379b 100644 --- a/tests/test_templater.rs +++ b/tests/test_templater.rs @@ -251,11 +251,86 @@ fn test_templater_list_method() { 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] + 'identity' = '|x| x' + 'too_many_params' = '|x, y| x' + "###, + ); insta::assert_snapshot!(render(r#""".lines().join("|")"#), @""); insta::assert_snapshot!(render(r#""a\nb\nc".lines().join("|")"#), @"a|b|c"); // Keyword as separator insta::assert_snapshot!(render(r#""a\nb\nc".lines().join(commit_id.short(2))"#), @"a00b00c"); + + insta::assert_snapshot!(render(r#""a\nb\nc".lines().map(|s| s ++ s)"#), @"aa bb cc"); + // Global keyword in item template + insta::assert_snapshot!( + render(r#""a\nb\nc".lines().map(|s| s ++ empty)"#), @"atrue btrue ctrue"); + // Override global keyword 'empty' + insta::assert_snapshot!( + render(r#""a\nb\nc".lines().map(|empty| empty)"#), @"a b c"); + // Nested map operations + insta::assert_snapshot!( + render(r#""a\nb\nc".lines().map(|s| "x\ny".lines().map(|t| s ++ t))"#), + @"ax ay bx by cx cy"); + + // Lambda expression in alias + insta::assert_snapshot!(render(r#""a\nb\nc".lines().map(identity)"#), @"a b c"); + + // Not a lambda expression + insta::assert_snapshot!(render_err(r#""a".lines().map(empty)"#), @r###" + Error: Failed to parse template: --> 1:17 + | + 1 | "a".lines().map(empty) + | ^---^ + | + = Expected lambda expression + "###); + // Bad lambda parameter count + insta::assert_snapshot!(render_err(r#""a".lines().map(|| "")"#), @r###" + Error: Failed to parse template: --> 1:18 + | + 1 | "a".lines().map(|| "") + | ^ + | + = Expected 1 lambda parameters + "###); + insta::assert_snapshot!(render_err(r#""a".lines().map(|a, b| "")"#), @r###" + Error: Failed to parse template: --> 1:18 + | + 1 | "a".lines().map(|a, b| "") + | ^--^ + | + = Expected 1 lambda parameters + "###); + // Error in lambda expression + insta::assert_snapshot!(render_err(r#""a".lines().map(|s| s.unknown())"#), @r###" + Error: Failed to parse template: --> 1:23 + | + 1 | "a".lines().map(|s| s.unknown()) + | ^-----^ + | + = Method "unknown" doesn't exist for type "String" + "###); + // Error in lambda alias + insta::assert_snapshot!(render_err(r#""a".lines().map(too_many_params)"#), @r###" + Error: Failed to parse template: --> 1:17 + | + 1 | "a".lines().map(too_many_params) + | ^-------------^ + | + = Alias "too_many_params" cannot be expanded + --> 1:2 + | + 1 | |x, y| x + | ^--^ + | + = Expected 1 lambda parameters + "###); } #[test]