[BUG] parameter ... set in enclosing scope #781
Comments
|
Also, when I cloned the repository, and loaded plugin from the latest master, errors slightly relocated: |
|
All of the referenced variables are declared global before being set. The test framework also sets WARN_CREATE_GLOBAL to catch issues like these. Could you see if something else in your configuration is necessary to reproduce the issue? |
|
Interesting. I'm using my dotfiles repository, branch |
|
Looking briefly at the code, I don't even know what could be wrong with it, or how zsh imagines it should be. # Set $0 to the expected value, regardless of functionargzero.
0=${(%):-%N}
if true; then
# $0 is reliable
typeset -g ZSH_HIGHLIGHT_VERSION=$(<"${0:A:h}"/.version)
typeset -g ZSH_HIGHLIGHT_REVISION=$(<"${0:A:h}"/.revision-hash)
if [[ $ZSH_HIGHLIGHT_REVISION == \$Format:* ]]; then
# When running from a source tree without 'make install', $ZSH_HIGHLIGHT_REVISION
# would be set to '$Format:%H$' literally. That's an invalid value, and obtaining
# the valid value (via `git rev-parse HEAD`, as Makefile does) might be costly, so:
ZSH_HIGHLIGHT_REVISION=HEAD
fi
fiThe problem is reported at |
|
Oh, I'm sorry. I provided the wrong info. I have On a second thought, maybe I shouldn't be using this option. It seems too restrictive without obvious benefits. What do you think? |
The error message under master is in the second post. Given that the option exists, z-sy-h should support it. However, for
It seems to me that the option does have bug-finding potential. Feel free to discuss this further on zsh's support channels. |
|
I agree it seems silly to set in interactive use, but as @danielshahaf said we still ought to support it. I can't reproduce the interactive bug, but this avoids warnings when the test harness sets WARN_NESTED_VAR |
|
I can't reproduce the interactive bug, but this avoids warnings when the test harness sets WARN_NESTED_VAR
Thanks. Reviewing.
```
diff --git a/tests/test-highlighting.zsh b/tests/test-highlighting.zsh
index 8b564a8..cab2fe3 100755
--- a/tests/test-highlighting.zsh
+++ b/tests/test-highlighting.zsh
@@ -29,7 +29,7 @@
# -------------------------------------------------------------------------------------------------
-setopt NO_UNSET WARN_CREATE_GLOBAL
+setopt NO_UNSET WARN_CREATE_GLOBAL WARN_NESTED_VAR
Hmm. It's a minor issue, but it'd be useful to standardize the spelling of option names in z-sy-h (i.e., capitalization, underscores, and "no" prefix) for greppability (whether via grep(1), via eyeballing a screenful of code, via $EDITOR's "search in current file" functionalities [e.g., in Vim, «*» in normal mode — and I'm aware of «g*», but it doesn't work for finding «foo» when cursor is on «nofoo»], or via github's issues database's search).
Not a blocker, of course.
# Required for add-zle-hook-widget.
zmodload zsh/zle
@@ -85,6 +85,8 @@ else
exit 1
fi > >($results_filter | $root/tests/tap-colorizer.zsh)
+setopt NOWARN_NESTED_VAR
+
Why set the option for part of the initialization code and unset it here?
# Overwrite _zsh_highlight_add_highlight so we get the key itself instead of the style
_zsh_highlight_add_highlight()
{
@@ -154,7 +156,9 @@ run_test_internal() {
: ${CURSOR=$#BUFFER} ${PENDING=0} ${WIDGET=z-sy-h-test-harness-test-widget}
# Process the data.
+ setopt WARN_NESTED_VAR
_zsh_highlight
+ setopt NOWARN_NESTED_VAR
I wonder whether this unsetopt could break anything, e.g., if these three lines (not just the first two) are some day moved into a 'try' block and then «_zsh_highlight» croaks, the «always» block will be executed with the option remaining set. So, it feels like a try/always block is in order. (Or we could rely on LOCAL_OPTIONS being in effect.)
}; [[ -z $RETURN ]] || return $RETURN
unset ARG
diff --git a/zsh-syntax-highlighting.zsh b/zsh-syntax-highlighting.zsh
index d20dc5b..2a45136 100644
--- a/zsh-syntax-highlighting.zsh
+++ b/zsh-syntax-highlighting.zsh
@@ -45,7 +45,7 @@ if true; then
# When running from a source tree without 'make install', $ZSH_HIGHLIGHT_REVISION
# would be set to '$Format:%H$' literally. That's an invalid value, and obtaining
# the valid value (via `git rev-parse HEAD`, as Makefile does) might be costly, so:
- ZSH_HIGHLIGHT_REVISION=HEAD
+ typeset -g ZSH_HIGHLIGHT_REVISION=HEAD
I'm not sure I'm happy with this strategy. Since the variable was already declared global above, setting WARN_NESTED_VAR requires re-declaring the variable as global on every assignment (including assignments in the same scope, augmenting assignments, and «foo=${foo:#…}» assignments, but excluding slice assignments), makes the code harder to read, violates DRY, and introduces no semantic difference; in exchange, a «typeset -g» is added that reminds the reader that the variable is global. I would consider the costs to outweigh the benefit, since the all-uppercase name already conveys that variable is global. [This paragraph is also applicable to $ZSH_HIGHLIGHT_HIGHLIGHTERS, and even to $region_highlight with s/all-uppercase/well-known/.]
Incidentally, even though sourcing z-sy-h multiple times is supposed to work, the version info globals don't provide for that. Not a blocker, and low priority, of course.
(With my -workers@ hat:)
Would it make sense to disable WARN_NESTED_VAR at the C level for variables that are PM_SPECIAL or their name consists of uppercase ASCII and underscores only (and isn't all underscores)? (This wouldn't catch _all_ well-known variables, of course, since some of them aren't PM_SPECIAL: e.g., $REPLY, vcs_info's $hook_com.)
Would it make sense for WARN_NESTED_VAR to handle augmenting assignments and slice assignments the same way? I.e., either trip on both, or trip on neither?
fi
fi
@@ -130,7 +130,8 @@ _zsh_highlight()
}
# Probe the memo= feature, once.
- (( ${+zsh_highlight__memo_feature} )) || {
+ (( ${+zsh_highlight__memo_feature} )) || (){
+ setopt localoptions nowarnnestedvar
Anonymous functions break «return»/«exit» (by design) and «"$@"» (unless the pre-5.0.7 compatibility workaround). WDYT of avoiding an anonymous function here, in order to reduce the surface area for bugs to enter through?
region_highlight+=( " 0 0 fg=red, memo=zsh-syntax-highlighting" )
case ${region_highlight[-1]} in
("0 0 fg=red")
@@ -175,10 +176,10 @@ _zsh_highlight()
# Reset region_highlight to build it from scratch
if (( zsh_highlight__memo_feature )); then
- region_highlight=( "${(@)region_highlight:#*memo=zsh-syntax-highlighting*}" )
+ typeset -g region_highlight=( "${(@)region_highlight:#*memo=zsh-syntax-highlighting*}" )
else
# Legacy codepath. Not very interoperable with other plugins (issue #418).
- region_highlight=()
+ typeset -g region_highlight=()
fi
# Remove all highlighting in isearch, so that only the underlining done by zsh itself remains.
@@ -581,7 +582,7 @@ add-zsh-hook preexec _zsh_highlight_preexec_hook 2>/dev/null || {
zmodload zsh/parameter 2>/dev/null || true
# Initialize the array of active highlighters if needed.
-[[ $#ZSH_HIGHLIGHT_HIGHLIGHTERS -eq 0 ]] && ZSH_HIGHLIGHT_HIGHLIGHTERS=(main)
+[[ $#ZSH_HIGHLIGHT_HIGHLIGHTERS -eq 0 ]] && typeset -g ZSH_HIGHLIGHT_HIGHLIGHTERS=(main)
Does the assignment on line 239 need the same fix?
… if (( $+X_ZSH_HIGHLIGHT_DIRS_BLACKLIST )); then
print >&2 'zsh-syntax-highlighting: X_ZSH_HIGHLIGHT_DIRS_BLACKLIST is deprecated. Please use ZSH_HIGHLIGHT_DIRS_BLACKLIST.'
```
|
Which is to say: why don't we just unset the option? Cross-referencing #758. |
updateSorry for the spam. I wasn’t aware that this error message is about original commentWith my current Zsh configuration, I can reproduce the second bug: This is weird since I can clearly see the variable set as global in I’ll try finding a minimal example to reproduce. |
FranklinYu
added a commit
to FranklinYu/XDG-configuration
that referenced
this issue
Sep 14, 2021
This reverts commit 42332c6. Reason: maintainer of zsh-syntax-highlighting [doesn’t like][1] `warn_nested_var`. The other option, `warn_create_global`, might be fine, but more experiments should be done with it. [1]: zsh-users/zsh-syntax-highlighting#781 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
While sourcing
zsh-syntax-highlighting.zsh, zsh encounter couple of warnings.I have my
WARN_CREATE_GLOBALandWARN_NESTED_VARzsh options turned on.See also
#438
The text was updated successfully, but these errors were encountered: