Attempt to fix #670

[QBobWatson]

Don't highlight parameters based on parameter expansion if the expansion type is
'none'.

As noted in the code comments, it's useful to highlight a parameter whose value happens to be a reserved word, command, etc. as the value would be highlighted. However, if the type is "none", then there's not enough information to highlight the argument, so just highlight the un-expanded parameter.

[danielshahaf]

Thanks for looking into this! Triaged. (For me this is higher priority than #680 because #670 is milestoned for an earlier release than #680.)

[danielshahaf]

Correct me if I'm wrong, but if the type is none (and the value isn't empty), doesn't that mean we should highlight as unknown-token? I assume this would happen anyway because there's no command/function literally called $foo (4 bytes), but still.

[danielshahaf]

Also, the PR fails tests.

[QBobWatson]

Correct me if I'm wrong, but if the type is none (and the value isn't empty), doesn't that mean we should highlight as unknown-token? I assume this would happen anyway because there's no command/function literally called $foo (4 bytes), but still.

If the type is none then the logic for the none case in the command-word code gets run, which is what would've happened if the parameter-expansion code weren't there at all. I believe this is the correct thing to do.

I've fixed the thing that broke the test -- the subcase of none that deals with command paths is now handled. I'm not sure how useful this is, since if x=$PWD and you have a command called foobar in the current directory, then $x/foobar is highlighted as unknown-token because parameter_name is parsed as x/foobar, which doesn't satisfy $parameter_name_pattern. (The same thing happens without the PR.)

I'm not willing to rewrite the pattern matching code to deal with things like $x/foobar. I recommend intentionally limiting the pattern matching code to only deal with the simple patterns $x and ${x} in command position that expand to reserved words, aliases, builtins, and functions, and to delete the path expansion code (and associated test). Or if you prefer, the current PR fixes the bug and leaves most previous functionality intact.

[danielshahaf]

If the type is none then the logic for the none case in the command-word code gets run, which is what would've happened if the parameter-expansion code weren't there at all. I believe this is the correct thing to do.

Right, $res will already be none, but why doesn't the code set arg=$testarg in the none case? Basically, this:

diff --git a/highlighters/main/main-highlighter.zsh b/highlighters/main/main-highlighter.zsh
index aa50589..1730b02 100644
--- a/highlighters/main/main-highlighter.zsh
+++ b/highlighters/main/main-highlighter.zsh
@@ -642,13 +642,8 @@ _zsh_highlight_main_highlighter_highlight_list()
             ;;
         esac
         _zsh_highlight_main__type "$testarg" 0
-        if [[ -n $REPLY ]] && [[ $REPLY != none ]]; then
-            res=$REPLY
-            arg=$testarg
-        elif _zsh_highlight_main_highlighter_check_path $testarg; then
-            res=none
-            arg=$testarg
-        fi
+        res=$REPLY
+        arg=$testarg
       fi
     }
 

Tests pass with and without this.

I've fixed the thing that broke the test -- the subcase of none that deals with command paths is now handled.

Thanks.

I'm not sure how useful this is, since if x=$PWD and you have a command called foobar in the current directory, then $x/foobar is highlighted as unknown-token because parameter_name is parsed as x/foobar, which doesn't satisfy $parameter_name_pattern. (The same thing happens without the PR.)

I recommend intentionally limiting the pattern matching code to only deal with the simple patterns $x and ${x} in command position that expand to reserved words, aliases, builtins, and functions

Yes, that's exactly how it's designed. $x/foobar was never supported and isn't planned work either. (I'm not opposed to adding it, but if anyone wants that to happen, they should file a separate feature request; making that happen is not part of the regression that blocks 0.7.0.)

I'm not willing to rewrite the pattern matching code to deal with things like $x/foobar.

Supporting $x/foobar would be a feature request, and as such is out of scope of what we're trying to do here, which is to fix #670.2.

Or if you prefer, the current PR fixes the bug and leaves most previous functionality intact.

What do you mean, "most" previous functionality? Are there some cases that worked before that don't work now?

[QBobWatson]

Right, $res will already be none, but why doesn't the code set arg=$testarg in the none case? Basically, this:
...
Tests pass with and without this.

This was what happened before the PR. If arg is foobar=$(ls) then the parameter will be highlighted as in #670. This is why you have to intentionally limit the cases in which you highlight the parameter as its expansion would be highlighted, and pass through the un-expanded parameter otherwise.

What do you mean, "most" previous functionality? Are there some cases that worked before that don't work now?

Well, anything else that highlighted when $res = none will of course be disabled, but I think those cases were unintended behavior exactly like #670 . For instance, if foobar='!!' then $foobar would be highlighted as history-expansion, even though running $foobar on the command line results in zsh: command not found: !!.

[danielshahaf]

This was what happened before the PR.

Right, so the diff I posted wouldn't be the complete fix.

This is why you have to intentionally limit the cases in which you highlight the parameter as its expansion would be highlighted, and pass through the un-expanded parameter otherwise.

Sorry, but i think that's wrong. What you're saying is, in terms of the language semantics, like saying that parameter expansion in command position only happens when the expansion of the parameter is itself valid at command position or is a path. That is not how the language works.

The distinction between $arg and $testarg, on the other hand, is spot on. We should make a distinction between the word in $BUFFER and the word after parameter and alias expansions. The former should be used for $region_highlight calculations and the latter for parsing.

Well, anything else that highlighted when $res = none will of course be disabled, but I think those cases were unintended behavior exactly like #670 . For instance, if foobar='!!' then $foobar would be highlighted as history-expansion, even though running $foobar on the command line results in zsh: command not found: !!.

Yes, that's wrong too. We don't currently observe the order of expansions. However, I don't consider this defect to be a blocker.

[danielshahaf]

You may have seen #670 (comment) where I posted several previous attempts to fix #670. I just found what was wrong about one of those attempts, fixed it, and posted that as #684. So now I have to choose which PR to merge: #675, #682 (this), or #684.

[danielshahaf]

Closed per discussion on #684. (Would have said so earlier but my client indicated the [Close] action had failed.)

[danielshahaf]

For instance, if foobar='!!' then $foobar would be highlighted as history-expansion, even though running $foobar on the command line results in zsh: command not found: !!.

Fixed on #684.