Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

revset: parse "all:" prefix rule by pest #3422

Merged
merged 2 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ use jj_lib::repo::{
};
use jj_lib::repo_path::{FsPathParseError, RepoPath, RepoPathBuf};
use jj_lib::revset::{
RevsetAliasesMap, RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt,
RevsetAliasesMap, RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt, RevsetModifier,
RevsetParseContext, RevsetWorkspaceContext,
};
use jj_lib::rewrite::restore_tree;
Expand Down Expand Up @@ -771,7 +771,11 @@ impl WorkspaceCommandHelper {
) -> Result<IndexSet<Commit>, CommandError> {
let mut all_commits = IndexSet::new();
for revision_str in revision_args {
let (expression, all) = self.parse_revset_with_all_prefix(revision_str)?;
let (expression, modifier) = self.parse_revset_with_modifier(revision_str)?;
let all = match modifier {
Some(RevsetModifier::All) => true,
None => false,
};
if all {
yuja marked this conversation as resolved.
Show resolved Hide resolved
for commit in expression.evaluate_to_commits()? {
all_commits.insert(commit?);
Expand Down Expand Up @@ -807,16 +811,15 @@ impl WorkspaceCommandHelper {
self.attach_revset_evaluator(expression)
}

fn parse_revset_with_all_prefix(
// TODO: maybe better to parse all: prefix even if it is the default? It
// shouldn't be allowed in aliases, though.
fn parse_revset_with_modifier(
&self,
revision_str: &str,
) -> Result<(RevsetExpressionEvaluator<'_>, bool), CommandError> {
// TODO: Let pest parse the prefix too once we've dropped support for `:`
if let Some(revision_str) = revision_str.strip_prefix("all:") {
Ok((self.parse_revset(revision_str)?, true))
} else {
Ok((self.parse_revset(revision_str)?, false))
}
) -> Result<(RevsetExpressionEvaluator<'_>, Option<RevsetModifier>), CommandError> {
let context = self.revset_parse_context();
let (expression, modifier) = revset::parse_with_modifier(revision_str, &context)?;
Ok((self.attach_revset_evaluator(expression)?, modifier))
}

/// Parses the given revset expressions and concatenates them all.
Expand Down
12 changes: 12 additions & 0 deletions cli/tests/test_revset_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,18 @@ fn test_syntax_error() {
= '^' is not a postfix operator
Hint: Did you mean '-' for parents?
"###);

// "jj new" supports "all:" prefix
let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "ale:x"]);
insta::assert_snapshot!(stderr, @r###"
Error: Failed to parse revset: Modifier "ale" doesn't exist
Caused by: --> 1:1
|
1 | ale:x
| ^-^
|
= Modifier "ale" doesn't exist
"###);
}

#[test]
Expand Down
4 changes: 4 additions & 0 deletions lib/src/revset.pest
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ expression = {
}

program = _{ SOI ~ whitespace* ~ expression ~ whitespace* ~ EOI }
program_modifier = { identifier ~ pattern_kind_op ~ !":" }
program_with_modifier = _{
SOI ~ whitespace* ~ (program_modifier ~ whitespace*)? ~ expression ~ whitespace* ~ EOI
}

alias_declaration_part = _{
function_name ~ "(" ~ whitespace* ~ formal_parameters ~ whitespace* ~ ")"
Expand Down
194 changes: 185 additions & 9 deletions lib/src/revset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ impl Rule {
Rule::range_expression => None,
Rule::expression => None,
Rule::program => None,
Rule::program_modifier => None,
Rule::program_with_modifier => None,
Rule::alias_declaration_part => None,
Rule::alias_declaration => None,
}
Expand Down Expand Up @@ -171,6 +173,8 @@ pub enum RevsetParseErrorKind {
similar_op: String,
description: String,
},
#[error(r#"Modifier "{0}" doesn't exist"#)]
NoSuchModifier(String),
#[error(r#"Function "{name}" doesn't exist"#)]
NoSuchFunction {
name: String,
Expand Down Expand Up @@ -271,6 +275,17 @@ fn rename_rules_in_pest_error(mut err: pest::error::Error<Rule>) -> pest::error:
pub const GENERATION_RANGE_FULL: Range<u64> = 0..u64::MAX;
pub const GENERATION_RANGE_EMPTY: Range<u64> = 0..0;

/// Global flag applied to the entire expression.
///
/// The core revset engine doesn't use this value. It's up to caller to
/// interpret it to change the evaluation behavior.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum RevsetModifier {
/// Expression can be evaluated to multiple revisions even if a single
/// revision is expected by default.
All,
}

/// Symbol or function to be resolved to `CommitId`s.
#[derive(Clone, Debug, Eq, PartialEq)]
pub enum RevsetCommitRef {
Expand Down Expand Up @@ -784,7 +799,21 @@ struct ParseState<'a> {
allow_string_pattern: bool,
}

impl ParseState<'_> {
impl<'a> ParseState<'a> {
fn new(
context: &'a RevsetParseContext,
locals: &'a HashMap<&str, Rc<RevsetExpression>>,
) -> Self {
ParseState {
aliases_map: context.aliases_map,
aliases_expanding: &[],
locals,
user_email: &context.user_email,
workspace_ctx: &context.workspace,
allow_string_pattern: false,
}
}

fn with_alias_expanding<T>(
self,
id: RevsetAliasId<'_>,
Expand Down Expand Up @@ -828,6 +857,39 @@ fn parse_program(
parse_expression_rule(first.into_inner(), state)
}

fn parse_program_with_modifier(
revset_str: &str,
state: ParseState,
) -> Result<(Rc<RevsetExpression>, Option<RevsetModifier>), RevsetParseError> {
let mut pairs = RevsetParser::parse(Rule::program_with_modifier, revset_str)?;
let first = pairs.next().unwrap();
match first.as_rule() {
Rule::expression => {
let expression = parse_expression_rule(first.into_inner(), state)?;
Ok((expression, None))
}
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 modififer = match lhs.as_str() {
"all" => RevsetModifier::All,
name => {
return Err(RevsetParseError::with_span(
RevsetParseErrorKind::NoSuchModifier(name.to_owned()),
lhs.as_span(),
));
}
};
let expression = parse_expression_rule(rhs.into_inner(), state)?;
Ok((expression, Some(modififer)))
}
r => panic!("unexpected revset parse rule: {r:?}"),
}
}

fn parse_expression_rule(
pairs: Pairs<Rule>,
state: ParseState,
Expand Down Expand Up @@ -1506,17 +1568,20 @@ pub fn parse(
revset_str: &str,
context: &RevsetParseContext,
) -> Result<Rc<RevsetExpression>, RevsetParseError> {
let state = ParseState {
aliases_map: context.aliases_map,
aliases_expanding: &[],
locals: &HashMap::new(),
user_email: &context.user_email,
workspace_ctx: &context.workspace,
allow_string_pattern: false,
};
let locals = HashMap::new();
let state = ParseState::new(context, &locals);
parse_program(revset_str, state)
}

pub fn parse_with_modifier(
revset_str: &str,
context: &RevsetParseContext,
) -> Result<(Rc<RevsetExpression>, Option<RevsetModifier>), RevsetParseError> {
let locals = HashMap::new();
let state = ParseState::new(context, &locals);
parse_program_with_modifier(revset_str, state)
}

/// `Some` for rewritten expression, or `None` to reuse the original expression.
type TransformedExpression = Option<Rc<RevsetExpression>>;

Expand Down Expand Up @@ -2562,6 +2627,29 @@ mod tests {
super::parse(revset_str, &context).map_err(|e| e.kind)
}

fn parse_with_modifier(
revset_str: &str,
) -> Result<(Rc<RevsetExpression>, Option<RevsetModifier>), RevsetParseErrorKind> {
parse_with_aliases_and_modifier(revset_str, [] as [(&str, &str); 0])
}

fn parse_with_aliases_and_modifier(
revset_str: &str,
aliases: impl IntoIterator<Item = (impl AsRef<str>, impl Into<String>)>,
) -> Result<(Rc<RevsetExpression>, Option<RevsetModifier>), RevsetParseErrorKind> {
let mut aliases_map = RevsetAliasesMap::new();
for (decl, defn) in aliases {
aliases_map.insert(decl, defn).unwrap();
}
let context = RevsetParseContext {
aliases_map: &aliases_map,
user_email: "[email protected]".to_string(),
workspace: None,
};
// Map error to comparable object
super::parse_with_modifier(revset_str, &context).map_err(|e| e.kind)
}

#[test]
#[allow(clippy::redundant_clone)] // allow symbol.clone()
fn test_revset_expression_building() {
Expand Down Expand Up @@ -2894,6 +2982,83 @@ mod tests {
assert!(parse("remote_branches(a,,remote=b)").is_err());
}

#[test]
fn test_parse_revset_with_modifier() {
let all_symbol = RevsetExpression::symbol("all".to_owned());
let foo_symbol = RevsetExpression::symbol("foo".to_owned());

// all: is a program modifier, but all:: isn't
assert_eq!(
parse_with_modifier("all:"),
Err(RevsetParseErrorKind::SyntaxError)
);
assert_eq!(
parse_with_modifier("all:foo"),
Ok((foo_symbol.clone(), Some(RevsetModifier::All)))
);
assert_eq!(
parse_with_modifier("all::"),
Ok((all_symbol.descendants(), None))
);
assert_eq!(
parse_with_modifier("all::foo"),
Ok((all_symbol.dag_range_to(&foo_symbol), None))
);

// all::: could be parsed as all:(::), but rejected for simplicity
assert_eq!(
parse_with_modifier("all:::"),
Err(RevsetParseErrorKind::SyntaxError)
);
assert_eq!(
parse_with_modifier("all:::foo"),
Err(RevsetParseErrorKind::SyntaxError)
);

assert_eq!(
parse_with_modifier("all:(foo)"),
Ok((foo_symbol.clone(), Some(RevsetModifier::All)))
);
assert_eq!(
parse_with_modifier("all:all::foo"),
Ok((
all_symbol.dag_range_to(&foo_symbol),
Some(RevsetModifier::All)
))
);
assert_eq!(
parse_with_modifier("all:all | foo"),
Ok((all_symbol.union(&foo_symbol), Some(RevsetModifier::All)))
);

assert_eq!(
parse_with_modifier("all: ::foo"),
Ok((foo_symbol.ancestors(), Some(RevsetModifier::All)))
);
assert_eq!(
parse_with_modifier(" all: foo"),
Ok((foo_symbol.clone(), Some(RevsetModifier::All)))
);
assert_matches!(
parse_with_modifier("(all:foo)"),
Err(RevsetParseErrorKind::NotInfixOperator { .. })
);
assert_matches!(
parse_with_modifier("all :foo"),
Err(RevsetParseErrorKind::SyntaxError)
);
assert_matches!(
parse_with_modifier("all:all:all"),
Err(RevsetParseErrorKind::NotInfixOperator { .. })
);

// Top-level string pattern can't be parsed, which is an error anyway
assert_eq!(
parse_with_modifier(r#"exact:"foo""#),
Err(RevsetParseErrorKind::NoSuchModifier("exact".to_owned()))
);
}

#[test]
fn test_parse_whitespace() {
let ascii_whitespaces: String = ('\x00'..='\x7f')
Expand Down Expand Up @@ -3278,6 +3443,12 @@ mod tests {
parse("a@B").unwrap()
);

// Modifier cannot be substituted.
assert_eq!(
parse_with_aliases_and_modifier("all:all", [("all", "ALL")]).unwrap(),
parse_with_modifier("all:ALL").unwrap()
);

// Multi-level substitution.
assert_eq!(
parse_with_aliases("A", [("A", "BC"), ("BC", "b|C"), ("C", "c")]).unwrap(),
Expand All @@ -3299,6 +3470,11 @@ 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]
Expand Down