From 26c182a0b0eef7381397aff15ca868a45c01e541 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 27 Feb 2024 20:37:00 +0900 Subject: [PATCH] templater: turn some integer overflow into evaluation error FWIW, this kind of errors can be checked at parsing phase if we implement constant folding. I don't think that would matter in practice, though. --- cli/src/commit_templater.rs | 12 +++------ cli/src/operation_templater.rs | 4 +-- cli/src/template_builder.rs | 45 +++++++++++++++++++--------------- cli/tests/test_log_command.rs | 4 +-- cli/tests/test_operations.rs | 9 +++---- 5 files changed, 37 insertions(+), 37 deletions(-) diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index db0412f03..b25f414b4 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -712,10 +712,10 @@ fn builtin_commit_or_change_id_methods<'repo>( map.insert("short", |language, build_ctx, self_property, function| { let ([], [len_node]) = template_parser::expect_arguments(function)?; let len_property = len_node - .map(|node| template_builder::expect_integer_expression(language, build_ctx, node)) + .map(|node| template_builder::expect_usize_expression(language, build_ctx, node)) .transpose()?; let out_property = TemplateFunction::new((self_property, len_property), |(id, len)| { - Ok(id.short(len.map_or(12, |l| l.try_into().unwrap_or(0)))) + Ok(id.short(len.unwrap_or(12))) }); Ok(language.wrap_string(out_property)) }); @@ -725,14 +725,10 @@ fn builtin_commit_or_change_id_methods<'repo>( let id_prefix_context = &language.id_prefix_context; let ([], [len_node]) = template_parser::expect_arguments(function)?; let len_property = len_node - .map(|node| template_builder::expect_integer_expression(language, build_ctx, node)) + .map(|node| template_builder::expect_usize_expression(language, build_ctx, node)) .transpose()?; let out_property = TemplateFunction::new((self_property, len_property), |(id, len)| { - Ok(id.shortest( - language.repo, - id_prefix_context, - len.and_then(|l| l.try_into().ok()).unwrap_or(0), - )) + Ok(id.shortest(language.repo, id_prefix_context, len.unwrap_or(0))) }); Ok(language.wrap_shortest_id_prefix(out_property)) }, diff --git a/cli/src/operation_templater.rs b/cli/src/operation_templater.rs index 58bfeb73b..e83db9f3f 100644 --- a/cli/src/operation_templater.rs +++ b/cli/src/operation_templater.rs @@ -238,11 +238,11 @@ fn builtin_operation_id_methods() -> OperationTemplateBuildMethodFnMap>( } UnaryOp::Negate => { let arg = expect_integer_expression(language, build_ctx, arg_node)?; - // TODO: propagate error on overflow? - language.wrap_integer(TemplateFunction::new(arg, |v| Ok(v.saturating_neg()))) + language.wrap_integer(TemplateFunction::new(arg, |v| { + v.checked_neg() + .ok_or_else(|| TemplatePropertyError("Attempt to negate with overflow".into())) + })) } }; Ok(Expression::unlabeled(property)) @@ -853,13 +856,11 @@ fn build_global_function<'a, L: TemplateLanguage<'a>>( let property = match function.name { "fill" => { let [width_node, content_node] = template_parser::expect_exact_arguments(function)?; - let width = expect_integer_expression(language, build_ctx, width_node)?; + let width = expect_usize_expression(language, build_ctx, width_node)?; let content = expect_template_expression(language, build_ctx, content_node)?; let template = ReformatTemplate::new(content, move |context, formatter, recorded| { match width.extract(context) { - Ok(width) => { - text_util::write_wrapped(formatter, recorded, width.try_into().unwrap_or(0)) - } + Ok(width) => text_util::write_wrapped(formatter, recorded, width), Err(err) => err.format(&(), formatter), } }); @@ -1029,6 +1030,17 @@ pub fn expect_integer_expression<'a, L: TemplateLanguage<'a>>( .ok_or_else(|| TemplateParseError::expected_type("Integer", node.span)) } +/// If the given expression `node` is of `Integer` type, converts it to `usize`. +pub fn expect_usize_expression<'a, L: TemplateLanguage<'a>>( + language: &L, + build_ctx: &BuildContext, + node: &ExpressionNode, +) -> TemplateParseResult + 'a>> { + let i64_property = expect_integer_expression(language, build_ctx, node)?; + let usize_property = TemplateFunction::new(i64_property, |v| Ok(usize::try_from(v)?)); + Ok(Box::new(usize_property)) +} + pub fn expect_plain_text_expression<'a, L: TemplateLanguage<'a>>( language: &L, build_ctx: &BuildContext, @@ -1490,8 +1502,10 @@ mod tests { insta::assert_snapshot!(env.render_ok(r#"--2"#), @"2"); insta::assert_snapshot!(env.render_ok(r#"-(3)"#), @"-3"); - // No panic on integer overflow. Runtime error might be better. - insta::assert_snapshot!(env.render_ok(r#"-i64_min"#), @"9223372036854775807"); + // No panic on integer overflow. + insta::assert_snapshot!( + env.render_ok(r#"-i64_min"#), + @""); } #[test] @@ -1866,20 +1880,11 @@ mod tests { dog "###); - // Filling to negatives are clamped to the same as zero + // Filling to negative width is an error insta::assert_snapshot!( env.render_ok(r#"fill(-10, "The quick fox jumps over the " ++ label("error", "lazy") ++ " dog\n")"#), - @r###" - The - quick - fox - jumps - over - the - lazy - dog - "###); + @""); // Word-wrap, then indent insta::assert_snapshot!( diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index 1dd598e9d..b0997b68d 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -647,8 +647,8 @@ fn test_log_short_shortest_length_parameter() { "###); insta::assert_snapshot!( render(r#"commit_id.short(-100) ++ "|" ++ commit_id.shortest(-100)"#), @r###" - @ |2 - ◉ |0 + @ | + ◉ | "###); insta::assert_snapshot!( render(r#"commit_id.short(100) ++ "|" ++ commit_id.shortest(100)"#), @r###" diff --git a/cli/tests/test_operations.rs b/cli/tests/test_operations.rs index c4e56ad46..d60694a8c 100644 --- a/cli/tests/test_operations.rs +++ b/cli/tests/test_operations.rs @@ -163,12 +163,11 @@ fn test_op_log_template() { ◉ 00000 false @ 1970-01-01 00:00:00.000 +00:00 1970-01-01 00:00:00.000 +00:00 less than a microsecond "###); - // Negative length shouldn't cause panic (and is clamped.) - // TODO: If we add runtime error, this will probably error out. + // Negative length shouldn't cause panic. insta::assert_snapshot!(render(r#"id.short(-1) ++ "|""#), @r###" - @ | - ◉ | - ◉ | + @ | + ◉ | + ◉ | "###); // Test the default template, i.e. with relative start time and duration. We