cli: render string pattern suggestion as a hint

cli/examples/custom-commit-templater/main.rs

@@ -108,7 +108,7 @@ impl CommitTemplateLanguageExtension for HexCounter {
                         let chars: Vec<_> = string.chars().collect();
                         match chars[..] {
                             [ch] => Ok(ch),
-                            _ => Err(TemplateParseError::unexpected_expression(
+                            _ => Err(TemplateParseError::expression(
                                 "Expected singular character argument",
                                 span,
                             )),

cli/examples/custom-operation-templater/main.rs

@@ -68,7 +68,7 @@ impl OperationTemplateLanguageExtension for HexCounter {
                         let chars: Vec<_> = string.chars().collect();
                         match chars[..] {
                             [ch] => Ok(ch),
-                            _ => Err(TemplateParseError::unexpected_expression(
+                            _ => Err(TemplateParseError::expression(
                                 "Expected singular character argument",
                                 span,
                             )),

cli/src/command_error.rs

@@ -30,6 +30,7 @@ use jj_lib::revset::{
     RevsetEvaluationError, RevsetParseError, RevsetParseErrorKind, RevsetResolutionError,
 };
 use jj_lib::signing::SignInitError;
+use jj_lib::str_util::StringPatternParseError;
 use jj_lib::working_copy::{ResetError, SnapshotError, WorkingCopyStateError};
 use jj_lib::workspace::WorkspaceInitError;
 use thiserror::Error;
@@ -423,6 +424,9 @@ impl From<RevsetParseError> for CommandError {
                 name: _,
                 candidates,
             } => format_similarity_hint(candidates),
+            RevsetParseErrorKind::InvalidFunctionArguments { .. } => {
+                find_source_parse_error_hint(bottom_err)
+            }
             _ => None,
         };
         let mut cmd_err =
@@ -470,6 +474,8 @@ impl From<TemplateParseError> for CommandError {
             | TemplateParseErrorKind::NoSuchMethod { candidates, .. } => {
                 format_similarity_hint(candidates)
             }
+            TemplateParseErrorKind::InvalidArguments { .. }
+            | TemplateParseErrorKind::Expression(_) => find_source_parse_error_hint(bottom_err),
             _ => None,
         };
         let mut cmd_err =
@@ -489,7 +495,10 @@ impl From<FsPathParseError> for CommandError {
 
 impl From<clap::Error> for CommandError {
     fn from(err: clap::Error) -> Self {
-        cli_error(err)
+        let hint = find_source_parse_error_hint(&err);
+        let mut cmd_err = cli_error(err);
+        cmd_err.extend_hints(hint);
+        cmd_err
     }
 }
 
@@ -511,6 +520,24 @@ impl From<GitIgnoreError> for CommandError {
     }
 }
 
+fn find_source_parse_error_hint(err: &dyn error::Error) -> Option<String> {
+    let source = err.source()?;
+    if let Some(source) = source.downcast_ref() {
+        string_pattern_parse_error_hint(source)
+    } else {
+        None
+    }
+}
+
+fn string_pattern_parse_error_hint(err: &StringPatternParseError) -> Option<String> {
+    match err {
+        StringPatternParseError::InvalidKind(_) => {
+            Some("Try prefixing with one of `exact:`, `glob:` or `substring:`".into())
+        }
+        StringPatternParseError::GlobPattern(_) => None,
+    }
+}
+
 const BROKEN_PIPE_EXIT_CODE: u8 = 3;
 
 pub(crate) fn handle_command_result(ui: &mut Ui, result: Result<(), CommandError>) -> ExitCode {

cli/src/commit_templater.rs

@@ -636,10 +636,13 @@ fn evaluate_immutable_revset<'repo>(
     // It's usually smaller than the immutable set. The revset engine can also
     // optimize "::<recent_heads>" query to use bitset-based implementation.
     let expression = revset_util::parse_immutable_expression(&language.revset_parse_context)
-        .map_err(|err| TemplateParseError::other("Failed to parse revset", err, span))?;
+        .map_err(|err| {
+            TemplateParseError::expression("Failed to parse revset", span).with_source(err)
+        })?;
     let symbol_resolver = revset_util::default_symbol_resolver(repo, language.id_prefix_context);
-    let revset = revset_util::evaluate(repo, &symbol_resolver, expression)
-        .map_err(|err| TemplateParseError::other("Failed to evaluate revset", err, span))?;
+    let revset = revset_util::evaluate(repo, &symbol_resolver, expression).map_err(|err| {
+        TemplateParseError::expression("Failed to evaluate revset", span).with_source(err)
+    })?;
     Ok(revset)
 }
 

cli/src/template_builder.rs

@@ -682,9 +682,8 @@ fn builtin_timestamp_methods<'a, L: TemplateLanguage<'a> + ?Sized>(
             let [format_node] = template_parser::expect_exact_arguments(function)?;
             let format =
                 template_parser::expect_string_literal_with(format_node, |format, span| {
-                    time_util::FormattingItems::parse(format).ok_or_else(|| {
-                        TemplateParseError::unexpected_expression("Invalid time format", span)
-                    })
+                    time_util::FormattingItems::parse(format)
+                        .ok_or_else(|| TemplateParseError::expression("Invalid time format", span))
                 })?
                 .into_owned();
             let out_property = self_property.and_then(move |timestamp| {
@@ -847,7 +846,7 @@ where
         if let [name] = lambda.params.as_slice() {
             local_variables.insert(name, &item_fn);
         } else {
-            return Err(TemplateParseError::unexpected_expression(
+            return Err(TemplateParseError::expression(
                 "Expected 1 lambda parameters",
                 lambda.params_span,
             ));
@@ -1028,7 +1027,7 @@ pub fn build_expression<'a, L: TemplateLanguage<'a> + ?Sized>(
             expression.labels.push(method.function.name.to_owned());
             Ok(expression)
         }
-        ExpressionKind::Lambda(_) => Err(TemplateParseError::unexpected_expression(
+        ExpressionKind::Lambda(_) => Err(TemplateParseError::expression(
             "Lambda cannot be defined here",
             node.span,
         )),

cli/src/template_parser.rs

@@ -75,8 +75,6 @@ pub struct TemplateParseError {
 pub enum TemplateParseErrorKind {
     #[error("Syntax error")]
     SyntaxError,
-    #[error("Invalid integer literal")]
-    ParseIntError,
     #[error(r#"Keyword "{name}" doesn't exist"#)]
     NoSuchKeyword {
         name: String,
@@ -98,7 +96,7 @@ pub enum TemplateParseErrorKind {
     #[error("Redefinition of function parameter")]
     RedefinedFunctionParameter,
     #[error("{0}")]
-    UnexpectedExpression(String),
+    Expression(String),
     #[error(r#"Alias "{0}" cannot be expanded"#)]
     BadAliasExpansion(String),
     #[error(r#"Alias "{0}" expanded recursively"#)]
@@ -119,21 +117,9 @@ impl TemplateParseError {
         }
     }
 
-    pub fn with_span_and_source(
-        kind: TemplateParseErrorKind,
-        span: pest::Span<'_>,
-        source: impl Into<Box<dyn error::Error + Send + Sync>>,
-    ) -> Self {
-        let message = kind.to_string();
-        let pest_error = Box::new(pest::error::Error::new_from_span(
-            pest::error::ErrorVariant::CustomError { message },
-            span,
-        ));
-        TemplateParseError {
-            kind,
-            pest_error,
-            source: Some(source.into()),
-        }
+    pub fn with_source(mut self, source: impl Into<Box<dyn error::Error + Send + Sync>>) -> Self {
+        self.source = Some(source.into());
+        self
     }
 
     // TODO: migrate all callers to table-based lookup_method()
@@ -163,35 +149,20 @@ impl TemplateParseError {
 
     pub fn expected_type(type_name: &str, span: pest::Span<'_>) -> Self {
         let message = format!(r#"Expected expression of type "{type_name}""#);
-        TemplateParseError::unexpected_expression(message, span)
+        TemplateParseError::expression(message, span)
     }
 
-    pub fn unexpected_expression(message: impl Into<String>, span: pest::Span<'_>) -> Self {
-        TemplateParseError::with_span(
-            TemplateParseErrorKind::UnexpectedExpression(message.into()),
-            span,
-        )
-    }
-
-    /// Some other expression error with the underlying error `source`.
-    pub fn other(
-        message: impl Into<String>,
-        source: impl Into<Box<dyn error::Error + Send + Sync>>,
-        span: pest::Span<'_>,
-    ) -> Self {
-        TemplateParseError::with_span_and_source(
-            TemplateParseErrorKind::UnexpectedExpression(message.into()),
-            span,
-            source,
-        )
+    /// Some other expression error.
+    pub fn expression(message: impl Into<String>, span: pest::Span<'_>) -> Self {
+        TemplateParseError::with_span(TemplateParseErrorKind::Expression(message.into()), span)
     }
 
     pub fn within_alias_expansion(self, id: TemplateAliasId<'_>, span: pest::Span<'_>) -> Self {
-        TemplateParseError::with_span_and_source(
+        TemplateParseError::with_span(
             TemplateParseErrorKind::BadAliasExpansion(id.to_string()),
             span,
-            self,
         )
+        .with_source(self)
     }
 
     /// If this is a `NoSuchKeyword` error, expands the candidates list with the
@@ -340,10 +311,7 @@ fn parse_identifier_name(pair: Pair<Rule>) -> TemplateParseResult<&str> {
     if let ExpressionKind::Identifier(name) = parse_identifier_or_literal(pair) {
         Ok(name)
     } else {
-        Err(TemplateParseError::unexpected_expression(
-            "Expected identifier",
-            span,
-        ))
+        Err(TemplateParseError::expression("Expected identifier", span))
     }
 }
 
@@ -435,11 +403,7 @@ fn parse_term_node(pair: Pair<Rule>) -> TemplateParseResult<ExpressionNode> {
         }
         Rule::integer_literal => {
             let value = expr.as_str().parse().map_err(|err| {
-                TemplateParseError::with_span_and_source(
-                    TemplateParseErrorKind::ParseIntError,
-                    span,
-                    err,
-                )
+                TemplateParseError::expression("Invalid integer literal", span).with_source(err)
             })?;
             ExpressionNode::new(ExpressionKind::Integer(value), span)
         }
@@ -868,7 +832,7 @@ pub fn expect_string_literal_with<'a, 'i, T>(
         | ExpressionKind::Concat(_)
         | ExpressionKind::FunctionCall(_)
         | ExpressionKind::MethodCall(_)
-        | ExpressionKind::Lambda(_) => Err(TemplateParseError::unexpected_expression(
+        | ExpressionKind::Lambda(_) => Err(TemplateParseError::expression(
             "Expected string literal",
             node.span,
         )),
@@ -892,7 +856,7 @@ pub fn expect_lambda_with<'a, 'i, T>(
         | ExpressionKind::Binary(..)
         | ExpressionKind::Concat(_)
         | ExpressionKind::FunctionCall(_)
-        | ExpressionKind::MethodCall(_) => Err(TemplateParseError::unexpected_expression(
+        | ExpressionKind::MethodCall(_) => Err(TemplateParseError::expression(
             "Expected lambda expression",
             node.span,
         )),
@@ -1262,7 +1226,7 @@ mod tests {
         );
         assert_matches!(
             parse_into_kind(&format!("{}", (i64::MAX as u64) + 1)),
-            Err(TemplateParseErrorKind::ParseIntError)
+            Err(TemplateParseErrorKind::Expression(_))
         );
     }
 

cli/tests/test_branch_command.rs

@@ -396,9 +396,10 @@ fn test_branch_delete_glob() {
     // Unknown pattern kind
     let stderr = test_env.jj_cmd_cli_error(&repo_path, &["branch", "forget", "whatever:branch"]);
     insta::assert_snapshot!(stderr, @r###"
-    error: invalid value 'whatever:branch' for '[NAMES]...': Invalid string pattern kind "whatever:", try prefixing with one of `exact:`, `glob:` or `substring:`
+    error: invalid value 'whatever:branch' for '[NAMES]...': Invalid string pattern kind "whatever:"
 
     For more information, try '--help'.
+    Hint: Try prefixing with one of `exact:`, `glob:` or `substring:`
     "###);
 }
 

cli/tests/test_revset_output.rs

@@ -167,13 +167,16 @@ fn test_bad_function_call() {
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "branches(bad:pattern)"]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: Failed to parse revset: Function "branches": Invalid string pattern kind "bad:", try prefixing with one of `exact:`, `glob:` or `substring:`
-    Caused by:  --> 1:10
+    Error: Failed to parse revset: Function "branches": Invalid string pattern
+    Caused by:
+    1:  --> 1:10
       |
     1 | branches(bad:pattern)
       |          ^---------^
       |
-      = Function "branches": Invalid string pattern kind "bad:", try prefixing with one of `exact:`, `glob:` or `substring:`
+      = Function "branches": Invalid string pattern
+    2: Invalid string pattern kind "bad:"
+    Hint: Try prefixing with one of `exact:`, `glob:` or `substring:`
     "###);
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "root()::whatever()"]);