Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refuse $status as a command #8171

Merged
merged 2 commits into from
Jul 27, 2021
Merged

Conversation

faho
Copy link
Member

@faho faho commented Jul 26, 2021

Description

Another one from the "please no more" department:

Using $status as a command is virtually guaranteed to be an error. Nobody has commands called "0", and if they do they certainly don't want them called at semi-random just because some command failed or succeeded.

So, we forbid $status specifically, and print a special error.

It would also be possible to check if $status was used after the command wasn't found, but that means it can't be done statically anymore. And, like I said, using $status this way is unlikely to be on purpose, and if it was on purpose it wasn't necessary and you can skip the cutesy trick.

(the first commit is because the test was throwing path errors otherwise, I don't know since when they've been printed, but they are bogus regardless)

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@faho faho added this to the fish 3.4.0 milestone Jul 26, 2021
@faho
Copy link
Member Author

faho commented Jul 26, 2021

Huh, surprising how github manages to capitalize the branch name into a title.

@ridiculousfish
Copy link
Member

This is nice, it will definitely catch some real errors. I suggest moving the check into detect_errors_in_decorated_statement, around here. The advantage of putting it there is that the error will be caught when the script is parsed, instead of when it is executed.

Copy link
Member

@krobelus krobelus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.
Moving the check will also avoid false negatives like set status_cmd echo 123; $status_cmd

src/parse_execution.cpp Outdated Show resolved Hide resolved
@faho
Copy link
Member Author

faho commented Jul 27, 2021

I'm sorry, is detect_errors_in_decorated_statement somehow not run for commands given via fish -c?

I've added this there, and if I do echo 'if $status; true; end' | ./fish I get the correct error, but if I do ./fish -c 'if $status; true; end' I get "Unknown command: 0"?

Also reproducable with e.g.

./fish -c 'echo foo | exec banana'

which fails with "Unknown command", and

./fish -c 'echo foo | exec grep'

which crashes with

error: /home/alfa/dev/fish-shell/src/exec.cpp:942: failed assertion: process_type_t::exec process found in pipeline, where it should never be. Aborting.

@faho
Copy link
Member Author

faho commented Jul 27, 2021

I'm sorry, is detect_errors_in_decorated_statement somehow not run for commands given via fish -c?

Okay, added a commit for that. There might be a cleaner way, but I don't know how or where.

@faho faho changed the title Status isnt everything Refuse $status as a command Jul 27, 2021
@faho faho force-pushed the status-isnt-everything branch 2 times, most recently from a7ea1b3 to f8f8daa Compare July 27, 2021 16:36
faho added 2 commits July 27, 2021 18:36
This is slightly unclean. Even tho it would otherwise be syntactically
valid, using $status as a command is very very very likely to be an
error, like

    if not $status

We have reports of this surprisingly regularly, including fish-shell#2773.

Because $status can only ever be a value from 0 to 255, it is also
very unlikely to be an actual command, and that command is very
unlikely to do what you want.

So we simply point the user towards the "conditions" help section,
that should explain things.
This didn't do all the syntax checks, so something like

    fish -c 'echo foo; and $status'

complained of a missing command `0` (i.e. $status), and

    fish -c 'echo foo | exec grep'

hit an assert!

So we do what read_ni does, parse each command into an ast, run
parse_util_detect_errors on it if it worked and then eval the ast.

It is possible to do this neater by modifying parser::eval, but I
can't find where.
@faho faho merged commit 29e9f48 into fish-shell:master Jul 27, 2021
@faho
Copy link
Member Author

faho commented Jul 27, 2021

Okay, merged. If someone finds a nicer way to detect_errors for the -c command, feel free to just do that. In the meantime this works.

@faho faho deleted the status-isnt-everything branch September 6, 2021 12:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants