From eda7069aeee4b6f25cfcb764ab393de3f63df785 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Mon, 3 Jun 2024 16:26:52 +0900 Subject: [PATCH] revset: represent program modifier as AST node The goal is to remove special case from parsing functions and provide slightly better error message. I don't know if we'd want to use "all:" in aliases, but there are no strong reasons to disable it. --- CHANGELOG.md | 4 +++ cli/tests/test_revset_output.rs | 21 +++++------ lib/src/revset.rs | 62 +++++++++++++++++++++------------ lib/src/revset_parser.rs | 52 ++++++++++++++++++--------- 4 files changed, 88 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d5ea6778..cc66e89ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Breaking changes +* In revset aliases, top-level `kind:pattern` expression is now parsed as + modifier. Surround with parentheses if it should be parsed as string/file + pattern. + ### Deprecations ### New features diff --git a/cli/tests/test_revset_output.rs b/cli/tests/test_revset_output.rs index f5b4361ca..85556ec61 100644 --- a/cli/tests/test_revset_output.rs +++ b/cli/tests/test_revset_output.rs @@ -549,29 +549,26 @@ fn test_all_modifier() { = Modifier "ale" doesn't exist "###); - // Modifier shouldn't be allowed in aliases (This restriction might be - // lifted later. "all:" in alias could be allowed if it is expanded to the - // top-level expression.) + // Modifier shouldn't be allowed in sub expression let stderr = test_env.jj_cmd_failure( &repo_path, - &["new", "x", "--config-toml=revset-aliases.x='all:@'"], + &["new", "x..", "--config-toml=revset-aliases.x='all:@'"], ); insta::assert_snapshot!(stderr, @r###" Error: Failed to parse revset: Alias "x" cannot be expanded Caused by: 1: --> 1:1 | - 1 | x + 1 | x.. | ^ | = Alias "x" cannot be expanded - 2: --> 1:4 + 2: --> 1:1 | 1 | all:@ - | ^ + | ^-^ | - = ':' is not an infix operator - Hint: Did you mean '::' for DAG range? + = Modifier "all:" is not allowed in sub expression "###); // immutable_heads() alias may be parsed as a top-level expression, but @@ -586,12 +583,12 @@ fn test_all_modifier() { ); insta::assert_snapshot!(stderr, @r###" Config error: Invalid `revset-aliases.immutable_heads()` - Caused by: --> 1:4 + Caused by: --> 1:1 | 1 | all:@ - | ^ + | ^-^ | - = ':' is not an infix operator + = Modifier "all:" is not allowed in sub expression For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md. "###); } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 479092ac8..cdacad54f 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -830,6 +830,13 @@ pub fn lower_expression( } } ExpressionKind::FunctionCall(function) => lower_function_call(function, context), + ExpressionKind::Modifier(modifier) => { + let name = modifier.name; + Err(RevsetParseError::expression( + format!(r#"Modifier "{name}:" is not allowed in sub expression"#), + modifier.name_span, + )) + } ExpressionKind::AliasExpanded(id, subst) => { lower_expression(subst, context).map_err(|e| e.within_alias_expansion(*id, node.span)) } @@ -850,20 +857,20 @@ pub fn parse_with_modifier( revset_str: &str, context: &RevsetParseContext, ) -> Result<(Rc, Option), RevsetParseError> { - let (node, modifier) = revset_parser::parse_program_with_modifier(revset_str)?; - let modifier = modifier - .map(|n| match n.name { - "all" => Ok(RevsetModifier::All), - name => Err(RevsetParseError::with_span( - RevsetParseErrorKind::NoSuchModifier(name.to_owned()), - n.name_span, - )), - }) - .transpose()?; + let node = revset_parser::parse_program(revset_str)?; let node = dsl_util::expand_aliases(node, context.aliases_map)?; - let expression = lower_expression(&node, context) - .map_err(|err| err.extend_function_candidates(context.aliases_map.function_names()))?; - Ok((expression, modifier)) + revset_parser::expect_program_with( + &node, + |node| lower_expression(node, context), + |name, span| match name { + "all" => Ok(RevsetModifier::All), + _ => Err(RevsetParseError::with_span( + RevsetParseErrorKind::NoSuchModifier(name.to_owned()), + span, + )), + }, + ) + .map_err(|err| err.extend_function_candidates(context.aliases_map.function_names())) } /// `Some` for rewritten expression, or `None` to reuse the original expression. @@ -2605,10 +2612,6 @@ mod tests { ); // String pattern isn't allowed at top level. - assert_matches!( - parse(r#"exact:"foo""#), - Err(RevsetParseErrorKind::NotInfixOperator { .. }) - ); assert_matches!( parse(r#"(exact:"foo")"#), Err(RevsetParseErrorKind::NotInfixOperator { .. }) @@ -2902,8 +2905,10 @@ mod tests { parse_with_aliases("author(A)", [("A", "a")]).unwrap(), parse("author(a)").unwrap() ); + // However, parentheses are required because top-level x:y is parsed as + // program modifier. assert_eq!( - parse_with_aliases("author(A)", [("A", "exact:a")]).unwrap(), + parse_with_aliases("author(A)", [("A", "(exact:a)")]).unwrap(), parse("author(exact:a)").unwrap() ); @@ -2933,6 +2938,16 @@ mod tests { parse_with_modifier("all:ALL").unwrap() ); + // Top-level alias can be substituted to modifier expression. + assert_eq!( + parse_with_aliases_and_modifier("A", [("A", "all:a")]), + parse_with_modifier("all:a") + ); + assert_eq!( + parse_with_aliases_and_modifier("A-", [("A", "all:a")]), + Err(RevsetParseErrorKind::BadAliasExpansion("A".to_owned())) + ); + // Multi-level substitution. assert_eq!( parse_with_aliases("A", [("A", "BC"), ("BC", "b|C"), ("C", "c")]).unwrap(), @@ -2954,11 +2969,6 @@ mod tests { parse_with_aliases("A", [("A", "a(")]), Err(RevsetParseErrorKind::BadAliasExpansion("A".to_owned())) ); - // Modifier isn't allowed in alias definition. - assert_eq!( - parse_with_aliases_and_modifier("A", [("A", "all:a")]), - Err(RevsetParseErrorKind::BadAliasExpansion("A".to_owned())) - ); } #[test] @@ -3016,6 +3026,12 @@ mod tests { parse("author(a)|committer(a)").unwrap() ); + // Modifier expression body as parameter. + assert_eq!( + parse_with_aliases_and_modifier("F(a|b)", [("F(x)", "all:x")]).unwrap(), + parse_with_modifier("all:(a|b)").unwrap() + ); + // Function and symbol aliases reside in separate namespaces. assert_eq!( parse_with_aliases("A()", [("A()", "A"), ("A", "a")]).unwrap(), diff --git a/lib/src/revset_parser.rs b/lib/src/revset_parser.rs index c80c1456c..752505397 100644 --- a/lib/src/revset_parser.rs +++ b/lib/src/revset_parser.rs @@ -314,6 +314,8 @@ pub enum ExpressionKind<'i> { Unary(UnaryOp, Box>), Binary(BinaryOp, Box>, Box>), FunctionCall(Box>), + /// `name: body` + Modifier(Box>), /// Identity node to preserve the span in the source text. AliasExpanded(AliasId<'i>, Box>), } @@ -342,6 +344,14 @@ impl<'i> FoldableExpression<'i> for ExpressionKind<'i> { Ok(ExpressionKind::Binary(op, lhs, rhs)) } ExpressionKind::FunctionCall(function) => folder.fold_function_call(function, span), + ExpressionKind::Modifier(modifier) => { + let modifier = Box::new(ModifierNode { + name: modifier.name, + name_span: modifier.name_span, + body: folder.fold_expression(modifier.body)?, + }); + Ok(ExpressionKind::Modifier(modifier)) + } ExpressionKind::AliasExpanded(id, subst) => { let subst = Box::new(folder.fold_expression(*subst)?); Ok(ExpressionKind::AliasExpanded(id, subst)) @@ -399,42 +409,36 @@ pub enum BinaryOp { pub type ExpressionNode<'i> = dsl_util::ExpressionNode<'i, ExpressionKind<'i>>; pub type FunctionCallNode<'i> = dsl_util::FunctionCallNode<'i, ExpressionKind<'i>>; +/// Expression with modifier `name: body`. #[derive(Clone, Debug, Eq, PartialEq)] pub struct ModifierNode<'i> { /// Modifier name. pub name: &'i str, /// Span of the modifier name. pub name_span: pest::Span<'i>, + /// Expression body. + pub body: ExpressionNode<'i>, } pub(super) fn parse_program(revset_str: &str) -> Result { - let mut pairs = RevsetParser::parse(Rule::program, revset_str)?; - let first = pairs.next().unwrap(); - parse_expression_node(first.into_inner()) -} - -pub(super) fn parse_program_with_modifier( - revset_str: &str, -) -> Result<(ExpressionNode, Option), RevsetParseError> { let mut pairs = RevsetParser::parse(Rule::program_with_modifier, revset_str)?; let first = pairs.next().unwrap(); match first.as_rule() { - Rule::expression => { - let node = parse_expression_node(first.into_inner())?; - Ok((node, None)) - } + Rule::expression => parse_expression_node(first.into_inner()), Rule::program_modifier => { let (lhs, op) = first.into_inner().collect_tuple().unwrap(); let rhs = pairs.next().unwrap(); assert_eq!(lhs.as_rule(), Rule::identifier); assert_eq!(op.as_rule(), Rule::pattern_kind_op); assert_eq!(rhs.as_rule(), Rule::expression); - let modifier = ModifierNode { + let span = lhs.as_span().start_pos().span(&rhs.as_span().end_pos()); + let modifier = Box::new(ModifierNode { name: lhs.as_str(), name_span: lhs.as_span(), - }; - let node = parse_expression_node(rhs.into_inner())?; - Ok((node, Some(modifier))) + body: parse_expression_node(rhs.into_inner())?, + }); + let expr = ExpressionKind::Modifier(modifier); + Ok(ExpressionNode::new(expr, span)) } r => panic!("unexpected revset parse rule: {r:?}"), } @@ -728,6 +732,22 @@ impl AliasDefinitionParser for RevsetAliasParser { } } +/// Applies the given functions to the top-level expression body node with an +/// optional modifier. Alias expansion nodes are unwrapped accordingly. +pub(super) fn expect_program_with( + node: &ExpressionNode, + parse_body: impl FnOnce(&ExpressionNode) -> Result, + parse_modifier: impl FnOnce(&str, pest::Span<'_>) -> Result, +) -> Result<(B, Option), RevsetParseError> { + expect_literal_with(node, |node| match &node.kind { + ExpressionKind::Modifier(modifier) => { + let parsed_modifier = parse_modifier(modifier.name, modifier.name_span)?; + Ok((parse_body(&modifier.body)?, Some(parsed_modifier))) + } + _ => Ok((parse_body(node)?, None)), + }) +} + pub(super) fn expect_pattern_with>>( type_name: &str, node: &ExpressionNode,