'main': Highlight pipes inside array assignments as errors

[danielshahaf]

No description provided.

[phy1729]

Instead of pushing the logic into each case where z-sy-h needs to distinguish between ; and \n; perhaps it'd be better to assign arg=$'\n' as appropriate when updating proc_buf. WDYT?

[phy1729]

Something like this (passes tests, but should probably be more thoroughly reviewed. (Has the side effect of more correctly handling histchars=$'!^\n' if one is so insane. Although it seems to mishandle alias $'\n'=...,)

diff --git a/highlighters/main/main-highlighter.zsh b/highlighters/main/main-highlighter.zsh
index c0ca575..731ada4 100644
--- a/highlighters/main/main-highlighter.zsh
+++ b/highlighters/main/main-highlighter.zsh
@@ -334,7 +334,7 @@ _zsh_highlight_highlighter_main_paint()
   fi
 
   ZSH_HIGHLIGHT_TOKENS_COMMANDSEPARATOR=(
-    '|' '||' ';' '&' '&&'
+    '|' '||' ';' $'\n' '&' '&&'
     '|&'
     '&!' '&|'
     # ### 'case' syntax, but followed by a pattern, not by a command
@@ -509,6 +509,11 @@ _zsh_highlight_main_highlighter_highlight_list()
       (( start_pos = end_pos + offset ))
       (( end_pos = start_pos + $#arg ))
 
+      # The zsh lexer considers ; and newline to be the same token, so (z)
+      # converts all newlines to semicolons. Convert them back here to make
+      # later processing simplier.
+      [[ $arg == ';' && $proc_buf[1] == $'\n' ]] && arg=$'\n'
+
       # Compute the new $proc_buf. We advance it
       # (chop off characters from the beginning)
       # beyond what end_pos points to, by skipping
@@ -694,8 +699,7 @@ _zsh_highlight_main_highlighter_highlight_list()
      else
        style=unknown-token
      fi
-     if [[ $arg == ';' ]] && $in_array_assignment; then
-       # literal newline inside an array assignment
+     if [[ $arg == $'\n' ]] && $in_array_assignment; then
        next_word=':regular:'
      else
        next_word=':start:'

[danielshahaf]

Agreed in principle. I didn't write it this way because I figured YAGNI: at the time there was only one instance, so the factoring out could be deferred until needed.

I don't object to DTRTing from day one but I haven't reviewed all uses of ';' in the code to assess destabilizingness of that. Can do that later.

[danielshahaf]

There aren't any literal uses of ';', so we're clear on that front.

I adapted your diff onto the current branch as follows:

diff --git a/highlighters/main/main-highlighter.zsh b/highlighters/main/main-highlighter.zsh
index 195cead..2088e11 100644
--- a/highlighters/main/main-highlighter.zsh
+++ b/highlighters/main/main-highlighter.zsh
@@ -335,6 +335,7 @@ _zsh_highlight_highlighter_main_paint()
 
   ZSH_HIGHLIGHT_TOKENS_COMMANDSEPARATOR=(
     '|' '||' ';' '&' '&&'
+    $'\n' # ${(z)} returns ';' but we convert it to $'\n'
     '|&'
     '&!' '&|'
     # ### 'case' syntax, but followed by a pattern, not by a command
@@ -509,6 +510,11 @@ _zsh_highlight_main_highlighter_highlight_list()
       (( start_pos = end_pos + offset ))
       (( end_pos = start_pos + $#arg ))
 
+      # The zsh lexer considers ';' and newline to be the same token, so
+      # ${(z)} converts all newlines to semicolons. Convert them back here to
+      # make later processing simplier.
+      [[ $arg == ';' && $proc_buf[1] == $'\n' ]] && arg=$'\n'
+
       # Compute the new $proc_buf. We advance it
       # (chop off characters from the beginning)
       # beyond what end_pos points to, by skipping
@@ -688,20 +694,12 @@ _zsh_highlight_main_highlighter_highlight_list()
        # Missing closing square bracket(s)
        style=unknown-token
      elif $in_array_assignment && [[ $arg == ';' ]] && [[ $this_word == *':regular:'* ]]; then
-       # Okay, this is hairy.
-       # 
-       # Literal newlines inside array assignment are just fine.
-       #
-       # Literal semicolons inside array assignments are accepted by zsh (don't ask),
-       # but we want to highlight them as errors anyway, because _nobody_ expects
-       # them to behave as they do.
-       #
-       # However, BOTH cases are reported as ";" by ${(z)}, so we have to
-       # actually check the buffer:
-       case "${buf[start_pos+1,end_pos]}" in
-         (';')   style=unknown-token;;
-         ($'\n') style=default;;
-       esac
+       # Literal semicolons inside array assignments are accepted by zsh and silently
+       # ignored.  However, we want to highlight them as errors anyway, because
+       # _nobody_ expects them to behave as they do.
+       style=unknown-token-XXX
+     elif $in_array_assignment && [[ $arg == $'\n' ]] && [[ $this_word == *':regular:'* ]]; then
+       style=default
      elif [[ $this_word == *':regular:'* ]] && ! $in_array_assignment; then
        # This highlights empty commands (semicolon follows nothing) as an error.
        # Zsh accepts them, though.
@@ -710,8 +708,7 @@ _zsh_highlight_main_highlighter_highlight_list()
        style=unknown-token
      fi
      # Second, determine the style of next_word.
-     if [[ $arg == ';' ]] && $in_array_assignment; then
-       # Literal semicolon or literal newline inside an array assignment
+     if [[ $arg == (';'|$'\n') ]] && $in_array_assignment; then
        next_word=':regular:'
      else
        next_word=':start:'

However, it fails one of the new tests:

# array-cmdsep3
1..6
ok 1 - [1,3] «a=(»
ok 2 - [5,7] «foo»
not ok 3 - [9,9] «↵» - expected (9 9 "default"), observed (9 9 "unknown-token-XXX").
ok 4 - [11,13] «bar»
ok 5 - [15,15] «)»
ok 6 - cardinality check

I haven't been able to reproduce this failure. If I run zsh -f, source z-sy-h, enable function tracing, and type in a=(foo <Shift+Enter> bar), the tracing doesn't show unknown-token-XXX anywhere. Hints?

(I added the XXX for debugging purposes, to demonstrate where exactly the style setting that fails the test comes from. The XXX won't be committed.)

[danielshahaf]

Implemented and pushed to master:

e58e452 tests: Add some tests for unusual or invalid elements in array assignments:
a4525a0 'main': Add infrastructure for treating literal newlines differently to semicolons.
3ca93f8 'main': Highlight literal semicolons in array assignments as errors.
bfd44f5 noop: Add comments.
81267ca 'main': Highlight pipes inside array assignments as errors
498cc76 tests: Extend and document the after-a-parse-error aspects of the issue #651 test.
c5878ae changelog: Update through HEAD.

The second commit implements a variant of your suggestion. (At that point, ${proc_buf[1]} is the intertoken whitespace.)

I didn't test the alias $'\n'= and histchars=$'!^\n' cases.