parameters-multi-word-machinery-v2

[danielshahaf]

Fix #670.

This is a revision of #675, an extension of the branch linked from #670 (comment), and obsoletes the patch posted in that comment. Relation to #682 TBD.

Reviews, please? If no issues are found, I'd like to merge this, wait for regression reports from users of master, and if none are received, release 0.7.0.

[QBobWatson]

Works for me. Feel free to close #682.

[danielshahaf]

Will do. Thanks for the testing #684 and for writing #682!

[danielshahaf]

Not sure the commit from yesterday is quite right: would changing the condition have fixed the issue had the if block's _zsh_highlight_main_add_region_highlight $start_pos $end_pos assign; continue been style=assign?

Blacklisting assign when in_param is correct, because words expanded from parameters can't form syntactic assignments; however, I think it might have fixed only one bug, out of two. (Cf. the Swiss cheese model)

Also, review other _zsh_highlight_main_add_region_highlight calls, and the underlying $arg-v.-its-expansion issue.

[danielshahaf]

Not sure the commit from yesterday is quite right: would changing the condition have fixed the issue had the if block's _zsh_highlight_main_add_region_highlight $start_pos $end_pos assign; continue been style=assign?

Making that change on master fails tests, but passes them if if (( unsorted )) is changed to if true in the test harness. I'm not sure whether it should be made — it's a code simplification but IIRC $region_highlight is order-sensitive, so this could affect highlighting using custom themes. (US)Tabling it for now.

Blacklisting assign when in_param is correct, because words expanded from parameters can't form syntactic assignments; however, I think it might have fixed only one bug, out of two. (Cf. the Swiss cheese model)

That bug can be seen by changing the else branch of that if-else chain to simulate what used to happen (until two days ago) in the assignment case:

diff --git a/highlighters/main/main-highlighter.zsh b/highlighters/main/main-highlighter.zsh
index ea10268..44713a8 100644
--- a/highlighters/main/main-highlighter.zsh
+++ b/highlighters/main/main-highlighter.zsh
@@ -894,7 +894,7 @@ _zsh_highlight_main_highlighter_highlight_list()
                           _zsh_highlight_main__stack_pop 'R' reserved-word
                         else
                           if _zsh_highlight_main_highlighter_check_path $arg; then
-                            style=$REPLY
+                            _zsh_highlight_main_add_region_highlight $start_pos $end_pos $REPLY
                           else
                             style=unknown-token
                           fi

Then, trigger it:

% tests/generate.zsh '$x foo' main x 'local x=/etc/passwd'
Set copyright year to 2020? y
local x=/etc/passwd

BUFFER=$'$x foo'

expected_region_highlight=(
  '1 2 path' # $x
  '1 2 unknown-token' # $x
  '4 6 default' # foo
)

The fact that the first addition's style is path rather than unknown-token is of independent interest; the point of the example is that the 1 2 range is added twice.

[danielshahaf]

the point of the example is that the 1 2 range is added twice.

This happens because _zsh_highlight_main_add_region_highlight is called twice: once by the loop body and once by the (( in_param == 1 )) && … case after the loop. That can only happen when the unexpanded parameter is the last word in the list ($BUFFER / subshell / cmdsubst / etc).

Presumably, the condition for that after-the-loop call should be changed. Perhaps something like (( ${#list_highlights} == 0 )) could work. However, I am not convinced that fixing this is part of the blocker. The blocker was fixing the BUG message; using the multi-word machinery addresses that, doesn't it? If that's so, then we can (a) merge this PR, (b) fix #670, (c) downgrade the case in the previous comment to a normal bug (a low-priority bug, in fact, since it's latent), (d) release 0.7.0 at long last…

If anyone sees a flaw in this plan, please speak up.

[danielshahaf]

Try adding return after line 87 to prevent the _zsh_highlight_main_add_region_highlight call in the loop from adding anything to $list_highlights.

[danielshahaf]

With the diff in #684 (comment):

## path-dollared-word3
# BUFFER='$PWD; ${PWD}'
1..4
ok 1 - [1,4] «$PWD»
not ok 2 - [1,4] «$PWD» - expected (5 5 "commandseparator"), observed (1 4 "unknown-token"). 
not ok 3 - [5,5] «;» - expected (7 12 "path"), observed (5 5 "commandseparator"). 
not ok 4 - have 3 expectations and 5 region_highlight entries: «expected_region_highlight=( '1 4 path' '5 5 commandseparator' '7 12 path' )» «region_highlight=( '0 4 path' '0 4 unknown-token' '4 5 commandseparator' '6 12 path' '6 12 unknown-token' )» 
Running test pattern
Running test regexp
make[1]: Leaving directory '/home/daniel/in/zsh-syntax-highlighting'

With that diff and the return as described in the previous comment:

## path-dollared-word3
# BUFFER='$PWD; ${PWD}'
1..4
not ok 1 - [1,4] «$PWD» - expected (1 4 "path"), observed (1 4 "unknown-token"). 
ok 2 - [5,5] «;»
not ok 3 - [7,12] «${PWD}» - expected (7 12 "path"), observed (7 12 "unknown-token"). 
ok 4 - cardinality check 
Running test pattern
Running test regexp
make[1]: Leaving directory '/home/daniel/in/zsh-syntax-highlighting'

(Note: That's just the differences between the two test runs. In both test runs, alias-to-dir (#668) XPASSed and abspath-in-command-position1, …, abspath-in-command-position4 failed with have 1 expectations and 2 region_highlight entries. The failures are expected, since that diff isn't intended to be committed but only to allow the latent bug to be reproduced.)

The first output shows the double entries; the second one does not. The second one also has unknown-token rather than path, which is correct behaviour when AUTO_CD is unset (cf. #669). It's also analogous to the alias case, so I'll add it.

[danielshahaf]

Rebased to master. Still have to handle the two WIP commits that tests fail on — can't merge that as-is, for future bisectors' benefit.

[danielshahaf]

I've edited the history to remove the fallback assignments to $param_style (since tests passed without them) and to make tests pass on every commit in the history (for future bisects). I pushed that edit as a new branch, https://github.com/zsh-users/zsh-syntax-highlighting/compare/master...danielshahaf:parameters-multi-word-machinery-v3?expand=1, since I didn't want to lose the pre-rebase version of the history.

[danielshahaf]

Merged in 2b3638a.