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.

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

Return nil from passthrough-try-completion when table is nil #4682

Merged

Conversation

wyuenho
Copy link
Contributor

@wyuenho wyuenho commented Jan 11, 2025

Since #4602 the passthrough-try-completion function returns a cons cell even the list of completions is empty, so completion-in-region-functions that rely on the old behavior to call the exit function with an empty string candidate and nil candidates, which will make the exit func throw. This PR fixes this issue.

@kiennq
Copy link
Member

kiennq commented Jan 12, 2025

I wonder in which case candidate can be nil though. Have exit function called on nil seems to be bug of completion framework to me honestly.

@wyuenho
Copy link
Contributor Author

wyuenho commented Jan 12, 2025

candidate won't be nil when given to the the exit func as a parameter, but it can be an empty string, such as the case of point sitting at the end of a line.

candidate in the let form can be nil here when the CAPF returns nil as the list of candidates. In which case, everything that follows blows up.

@kiennq
Copy link
Member

kiennq commented Jan 12, 2025

candidate in the let form can be nil here when the CAPF returns nil as the list of candidates. In which case, everything that follows blows up.

When CAPF return nil as candidates, I don't think the exit-fn can be triggered in that case unless that's the new thing from CAPF (and I'm curious of why that even a thing to consider). Normally exit-fn is invoked when a candidate is selected, if there's no candidate, normally nothing can be selected and nothing should be run.
I purposely fallback to a candidate item in the candidates list if the exit-fn is invoked with a new string instead of the string that contains the necessary properties. The blow up can happen if the candidate and candidates list are out-of-sync. In that case, it would be better to allow the blow up to happen instead of silent fail so we can try to fix that, as the out-of-sync shouldn't happen at all.
Do you have a repro of when this happens so we can take a closer look to figure out why the out-of-sync happens?

@wyuenho
Copy link
Contributor Author

wyuenho commented Jan 13, 2025

Ahhh, it appears I've opened a whole new can of worms...

Corfu's completion-in-region-function calls completion-try-completion, which will call lsp-completion-passthrough-try-completion, which just returns ("" . nil), then corfu proceeds to here, which is when the exit func is called with an empty string and an empty candidates list.

I guess the correct fix should be that lsp-completion-passthrough-try-completion should return nil when table is nil.

@wyuenho
Copy link
Contributor Author

wyuenho commented Jan 13, 2025

This appears to be a regression from #4602

@kiennq
Copy link
Member

kiennq commented Jan 13, 2025

I guess the correct fix should be that lsp-completion-passthrough-try-completion should return nil when table is nil.

I agree, this should be a correct fix for the passthough function.

@wyuenho wyuenho force-pushed the ensure-non-nil-candidates-in-exit-fn branch from bfa3e9e to b1ee4d2 Compare January 13, 2025 08:54
@wyuenho wyuenho changed the title Ensure candidate is valid in exit-fn Return nil from passthrough-try-completion when table is nil Jan 13, 2025
@wyuenho
Copy link
Contributor Author

wyuenho commented Jan 13, 2025

Should be good to go now

@kiennq kiennq merged commit d2adcdf into emacs-lsp:master Jan 13, 2025
10 of 13 checks passed
@kiennq
Copy link
Member

kiennq commented Jan 13, 2025

Thank you

@wyuenho wyuenho deleted the ensure-non-nil-candidates-in-exit-fn branch January 13, 2025 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants