Skip to content

'main': Highlight pipes inside array assignments as errors #651

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions highlighters/main/main-highlighter.zsh
Original file line number Diff line number Diff line change
Expand Up @@ -683,18 +683,35 @@ _zsh_highlight_main_highlighter_highlight_list()

# The Great Fork: is this a command word? Is this a non-command word?
if [[ -n ${(M)ZSH_HIGHLIGHT_TOKENS_COMMANDSEPARATOR:#"$arg"} ]]; then
# First, determine the style of the command separator itself.
if _zsh_highlight_main__stack_pop T || _zsh_highlight_main__stack_pop Q; then
# Missing closing square bracket(s)
style=unknown-token
elif [[ $this_word == *':regular:'* ]]; then
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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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:'

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

(';') style=unknown-token;;
($'\n') style=default;;
esac
elif [[ $this_word == *':regular:'* ]] && ! $in_array_assignment; then
# This highlights empty commands (semicolon follows nothing) as an error.
# Zsh accepts them, though.
style=commandseparator
else
style=unknown-token
fi
# Second, determine the style of next_word.
if [[ $arg == ';' ]] && $in_array_assignment; then
# literal newline inside an array assignment
# Literal semicolon or literal newline inside an array assignment
next_word=':regular:'
else
next_word=':start:'
Expand Down
39 changes: 39 additions & 0 deletions highlighters/main/test-data/array-cmdsep1.zsh
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#!/usr/bin/env zsh
# -------------------------------------------------------------------------------------------------
# Copyright (c) 2019 zsh-syntax-highlighting contributors
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without modification, are permitted
# provided that the following conditions are met:
#
# * Redistributions of source code must retain the above copyright notice, this list of conditions
# and the following disclaimer.
# * Redistributions in binary form must reproduce the above copyright notice, this list of
# conditions and the following disclaimer in the documentation and/or other materials provided
# with the distribution.
# * Neither the name of the zsh-syntax-highlighting contributors nor the names of its contributors
# may be used to endorse or promote products derived from this software without specific prior
# written permission.
#
# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR
# IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
# FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
# IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
# -------------------------------------------------------------------------------------------------
# -*- mode: zsh; sh-indentation: 2; indent-tabs-mode: nil; sh-basic-offset: 2; -*-
# vim: ft=zsh sw=2 ts=2 et
# -------------------------------------------------------------------------------------------------

BUFFER=$'a=( foo | bar )'

expected_region_highlight=(
'1 3 assign' # a=(
'5 7 default' # foo
'9 9 unknown-token' # |
'11 13 unknown-token' # bar
'15 15 unknown-token' # )
)
39 changes: 39 additions & 0 deletions highlighters/main/test-data/array-cmdsep2.zsh
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#!/usr/bin/env zsh
# -------------------------------------------------------------------------------------------------
# Copyright (c) 2019 zsh-syntax-highlighting contributors
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without modification, are permitted
# provided that the following conditions are met:
#
# * Redistributions of source code must retain the above copyright notice, this list of conditions
# and the following disclaimer.
# * Redistributions in binary form must reproduce the above copyright notice, this list of
# conditions and the following disclaimer in the documentation and/or other materials provided
# with the distribution.
# * Neither the name of the zsh-syntax-highlighting contributors nor the names of its contributors
# may be used to endorse or promote products derived from this software without specific prior
# written permission.
#
# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR
# IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
# FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
# IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
# -------------------------------------------------------------------------------------------------
# -*- mode: zsh; sh-indentation: 2; indent-tabs-mode: nil; sh-basic-offset: 2; -*-
# vim: ft=zsh sw=2 ts=2 et
# -------------------------------------------------------------------------------------------------

BUFFER=$'a=( foo ; bar )'

expected_region_highlight=(
'1 3 assign' # a=(
'5 7 default' # foo
'9 9 unknown-token' # ;
'11 13 default' # bar
'15 15 assign' # )
)
39 changes: 39 additions & 0 deletions highlighters/main/test-data/array-cmdsep3.zsh
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#!/usr/bin/env zsh
# -------------------------------------------------------------------------------------------------
# Copyright (c) 2019 zsh-syntax-highlighting contributors
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without modification, are permitted
# provided that the following conditions are met:
#
# * Redistributions of source code must retain the above copyright notice, this list of conditions
# and the following disclaimer.
# * Redistributions in binary form must reproduce the above copyright notice, this list of
# conditions and the following disclaimer in the documentation and/or other materials provided
# with the distribution.
# * Neither the name of the zsh-syntax-highlighting contributors nor the names of its contributors
# may be used to endorse or promote products derived from this software without specific prior
# written permission.
#
# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR
# IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
# FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
# IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
# -------------------------------------------------------------------------------------------------
# -*- mode: zsh; sh-indentation: 2; indent-tabs-mode: nil; sh-basic-offset: 2; -*-
# vim: ft=zsh sw=2 ts=2 et
# -------------------------------------------------------------------------------------------------

BUFFER=$'a=( foo \n bar )'

expected_region_highlight=(
'1 3 assign' # a=(
'5 7 default' # foo
'9 9 default' # \n
'11 13 default' # bar
'15 15 assign' # )
)
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ BUFFER=$'foo=(\nbar) env'

expected_region_highlight=(
'1 5 assign' # foo=(
'6 6 commandseparator' # \n
'6 6 default' # \n
'7 9 default' # bar
'10 10 assign' # )
'12 14 command' # env
Expand Down