From c894a7435f2e93aede4be5a8124f288b502c0edb Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 23 Apr 2021 10:37:42 -0700 Subject: [PATCH] revsets: make function arguments always be revset expressions Now that expressions may contain literal strings, we can simply have functions accept only expressions arguments. That simplifies both the grammar and the code. A small drawback is that `description((foo), bar)` is now allowed and does a search for the string "foo" (not "(foo)"). That seems unlikely to trip up users. --- lib/src/revset.pest | 13 +---- lib/src/revset.rs | 120 ++++++++++---------------------------------- 2 files changed, 28 insertions(+), 105 deletions(-) diff --git a/lib/src/revset.pest b/lib/src/revset.pest index 86e222f14..c0703c4f9 100644 --- a/lib/src/revset.pest +++ b/lib/src/revset.pest @@ -34,19 +34,8 @@ difference_op = { "-" } infix_op = _{ union_op | intersection_op | difference_op } function_name = @{ (ASCII_ALPHANUMERIC | "_")+ } -// The grammar accepts a string literal or an expression for function -// arguments. We then decide when walking the parse tree if we -// should interpret the string value of the literal string or the expression -// as a string or an expression, or maybe as an integer. For example, -// parents(foo) might interpret "foo" as an expression, while limit(*:foo,5) -// might interpret its second argument as an integer. Also, parents("foo") -// might be disallowed, just as description(heads()) might be. -function_argument = { - literal_string - | expression -} function_arguments = { - (whitespace* ~ function_argument ~ whitespace* ~ ",")* ~ whitespace* ~ function_argument ~ whitespace* + (whitespace* ~ expression ~ whitespace* ~ ",")* ~ whitespace* ~ expression ~ whitespace* | whitespace* } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index c09a53ea3..0136c8584 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -269,12 +269,9 @@ fn parse_function_expression( match name.as_str() { "parents" => { if arg_count == 1 { - Ok(RevsetExpression::Parents(Box::new( - parse_function_argument_to_expression( - &name, - argument_pairs.next().unwrap().into_inner(), - )?, - ))) + Ok(RevsetExpression::Parents(Box::new(parse_expression_rule( + argument_pairs.next().unwrap().into_inner(), + )?))) } else { Err(RevsetParseError::InvalidFunctionArguments { name, @@ -284,10 +281,8 @@ fn parse_function_expression( } "children" => { if arg_count == 1 { - let expression = parse_function_argument_to_expression( - &name, - argument_pairs.next().unwrap().into_inner(), - )?; + let expression = + parse_expression_rule(argument_pairs.next().unwrap().into_inner())?; Ok(RevsetExpression::Children { base_expression: Box::new(expression), candidates: RevsetExpression::non_obsolete_commits(), @@ -302,10 +297,7 @@ fn parse_function_expression( "ancestors" => { if arg_count == 1 { Ok(RevsetExpression::Ancestors(Box::new( - parse_function_argument_to_expression( - &name, - argument_pairs.next().unwrap().into_inner(), - )?, + parse_expression_rule(argument_pairs.next().unwrap().into_inner())?, ))) } else { Err(RevsetParseError::InvalidFunctionArguments { @@ -316,10 +308,8 @@ fn parse_function_expression( } "descendants" => { if arg_count == 1 { - let expression = parse_function_argument_to_expression( - &name, - argument_pairs.next().unwrap().into_inner(), - )?; + let expression = + parse_expression_rule(argument_pairs.next().unwrap().into_inner())?; Ok(RevsetExpression::Descendants { base_expression: Box::new(expression), candidates: RevsetExpression::non_obsolete_commits(), @@ -348,10 +338,7 @@ fn parse_function_expression( ))) } else if arg_count == 1 { Ok(RevsetExpression::NonObsoleteHeads(Box::new( - parse_function_argument_to_expression( - &name, - argument_pairs.next().unwrap().into_inner(), - )?, + parse_expression_rule(argument_pairs.next().unwrap().into_inner())?, ))) } else { Err(RevsetParseError::InvalidFunctionArguments { @@ -386,10 +373,7 @@ fn parse_function_expression( RevsetExpression::AllHeads, )))) } else { - parse_function_argument_to_expression( - &name, - argument_pairs.next().unwrap().into_inner(), - )? + parse_expression_rule(argument_pairs.next().unwrap().into_inner())? }; Ok(RevsetExpression::Description { needle, @@ -400,73 +384,23 @@ fn parse_function_expression( } } -fn parse_function_argument_to_expression( +fn parse_function_argument_to_string( name: &str, - mut pairs: Pairs, -) -> Result { - // Make a clone of the pairs for error messages - let pairs_clone = pairs.clone(); - let first = pairs.next().unwrap(); - assert!(pairs.next().is_none()); - match first.as_rule() { - Rule::expression => Ok(parse_expression_rule(first.into_inner())?), + pairs: Pairs, +) -> Result { + let expression = parse_expression_rule(pairs.clone())?; + match expression { + RevsetExpression::Symbol(symbol) => Ok(symbol), _ => Err(RevsetParseError::InvalidFunctionArguments { name: name.to_string(), message: format!( - "Expected function argument of type expression, found: {}", - pairs_clone.as_str() + "Expected function argument of type string, found: {}", + pairs.as_str() ), }), } } -fn parse_function_argument_to_string( - name: &str, - mut pairs: Pairs, -) -> Result { - // Make a clone of the pairs for error messages - let pairs_clone = pairs.clone(); - let first = pairs.next().unwrap(); - assert!(pairs.next().is_none()); - match first.as_rule() { - Rule::literal_string => { - return Ok(first - .as_str() - .strip_prefix('"') - .unwrap() - .strip_suffix('"') - .unwrap() - .to_owned()); - } - Rule::expression => { - let first = first.into_inner().next().unwrap(); - if first.as_rule() == Rule::infix_expression { - let first = first.into_inner().next().unwrap(); - if first.as_rule() == Rule::postfix_expression { - let first = first.into_inner().next().unwrap(); - if first.as_rule() == Rule::prefix_expression { - let first = first.into_inner().next().unwrap(); - if first.as_rule() == Rule::primary { - let first = first.into_inner().next().unwrap(); - if first.as_rule() == Rule::symbol { - return Ok(first.as_str().to_owned()); - } - } - } - } - } - } - _ => {} - } - Err(RevsetParseError::InvalidFunctionArguments { - name: name.to_string(), - message: format!( - "Expected function argument of type string, found: {}", - pairs_clone.as_str() - ), - }) -} - pub fn parse(revset_str: &str) -> Result { let mut pairs = RevsetParser::parse(Rule::expression, revset_str)?; let first = pairs.next().unwrap(); @@ -1055,10 +989,9 @@ mod tests { ); assert_eq!( parse("parents(\"@\")"), - Err(RevsetParseError::InvalidFunctionArguments { - name: "parents".to_string(), - message: "Expected function argument of type expression, found: \"@\"".to_string() - }) + Ok(RevsetExpression::Parents(Box::new( + RevsetExpression::Symbol("@".to_string()) + ))) ); assert_eq!( parse("ancestors(parents(@))"), @@ -1087,17 +1020,18 @@ mod tests { }) ); assert_eq!( - parse("description(foo(),bar)"), + parse("description(all_heads(),bar)"), Err(RevsetParseError::InvalidFunctionArguments { name: "description".to_string(), - message: "Expected function argument of type string, found: foo()".to_string() + message: "Expected function argument of type string, found: all_heads()" + .to_string() }) ); assert_eq!( parse("description((foo),bar)"), - Err(RevsetParseError::InvalidFunctionArguments { - name: "description".to_string(), - message: "Expected function argument of type string, found: (foo)".to_string() + Ok(RevsetExpression::Description { + needle: "foo".to_string(), + base_expression: Box::new(RevsetExpression::Symbol("bar".to_string())) }) ); assert_eq!(