From 038497638f82afa0a81e261e9149078eb3cfd9e2 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 8 Feb 2023 18:55:15 +0900 Subject: [PATCH] revset: parse keyword arguments, accept remote_branches(remote=needle) The syntax is identical to Mercurial's revset, which is derived from Python. --- docs/revsets.md | 6 +- lib/src/revset.pest | 4 +- lib/src/revset.rs | 207 ++++++++++++++++++++++++++++-------- tests/test_revset_output.rs | 44 ++++++++ 4 files changed, 211 insertions(+), 50 deletions(-) diff --git a/docs/revsets.md b/docs/revsets.md index 7a9bbe6a9..070e10087 100644 --- a/docs/revsets.md +++ b/docs/revsets.md @@ -90,8 +90,8 @@ revsets (expressions) as arguments. `branches(push)` would match the branches `push-123` and `repushed` but not the branch `main`. If a branch is in a conflicted state, all its possible targets are included. -* `remote_branches([branch_needle[, remote_needle]])`: All remote branch - targets across all remotes. If just the `branch_needle` is specified, +* `remote_branches([branch_needle[, [remote=]remote_needle]])`: All remote + branch targets across all remotes. If just the `branch_needle` is specified, branches whose name contains the given string across all remotes are selected. If both `branch_needle` and `remote_needle` are specified, the selection is further restricted to just the remotes whose name contains @@ -151,7 +151,7 @@ jj log -r 'remote_branches()..' Show commits not on `origin` (if you have other remotes like `fork`): ``` -jj log -r 'remote_branches("", origin)..' +jj log -r 'remote_branches(remote=origin)..' ``` Show all ancestors of the working copy (almost like plain `git log`) diff --git a/lib/src/revset.pest b/lib/src/revset.pest index 810e1945c..100441090 100644 --- a/lib/src/revset.pest +++ b/lib/src/revset.pest @@ -47,8 +47,10 @@ compat_sub_op = { "-" } infix_op = _{ union_op | intersection_op | difference_op | compat_add_op | compat_sub_op } function_name = @{ (ASCII_ALPHANUMERIC | "_")+ } +keyword_argument = { identifier ~ whitespace* ~ "=" ~ whitespace* ~ expression } +argument = _{ keyword_argument | expression } function_arguments = { - expression ~ (whitespace* ~ "," ~ whitespace* ~ expression)* ~ (whitespace* ~ ",")? + argument ~ (whitespace* ~ "," ~ whitespace* ~ argument)* ~ (whitespace* ~ ",")? | "" } formal_parameters = { diff --git a/lib/src/revset.rs b/lib/src/revset.rs index dc02ce52c..aaaade163 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -15,7 +15,7 @@ use std::borrow::Borrow; use std::cmp::{Ordering, Reverse}; use std::collections::{HashMap, HashSet}; -use std::iter::{self, Peekable}; +use std::iter::Peekable; use std::ops::Range; use std::path::Path; use std::rc::Rc; @@ -846,25 +846,17 @@ fn parse_function_expression( if let Some((id, params, defn)) = state.aliases_map.get_function(name) { // Resolve arguments in the current scope, and pass them in to the alias // expansion scope. - let arguments_span = arguments_pair.as_span(); - let args: Vec<_> = arguments_pair - .into_inner() + let (required, optional) = + expect_named_arguments_vec(name, &[], arguments_pair, params.len(), params.len())?; + assert!(optional.is_empty()); + let args: Vec<_> = required + .into_iter() .map(|arg| parse_expression_rule(arg.into_inner(), state)) .try_collect()?; - if params.len() == args.len() { - let locals = params.iter().map(|s| s.as_str()).zip(args).collect(); - state.with_alias_expanding(id, &locals, primary_span, |state| { - parse_program(defn, state) - }) - } else { - Err(RevsetParseError::with_span( - RevsetParseErrorKind::InvalidFunctionArguments { - name: name.to_owned(), - message: format!("Expected {} arguments", params.len()), - }, - arguments_span, - )) - } + let locals = params.iter().map(|s| s.as_str()).zip(args).collect(); + state.with_alias_expanding(id, &locals, primary_span, |state| { + parse_program(defn, state) + }) } else { parse_builtin_function(name_pair, arguments_pair, state) } @@ -938,7 +930,8 @@ fn parse_builtin_function( Ok(RevsetExpression::branches(needle)) } "remote_branches" => { - let ([], [branch_opt_arg, remote_opt_arg]) = expect_arguments(name, arguments_pair)?; + let ([], [branch_opt_arg, remote_opt_arg]) = + expect_named_arguments(name, &["", "remote"], arguments_pair)?; let branch_needle = if let Some(branch_arg) = branch_opt_arg { parse_function_argument_to_string(name, branch_arg, state)? } else { @@ -1064,46 +1057,108 @@ fn expect_one_argument<'i>( Ok(arg) } -/// Extracts N required arguments and M optional arguments. fn expect_arguments<'i, const N: usize, const M: usize>( function_name: &str, arguments_pair: Pair<'i, Rule>, ) -> Result<([Pair<'i, Rule>; N], [OptionalArg<'i>; M]), RevsetParseError> { + expect_named_arguments(function_name, &[], arguments_pair) +} + +/// Extracts N required arguments and M optional arguments. +/// +/// `argument_names` is a list of argument names. Unnamed positional arguments +/// should be padded with `""`. +fn expect_named_arguments<'i, const N: usize, const M: usize>( + function_name: &str, + argument_names: &[&str], + arguments_pair: Pair<'i, Rule>, +) -> Result<([Pair<'i, Rule>; N], [OptionalArg<'i>; M]), RevsetParseError> { + let (required, optional) = + expect_named_arguments_vec(function_name, argument_names, arguments_pair, N, N + M)?; + Ok((required.try_into().unwrap(), optional.try_into().unwrap())) +} + +fn expect_named_arguments_vec<'i>( + function_name: &str, + argument_names: &[&str], + arguments_pair: Pair<'i, Rule>, + min_arg_count: usize, + max_arg_count: usize, +) -> Result<(Vec>, Vec>), RevsetParseError> { + assert!(argument_names.len() <= max_arg_count); let arguments_span = arguments_pair.as_span(); - let make_error = || { - let message = if M == 0 { - format!("Expected {N} arguments") - } else { - format!("Expected {N} to {max} arguments", max = N + M) - }; + let make_error = |message, span| { RevsetParseError::with_span( RevsetParseErrorKind::InvalidFunctionArguments { name: function_name.to_owned(), message, }, - arguments_span, + span, ) }; - let mut argument_pairs = arguments_pair.into_inner().fuse(); - let required: [Pair; N] = argument_pairs - .by_ref() - .take(N) - .collect_vec() - .try_into() - .map_err(|_| make_error())?; - let optional: [OptionalArg; M] = argument_pairs - .by_ref() - .map(Some) - .chain(iter::repeat(None)) - .take(M) - .collect_vec() - .try_into() - .unwrap(); - if argument_pairs.next().is_none() { - Ok((required, optional)) - } else { - Err(make_error()) + let make_count_error = || { + let message = if min_arg_count == max_arg_count { + format!("Expected {min_arg_count} arguments") + } else { + format!("Expected {min_arg_count} to {max_arg_count} arguments") + }; + make_error(message, arguments_span) + }; + + let mut pos_iter = Some(0..max_arg_count); + let mut extracted_pairs = vec![None; max_arg_count]; + for pair in arguments_pair.into_inner() { + let span = pair.as_span(); + match pair.as_rule() { + Rule::expression => { + let pos = pos_iter + .as_mut() + .ok_or_else(|| { + make_error( + "Positional argument follows keyword argument".to_owned(), + span, + ) + })? + .next() + .ok_or_else(make_count_error)?; + assert!(extracted_pairs[pos].is_none()); + extracted_pairs[pos] = Some(pair); + } + Rule::keyword_argument => { + pos_iter = None; // No more positional arguments + let mut pairs = pair.into_inner(); + let name = pairs.next().unwrap(); + let expr = pairs.next().unwrap(); + assert_eq!(name.as_rule(), Rule::identifier); + assert_eq!(expr.as_rule(), Rule::expression); + let pos = argument_names + .iter() + .position(|&n| n == name.as_str()) + .ok_or_else(|| { + make_error( + format!(r#"Unexpected keyword argument "{}""#, name.as_str()), + span, + ) + })?; + if extracted_pairs[pos].is_some() { + return Err(make_error( + format!(r#"Got multiple values for keyword "{}""#, name.as_str()), + span, + )); + } + extracted_pairs[pos] = Some(expr); + } + r => panic!("unexpected argument rule {r:?}"), + } } + + assert_eq!(extracted_pairs.len(), max_arg_count); + let optional = extracted_pairs.split_off(min_arg_count); + let required = extracted_pairs.into_iter().flatten().collect_vec(); + if required.len() != min_arg_count { + return Err(make_count_error()); + } + Ok((required, optional)) } fn parse_function_argument_to_string( @@ -2386,6 +2441,11 @@ mod tests { .minus(&RevsetExpression::visible_heads()) ) ); + // Space is allowed around keyword arguments + assert_eq!( + parse("remote_branches( remote = foo )").unwrap(), + parse("remote_branches(remote=foo)").unwrap(), + ); // Trailing comma isn't allowed for empty argument assert!(parse("branches(,)").is_err()); @@ -2397,6 +2457,8 @@ mod tests { assert!(parse("branches(a , , )").is_err()); assert!(parse("file(a,b,)").is_ok()); assert!(parse("file(a,,b)").is_err()); + assert!(parse("remote_branches(a,remote=b , )").is_ok()); + assert!(parse("remote_branches(a,,remote=b)").is_err()); } #[test] @@ -2557,6 +2619,50 @@ mod tests { ); } + #[test] + fn test_parse_revset_keyword_arguments() { + assert_eq!( + parse("remote_branches(remote=foo)").unwrap(), + parse(r#"remote_branches("", foo)"#).unwrap(), + ); + assert_eq!( + parse("remote_branches(foo, remote=bar)").unwrap(), + parse(r#"remote_branches(foo, bar)"#).unwrap(), + ); + insta::assert_debug_snapshot!( + parse(r#"remote_branches(remote=foo, bar)"#).unwrap_err(), + @r###" + InvalidFunctionArguments { + name: "remote_branches", + message: "Positional argument follows keyword argument", + } + "###); + insta::assert_debug_snapshot!( + parse(r#"remote_branches("", foo, remote=bar)"#).unwrap_err(), + @r###" + InvalidFunctionArguments { + name: "remote_branches", + message: "Got multiple values for keyword \"remote\"", + } + "###); + insta::assert_debug_snapshot!( + parse(r#"remote_branches(remote=bar, remote=bar)"#).unwrap_err(), + @r###" + InvalidFunctionArguments { + name: "remote_branches", + message: "Got multiple values for keyword \"remote\"", + } + "###); + insta::assert_debug_snapshot!( + parse(r#"remote_branches(unknown=bar)"#).unwrap_err(), + @r###" + InvalidFunctionArguments { + name: "remote_branches", + message: "Unexpected keyword argument \"unknown\"", + } + "###); + } + #[test] fn test_expand_symbol_alias() { assert_eq!( @@ -2693,6 +2799,15 @@ mod tests { }) ); + // Keyword argument isn't supported for now. + assert_eq!( + parse_with_aliases("F(x=y)", [("F(x)", "x")]), + Err(RevsetParseErrorKind::InvalidFunctionArguments { + name: "F".to_owned(), + message: r#"Unexpected keyword argument "x""#.to_owned() + }) + ); + // Infinite recursion, where the top-level error isn't of RecursiveAlias kind. assert_eq!( parse_with_aliases( diff --git a/tests/test_revset_output.rs b/tests/test_revset_output.rs index 1835c3ae6..55c5eeeee 100644 --- a/tests/test_revset_output.rs +++ b/tests/test_revset_output.rs @@ -138,6 +138,50 @@ fn test_bad_function_call() { | = Revset function "whatever" doesn't exist "###); + + let stderr = test_env.jj_cmd_failure( + &repo_path, + &["log", "-r", "remote_branches(a, b, remote=c)"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Failed to parse revset: --> 1:23 + | + 1 | remote_branches(a, b, remote=c) + | ^------^ + | + = Invalid arguments to revset function "remote_branches": Got multiple values for keyword "remote" + "###); + + let stderr = + test_env.jj_cmd_failure(&repo_path, &["log", "-r", "remote_branches(remote=a, b)"]); + insta::assert_snapshot!(stderr, @r###" + Error: Failed to parse revset: --> 1:27 + | + 1 | remote_branches(remote=a, b) + | ^ + | + = Invalid arguments to revset function "remote_branches": Positional argument follows keyword argument + "###); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "remote_branches(=foo)"]); + insta::assert_snapshot!(stderr, @r###" + Error: Failed to parse revset: --> 1:17 + | + 1 | remote_branches(=foo) + | ^--- + | + = expected identifier or expression + "###); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "remote_branches(remote=)"]); + insta::assert_snapshot!(stderr, @r###" + Error: Failed to parse revset: --> 1:24 + | + 1 | remote_branches(remote=) + | ^--- + | + = expected expression + "###); } #[test]