fix: completion generation inside $()

[richardkmichael]

Closes #630

• python -m pytest test/t/unit has two failing tests and two skipped tests in test_unit_compgen_filedir.py. This is the same as on main, so I do not believe they are related to this patch.

• python -m mypy . and python -m ruff chec pass

• I am unsure what to make of the runLint output, but it has exit status 0, so I presume it is OK

[yedayak]

In main echo $(ip <TAB> completes all files (the default completion, it should complete sub-commands), but with this PR it doesn't complete anything. Maybe you can use _comp_command_offset for this?

[richardkmichael]

@yedayak Thanks for the quick review. I'll investigate.

[scop]

Just a note to set "changes requested" per earlier comments so this shows up as such in PR list.

[richardkmichael]

@yedayak

In main echo $(ip <TAB> completes all files (the default completion, it should complete sub-commands), but with this PR it doesn't complete anything. Maybe you can use _comp_command_offset for this?

Thanks for mentioning _comp_command_offset.

All completions should now be correct, e.g.,

bash-5.3$ echo $(git describe --<TAB>
--abbrev         --broken         --debug          --exclude=       --match=         --tags
--all            --candidates=    --dirty          --first-parent   --no-...
--always         --contains       --exact-match    --long           --no-contains


# Empty command substitution completion is empty, consistent with empty prompt completion.
bash-5.3$ echo $(<TAB>
bash-5.3$ <TAB>

Handling nesting and completion position is a little complicated, so I commented the code somewhat more than other areas. Let me know if I should reduce them -- I'd find it helpful being new to the code base, but it might be verbose for experts.

• I added more test cases
• I checked lint and reviewed the code style and API guidelines
• I ran the GitHub Actions in my fork

[yedayak]

Nice, thanks for adding tests
Maybe add test for something like echo $(csub_cmd) where the substitution is closed?
Edit: Also maybe one where one subsitution is closed but the outer one is still opened, echo $(csub_cmd $(csub_cmd)

[yedayak]

Suggested change

	inner_line="${COMP_WORDS[*]:nested_idx}"
	inner_line="${COMP_WORDS[@]:nested_idx}"

[richardkmichael]

I believe either will work for the scalar assignment. However, pre-commit shellcheck fails with @. Should it be disabled?

# shellcheck disable=SC2124
inner_line="${COMP_WORDS[@]:nested_idx}"

[akinomyoga]

[*] is affected by IFS, while [@] always joins the words by whitespace. Actually, one can set local IFS=$' \t\n' and use [*].

[richardkmichael]

Thanks for the explanation. Given the preference for @, what should be done about shellcheck?

[akinomyoga]

Actually, we have never used @ in this context. I think we just used IFS and * for this type of processing.

[yedayak]

Yeah sorry, I didn't know about the shellcheck rule. [*] with setting IFS locally is fine

[akinomyoga]

I searched the codebase. It seems we haven't had similar cases (with [*] or [@] in the right-hand side of a scalar assignment) so far. The following cases explicitly set IFS to a special value:

./completions-core/gnokii.bash-229-    local IFS='|'
./completions-core/gnokii.bash:230:    local regex_main_cmd="(${main_cmd[*]})($|[^_[:alnum:]])"
--
./completions-core/invoke-rc.d.bash-20-        local IFS='|'
./completions-core/invoke-rc.d.bash:21:        local exclude="@(${words[*]})"
--
./completions-core/lintian.bash-33-        local IFS='|'
./completions-core/lintian.bash:34:        todisable="@(${todisable[*]})"
--
./completions-core/mr.bash-23-    local IFS='|'
./completions-core/mr.bash:24:    local glob_commands="@(${commands[*]})"

The following case is not accompanied by any adjustment of IFS, so it's broken with a custom IFS:

./completions-core/tree.bash:37:    local line="${words[*]}"

If we search for ${a[*]} not restricted in the right-hand sides of assignments, there seem to be many cases affected by custom IFS, which I think we can work on in another PR.

There was no occurrence of "${a[@]}" in the right-hand sides of assignments.

[richardkmichael]

Thank you 👍

I did a similar exploration after this feedback.

I was unsure about using IFS, due to scope effects elsewhere -- especially since there is no RHS scalar assignment like this in bash_completion, and _comp_initialize is a function called quite early. For example, it will affect _comp_command_offset.

I also wondered if, when IFS is used, it's set at the top of a function (i.e., for reader visibility), or instead near the site needed / affected. The approaches seems evenly split. For example, _comp_split is at the top, so is ssh-keygen.bash. Usage in _comp_compgen is at the usage site with a comment, so I've set it just above usage as well. (I also considered a new function, to confine IFS scope, but it seemed unnecessary.)

Aside from this PR --

Since lint can't be used for this, an addition to the quoting section of the style guide could be helpful. Perhaps something like:

diff --git a/doc/styleguide.md b/doc/styleguide.md
index cff6d95b..436a631b 100644
--- a/doc/styleguide.md
+++ b/doc/styleguide.md
@@ -171,6 +171,10 @@ exceptions where the quoting is still needed for other reasons.
   suggests that e.g. `var= cmd` is confusing with `var=cmd`.
 - `$*` and `${array[*]}` need to be always quoted because they can be affected
   by the word splitting in bash <= 4.2 even in the above contexts.
+- When joining array elements into a scalar variable, prefer `"${array[*]}"`
+  over `"${array[@]}"`, and ensure `IFS` is set to the intended separator
+  (typically `local IFS=$' \t\n'`), because `[*]` joins by the first character
+  of `IFS`.
 - In the following contexts, double-quoting of shell expansions is needed
   unless the result of expansions is intentionally treated as glob patterns or
   regular expressions.

I used an LLM for code exploration and review (help ensuring adherence to the style guide and API naming documentation), and such an explanation would have caught this usage.

[akinomyoga]

For example, _comp_split is at the top,

That is because IFS is determined by the options specified to _comp_split, and we need to give the default value at the top of _comp_split before starting the argument parsing.

so is ssh-keygen.bash.

I think that is because ${words[*]} and ${protocols[*]} are used many times (8 times) in the function, and the smallest common scope seems to be the function scope.

However, there doesn't seem to be a particular reason for IFS set at the top of _comp_load. Git Blame reveals that it was introduced in commit d9082d2, but I don't remember the reason that I put it at the top. The IFS is necessary for the line $(complete -p "$cmd" 2>/dev/null || echo false) "\\$cmd" && return 0, so it can be set just above this line technically.

(I also considered a new function, to confine IFS scope, but it seemed unnecessary.)

Yeah, that is another possibility, something like _comp_join.

[richardkmichael]

Maybe add test for something like echo $(csub_cmd) where the substitution is closed?

Added. I noticed the fixture directory, so I used it for stable echo completion. I considered csub_cmd $(echo hi) , but I wondered if the echo <TAB> suggestion was to have one real completion.

Edit: Also maybe one where one subsitution is closed but the outer one is still opened, echo $(csub_cmd $(csub_cmd)

Added, and thank you, this caught a bug. I hadn't considered the _comp_split returning COMP_WORDS with $( and its ) complement in two different words, e.g., (... "$(echo" "hi)" ...), so the counting and traversal was wrong.

[akinomyoga]

The current implementation falls into an infinite loop with the following completion attempt:

$ cmd1 $(cmd2 a$(cmd3)b$(cmd4 [tab]

Also, the command name is wrongly extracted to be pwd)/$(cmd3 with the following completion attempt:

$ cmd1 $(cmd2 $(pwd)/$(cmd3 [tab]

Actually, I have difficulty understanding the choice of the present algorithm. Even if the above issues are fixed somehow, is this algorithm stable and robust in general? Since I currently don't see a clear path or picture to understand the construction of the current algorithm, the algorithm appears to be a random trial currently.

Can you explain what is the assumptions of the algorithm, and how it is ensured to work under the given assumptions?

[richardkmichael]

Thank you!

Given the failures, yes, one assumption that words do not contain unbalanced pairs is a mistake, and another being that the first $( in the COMP_WORD is correct one. Evidently not robust. :) I was trying to find the word right-adjacent to the last open $( but the pair-wise counting by line length difference won't work, and the strip is to the wrong $(. I wouldn't say "random trial" :-) but definitely too naive. I was experimenting with test cases on my command line, and hoping to avoid writing a parens matcher with character position.

I'll go through it more carefully, for another approach.

[akinomyoga]

Thank you for the explanation!

Thank you for thinking about another approach! If it turns out to be difficult, we don't necessarily have to support all possible cases. I think the important thing is that we want to gracefully fail in the unsupported cases (instead of causing an infinite loop or producing a strange result); for example, if the required assumptions are clear, we may examine the contents of cur and give up the special treatment when the assumptions are unsatisfied.