main: Allow for "]" in shell aliases

[knl]

PR #776 fixed an issue with complex aliases and expansion. However, this change
also introduced a problem with aliases which contain ] (for example, commonly
seen on macOS: alias ]=open), due to using an associative array seen_alias,
indexed by the alias name. Due to "$seen_alias[$arg]", it would fail when
$arg is expanded to anything containing ]'. Thus, typing ] / would result
in:

> ] /
(anon):unset:3: seen_alias[]]: invalid parameter name

This change fixes the issue by ensuring we properly access keys in the
associative array seen_alias.

Older versions of zsh have issues with map keys having special
characters, especially lacking ways to remove such keys. The
issue is described in detail in
https://unix.stackexchange.com/questions/626393/in-zsh-how-do-i-unset-an-arbitrary-associative-array-element.

This fix uses proposal from
zsh-workers/43269,
discovered by Stephane Chazelas, that boils down to avoid removing keys
from the map, and reconstruct the map anew with some keys omitted.

Co-authored-by: @phy1729

[phy1729]

I think unset "seen_alias[${(q)alias_name}]" will fix the failing tests and is sufficient to handle the ] case; however, (e)ing is still a good idea for pattern like aliases. Could you add some tests to ensure this isn't broken in the future? There's a script tests/generate.zsh to assist with generating test cases and it outputs usage when run with no arguments. In this case the preamble would be something like alias ]=open.

Small nit but in commit messages we generally prepend the highlighter name or component and then a colon. Could you prepend main: to the commit message? (It otherwise looks great.)

Thanks for catching this and submitting a PR!

[knl]

Hi @phy1729, thanks for a quick response and suggestions. I updated the commit message, as well as PR title.

I also changed to use (q), which allowed me to have all tests pass on my machine:

❯ make quiet-test
/Library/Developer/CommandLineTools/usr/bin/make test QUIET=y
ZSH_PATCHLEVEL=zsh-5.8-0-g77d203f
Running test brackets
Running test main
Running test pattern
Running test regexp

However, I don't understand why tests are failing on CI.

[phy1729]

In the commit pushed to github (be93c7d), (q) is still used on the unset line. May have just been missed in the rebase.

[knl]

In the commit pushed to github (be93c7d), (q) is still used on the unset line. May have just been missed in the rebase.

Sorry, I don't follow?

I reduced the changes to the minimum necessary. All the tests pass on my machines (macOS and Raspbian), yet they fail on the CI. I really don't understand where is the discrepancy.

[phy1729]

The branch passes locally here as well. I'll take a closer look at the CI system later today.

[danielshahaf]

The failure in CI is:

## alias-brackets
# BUFFER=$'] /'
not ok 1 - [1,1] «]» - expected (1 1 "alias"), observed (1 1 "unknown-token"). 
ok 2 - [3,3] «/»
ok 3 - cardinality check
# expected_region_highlight  region_highlight
# '1 1 alias'                '0 1 unknown-token'
# '3 3 path'                 '2 3 path'

I can reproduce that locally with both ZSH_PATCHLEVEL=debian/5.7.1-1 and ZSH_PATCHLEVEL=zsh-5.8-177-gd14a924 which I had lying around.

[knl]

I've tried running it locally (Darwin Mojave) on 4 different versions of zsh, and tests succeed every time:

knl@fenrir ❯ make quiet-test ZSH=/nix/store/ydg3mmn5gmnkq5gf11vm3sx94x2cyh1k-zsh-5.7.1/bin/zsh
/Library/Developer/CommandLineTools/usr/bin/make test QUIET=y
ZSH_PATCHLEVEL=zsh-5.7.1-0-g8b89d0d
Running test brackets
Running test main
Running test pattern
Running test regexp

~/work/github.com/knl/zsh-syntax-highlighting ·· (fix-seen-aliases ?1) ·········································································· 7s [00:14:03]
knl@fenrir ❯ make quiet-test ZSH=/nix/store/k78pqmpyggi7rkk80a0chq9cmm94nknv-home-manager-path/bin/zsh
/Library/Developer/CommandLineTools/usr/bin/make test QUIET=y
ZSH_PATCHLEVEL=zsh-5.8-0-g77d203f
Running test brackets
Running test main
Running test pattern
Running test regexp

~/work/github.com/knl/zsh-syntax-highlighting ·· (fix-seen-aliases ?1) ·········································································· 8s [00:14:43]
knl@fenrir ❯ make quiet-test ZSH=/usr/local/bin/zsh
/Library/Developer/CommandLineTools/usr/bin/make test QUIET=y
ZSH_PATCHLEVEL=zsh-5.8-0-g77d203f
Running test brackets
Running test main
Running test pattern
Running test regexp

~/work/github.com/knl/zsh-syntax-highlighting ·· (fix-seen-aliases ?1) ·········································································· 8s [00:14:59]
knl@fenrir ❯ make quiet-test ZSH=/bin/zsh
/Library/Developer/CommandLineTools/usr/bin/make test QUIET=y
ZSH_PATCHLEVEL=zsh-5.3-0-g4cfdbdb
Running test brackets
Running test main
Running test pattern
Running test regexp

However, when I run on my raspberry pi, I get:

raspberrypi% make quiet-test
make test QUIET=y
make[1]: Entering directory '/home/pi/work/zsh-syntax-highlighting'
ZSH_PATCHLEVEL=raspbian/5.7.1-1
Running test brackets
Running test main

## alias-brackets
# BUFFER=$'] /'
not ok 1 - [1,1] «]» - expected (1 1 "alias"), observed (1 1 "unknown-token").
ok 2 - [3,3] «/»
ok 3 - cardinality check
# expected_region_highlight  region_highlight
# '1 1 alias'                '0 1 unknown-token'
# '3 3 path'                 '2 3 path'
Running test pattern
Running test regexp
make[1]: *** [Makefile:40: test] Error 1
make[1]: Leaving directory '/home/pi/work/zsh-syntax-highlighting'
make: *** [Makefile:51: quiet-test] Error 2

I'll try to dig further.

The notable difference is that I have .zshrc set on Darwin, with the given alias, while RPi is pristine.

[danielshahaf]

Tests run under zsh -f which ignores all dotfiles except for the systemwide zshenv.

Note that Raspbian is a Debian derivative.

[danielshahaf]

Could the failure be caused by the RHS of the alias?

[knl]

Could the failure be caused by the RHS of the alias?

Yes, it is caused by using open as the RHS of the alias. That command doesn't exist on Linux.

Thanks for helping @danielshahaf !

[knl]

I've just installed zsh 4.3.11 to test the failures on CI, and it seems this is causing problems: https://unix.stackexchange.com/questions/626393/in-zsh-how-do-i-unset-an-arbitrary-associative-array-element

tl;dr in older zsh, it's not possible to unset map elements that contain ], \, and some other special cases...

If there is no other proposal, I'll suggest that we don't insert values directly, but map them. For example, we represent any string as its ascii/utf8 representation. That is, instead of storing ] directly in seen_alias, we transform ] into 5d (its ascii hex code) and then store that string.

[phy1729]

The solution given by Stephane Chazelas in workers/43269 appears to work as long as the resulting array is non-empty. A bit of code refactoring can take care of that. Not sure if there's a significant speed impact though.

diff --git a/highlighters/main/main-highlighter.zsh b/highlighters/main/main-highlighter.zsh
index 4ea3f34..5ec6985 100644
--- a/highlighters/main/main-highlighter.zsh
+++ b/highlighters/main/main-highlighter.zsh
@@ -601,16 +601,17 @@ _zsh_highlight_main_highlighter_highlight_list()
       (( in_alias[1]-- ))
       # Remove leading 0 entries
       in_alias=($in_alias[$in_alias[(i)<1->],-1])
-      (){
-        local alias_name
-        for alias_name in ${(k)seen_alias[(R)<$#in_alias->]}; do
-          unset "seen_alias[$alias_name]"
-        done
-      }
       if (( $#in_alias == 0 )); then
         seen_alias=()
         # start_pos and end_pos are of the alias (previous $arg) here
         _zsh_highlight_main_add_region_highlight $start_pos $end_pos $alias_style
+      else
+        (){
+          local alias_name
+          for alias_name in ${(k)seen_alias[(R)<$#in_alias->]}; do
+            seen_alias=("${(@kv)seen_alias[(I)^$arg]}")
+          done
+        }
       fi
     fi
     if (( in_param )); then
@@ -890,7 +891,7 @@ _zsh_highlight_main_highlighter_highlight_list()
         (){
           local alias_name
           for alias_name in ${(k)seen_alias[(R)<$#in_alias->]}; do
-            unset "seen_alias[$alias_name]"
+            seen_alias=("${(@kv)seen_alias[(I)^$arg]}")
           done
         }
         if [[ $arg != '|' && $arg != '|&' ]]; then

[danielshahaf]

@phy1729 The loop body doesn't use the loop variable.

[knl]

@phy1729 The loop body doesn't use the loop variable.

Thanks for catching that. I updated the second commit on this PR to use the loop variable, and all tests pass.

[phy1729]

The diff looks ok to me. Just should rebase it back down to one commit. Credit in the message should go to Stephane Chazelas as he found the workaround; all I did was the obvious application of it.

[knl]

@danielshahaf @phy1729 I've rebased into a single commit and hopefully gave correct credits. Please take another look and thanks for the help!

[danielshahaf]

@knl The rebase and log message look good. I was going to leave reviewing this to @phy1729, but since you asked:

Is there a measurable performance impact (e.g., due to using the (I) subscript flag)?

If there is, we could consider what to do about it: e.g., accept it, or use (I) with a pattern, or use unset when possible, or teach upstream's unset the needed functionality.

The last one seems like an independently good idea, actually. Upstream's code is here: https://github.com/zsh-users/zsh/blob/5ede2c55f144593c16498c3131a76e188114a9c6/Src/builtin.c#L3746-L3750

[phy1729]

Merged. Thanks for the contributions and sticking with it!