From b938b5e907546b4850403797d877ed722994074a Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Tue, 1 Nov 2022 23:13:18 +0900
Subject: [PATCH] revset: report invalid string argument with span

Also dropped "found: {}" from the error summary as it's obvious.
---
 lib/src/revset.rs           | 19 +++++++++----------
 tests/test_revset_output.rs | 10 ++++++++++
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/lib/src/revset.rs b/lib/src/revset.rs
index 50de9dc5c..971764a8c 100644
--- a/lib/src/revset.rs
+++ b/lib/src/revset.rs
@@ -736,7 +736,7 @@ fn parse_function_expression(
         }
         "description" | "author" | "committer" => {
             let arg = expect_one_argument(name, argument_pairs)?;
-            let needle = parse_function_argument_to_string(name, arg.into_inner())?;
+            let needle = parse_function_argument_to_string(name, arg)?;
             let candidates = RevsetExpression::all();
             match name {
                 "description" => Ok(candidates.with_description(needle)),
@@ -752,7 +752,7 @@ fn parse_function_expression(
                 let paths = argument_pairs
                     .map(|arg| {
                         let span = arg.as_span();
-                        let needle = parse_function_argument_to_string(name, arg.into_inner())?;
+                        let needle = parse_function_argument_to_string(name, arg)?;
                         let path = RepoPath::parse_fs_path(ctx.cwd, ctx.workspace_root, &needle)
                             .map_err(|e| {
                                 RevsetParseError::with_span(
@@ -838,20 +838,19 @@ fn expect_one_optional_argument<'i>(
 
 fn parse_function_argument_to_string(
     name: &str,
-    pairs: Pairs<Rule>,
+    pair: Pair<Rule>,
 ) -> Result<String, RevsetParseError> {
+    let span = pair.as_span();
     let workspace_ctx = None; // string literal shouldn't depend on workspace information
-    let expression = parse_expression_rule(pairs.clone(), workspace_ctx)?;
+    let expression = parse_expression_rule(pair.into_inner(), workspace_ctx)?;
     match expression.as_ref() {
         RevsetExpression::Symbol(symbol) => Ok(symbol.clone()),
-        _ => Err(RevsetParseError::new(
+        _ => Err(RevsetParseError::with_span(
             RevsetParseErrorKind::InvalidFunctionArguments {
                 name: name.to_string(),
-                message: format!(
-                    "Expected function argument of type string, found: {}",
-                    pairs.as_str()
-                ),
+                message: "Expected function argument of type string".to_owned(),
             },
+            span,
         )),
     }
 }
@@ -1917,7 +1916,7 @@ mod tests {
             parse("description(heads())"),
             Err(RevsetParseErrorKind::InvalidFunctionArguments {
                 name: "description".to_string(),
-                message: "Expected function argument of type string, found: heads()".to_string()
+                message: "Expected function argument of type string".to_string()
             })
         );
         assert_eq!(
diff --git a/tests/test_revset_output.rs b/tests/test_revset_output.rs
index cc78595bb..d51ba2c90 100644
--- a/tests/test_revset_output.rs
+++ b/tests/test_revset_output.rs
@@ -22,6 +22,16 @@ fn test_bad_function_call() {
     test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
     let repo_path = test_env.env_root().join("repo");
 
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "file(a, not:a-string)"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Failed to parse revset:  --> 1:9
+      |
+    1 | file(a, not:a-string)
+      |         ^----------^
+      |
+      = Invalid arguments to revset function "file": Expected function argument of type string
+    "###);
+
     let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", r#"file(a, "../out")"#]);
     insta::assert_snapshot!(stderr, @r###"
     Error: Failed to parse revset:  --> 1:9