Support `$foo/bar` in command position: parameter expansions with pasted constant strings

[segevfiner]

[phy1729]

Something like

diff --git a/highlighters/main/main-highlighter.zsh b/highlighters/main/main-highlighter.zsh
index 647eb8b..e905268 100644
--- a/highlighters/main/main-highlighter.zsh
+++ b/highlighters/main/main-highlighter.zsh
@@ -492,10 +492,14 @@ _zsh_highlight_highlighter_main_paint()
         local -a match mbegin mend
         local MATCH; integer MBEGIN MEND
         if [[ $res == none ]] && (( ${+parameters} )) &&
-           [[ ${arg[1]} == \$ ]] && [[ ${arg:1} =~ ^([A-Za-z_][A-Za-z0-9_]*|[0-9]+)$ ]] &&
+           [[ ${arg[1]} == \$ ]] && [[ ${${arg:1}%%/*} =~ ^([A-Za-z_][A-Za-z0-9_]*|[0-9]+)$ ]] &&
            (( ${+parameters[${MATCH}]} ))
            then
-          _zsh_highlight_main__type ${(P)MATCH}
+          if [[ $arg == */* ]]; then
+            _zsh_highlight_main__type ${(P)MATCH}/${arg#*/}
+          else
+            _zsh_highlight_main__type ${(P)MATCH}
+          fi
           res=$REPLY
         fi
       }

would work, but that's just special casing $var/string when many different characters can end a variable name.

I'm not sure this case should handled better. The current condition is a decent mix of correctness and simplicity. zsh-syntax-highlighting cannot just outsource the job of expanding parameters to zsh since parameter expansion can have side effects (e.g. ${foo:=bar}). Although I wonder if we can test for ( (to exclude glob qualifiers which can execute code) and if there's no ( expand $arg in a subshell.

[danielshahaf]

would work, but that's just special casing `$var/string` when many different characters can end a variable name.

It wouldn't be hard to rewrite it to support «$var.string», «$var,string», etc — strip the leading dollar-followed-by-alphanumerics and see what the rest of the string is — but then what about «$foo/$bar». In the end it's, as you say, a tradeoff between generality and simplicity (which relates to correctness, readability, etc). It would probably be useful to highlight «$var:hey» as «$var:h» followed by «ey». I'll file a separate issue to investigate that.

Although I wonder if we can test for `(` (to exclude glob qualifiers which can execute code) and if there's no `(` expand `$arg` in a subshell.

Do you mean parameter expansion flags «${(foo)bar}» or glob qualifiers «./foo(bar)»? I'm not sure off the top of my head, but in general, in such situations it's better to use a whitelist than a blacklist; i.e., not to blacklist '(' but to enumerate constructs that are explicitly known to work. (For example, blacklisting '(' wouldn't block «${:-`foo`}», and if you block backticks you wouldn't be able to be sure that there isn't some _third_ syntax that's problematic. Even allowing arbitrary parameter names could be problematic with parameters implemented by modules, e.g., imagine a module-provided $AUTOINCREMENT that increments by 1 every time it's eval'd.)

[danielshahaf]

Opened #485.

[phy1729]

I certainly prefer a whitelist, and agree it's the correct approach. From the $AUTOINCREMENT example it sounds like no parameters should ever be expanded since they could have side effects. I would not object to that and it's certainly a clearer line than what we have now or might have in the foreseeable future.

[danielshahaf]

$AUTOINCREMENT was a made-up example. Let's ask upstream's opinion.

[danielshahaf]

Posted users/23095. While writing that I realised that evaluating only if ${parameters[$foo]} != *special* would probably be safe (but let's see what the thread results in).

[danielshahaf]

Assigning this back to the pool: the $AUTOINCREMENT issue was handled by #489.

[danielshahaf]

Issue sweep: two separate concerns here:

1. Don't make the command word red when we don't know whether or not it'll execute successfully or not.
2. Support ${foo}/bar in command position where bar is a constant string.

We have #695 for (1) so I'm repurposing this issue to track (2). Patches welcome.