Skip to content

Commit

Permalink
revset: duplicate Rule::identifier to strict_identifier, replace some…
Browse files Browse the repository at this point in the history
… usages

The "identifier" rule will be changed to accept unicode characters. It's not
super important to reject non-ASCII string pattern prefixes or alias symbols,
but this is more consistent with templater and fileset. We can relax the rule
later if needed.
  • Loading branch information
yuja committed Jan 15, 2025
1 parent 8852c43 commit 7f76f50
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 19 deletions.
6 changes: 3 additions & 3 deletions cli/tests/test_log_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -454,16 +454,16 @@ fn test_log_bad_short_prefixes() {
test_env.add_config(r#"revsets.short-prefixes = "!nval!d""#);
let stderr = test_env.jj_cmd_failure(&repo_path, &["status"]);
insta::assert_snapshot!(stderr,
@r###"
@r"
Config error: Invalid `revsets.short-prefixes`
Caused by: --> 1:1
|
1 | !nval!d
| ^---
|
= expected <identifier> or <expression>
= expected <strict_identifier> or <expression>
For help, see https://jj-vcs.github.io/jj/latest/config/.
"###);
");

// Warn on resolution of short prefixes
test_env.add_config("revsets.short-prefixes = 'missing'");
Expand Down
12 changes: 6 additions & 6 deletions cli/tests/test_revset_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,15 +266,15 @@ fn test_bad_function_call() {
"###);

let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "remote_bookmarks(=foo)"]);
insta::assert_snapshot!(stderr, @r###"
insta::assert_snapshot!(stderr, @r"
Error: Failed to parse revset: Syntax error
Caused by: --> 1:18
|
1 | remote_bookmarks(=foo)
| ^---
|
= expected <identifier> or <expression>
"###);
= expected <strict_identifier> or <expression>
");

let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "remote_bookmarks(remote=)"]);
insta::assert_snapshot!(stderr, @r###"
Expand Down Expand Up @@ -601,20 +601,20 @@ fn test_bad_alias_decl() {
insta::assert_snapshot!(stdout, @r###"
◆ zzzzzzzz root() 00000000
"###);
insta::assert_snapshot!(stderr, @r###"
insta::assert_snapshot!(stderr, @r#"
Warning: Failed to load "revset-aliases."bad"": --> 1:1
|
1 | "bad"
| ^---
|
= expected <identifier> or <function_name>
= expected <strict_identifier> or <function_name>
Warning: Failed to load "revset-aliases.badfn(a, a)": --> 1:7
|
1 | badfn(a, a)
| ^--^
|
= Redefinition of function parameter
"###);
"#);
}

#[test]
Expand Down
16 changes: 11 additions & 5 deletions lib/src/revset.pest
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ identifier_part = @{ (ASCII_ALPHANUMERIC | "_" | "/")+ }
identifier = @{
identifier_part ~ (("." | "-" | "+") ~ identifier_part)*
}
// TODO: remove "/", ".", "+" for consistency with fileset?
strict_identifier_part = @{ (ASCII_ALPHANUMERIC | "_" | "/")+ }
strict_identifier = @{
strict_identifier_part ~ (("." | "-" | "+") ~ strict_identifier_part)*
}

symbol = _{
identifier
| string_literal
Expand Down Expand Up @@ -68,19 +74,19 @@ infix_op = _{ union_op | intersection_op | difference_op | compat_add_op | compa

function = { function_name ~ "(" ~ whitespace* ~ function_arguments ~ whitespace* ~ ")" }
function_name = @{ (ASCII_ALPHA | "_") ~ (ASCII_ALPHANUMERIC | "_")* }
keyword_argument = { identifier ~ whitespace* ~ "=" ~ whitespace* ~ expression }
keyword_argument = { strict_identifier ~ whitespace* ~ "=" ~ whitespace* ~ expression }
argument = _{ keyword_argument | expression }
function_arguments = {
argument ~ (whitespace* ~ "," ~ whitespace* ~ argument)* ~ (whitespace* ~ ",")?
| ""
}
formal_parameters = {
identifier ~ (whitespace* ~ "," ~ whitespace* ~ identifier)* ~ (whitespace* ~ ",")?
strict_identifier ~ (whitespace* ~ "," ~ whitespace* ~ strict_identifier)* ~ (whitespace* ~ ",")?
| ""
}

// TODO: change rhs to string_literal to require quoting? #2101
string_pattern = { identifier ~ pattern_kind_op ~ symbol }
string_pattern = { strict_identifier ~ pattern_kind_op ~ symbol }

primary = {
"(" ~ whitespace* ~ expression ~ whitespace* ~ ")"
Expand Down Expand Up @@ -108,7 +114,7 @@ expression = {
~ (whitespace* ~ infix_op ~ whitespace* ~ (negate_op ~ whitespace*)* ~ range_expression)*
}

program_modifier = { identifier ~ pattern_kind_op ~ !":" }
program_modifier = { strict_identifier ~ pattern_kind_op ~ !":" }
program = _{
SOI ~ whitespace* ~ (program_modifier ~ whitespace*)? ~ expression ~ whitespace* ~ EOI
}
Expand All @@ -117,5 +123,5 @@ function_alias_declaration = {
function_name ~ "(" ~ whitespace* ~ formal_parameters ~ whitespace* ~ ")"
}
alias_declaration = _{
SOI ~ (function_alias_declaration | identifier) ~ EOI
SOI ~ (function_alias_declaration | strict_identifier) ~ EOI
}
12 changes: 7 additions & 5 deletions lib/src/revset_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ const FUNCTION_CALL_PARSER: FunctionCallParser<Rule> = FunctionCallParser {
function_name_rule: Rule::function_name,
function_arguments_rule: Rule::function_arguments,
keyword_argument_rule: Rule::keyword_argument,
argument_name_rule: Rule::identifier,
argument_name_rule: Rule::strict_identifier,
argument_value_rule: Rule::expression,
};

Expand All @@ -83,6 +83,8 @@ impl Rule {
Rule::whitespace => None,
Rule::identifier_part => None,
Rule::identifier => None,
Rule::strict_identifier_part => None,
Rule::strict_identifier => None,
Rule::symbol => None,
Rule::string_escape => None,
Rule::string_content_char => None,
Expand Down Expand Up @@ -469,7 +471,7 @@ pub(super) fn parse_program(revset_str: &str) -> Result<ExpressionNode, RevsetPa
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!(lhs.as_rule(), Rule::strict_identifier);
assert_eq!(op.as_rule(), Rule::pattern_kind_op);
assert_eq!(rhs.as_rule(), Rule::expression);
let span = lhs.as_span().start_pos().span(&rhs.as_span().end_pos());
Expand Down Expand Up @@ -629,7 +631,7 @@ fn parse_primary_node(pair: Pair<Rule>) -> Result<ExpressionNode, RevsetParseErr
}
Rule::string_pattern => {
let (lhs, op, rhs) = first.into_inner().collect_tuple().unwrap();
assert_eq!(lhs.as_rule(), Rule::identifier);
assert_eq!(lhs.as_rule(), Rule::strict_identifier);
assert_eq!(op.as_rule(), Rule::pattern_kind_op);
let kind = lhs.as_str();
let value = parse_as_string_literal(rhs);
Expand Down Expand Up @@ -691,7 +693,7 @@ impl AliasDeclarationParser for RevsetAliasParser {
let mut pairs = RevsetParser::parse(Rule::alias_declaration, source)?;
let first = pairs.next().unwrap();
match first.as_rule() {
Rule::identifier => Ok(AliasDeclaration::Symbol(first.as_str().to_owned())),
Rule::strict_identifier => Ok(AliasDeclaration::Symbol(first.as_str().to_owned())),
Rule::function_alias_declaration => {
let (name_pair, params_pair) = first.into_inner().collect_tuple().unwrap();
assert_eq!(name_pair.as_rule(), Rule::function_name);
Expand All @@ -701,7 +703,7 @@ impl AliasDeclarationParser for RevsetAliasParser {
let params = params_pair
.into_inner()
.map(|pair| match pair.as_rule() {
Rule::identifier => pair.as_str().to_owned(),
Rule::strict_identifier => pair.as_str().to_owned(),
r => panic!("unexpected formal parameter rule {r:?}"),
})
.collect_vec();
Expand Down

0 comments on commit 7f76f50

Please sign in to comment.