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

Give syntax commands their own colorization #7678

Closed
IlanCosman opened this issue Feb 4, 2021 · 9 comments · Fixed by #7689
Closed

Give syntax commands their own colorization #7678

IlanCosman opened this issue Feb 4, 2021 · 9 comments · Fixed by #7689

Comments

@IlanCosman
Copy link
Contributor

IlanCosman commented Feb 4, 2021

In "real" languages, syntax elements like if, for, while, etc., are given their own color. While I know (at least I think I do) that Fish's if, end etc. are really just commands, it would still be nice if they were given their own color to distinguish them from "regular" commands.

@faho
Copy link
Member

faho commented Feb 4, 2021

These are "keywords", not normal commands - otherwise if wouldn't introduce a block or bind to an else.

Coloring them seems fair.

@faho faho added this to the fish-future milestone Feb 4, 2021
@kopischke
Copy link

kopischke commented Feb 4, 2021

Sorry for jumping in, but as I happen to be working my way through a fish syntax plugin for the Nova macOS editor, I have had to think about the same highlighting issue. My takeaways:

  1. highlighting fish keywords like if, while et al. as keywords is pretty obviously the right thing to do; however,
  2. what about fish's reserved commands (i.e. those builtins that get special treatment insofar as they cannot be overridden by functions; currently argparse read, string, set, statusand test)? Should these be marked as keyword equivalents to denote their special status? I’ve tentatively opted for “yes” in my plugin, but would of course love to align this with fish’s own logic.

@faho
Copy link
Member

faho commented Feb 4, 2021

Should these be marked as keyword equivalents to denote their special status?

Probably not, but it depends on what you want. For our highlighter, we're not going to do that because it's not worth pointing out, especially not in a terminal where formatting options are limited.

Does this special status help you write better code? I don't really think so, the only case where it matters is when you're thinking of defining a function by that name (in which case you could color the function name as invalid), otherwise they are just normal builtins. Do you color builtins differently?

@faho
Copy link
Member

faho commented Feb 4, 2021

I've had a look at this, and it's simple enough - we already see that they are keywords, we just don't pick another highlighting role for them.

One caveat, tho: if and a few others are only a keyword once followed by something that isn't --help, to make if --help work. That means the naive solution colors it as a command until you type an argument and then switches to a keyword. We probably don't want to expose that oddity.

(tbh we might want to remove it entirely, I'm not sure anyone uses it - we used to have a hack to make a literal count --help print the help and we removed that)

@kopischke
Copy link

kopischke commented Feb 4, 2021

Probably not, but it depends on what you want. For our highlighter, we're not going to do that because it's not worth pointing out, especially not in a terminal where formatting options are limited.

That makes sense.

Does this special status help you write better code? I don't really think so, the only case where it matters is when you're thinking of defining a function by that name (in which case you could color the function name as invalid), otherwise they are just normal builtins. Do you color builtins differently?

No, I don’˝t, just the reserved ones, for the exact reason you outlined; to signal they cannot be overridden by functions. Alas, identifying invalid function definitions is only feasible in a very naive, fragile way when statically highlighting code (unlike fish itself, I cannot evaluate variables and substitutions; I can‘t even assume fish is present on the user’s system).

Sorry for being so lengthy (and In perfectly understand if you want to mark this OT), but I would actually appreciate your thoughts on on that, if you find the time? Would you tend to say the special casing as an override prevention is not worth introducing ambiguities in highlighting at all? That would actually simplify my syntax rules file …

@IlanCosman
Copy link
Contributor Author

I wouldn't bother having a different color for reserved builtins. It's incredibly uncommon that you would ever want to override a builtin, not the mention making it vastly harder to parse the code for anyone unfamiliar with your changes. Just don't do it lol.
Commands, builtins, and reserved builtins should all be colorized the same way, because, at least to the user, those differences are very rarely important.

@kopischke
Copy link

kopischke commented Feb 4, 2021

Hmm, I see; I’ll mull that over, thanks a lot for bearing with me. Out of interest; on which side of the builtin command / keyword divide would and, or, not, continue, break, builtin, command, time and exec fall? All of these are subsumed under “keywords” here – but then, so are the reserved commands (the fish source calls them reserved_keywords so … ¯_(ツ)_/¯, I guess?).

@faho
Copy link
Member

faho commented Feb 4, 2021

Out of interest; on which side of the builtin command / keyword divide would and, or, not, continue, break, builtin, command, time and exec fall?

In short: All keywords.

In longer: There is no one-size-fits-all here, they are handled differently.

and and or are almost normal builtins, except they bind to an if-condition:

if a; and b

used to be seen as having and b inside the actual if-block, so you had to do

if begin; a; and b; end

which was super awkward, so fish 2.3 (IIRC) changed it.

builtin, command, time and exec are "decorations" - they still expect a command instead of replacing it (well, time can also still take another decoration - time command grep is valid, command time grep is something very different).

continue and break appear in command position but are invalid outside of loops, and that's an error while parsing - echo foo; continue will refuse to execute even the echo.

(all of this is ignoring the thing --help special case - in that case they are commands, but only if the --help appears literally)

so are the reserved commands (the fish source calls them reserved_keywords so

Yeah, just "reserved word" would probably be a tad clearer - they don't have syntactical significance and everything you can do wrong with them is a runtime error, not a parsing one, so they aren't in a programming language sense "keywords".

@kopischke
Copy link

kopischke commented Feb 4, 2021

In short: All keywords.

Good to know, that would have been my interpretation too. As to the differences in usage, I’m, by now painfully, aware of these. Teaching a statical highlighting system based on regex’ and capture precedence positional semantics (for lack of a better term) does have a tendency to make you acutely conscious of even minute nuances of a shell’s keyword semantics and of their relationship to “ordinary” commands (in fish’s case, for instance, I’d argue and and or are far closer to if et al. than to “almost normal builtins”). Better not go into this – you have no idea into what dark corners exploring fish syntax permutations and trying to map valid / invalid variants onto highlighting robustly and reliably can take the unwary traveler. That way madness lies!

As to thing --help for keywords … is that even documented anywhere? It’s most certainly not on the respective command help page of keywords. One learns something new every day …

@faho faho closed this as completed in #7689 Feb 7, 2021
@faho faho modified the milestones: fish-future, fish 3.2.0 Feb 7, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants