NoModuleOnExposedName fix can introduce name collision

[MartinSStewart]

In the following rule, NoModuleOnExposedName will suggest a fix by replacing convert : Color -> Element.Color with convert : Color -> Color which will lead to a compile error. It should instead suggest removing Color from exposing or not suggest any fix in this case.

import Element exposing (Color)

type Color = Red | Green | Blue

convert : Color -> Element.Color
convert color =
    ...

Edit: I think the correct behavior would be to not show an error at all. Consider this case:

import Subcomponent exposing (Msg(..))

type Msg = A | B

foo : Subcomponent.Msg
foo = 
   MsgVariantInSubComponent

Subcomponent.Msg can't be changed to Msg due to a name collision but exposing (Msg(..)) can't be removed either because of MsgVariantInSubComponent.

[sparksp]

Ooo that's an interesting case there. I don't think this rule should be responsible for fixing your exposing (that's for another rule in this package perhaps) but I'll certainly take a look to remove the auto-fix in this case.

[MartinSStewart]

To be clear, I think the second example shouldn't be an error at all due to there being no good way to fix it. I agree the first example should just not have an auto-fix though.

[sparksp]

There are a number of 'good' ways the second can be fixed... the author could choose not to expose the Subcomponent's Msg variants and fully qualify all their subcomponent use or they could choose to rename the local Msg type so there's no shadowing.

What happens if the author decides to add MsgVariantInSubComponent to a local type or import it again from another module? The compiler will then complain about a type mismatch, the author will need to prefix the module name, and the exposing becomes further pointless.

Personally, I'm not a fan of imports exposing type constructors because it makes it much harder (even than normal imports) to figure out where things are coming from.

[jfmengels]

I agree with @sparksp that there are multiple ways to fix the issue, qualified imports of the constructors being one of them.

But I think that the original example as it is is just fine too and that there is not a lot of ways the situation can be improved upon without potentially a lot of (manual) renaming. I therefore think the rule should not report an error in the case where the imported type is the same as a local type (and probably the same thing for values?).

[MartinSStewart]

@sparksp Fair enough, I guess I'm okay with it showing an error but not an auto-fix in the second example as well. That said, if it is an error and you're feeling particularly motivated, maybe NoModuleOnExposedName could detect this situation and suggest to the user to not have Msg(..) and instead provide qualified names to the Msg variants.

[mthadley]

Ran into this situation as well, today.

Seems like folks have settled on "Don't report an error" in this case. Has anyone made any progress on adding that behavior to the rule?