From c4769e0b7c2a54b8db5e839e120184bf1b71f69d Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 7 Sep 2023 10:11:48 +0900 Subject: [PATCH] revset: translate symbol rules in error message Since we have overloaded operator symbols, we need to deduplicate them upfront. Legacy and compat operators are also removed from the suggestion. It's a bit ugly to mutate the error struct before calling Error::renamed_rule(), but I think it's still better than reimplementing message formatting function. --- cli/tests/test_revset_output.rs | 10 ++-- lib/src/revset.rs | 98 ++++++++++++++++++++++++++++++++- 2 files changed, 101 insertions(+), 7 deletions(-) diff --git a/cli/tests/test_revset_output.rs b/cli/tests/test_revset_output.rs index 6b72ca5e9..4b8daefd9 100644 --- a/cli/tests/test_revset_output.rs +++ b/cli/tests/test_revset_output.rs @@ -29,7 +29,7 @@ fn test_syntax_error() { 1 | x & | ^--- | - = expected dag_range_pre_op, dag_range_all_op, legacy_dag_range_pre_op, range_pre_op, range_all_op, negate_op, or primary + = expected `::`, `..`, `~`, or "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "x - y"]); @@ -192,7 +192,7 @@ fn test_bad_function_call() { 1 | remote_branches(=foo) | ^--- | - = expected identifier or expression + = expected or "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "remote_branches(remote=)"]); @@ -202,7 +202,7 @@ fn test_bad_function_call() { 1 | remote_branches(remote=) | ^--- | - = expected expression + = expected "###); } @@ -287,7 +287,7 @@ fn test_alias() { 1 | whatever & | ^--- | - = expected dag_range_pre_op, dag_range_all_op, legacy_dag_range_pre_op, range_pre_op, range_all_op, negate_op, or primary + = expected `::`, `..`, `~`, or "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "identity()"]); @@ -401,7 +401,7 @@ fn test_bad_alias_decl() { 1 | "bad" | ^--- | - = expected identifier or function_name + = expected or Failed to load "revset-aliases.badfn(a, a)": --> 1:7 | 1 | badfn(a, a) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index b3c60f92e..abc78d8e3 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -14,7 +14,7 @@ #![allow(missing_docs)] -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::convert::Infallible; use std::ops::Range; use std::path::Path; @@ -76,6 +76,74 @@ pub enum RevsetEvaluationError { #[grammar = "revset.pest"] pub struct RevsetParser; +impl Rule { + /// Whether this is a placeholder rule for compatibility with the other + /// systems. + fn is_compat(&self) -> bool { + matches!( + self, + Rule::compat_parents_op | Rule::compat_add_op | Rule::compat_sub_op + ) + } + + /// Whether this is a deprecated rule to be removed in later version. + fn is_legacy(&self) -> bool { + matches!( + self, + Rule::legacy_dag_range_op + | Rule::legacy_dag_range_pre_op + | Rule::legacy_dag_range_post_op + ) + } + + fn to_symbol(self) -> Option<&'static str> { + match self { + Rule::EOI => None, + Rule::identifier_part => None, + Rule::identifier => None, + Rule::symbol => None, + Rule::literal_string => None, + Rule::whitespace => None, + Rule::at_op => Some("@"), + Rule::parents_op => Some("-"), + Rule::children_op => Some("+"), + Rule::compat_parents_op => Some("^"), + Rule::dag_range_op + | Rule::dag_range_pre_op + | Rule::dag_range_post_op + | Rule::dag_range_all_op => Some("::"), + Rule::legacy_dag_range_op + | Rule::legacy_dag_range_pre_op + | Rule::legacy_dag_range_post_op => Some(":"), + Rule::range_op => Some(".."), + Rule::range_pre_op | Rule::range_post_op | Rule::range_all_op => Some(".."), + Rule::range_ops => None, + Rule::range_pre_ops => None, + Rule::range_post_ops => None, + Rule::range_all_ops => None, + Rule::negate_op => Some("~"), + Rule::union_op => Some("|"), + Rule::intersection_op => Some("&"), + Rule::difference_op => Some("~"), + Rule::compat_add_op => Some("+"), + Rule::compat_sub_op => Some("-"), + Rule::infix_op => None, + Rule::function_name => None, + Rule::keyword_argument => None, + Rule::argument => None, + Rule::function_arguments => None, + Rule::formal_parameters => None, + Rule::primary => None, + Rule::neighbors_expression => None, + Rule::range_expression => None, + Rule::expression => None, + Rule::program => None, + Rule::alias_declaration_part => None, + Rule::alias_declaration => None, + } + } +} + #[derive(Debug)] pub struct RevsetParseError { kind: RevsetParseErrorKind, @@ -175,7 +243,7 @@ impl From> for RevsetParseError { fn from(err: pest::error::Error) -> Self { RevsetParseError { kind: RevsetParseErrorKind::SyntaxError, - pest_error: Some(Box::new(err)), + pest_error: Some(Box::new(rename_rules_in_pest_error(err))), origin: None, } } @@ -207,6 +275,32 @@ impl error::Error for RevsetParseError { } } +fn rename_rules_in_pest_error(mut err: pest::error::Error) -> pest::error::Error { + let pest::error::ErrorVariant::ParsingError { + positives, + negatives, + } = &mut err.variant + else { + return err; + }; + + // Remove duplicated symbols. Legacy or compat symbols are also removed from the + // (positive) suggestion. + let mut known_syms = HashSet::new(); + positives.retain(|rule| { + !rule.is_compat() + && !rule.is_legacy() + && rule.to_symbol().map_or(true, |sym| known_syms.insert(sym)) + }); + let mut known_syms = HashSet::new(); + negatives.retain(|rule| rule.to_symbol().map_or(true, |sym| known_syms.insert(sym))); + err.renamed_rules(|rule| { + rule.to_symbol() + .map(|sym| format!("`{sym}`")) + .unwrap_or_else(|| format!("<{rule:?}>")) + }) +} + // assumes index has less than u64::MAX entries. pub const GENERATION_RANGE_FULL: Range = 0..u64::MAX; pub const GENERATION_RANGE_EMPTY: Range = 0..0;