Simpler error messages #10054
Simpler error messages #10054
Conversation
These printed "Unknown error while evaluating command substitution".
Now they print something like
```
fish: for: status: cannot overwrite read-only variable
for status in foo; end
^~~~~^
in command substitution
fish: Invalid arguments
echo (for status in foo; end)
^~~~~~~~~~~~~~~~~~~~~~~^
```
for `echo (for status in foo; end)`
This is, of course, still not *great*. Mostly the `fish: Invalid
arguments` is basically entirely redundant.
An alternative is to simply skip the error message, but that requires some
more scaffolding (describe_with_prefix adds some error messages on its
own, so we can't simply say "don't add the prefix if we don't have a
message")
When fish prints an error, it will often add a stack trace.
This stack trace could look like
```
test: Missing argument at index 3
-eq 5
^
./foo.fish (line 3):
test $args -eq 5
^
in function 'this-errors' with arguments '2'
called on line 7 of file ./foo.fish
in function 'call-this'
called on line 1 of file ./foo.fish
in command substitution
called on line 10 of file ./foo.fish
```
which is a bit long, and the "called on" lines start with tab
characters, which is weird.
So, this shortens it to:
```
test: Missing argument at index 3
-eq 5
^
./foo.fish (line 3):
test $args -eq 5
^
in function 'this-errors' with arguments '2' called on line 7 of file ./foo.fish
in function 'call-this' called on line 1 of file ./foo.fish
in command substitution called on line 10 of file ./foo.fish
```
Note: We expose stack traces via `status print-stack-trace`, so this
can break people's scripts if they rely on that.
However, we did a similar change in
7095de6,
where we put the function arguments on the same line, and haven't
heard a peep from anyone.
For example `fish -c "nonexistent-command-1234 banana rama"` prints: ``` fish: Unknown command: nonexistent-command-1234 fish: nonexistent-command-1234 banana rama ^~~~~~~~~~~~~~~~~~~~~~~^ ``` That "fish: " line is entirely useless, and this removes it.
| break; | ||
| } | ||
| case block_type_t::event: { | ||
| assert(b.event && "Should have an event"); | ||
| wcstring description = *event_get_desc(parser, **b.event); | ||
| append_format(trace, _(L"in event handler: %ls\n"), description.c_str()); | ||
| print_call_site = true; | ||
| append_format(trace, _(L"in event handler: %ls"), description.c_str()); |
There was a problem hiding this comment.
The only potential downside is it could look worse if the argument strings are overlong.
I think that's an edge case so probably this is fine and overall a great change.
As a programmer, the stack traces are a bit confusing to me, if I see something like
function foo file bar.fish:3
function bar ...
I expect that line 3 in bar.fish is inside function foo. But that's a separate concern
Also I expect the ordering file,line,function,arguments instead of function,arguments,file,line (which would alleviate the concern about long arguments)
I would be entirely fine if we decided to go with just the first one, or if we decided to put the rest off for 4.0, or if the rest gets in the way of someone's porting and I need to write it again later, or if we decide to go for a cleaner solution.
I don't mind merging things like this first (it's small and I don't expect too many of them).
There was a problem hiding this comment.
The only potential downside is it could look worse if the argument strings are overlong.
There are a bunch of ways to solve this:
- Truncate/ellipsize the arguments (not great)
- Move the arguments onto another line if they are very long
Something like
fish: Unknown command: nonexistent
foo.fish (line 4):
if nonexistent
^~~~~~~~~~^
in function 'this-errors' with arguments:
'foo\ bar aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa baz'
called on line 11 of file foo.fish
in function 'call-this' called on line 1 of file foo.fish
for the latter. I have something whipped up, I'll add it here.
Also I expect the ordering file,line,function,arguments instead of function,arguments,file,line
I think that would be awkward because we don't tend to use tons of files. So you would have a lot of
foo.fish:50:foo bar baz
foo.fish:42:function2 arguments
foo.fish:38:function1 arguments
which is a bunch of noise.
I've been experimenting with only showing the file when it changed
This should alleviate concerns over the argument list being too long and pushing the file off-screen (well, onto the new line)
|
Alright, I'm going to be merging the first commit of this, also to 3.7.0 and then rework the rest later. |
Description
These are three fairly independent commits with a common goal:
To make our error messages better.
The first adds some extra cases where we used to print "Unknown error while evaluating command substitution". The messages it adds are pretty redundant, but not adding an error there would require some extra scaffolding. Or we could pull these errors apart, which requires separate status codes. I would like to add this one to 3.7.0 as-is, and think it's fairly uncontroversial.
The second puts the "called on" on the same line as the block description in stack traces. So instead of
you get
This makes some possibly very large messages a bunch smaller.
One potential issue is that this also changes the output of
status print-stack-trace, which people could potentially rely on. Last time we did that I can't remember anyone complaining.It also changes all these messages, which means the translations would need to be fixed (which should be fine to just do because it's removing a
\n).The third one removes a potential redundant "fish:" line we get in some cases, because we add an error without a message to add context. It's a bit hacky, but not too odious I think. Mostly it would be easier if
describe_with_prefixdidn't have to add error messages for bare variable assignments or andor_in_pipeline, because then the outside could just drive it, and decide to set the prefix to empty if there is no message.There's some slight concern that there are errors where it's unclear they are from fish - if so, I'd argue that they should just be improved.
Here's a demonstration of how messages change:
fish -c 'nonexistent'changes fromto
And a nonsense script I wrote changes from:
to
A 50% reduction in number of lines - from 30 lines to 20, for a single wrong variable name and nonexistent command. The script itself is 15 lines long. Or see the stack-overflow test, where this removes 130 lines.
It would also be possible to reword this, while we're at it (I've seen things phrase line positions as "file:line", so "called on foo.fish:14").
TODOs:
I would be entirely fine if we decided to go with just the first one, or if we decided to put the rest off for 4.0, or if the rest gets in the way of someone's porting and I need to write it again later, or if we decide to go for a cleaner solution.