Quickfix to import constructor only imports the type in certain scenarios

nwolverson / purescript-language-server

MIT License

184 stars 41 forks source link

Quickfix to import constructor only imports the type in certain scenarios #126

Closed triallax closed 2 years ago

triallax commented 3 years ago

I've still not been able to determine the exact conditions, but here's what I tried so far:

Nothing and Just work (imports Data.Maybe (Maybe(..)) as expected)
V doesn't (imports Data.Validation.Semigroup (V))
Identity doesn't (imports Data.Identity (Identity))

Possible scenarios I came up with:

Works when type isn't newtype
Works when type has more than one constructor
Doesn't work when type has same name as constructor

I'll investigate more later. If you want logs or anything else feel free to ask me.

thomashoneyman commented 3 years ago

It may be when the type and constructor are the same name, from the cases you've pointed out here.

nwolverson commented 3 years ago

I think this should have been resolved with #118 - there should be multiple options presented

Completion offers both options and the documentation notes the type/icon should differ

nwolverson commented 3 years ago

Worth saying that there are changes around here related to 0.14 - things work differently, so if you're seeing different behaviour can you clarify which purs purescript-language-server versions you are using (and which editor).

triallax commented 3 years ago

I am not speaking about actions, but about quickfixes. I'm not sure if other editors make this distinction (I use Neovim with coc.nvim), but yes, I can choose to import constructors in actions. However, what I'm talking about here is quickfixes, where you don't get any dialog to choose what you want to happen.

nwolverson commented 3 years ago

As far as I can see, the only relation of "quickfix" in the LSP spec is a particular type of CodeAction https://github.com/nwolverson/purescript-language-server/blob/6d376d3cc344540e77abcee7e47593a4659f5e76/src/LanguageServer/Types.purs#L357

Unless the code search is wrong, we're not tagging any code action as such.

triallax commented 3 years ago

@nwolverson I'm not sure I follow what you're saying to be honest.

nwolverson commented 3 years ago

@mhmdanas specifically I am saying the language server is not providing a quick fix, as far as I am aware - just a list of code actions. If one of those is presented as a quick fix without interaction, that would be a coc bug/feature, as far as I'm aware. I don't use coc or any form of vim, so a log of the LSP interaction would be required to make progress if you think it's an issue on the language server side, I'm afraid.

Given a choice is required, I feel this should not be available as a quick fix, though I could certainly be persuaded that importing the constructor over the type is an obvious choice, there are other cases where a choice is certainly required - as a workaround I would suggest not invoking it :)

nwolverson commented 2 years ago

Does this still seem to be an issue?

triallax commented 2 years ago

Ah, sorry, I forgot to reply to this.

Since I opened this issue I've moved to Neovim's built-in LSP. It used to work great with purescript-language-server, but now for whatever reason it shows the following error in Neovim several seconds after opening a PS file:

Client 1 quit with exit code 1 and signal 0

You can close this issue for now, I will make a comment when/if I can figure this problem out and then attempt to reproduce the issue again.

triallax commented 2 years ago

Well, my LSP client mysteriously started working again, and it seems like I can't reproduce the issue anymore.