Forbid subcommand keywords in variables-as-commands

[faho]

This stops you from doing e.g.

set pager command less
echo foo | $pager

Currently, it would run the command builtin, which can only do --search and similar, and would most likely end up printing its own help.

That means it very very likely won't work, and the code is misguided - it is trying to defeat function resolution in a way that won't do what the author wants it to.

The alternative would be to make the command builtin execute the command, but

1. That would require rearchitecting and rewriting a bunch of it and the parser
2. It would be a large footgun, in that set EDITOR command foo will only ever work inside fish, but $EDITOR is also used outside.

I don't want to add a feature that we would immediately have to discourage.

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
• This reuses the "subcommand" keywords list, which should be okay. Alternatively we can restrict it to just "builtin", "command" and "function", but that would lack "if" and "while", which people could also be trying

[mqudsi]

The change itself LGTM although I'm not sure how much this is warranted — but I assume you have seen a lot of this and this is in response?

The behavior this would intercept seems like a fundamental misunderstanding of what variables are and the difference between variable expansion and the use of eval. Which leads me to ask

1. Do we want to point people to eval in cases like this?
2. How do we feel about filling the codebase with workarounds for people that aren't (yet) understanding the fundamentals of shell architecture and behavior?

[faho]

The behavior this would intercept seems like a fundamental misunderstanding of what variables are and the difference between variable expansion and the use of eval

I'm fine with keywords not being seen as such in variable expansion. You wouldn't want to have set EDITOR "if" foo ";" bar "else" baz "end". I also believe allowing set EDITOR command foo to run the command foo does more harm than good (because now e.g. git won't open the correct editor). That's all okay.

My big problem here is that the error behavior is pretty terrible. We have a command builtin in addition to the keyword for mostly internal technical reasons, and this leaks the fact, and tries to run builtin command which isn't prepared for it and doesn't do anything sensible.

To clarify, you do set EDITOR command vim and run $EDITOR and you get the entirety of the command man page.

And the best place I can find to improve that is directly in the variable-expansion code, because if you changed it in the command builtin it would have to answer how you even got there and explain how not to get there. And we would have to do that in the if builtin and the while builtin and a bunch of others.

understanding the fundamentals of shell architecture and behavior?

I disagree that this specific case is all that fundamental. It's a weird and subtle edge case interaction between a number of different parts, exacerbated by our builtin/keyword duality.

And yes, we have a bunch of tweaks to prevent common errors by new people - try if $status or function test.

[krobelus]

This looks fine but I think we should consider removing the decorators from the ast and and make them proper builtins.
I think the existing state is historical. Unless we care about the highlighting.
I don't see a reason why set EDITOR command vim; $EDITOR shouldn't work.
I don't think it's a footgun because as soon as you run that outside the shell you get the "command not found" error.
The problem in #10248 was that we didn't give a good error message in fish but that will go away once we support this.

[faho]

I don't think it's a footgun because as soon as you run that outside the shell you get the "command not found" error.

That is why it's a footgun.

A "footgun" is a feature that is easy to misuse. The metaphor is a gun that is, by default, aimed at your feet when you hold it. In this case we know that's the case because the use we have in front of us is, by the definition of $EDITOR, already buggy. You set it, it works for some things, and then later you find out that git commit suddenly fails.

It does not appear to me that $EDITOR is an unusual use of variables-as-commands, I would assume that's the typical use.

Basically: We would enable something that we would then always recommend people don't use because it is only ever correct in edge cases.

Think about it in terms of teaching - if we enabled it and you saw someone doing set EDITOR command vim you would have to tell them "yeah that won't work with external commands, use set EDITOR (command -s vim) and it will always work". Why not just avoid that entirely?

[krobelus]

EDITOR is a bad example; what I wanted to say is it's perfectly valid for variables that are used inside the shell.
I don't have much time to delve into this now so I may be wrong but I believe the language would be better off if the commands were normal builtins.
They don't have any special syntactic requirements so I think they should work like other builtins (set x eval echo 123; $x etc.).
We wouldn't prohibit that either

[faho]

EDITOR is a bad example; what I wanted to say is it's perfectly valid for variables that are used inside the shell.

EDITOR is the typical example for variables-as-commands. $EDITOR, $PAGER and a few like this are widely set and used.

I don't have much time to delve into this now so I may be wrong but I believe the language would be better off if the commands were normal builtins.

I don't really mind if internally these things have the keyword/builtin duality or not.

I can tell you that having them as decorators makes some parts cleaner - like asking "what's the command for this commandline".

Note that this PR also touches on if and while etc, which would otherwise just exit. Which makes it seem to me like the builtins are a liability more than the keywords are for them. That's what makes keyword highlighting awkward (if is highlighted as a command until you arrive at if x, when it turns into a keyword).

They don't have any special syntactic requirements so I think they should work like other builtins (set x eval echo 123; $x etc.).

The difference there is that "set" is a thing on its own. It does something, it sets variables.

By contrast command doesn't perform a task, it changes how the next word is interpreted. The important thing about command foo is the "foo". That's why it's kinda natural as a decorator.

And tbh I question how often anyone would stumble over set EDITOR set foo bar. There's an attractive nuisance use for command in a variable, I don't think there really is for set or e.g. argparse.

[krobelus]

I can tell you that having them as decorators makes some parts cleaner - like asking "what's the command for this commandline".

interesting

They don't have any special syntactic requirements so I think they should work like other builtins (set x eval echo 123; $x etc.).

The difference there is that "set" is a thing on its own. It does something, it sets variables.

By contrast command doesn't perform a task, it changes how the next word is interpreted. The important thing about command foo is the "foo". That's why it's kinda natural as a decorator.

I was talking about command vs. eval not set. I'm not sure if there is a reason why we want to treat command and eval differently (or perhaps we declare eval as weird)

I don't feel great about making half-informed comments, probably not a good use of everyone's time.