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

feat(lsp): add options.sync for code actions #25152

Closed
wants to merge 2 commits into from

Conversation

laher
Copy link

@laher laher commented Sep 14, 2023

Addresses #24168

This feature helps to synchronously run code actions before writing a file - for example during a BufWritePre autocmd - so that they don't suffer from race conditions.

This PR adds a 'sync' option to vim.lsp.code_action() (aswell as a timeout_ms). This is similar to vim.lsp.format({ async = false, timeout_ms = 1000 }), but given that the exsiting behaviour is async, I used sync in this case - people would expect the default to be sync=false. I added some vim.notify notifications which seemed appropriate.

An implementation question-mark: vim.lsp.code_action() does not return the results/error of the code action, because there is nothing to return when it's run asynchronously. Would it be better to introduce a second variant vim.lsp.code_action_sync() and return results from that variant? It would be a bigger change, but I'm happy to do it, if that's preferred.

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

This still wouldn't be fully sync because there might be another asynchronous resolve step.

Besides that, I'd rather go into the direction outlined in #22598 (comment)

@laher
Copy link
Author

laher commented Sep 14, 2023

OK, fair enough. Thanks for the fast response. Is there something I could do to make this more amenable? even just adding a callback mechanism?

I'd be happy to try introducing a coroutine along the lines you described in #19624

... but it sounds a bit hasty to just throw it in there. Maybe it could be introduced with 'experimental' in the docs?

@@ -726,6 +742,14 @@ end
--- If in visual mode this defaults to the active selection.
--- Table must contain `start` and `end` keys with {row,col} tuples
--- using mark-like indexing. See |api-indexing|
--- - sync: (boolean|nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The vim.lsp.buf.format(...) param is called async. It would be nice to keep the two consistent.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, but I'm leaning towards implementing this as a separate method instead, because:

  • the defaults are different - .code_action() is async by default, but .format() is sync by default. I think it would still feel inconsistent.
  • if it's a separate method, it can return a result, err.

... but it sounds like this is likely not going to be accepted either way. If I (or someone) doesn't think of a way to do this which satisfies the direction described by @mfussenegger , I'm probably going to close this PR.

@laher
Copy link
Author

laher commented Sep 15, 2023

I have explored a couple of alternatives with callbacks & coroutines (I'm pretty new to lua's coroutines), and I haven't come up with something workable so far ... As soon as I hit an 'Attempt to yield across C-call boundary' I realised I need to learn a lot more about libuv and [lua]jit.

If I do try again, the solution would be completely different to what's here, so I may as well close this.

Thanks for the feedback and all the good work on neovim.

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

Successfully merging this pull request may close these issues.

3 participants