Refine imports should not refine qualified imports

[Ptival]

I appreciate the "refine imports" feature, but I'd love being able to disable it for qualified imports.

In general, it'd be nice to be able to control it (disable it entirely, disable it for qualified imports).

EDIT: see lower down for the diagnosis of the bug

[michaelpj]

Just to be completely clear about the problem, the issue is that you have an import like this:

import Foo qualified as F

and it suggests something like

import Foo qualified as F (things, actually, used)

?

If so, I think that's just plain questionable and we should not do it, who ever wants that? Generally we try and avoid configuration knobs where we can, so if we can just remove the problematic behaviour that's best.

[Ptival]

That's exactly the problem, yes.
In practice I used qualified precisely because the import list would be gigantic, so indeed the suggestion is completely useless to me.

In general, for these questions that are at the frontier between the VSCode plug-in and HLS, I'm never too sure who's responsible for the problem, so sorry if this should rather belong on the Haskell VSCode extension issue tracker.

Also, if anyone has a vague idea of where this problem may be coming from, I feel like I may have enough knowledge to be able to fix it myself (at least propose a patch). I'm just not too familiar with the code base. I'm going to assume that it's likely in this file, so I'll have a quick look to see if I can spot where to fix this.

[michaelpj]

Oh wait, what I wrote is the "make imports explicit" action, not the refine imports action. I'm not sure I know what the refine imports action actually does...

Do you have a clear example? making a test case would be a good start!

[Ptival]

My impression was that at some point the "refine imports" plugin may have been renamed "make imports explicit" plugin, since I don't find any trace of the former in the repo. I could be wrong.

Should be pretty easy to make a test, I may get to it only this weekend though.

[Ptival]

Ah, I looked into this a little more, and I was missing an important part.

Those suggestions only happen when the plugin is also trying to refine the module being imported. So for instance if I have:

`- Super.hs
`- Super
  `- SubA.hs
  `- SubB.hs

where Super.hs is just a "re-exporting" module for things in Super.SubA and Super.SubB, and I do:

import Super qualified as Super

foo = Super.somethingFromSubA

then it will suggest to refine the import into import Super.SubA qualified as Super (somethingFromSubA).

So this brings up two questions:

1. Should the suggestion either be replaced, or accompanied, with import Super.SubA qualified as Super?
2. Given re-export modules seem to be a fairly common library pattern, are module refinement suggestions even pertinent?

[michaelpj]

I'm still not sure I understand what you want. From 1, I think your original bug report is maybe just "refine imports adds an explicit import list, but that's pointless if the import is qualified". Is that right?

2 sounds like you just don't want "refine imports" suggestions at all. AIUI, replacing imports of a module with imports of the module that it re-exports is what refine imports does. The whole point of it is to import SubA instead of Super, so as to cut down on unnecessary inter-module dependencies. If you don't want that, I guess just ignore them or turn the plugin off.

[Ptival]

For 1., that's correct.

For 2., I agree it's a personal preference:

• I like the part of the plug-in that refines a blanket, unqualified import, into an explicit import of the necessary parts,
• but I don't care as much for the refinement of a supermodule import into a deeper, smaller module.
  But it shouldn't be the topic of the current issue.

I'd be happy with turning this issue into "Do not add explicit import list when refining a qualified module import".

[michaelpj]

I like the part of the plug-in that refines a blanket, unqualified import, into an explicit import of the necessary parts,
but I don't care as much for the refinement of a supermodule import into a deeper, smaller module.

I'm not sure I can see a clear distinction here. It seems like "blanket" imports are often imports of top-level super-modules and so refining them is exactly breaking them down into "sub-imports". If we can come up with a good heuristic here we could potentially implement it.

[joyfulmantis]

I'm preparing a patch to make sure that refine imports never returns a qualified import list with an explicit name list. Do you have a file or project that shows this situation, so that I can make sure I've fixed the issue?

[joyfulmantis]

Never mind, I can reproduce this myself. If qualified was used in the original import, then qualified is used in the new refined import.

The problem with dumping qualification is that besides having to replace all the calls to the qualified functions in the file, we can't be sure that there won't be name clashes, even with only having an explicit import list. Hence, I think this option is unworkable..

Hence, I propose two solutions which I think are more feasible

1. If an import is qualified, don't offer to refine it (the lazy way out)
2. If an import is qualified, refine it and keep the same qualification, but don't give it an explicit import list.

I would be interested in your opinion @michaelpj here

[michaelpj]

I definitely agree that we should keep the qualification, and I don't see any problem with keeping the original qualification (so option 2).