Skip to content

fix(_comp_compgen_*): avoid conflicts with "-v var" #1028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 26, 2023

Conversation

akinomyoga
Copy link
Collaborator

To avoid conflicts between local variables and variable names specified by _comp_compgen -v var, the local variables basically need to have the name _foo.

Handling of conflicts with internal generator calls

When a generator needs to call other generators to store results in a local variable, the local variable name cannot have the name _foo because it is rejected by the internal generator calls. In such a case, we first declare the variable without the underscore (e.g. foo), call the internal generator to store the result to foo, move the results to an underscored variable (_foo), and remove the local variable foo by _comp_unlocal foo. The current version of the PR uses this approach.

However, maybe this is too complicated. Another possibility is to add a new option _comp_compgen -U foo to internally perform _comp_unlocal foo immediately before storing the results.

@akinomyoga
Copy link
Collaborator Author

Another possibility is to add a new option _comp_compgen -U foo to internally perform _comp_unlocal foo immediately before storing the results.

This is an example implementation for this direction.

diff --git a/bash_completion b/bash_completion
index 98c67eb0..0799824e 100644
--- a/bash_completion
+++ b/bash_completion
@@ -512,6 +512,7 @@ _comp_compgen()
     local _dir=""
     local _ifs=$' \t\n' _has_ifs=""
     local _icmd="" _xcmd=""
+    local -a _upvars=()

     local _old_nocasematch=""
     if shopt -q nocasematch; then
@@ -519,7 +520,7 @@ _comp_compgen()
         shopt -u nocasematch
     fi
     local OPTIND=1 OPTARG="" OPTERR=0 _opt
-    while getopts ':av:Rc:C:lF:i:x:' _opt "$@"; do
+    while getopts ':av:U:Rc:C:lF:i:x:' _opt "$@"; do
         case $_opt in
             a) _append=set ;;
             v)
@@ -529,6 +530,16 @@ _comp_compgen()
                 fi
                 _var=$OPTARG
                 ;;
+            U)
+                if [[ $OPTARG == @(*[^_a-zA-Z0-9]*|[0-9]*|'') ]]; then
+                    printf 'bash_completion: %s: -U: invalid variable name `%s'\''\n' "$FUNCNAME" "$OPTARG" >&2
+                    return 2
+                else if [[ $OPTARG == @(_*|IFS|OPTIND|OPTARG|OPTERR) ]]; then
+                    printf 'bash_completion: %s: -U: `%s'\'' unneccessary to mark upvar\n' "$FUNCNAME" "$OPTARG" >&2
+                    return 2
+                fi
+                _upvars+=("$OPTARG")
+                ;;
             c) _cur=$OPTARG ;;
             R) _cur="" ;;
             C)
@@ -573,6 +584,9 @@ _comp_compgen()
         if [[ $_has_ifs ]]; then
             printf 'bash_completion: %s: `-l'\'' and `-F sep'\'' are not supported for generators\n' "$FUNCNAME" >&2
             return 2
+        elif ((${#_upvars[@]})); then
+            printf 'bash_completion: %s: `-U var'\'' is not supported for generator calls\n' "$FUNCNAME" >&2
+            return 2
         fi

         local -a _generator
@@ -653,6 +664,7 @@ _comp_compgen()
         return
     }

+    ((${#_upvars[@]})) && _comp_unlocal "${_upvars[@]}"
     _comp_split -l ${_append:+-a} "$_var" "$_result"
 }

@akinomyoga
Copy link
Collaborator Author

I decided to use the approach of the previous reply #1028 (comment).

@akinomyoga
Copy link
Collaborator Author

Rebased & resolved conflicts.

@akinomyoga
Copy link
Collaborator Author

Added fixes mentioned in #1026.

scop added a commit that referenced this pull request Aug 20, 2023
Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slowly getting started with this, got some trivial bits merged out of the way.

shellcheck does not allow using a local scalar variable that has the
same variable name as an array variable used in another function
(SC2178,SC2179).  To use array `options` in another function, we
switch `options` in `_known_hosts` to an array variable (or otherwise,
we will need to place disable=SC2178,SC2179 to every line using the
variable as a scalar).
When `-U var` is specified for a builtin `compgen` call, variable
`var` is unlocalized by `_comp_unlocal` just before storing the result
to the target array.  When `-U var` is specified for a generator call,
variable `var` is unlocalized by `_comp_unlocal` just before calling
the generator function `_comp_compgen_G2` (where `G2` is the generator
name)..

A generator should basically define local variables with the names
starting with `_`.  However, a generator sometimes needs to use local
variable names that do not start with `_`.  When the child generator
call with a variable name (such as `local var; _comp_compgen -v var`)
is used within the generator, the local variable can unexpectedly mask
a local variable of the upper call.

For example, the following call fails to obtain the result of
generator `mygen1` because the array `arr` is masked by the same name
of a local variable in `_comp_compgen_mygen1`.

  # generator with a problem
  _comp_compgen_mygen1()
  {
      local -a arr=(1 2 3)
      _comp_compgen -av arr -- -W '4 5 6'
      _comp_compgen_set "${arr[@]/#p}"
  }

  _comp_compgen -v arr mygen1 # fails to get the result in array `arr`

To avoid this, a generator that defines a local variable that does not
start with `_` can use the option `-U var` to unlocalize the variable
on assigning the final result.

  # properly designed generator
  _comp_compgen_mygen1()
  {
      local -a arr=(1 2 3)
      _comp_compgen -av arr -- -W '4 5 6'
      _comp_compgen -U arr set "${arr[@]/#p}"
  }
@akinomyoga
Copy link
Collaborator Author

Rebased.

Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will take me quite some time to become comfortable with this, but I do see the point and think I even understand some parts of this stuff :) So let's go with this, thanks!

@scop scop merged commit e2e11e6 into scop:master Aug 26, 2023
@akinomyoga akinomyoga deleted the generator-vars branch August 26, 2023 11:52
@akinomyoga
Copy link
Collaborator Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants