Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[builtin] Fix compadjust crashes with empty and sparse COMP_ARGV #2213

Merged
merged 2 commits into from
Jan 2, 2025
Merged

[builtin] Fix compadjust crashes with empty and sparse COMP_ARGV #2213

merged 2 commits into from
Jan 2, 2025

Conversation

akinomyoga
Copy link
Collaborator

@akinomyoga akinomyoga commented Jan 2, 2025 •
edited
Loading

There are two crashes with compadjust.

With the current master branch,

$ bin/osh -c 'COMP_ARGV=(); compadjust cur words'
Traceback (most recent call last):
  File "/home/murase/.mwg/git/oilshell/oil/bin/oils_for_unix.py", line 202, in <module>
    sys.exit(main(sys.argv))
  File "/home/murase/.mwg/git/oilshell/oil/bin/oils_for_unix.py", line 171, in main
    return AppBundleMain(argv)
  File "/home/murase/.mwg/git/oilshell/oil/bin/oils_for_unix.py", line 141, in AppBundleMain
    return shell.Main('osh', arg_r, environ, login_shell, loader, readline)
  File "/home/murase/.mwg/git/oilshell/oil/core/shell.py", line 1213, in Main
    cmd_flags=cmd_eval.IsMainProgram)
  File "/home/murase/.mwg/git/oilshell/oil/core/main_loop.py", line 375, in Batch
    is_return, is_fatal = cmd_ev.ExecuteAndCatch(node, cmd_flags)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 2098, in ExecuteAndCatch
    status = self._Execute(node)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 1897, in _Execute
    status = self._Dispatch(node, cmd_st)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 1735, in _Dispatch
    status = self._ExecuteList(node.children)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 1974, in _ExecuteList
    status = self._Execute(child)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 1897, in _Execute
    status = self._Dispatch(node, cmd_st)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 1624, in _Dispatch
    status = self._DoSimple(node, cmd_st)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 906, in _DoSimple
    status = self._RunSimpleCommand(cmd_val, cmd_st, run_flags)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 567, in _RunSimpleCommand
    run_flags)
  File "/home/murase/.mwg/git/oilshell/oil/core/executor.py", line 452, in RunSimpleCommand
    return self.RunBuiltin(builtin_id, cmd_val)
  File "/home/murase/.mwg/git/oilshell/oil/core/executor.py", line 313, in RunBuiltin
    return self.RunBuiltinProc(builtin_proc, cmd_val)
  File "/home/murase/.mwg/git/oilshell/oil/core/executor.py", line 323, in RunBuiltinProc
    status = builtin_proc.Run(cmd_val)
  File "/home/murase/.mwg/git/oilshell/oil/builtin/completion_osh.py", line 485, in Run
    cur = adjusted_argv[-1]
IndexError: list index out of range

Commit e0df355 fixes this.

Even having that fixed, there is another crash:

$ bin/osh -c 'COMP_ARGV=(); COMP_ARGV[1]=hello; compadjust cur prev cword words'
Traceback (most recent call last):
  File "/home/murase/.mwg/git/oilshell/oil/bin/oils_for_unix.py", line 202, in <module>
    sys.exit(main(sys.argv))
  File "/home/murase/.mwg/git/oilshell/oil/bin/oils_for_unix.py", line 171, in main
    return AppBundleMain(argv)
  File "/home/murase/.mwg/git/oilshell/oil/bin/oils_for_unix.py", line 141, in AppBundleMain
    return shell.Main('osh', arg_r, environ, login_shell, loader, readline)
  File "/home/murase/.mwg/git/oilshell/oil/core/shell.py", line 1213, in Main
    cmd_flags=cmd_eval.IsMainProgram)
  File "/home/murase/.mwg/git/oilshell/oil/core/main_loop.py", line 375, in Batch
    is_return, is_fatal = cmd_ev.ExecuteAndCatch(node, cmd_flags)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 2098, in ExecuteAndCatch
    status = self._Execute(node)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 1897, in _Execute
    status = self._Dispatch(node, cmd_st)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 1735, in _Dispatch
    status = self._ExecuteList(node.children)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 1974, in _ExecuteList
    status = self._Execute(child)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 1897, in _Execute
    status = self._Dispatch(node, cmd_st)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 1624, in _Dispatch
    status = self._DoSimple(node, cmd_st)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 906, in _DoSimple
    status = self._RunSimpleCommand(cmd_val, cmd_st, run_flags)
  File "/home/murase/.mwg/git/oilshell/oil/osh/cmd_eval.py", line 567, in _RunSimpleCommand
    run_flags)
  File "/home/murase/.mwg/git/oilshell/oil/core/executor.py", line 452, in RunSimpleCommand
    return self.RunBuiltin(builtin_id, cmd_val)
  File "/home/murase/.mwg/git/oilshell/oil/core/executor.py", line 313, in RunBuiltin
    return self.RunBuiltinProc(builtin_proc, cmd_val)
  File "/home/murase/.mwg/git/oilshell/oil/core/executor.py", line 323, in RunBuiltinProc
    status = builtin_proc.Run(cmd_val)
  File "/home/murase/.mwg/git/oilshell/oil/builtin/completion_osh.py", line 479, in Run
    completion.AdjustArg(a, break_chars, adjusted_argv)
  File "/home/murase/.mwg/git/oilshell/oil/core/completion.py", line 139, in AdjustArg
    for i, c in enumerate(arg):
TypeError: 'NoneType' object is not iterable

Commit b6f85b5 fixes it.

@akinomyoga akinomyoga changed the title [builtin/completion_osh] Fix compadjust crashes with empty and sparse COMP_ARGV [builtin] Fix compadjust crashes with empty and sparse COMP_ARGV Jan 2, 2025
@akinomyoga akinomyoga changed the base branch from master to soil-staging January 2, 2025 12:46
akinomyoga
akinomyoga commented Jan 2, 2025
View reviewed changes
builtin/completion_osh.py
Comment on lines +479 to +480
if a is not None:
completion.AdjustArg(a, break_chars, adjusted_argv)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I currently skip the unset elements, but an alternative behavior is to treat it as an empty string so that the indices of the words in COMP_ARGV would be preserved in the resulting cur, prev, cword, and words.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, to be honest I am not sure what is better

I'm not sure anyone uses "compadjust", but the reason I added it because I noticed the same hack being copied between the bash-completion and the git-completion.bash maintained by git

So I put it in the OSH interpreter instead

However now I feel kind of like this person

https://news.ycombinator.com/item?id=42536846

For context, I was the maintainer of FreeBSD's "bash-completion" port for a few years way back when

After working with the bash-completion codebase, I think it's kind of a dead end, even though it's the default on Debian ...

I actually want OSH to be able to run fish completions !!! From what I see, I think that is A LOT easier than running bash completions ! Curious if you have any thoughts on that, since as I understand ble.sh gives a fish-like experience

(Personally I prefer to hit tab, I don't like the "autosuggest" like fish has. But it seems like people like the completion logic)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also thought about ble.sh's integration with the Fish completion settings, the Zsh completion settings, and others such as Carapace. An issue with the Fish completion settings is that it sometimes calls a shell function, written in the Fish syntax. One needs to re-implement those Fish functions in Bash.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at the fish completion corpus, and I think the vast majority don't involve any fish functions

So there could be some kind of "best effort" conversion, where we just ignore the functions to start

It would be a way to "kick off" an OSH completion corpus.

The obvious candidate for that was the bash-completion project, since OSH is compatible with bash, but that codebase has a ton of obvious bugs IME. I think the fish completion is less ambitious, and therefore more likely to be correct

@andychu andychu merged commit 94ce2a5 into soil-staging Jan 2, 2025
18 checks passed
@andychu
Copy link
Contributor

andychu commented Jan 2, 2025

Thank you for fixing this!

@akinomyoga akinomyoga deleted the compadjust-crash branch January 2, 2025 16:03
@akinomyoga
Copy link
Collaborator Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andychu andychu andychu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@akinomyoga @andychu