Open arkzo opened 2 years ago
No. Yes.
Hi, can I take this up? What could be a possible fix?
Sure. Welcome aboard.
The fix is going to involve state machine work in the main highlighter, but we're not there yet. We should first establish exactly which syntaxes are / aren't valid, both when SHORT_LOOPS is set and when it isn't.
Upstream's documentation doesn't fully answer the question: it just says "suitably delimited, such as […]" but doesn't give a complete specification of valid and invalid cases.
Thanks :)
I have read the part of the documentation in the link, and if I'm understanding it correctly, invalid cases exist only for if
, while
and until
when the condition part is not delimited.
The delimited part should be of the form '[[ ... ]]` or '(( ... ))'.
Do we have to establish valid syntaxes for these two forms? If yes, what should be taken into consideration for defining which are valid and which aren't? Also, what exactly is going wrong here, is the highlighter not able to distinguish between the condition part and the body?
I have read the part of the documentation in the link, and if I'm understanding it correctly, invalid cases exist only for
if
,while
anduntil
when the condition part is not delimited.The delimited part should be of the form '[[ ... ]]` or '(( ... ))'.
Do we have to establish valid syntaxes for these two forms?
I'm not asking you to establish valid syntaxes for shell tests and for arithmetic expressions. I'm asking what, apart from the [[
builtin and the arithmetic evaluation statement-like syntax (( … ))
, zsh's manual means when it says "suitably delimited".
Also, what exactly is going wrong here, is the highlighter not able to distinguish between the condition part and the body?
No. The highlighter gets a stream of tokens —
% () { print -raC1 -- "${(@qqqq)${(z)argv}}" } 'while (( i++ < 10 )) { echo i is $i }'
$'while'
$'(( i++ < 10 ))'
$'{'
$'echo'
$'i'
$'is'
$'$i'
$'}'
— and has to determine whether that {
introduces a block (as it does in f() { foo; }
), is an ordinary word (as in echo bar { baz
), or is a syntax error (as in the last brace of { } }
, and as in repeat 42 pwd
with SHORT_LOOPS and SHORT_REPEAT both unset).
I guess what specifically happens is that the }
is recognized as special but the {
isn't, so the ${braces_stack} logic kicks in and flags the }
as an error.
Thanks for the detailed reply. The manual does not mention any other forms apart from [[ ... ]]
and (( ... ))
, so I think we can safely assume that these are the only two.
Thanks for the detailed reply. The manual does not mention any other forms apart from
[[ ... ]]
and(( ... ))
,
I'm well aware of that.
so I think we can safely assume that these are the only two.
Please check that assumption. If it's correct, we should report a documentation bug to zsh upstream. If it's not, we can move on to implementing this in z-sy-h.
Could you please tell me how to check that assumption? I tried searching through the zsh source code repository to find the exact implementation of the syntax but couldn't find it.
grepping for instances of SHORTLOOPS
finds several instances in Src/parse.c. The easiest of them to read seems to be the one in par_while(), which parses its condition here:
The meaning of SEPER
wasn't very clear, so I committed https://github.com/zsh-users/zsh/commit/5c9713603d571fd228efc4c25c0efc9064d95a87.
This suggests "suitably delimited" means "is a zshmisc(1) 'list' that, when parsed, is followed by do
or by {
that's a token" (let's not worry about the CSH_JUNKIE_LOOPS case for now).
This checks out: while (( 1 )) && (( 2 )) { pwd }
works, and with the braces removed, it works iff SHORT_LOOPS is set.
So:
while (( 1 )) && (( 2 )) { pwd }
while (( 1 )) && (( 2 )) pwd
Those two cases should be tested both with and without SHORT_LOOPS.
We can borrow additional test cases from zsh's own test suite: https://github.com/zsh-users/zsh/blob/5c9713603d571fd228efc4c25c0efc9064d95a87/Test/E01options.ztst#L1117-L1119
for
, while
and until
forms mentioned in the docs.I think that I'm missing something; the syntax without braces not working is confusing to me. Could you please link to some documentation about SHORT_LOOPS? I tried searching what SHORT_LOOPS option does but couldn't find related stuff.
So the first thing that needs to done is updating the documentation, right? Which file needs to be edited for this?
Yes, we can update the docs to make it clearer what is meant by "suitably delimited" and what alternate short forms are acceptable.
:+1:
What should be considered for determining which syntax should be supported? To me, it seems reasonable to support the
for
,while
anduntil
forms mentioned in the docs.
The criteria for supporting a syntax include:
So, feel free to add support for whichever SHORT_LOOPS syntaxes you may choose to spend time on — but expect a PR adding support for the short form of select
to be lower-priority to merge than one adding support for the short form of for
, for example. (Personally, I use short if
and repeat
regularly too; others may have different use patterns.)
I think that I'm missing something; the syntax without braces not working is confusing to me. Could you please link to some documentation about SHORT_LOOPS? I tried searching what SHORT_LOOPS option does but couldn't find related stuff.
The manual always spells the option name like you just did, so you can just grep zshall(1) for "SHORT_LOOPS" and find everything relevant:
https://zsh.sourceforge.io/Doc/Release/Options.html#index-SHORT_005fLOOPS https://zsh.sourceforge.io/Doc/Release/Shell-Grammar.html#Alternate-Forms-For-Complex-Commands (linked above)
So the first thing that needs to done is updating the documentation, right? Which file needs to be edited for this?
https://github.com/zsh-users/zsh/blob/master/Doc/Zsh/grammar.yo
Patches for that go to zsh-workers@zsh.org. See Etc/zsh-development-guide in that repository as well (the "Documentation" section). And thanks!
It is possible to write a loop using a different syntax like:
Issue:
The syntax highlighting fails on the { echo i is $i } part. Has this been a conscious decision made in the past? If not is it possible to fix this?