kovidgoyal / kitty

Cross-platform, fast, feature-rich, GPU based terminal
https://sw.kovidgoyal.net/kitty/
GNU General Public License v3.0
24.62k stars 987 forks source link

New shell integration not playing nicely with zsh plugins #4428

Closed sQVe closed 2 years ago

sQVe commented 2 years ago

Describe the bug

When running kitty with the new shell integrations some zsh plugins start to error maximum nested function level reached.

To Reproduce Steps to reproduce the behavior:

  1. Use kitty @ 0.24.0.
  2. Make sure shell integration is enabled.
  3. Install zsh-autosuggestions.
  4. Error like maximum nested function level reached should be echoed on every new call.

Screenshots

image

Environment details

Key action failed

debug_config
(25, 'Inappropriate ioctl for device')
kovidgoyal commented 2 years ago

Works fine for me with ~/.zshrc

source "/usr/share/zsh/plugins/zsh-autosuggestions/zsh-autosuggestions.zsh"
return

and shell integration enabled, with the following steps 1) Type ls -l and press enter 2) type ls 3) -l appears in gray as an autosuggestion

sQVe commented 2 years ago

My bad. I loaded a default zimfw (input) plugin too that seems to be problematic. Sorry for the bad first report.

I'm able to consistently replicate it, with this minimal .zshrc:

source "$HOME/code/input/init.zsh"
source "$HOME/code/zsh-syntax-highlighting/zsh-syntax-highlighting.zsh"

Clone the repos at:

My assumption is that input some how clashes with the shell integration?

kovidgoyal commented 2 years ago

This will likely be hitting some zsh internal limit or the other. I am not familiar enough with zsh to guess what, but offhand have you tried the suggestion in the error and increased FUNCNEST?

sQVe commented 2 years ago

@kovidgoyal Yes. I've tried increasing FUNCNEST incrementally til 1000, but by that point the terminal just crashes.

ldelossa commented 2 years ago

Can confirm, editing FUNCEST does not help.

kovidgoyal commented 2 years ago

well I can see nothing wrong in either the kitty code or the zimfw code, so most likely it is something in syntax highlighting, which is too big for me to review.

ldelossa commented 2 years ago

What change would occur between v0.23.1 to v0.24.0?

sQVe commented 2 years ago

@kovidgoyal Sadly it seems that this problem isn't isolated to zsh-syntax-highlighting though. I can replicate it with:

source "$HOME/code/input/init.zsh"
source "$HOME/code/zsh-autosuggestions/zsh-autosuggestions.zsh"
source "$HOME/code/zsh-history-substring-search/zsh-history-substring-search.zsh"

Relevant repos:

What makes this worse is that the repos involved here are extremely common within the zsh community - so this breaking behavior will be very noticable.

sQVe commented 2 years ago

@ldelossa You should read the changelog. Version 0.24.0 introduced an awesome shell intregration - which will be a great feature once issues are ironed out.

kovidgoyal commented 2 years ago

somebody will need to do a deep dive on those plugins to see what it is they are doing, I really dont have the time for that, at least for now. But maybe later.

sQVe commented 2 years ago

I tried commenting out blocks of code and have so far found that I'm not seeing any errors after commenting out:

https://github.com/kovidgoyal/kitty/blob/master/shell-integration/zsh/kitty-integration#L235

sQVe commented 2 years ago

Removing and commenting out https://github.com/kovidgoyal/kitty/blob/master/shell-integration/zsh/kitty-integration#L204-L205 obviously fixes it too.

ldelossa commented 2 years ago

@ldelossa You should read the changelog. Version 0.24.0 introduced an awesome shell intregration - which will be a great feature once issues are ironed out.

Ah, I see. Yes feature looks very cool, but terminal breaking my zsh env is a big problem. Will stick to v0.23.1 until fix. Thanks for new features Kovid, they do look aweome!

kovidgoyal commented 2 years ago

I tried commenting out blocks of code and have so far found that I'm not seeing any errors after commenting out:

https://github.com/kovidgoyal/kitty/blob/master/shell-integration/zsh/kitty-integration#L235

yes but there isnt anything actually wrong with that line. Its something the plugins are doing to zle-add-widget

kovidgoyal commented 2 years ago

Ah, I think I know the issue.

ldelossa commented 2 years ago

@kovidgoyal I just built from master, doesn't seem to be fixed for me.

page-down commented 2 years ago

I tried the three plugins mentioned above and still have problems. kitty --config=NONE -o shell=zsh --hold

I tried disabling shell integration and the kitty window was still closed instantly after startup.

kitty --config=NONE -o shell_integration=disabled -o shell=zsh --hold

I'm more curious as to why --hold doesn't seem to work.

kovidgoyal commented 2 years ago

Yeah with all three as opposed to just two its not fixed, sadly.

page-down commented 2 years ago

Is it possible that --hold is not working:

Because it was closed by kitty itself.

kovidgoyal commented 2 years ago

--hold works fine

kitty --hold false

what other character is kitty sending to the window?? You mean something in response to an escape code? You can check by modifying the hold() function in __main__.py

page-down commented 2 years ago

You mean something in response to an escape code?

Yes.

I mean when used with the problematic zsh rc. The program sends an escape code, then exits immediately due to an error, and the escape code returned by kitty is sent to the window and causes it (kitty -o shell=zsh --hold) to close.

I checked and confirmed that stdin received the data here, then exited.

def hold(args: List[str]) -> None:
    # ...
    with suppress(BaseException):
        # ...
        sys.stdin.buffer.read(1)
source /path/to/zimfw-input/init.zsh
source /path/to/zsh-history-substring-search.zsh
sQVe commented 2 years ago

I just built from master and it's now working great on my setup.

page-down commented 2 years ago

I just built from master and it's now working great on my setup.

Do you mean that all three plugins mentioned above are working correctly? I can only run zsh if I comment the following lines in the plugin.

    # if is-at-least 5.3; then
    #   autoload -Uz add-zle-hook-widget && \
    #       add-zle-hook-widget -Uz line-init start-application-mode && \
    #       add-zle-hook-widget -Uz line-finish stop-application-mode
    # else
      zle -N zle-line-init start-application-mode
      zle -N zle-line-finish stop-application-mode
    # fi
sQVe commented 2 years ago

Everything seems to be working fine on my setup with https://github.com/kovidgoyal/kitty/commit/69482b6a54e8e867a045a8f0c32adabc15a7d6c0.

sQVe commented 2 years ago

Ah, but I have shell_integration no-cursor set. If I remove that, and fallback to the default, I'm getting errors again.

kovidgoyal commented 2 years ago

Basically, the syntax/autosuggest/substring search plugins all rewrap all zle widgets. And that rewrap logic is broken by both zimfw and kitty calling add-zle-hook-widget. If both calls are present, then the rewrap logic creates an infinite recursion. If either one of those calls is removed, there is no infinite recursion.

I dont know of a good way to fix this sorry. I guess you have to use either zimfw or kitty shell integration. Someday I might read that rewrap logic and try to figure out a workaround, but frankly shell script makes my eyes bleed.

The only thing I can think of is to not dynamically create the functions used for zle widgets and instead set the at load time just once. This means a bit of a performance loss, but given the craziness that is the zsh ecosystem, maybe its the only way. Perhaps @romkatv can comment, since this is his code.

kovidgoyal commented 2 years ago

And actually setting them at load time wont work since various other plugins clear the zle hooks on initialization, le sigh. So yeah totally out of ideas here.

romkatv commented 2 years ago

add-zle-hook-widget is broken in a rather terrible way. If at least one plugin uses it, then all must use it or risk this sort of meltdown. There is a workaround though. I'll send you a PR.

P.S.

I really should've at least flagged the use of add-zle-hook-widget when I was rewriting zsh integration code in Kitty. Or, better yet, implemented the workaround.

kovidgoyal commented 2 years ago

Cool, thanks.

romkatv commented 2 years ago

Sent https://github.com/kovidgoyal/kitty/pull/4437.

ldelossa commented 2 years ago

Everything is working. Thanks a lot. The shell integration features are HUGELY helpful, and something I've been looking for, for a very long time. :rocket: