From 8d7416048d8cccfce03ff6ea16481e87fe14032e Mon Sep 17 00:00:00 2001
From: Fabian Boehm <FHomborg@gmail.com>
Date: Fri, 12 Aug 2022 17:04:30 +0200
Subject: [PATCH] Don't skip caret for some errors

This checked specifically for "| and" and "a=b" and then just gave the
error without a caret at all.

E.g. for a /tmp/broken.fish that contains

```fish
echo foo

echo foo | and cat
```

This would print:

```
/tmp/broken.fish (line 3): The 'and' command can not be used in a pipeline
warning: Error while reading file /tmp/broken.fish
```

without any indication other than the line number as to the location
of the error.

Now we do

```
/tmp/broken.fish (line 3): The 'and' command can not be used in a pipeline
echo foo | and cat
           ^~^
warning: Error while reading file /tmp/broken.fish
```

Another nice one:

```
fish --no-config -c 'echo notprinted; echo foo; a=b'
```

failed to give the error message!

(Note: Is it really a "warning" if we failed to read the one file we
wer told to?)

We should check if we should either centralize these error messages
completely, or always pass them and remove this "code" system, because
it's only used in some cases.
---
 src/parse_tree.cpp                    |  8 +++++---
 tests/checks/andor.fish               |  4 ++++
 tests/checks/invocation.fish          | 11 +++++++++++
 tests/checks/variable-assignment.fish |  6 ++++++
 4 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/src/parse_tree.cpp b/src/parse_tree.cpp
index 40f154bc1..efb6672d6 100644
--- a/src/parse_tree.cpp
+++ b/src/parse_tree.cpp
@@ -44,14 +44,17 @@ parse_error_code_t parse_error_from_tokenizer_error(tokenizer_error_t err) {
 wcstring parse_error_t::describe_with_prefix(const wcstring &src, const wcstring &prefix,
                                              bool is_interactive, bool skip_caret) const {
     wcstring result = prefix;
+    // Some errors don't have their message passed in, so we construct them here.
+    // This affects e.g. `eval "a=(foo)"`
     switch (code) {
         default:
             if (skip_caret && this->text.empty()) return L"";
+            result.append(this->text);
             break;
         case parse_error_andor_in_pipeline:
             append_format(result, INVALID_PIPELINE_CMD_ERR_MSG,
                           src.substr(this->source_start, this->source_length).c_str());
-            return result;
+            break;
         case parse_error_bare_variable_assignment: {
             wcstring assignment_src = src.substr(this->source_start, this->source_length);
             maybe_t<size_t> equals_pos = variable_assignment_equals_pos(assignment_src);
@@ -60,10 +63,9 @@ wcstring parse_error_t::describe_with_prefix(const wcstring &src, const wcstring
             wcstring value = assignment_src.substr(*equals_pos + 1);
             append_format(result, ERROR_BAD_COMMAND_ASSIGN_ERR_MSG, variable.c_str(),
                           value.c_str());
-            return result;
+            break;
         }
     }
-    result.append(this->text);
 
     size_t start = source_start;
     size_t len = source_length;
diff --git a/tests/checks/andor.fish b/tests/checks/andor.fish
index 1698a1884..c228c9ccf 100644
--- a/tests/checks/andor.fish
+++ b/tests/checks/andor.fish
@@ -4,9 +4,13 @@ set -xl LANG C # uniform quotes
 
 eval 'true | and'
 # CHECKERR: {{.*}}: The 'and' command can not be used in a pipeline
+# CHECKERR: true | and
+# CHECKERR:        ^~^
 
 eval 'true | or'
 # CHECKERR: {{.*}}: The 'or' command can not be used in a pipeline
+# CHECKERR: true | or
+# CHECKERR:        ^^
 
 # Verify and/or behavior with if and while
 if false; or true
diff --git a/tests/checks/invocation.fish b/tests/checks/invocation.fish
index 1e5bd3722..f81a67ed0 100644
--- a/tests/checks/invocation.fish
+++ b/tests/checks/invocation.fish
@@ -94,3 +94,14 @@ $fish --no-config -c 'echo $$ oh no syntax error' -c 'echo this works'
 $fish --no-config .
 # CHECKERR: error: Unable to read input file: Is a directory
 # CHECKERR: warning: Error while reading file .
+
+$fish --no-config -c 'echo notprinted; echo foo; a=b'
+# CHECKERR: fish: Unsupported use of '='. In fish, please use 'set a b'.
+# CHECKERR: echo notprinted; echo foo; a=b
+# CHECKERR:                            ^~^
+
+$fish --no-config -c 'echo notprinted | and true'
+# CHECKERR: fish: The 'and' command can not be used in a pipeline
+# CHECKERR: echo notprinted | and true
+# CHECKERR:                   ^~^
+
diff --git a/tests/checks/variable-assignment.fish b/tests/checks/variable-assignment.fish
index 7071f96a3..d5fb6b6ef 100644
--- a/tests/checks/variable-assignment.fish
+++ b/tests/checks/variable-assignment.fish
@@ -87,10 +87,16 @@ complete -C'a=b envxalias '
 # Eval invalid grammar to allow fish to parse this file
 eval 'a=(echo b)'
 # CHECKERR: {{.*}}: Unsupported use of '='. In fish, please use 'set a (echo b)'.
+# CHECKERR: a=(echo b)
+# CHECKERR: ^~~~~~~~~^
 eval ': | a=b'
 # CHECKERR: {{.*}}: Unsupported use of '='. In fish, please use 'set a b'.
+# CHECKERR: : | a=b
+# CHECKERR:     ^~^
 eval 'not a=b'
 # CHECKERR: {{.*}}: Unsupported use of '='. In fish, please use 'set a b'.
+# CHECKERR: not a=b
+# CHECKERR:     ^~^
 
 complete -c foo -xa '$a'
 a=b complete -C'foo '