Extend import list automatically

[gdevanla]

Address #576 requirement of automatically extending the import list.

Few points to address (and I would like some pointers on this)

Simple working Demo

Items to address:

• Multi-line edits handled, but may need post hlint step to align with other imports

import Control.Monad as M (
   join,
     msum)

This PR adds the new item to the last item like this:

import Control.Monad as M (
   join,
     msum, mplus)

Demo:

• After edits, due to caching, inserts might be duplicated. We need hlint to handle it.

[pepeiborra]

The caching issue is a bit tricky. We want completions to come instantly, and therefore we use useWithStaleFast which doesn't wait for recomputation. This makes a difference because completions can be quite costly to recompute:

https://github.com/haskell/ghcide/blob/master/src/Development/IDE/Plugin/Completions.hs#L134

There are many ways to solve this problem:

1. Replace useWithStaleFast with a non-caching form of use.
   But: this will make all completions noticeably slower

2. Get rid of ProduceCompletions, have getCompletionsLSP use Local and NonLocal directly. This allows to select a different strategy for each, so NonLocal can be non-cached.
   But: when module imports change and non local completions need to recompute, all completions will be slower. Seems an acceptable trade-off

3. Change the TextEdit produced by extendImportList to append rather than overwrite. You can accomplish this by tweaking the Range in the TextEdit.
   But: is a bit hacky and may not work well in all cases

4. Use a Command to perform the additional edit, as per the LSP spec.
   But: the LSP spec says that additionalTextEdits should be used where possible

My preference would be for 3 or 2. 1 is a non solution.

[gdevanla]

Note (3), we append and do not overwrite. We just happen to overwrite if we have a invalid cache.

Another option for caching problem would be to always prepend the new item. For example,

import Control.Monad (join)

would become, say,

import Control.Monad(msum, join)

This option can be agnostic to cache. But, it is a little counter intuitive that we prepend rather append to the list. So, may be worth considering that option. But, note that, duplicate inserts will still an issue due to caching.

---

It feels like we should just go with (2), and solve the performance problems. The above solutions are not complete and will lead to unnecessary noise and user frustration.

[pepeiborra]

Note (3), we append and do not overwrite. We just happen to overwrite if we have a invalid cache.

Sorry I wasn't more clear. Assuming we want to extend an import declaration with the identifier readFile and the desired insert position for the new imported name is line 10, column 20, you create a TextEdit (Range (Position 10 20) (Position (10 28)) "readFile. If we define the range instead as Range (Position 10 20) (Position 10 20) then it will not delete the contents between columns 20 and 28 when applied.

Another option for caching problem would be to always prepend the new item. For example,

import Control.Monad (join)

would become, say,

import Control.Monad(msum, join)

This option can be agnostic to cache. But, it is a little counter intuitive that we prepend rather append to the list. So, may be worth considering that option. But, note that, duplicate inserts will still an issue due to caching.

Yes, duplicate inserts, if the same completion is used twice in quick succession, will lead to a duplicate identifier in the import list. But this does not constitute a compilation error and hlint will highlight it.

It feels like we should just go with (2), and solve the performance problems. The above solutions are not complete and will lead to unnecessary noise and user frustration.

Definitely, (2) is worth exploring. But the end result will be slower completions in some cases, whereas (3) will not impact the performance properties.

[gdevanla]

@pepeiborra I chose option (3), and that setup of TextEdit seems to work (wonder why that works though. Is that part of the LSP spec)?

Either ways, that approach worked. The only other wart is when starting from an empty list and caching strikes. Here is an example,

import  Control.Monad as M ()

After first insert we have, say

import Control.Monad as M (join)

Now say, before cache is refreshed, if the user types M.msum, we will end up with a badly formed import

import Control.Monad as (joinmsum)

Do you see a way to solve this?

---

Update: With the suggestions below, I was able to resolve this issue by adding a trailing comma.

[konn]

It seems Haskell permits trailing comma at the end of import list. So, just always prepending, right after (, new imported symbol with trailing comma should work, IIUC.

[gdevanla]

It seems Haskell permits trailing comma at the end of import list. So, just always prepending, right after (, new imported symbol with trailing comma should work, IIUC.

Interesting. Thanks for pointing that out.

[ndmitchell]

It seems Haskell permits trailing comma at the end of import list. So, just always prepending, right after (, new imported symbol with trailing comma should work, IIUC.

Haskell permits it, but it's not common style - I would be a bit upset if my IDE automatically changed my code for me, but then I had to go and adjust it back to follow common style.

[pepeiborra]

It seems Haskell permits trailing comma at the end of import list. So, just always prepending, right after (, new imported symbol with trailing comma should work, IIUC.

Haskell permits it, but it's not common style - I would be a bit upset if my IDE automatically changed my code for me, but then I had to go and adjust it back to follow common style.

To me this is a minor deal. As long as automated formatting tools can parse the modified import list it's ok: I use format-diff-on-save.

[konn]

Haskell permits it, but it's not common style - I would be a bit upset if my IDE automatically changed my code for me, but then I had to go and adjust it back to follow common style.

To me this is a minor deal. As long as automated formatting tools can parse the modified import list it's ok: I use format-diff-on-save.

I agree. To be honest, I realised this spec when I used ormolu / fourmolu formatter - they prefer trailing commas at the end of vertical import lists, perhaps to minimise diffs.

[gdevanla]

I have made all the changes with the assumption that we can add a trailing ,

Looks like the only alternative solution would be to by pass caching.

[pepeiborra]

I have made all the changes with the assumption that we can add a trailing ,

Looks like the only alternative solution would be to by pass caching.

We can always do that in a follow-up change if people find the extra commas too annoying.

[ndmitchell]

Format on save is not common in the Haskell community. In Rust, it inserts imports badly and then format on save fixes them, and it's a bit of a pain. I think we've going to need to remove the trailing commas sooner or later, to improve user experience.

[berberman]

We should not insert the identifier to the import list directly in some cases, for example:

where in A.hs:

module I.A where

data Foo = ConsA | ConsB

data Bar = ConsC

Neither "render" the identifier with its parent would be enough, because the parent may be already in the import list, thus we should extend it instead. In #916, I try to implement both cases in the code action, just like:

and

But I haven't solve this problem elegantly, which leaves out some cases. As you can see in the last screenshot, the code action adds a wrong comma :(
Sorry for noisy, I would prefer to revert #916, remain it to next monthly release, so I will have time to give a thorough refactor // cc @pepeiborra I'm doing the refactor in #942

Actually, extending the imports we do in completions or code actions are similar but in different ways. It would be better if we can reuse some code here...

[gdevanla]

@berberman Those are good points I did not consider in this PR. I think its good to a create a new issue and see how the code and behavior can be consolidated.

[ttuegel]

It would be nicer to have a configuration option for any new feature that is going to automatically edit source code without the user's input or permission, so that users can opt-in (or at least opt-out).

[pepeiborra]

Adding a configuration option for every new feature is the way of enterprise. You wouldn't need a configuration option if the feature worked all the time and always did what you want. That's what we should strive for.

[ttuegel]

You wouldn't need a configuration option if the feature worked all the time and always did what you want.

If this feature worked correctly, it would not do what I want. If it worked 100%, I think it would largely defeat the purpose of explicit imports. But more generally, when I ask for something in one place (completion) I don't like my editor to make changes in another place.

[pepeiborra]

I don't see how autoimports would defeat explicit import lists, maybe you can elaborate. But it's perfectly reasonable to disagree with any feature. However, when a feature does what 99% of users want, I expect the 1% to either send a PR or adjust their expectations.

[jneira]

Mmm, i agree with @ttuegel, that dont propose add config option for all features but for "invasive" ones.
Invasive cause they do modifications without explicit user consent.

I think that those ones deserves a config option even if they work for most users. Not sure if is enterprisey but i personally consider it a nice ux.
Otoh i think that no feature works for the 99% of users , 99% of times even it is perfect, and less probable if it is "invasive".

[pepeiborra]

I still don't see how auto-extending import lists on completions is an invasive feature.
But as I said, a PR adding an option to turn it off would be welcome