NoMissingTypeExpose should ignore local aliases

[edgerunner]

NoMissingTypeExpose aims to make sure that a type is accessible if its value is accessible outside a module. However, it seems that it is giving a false positive result on a specific case.

That specific case is when a type alias is used inside a module to refer to an imported type with another name (usually to shorten it or to avoid clashes or confusion). NoMissingTypeExpose raises an error in that case, but I think it should not, because the type is actually available.

module ThisOne exposing (something)

import SomeModule exposing (OneLongTypeName)

type alias Short = OneLongTypeName
-- elm-review complains that Short isn't exposed, but that's unnecessary
-- because OneLongTypeName is already exposed by SomeModule and is accessible.

something : Int -> Short
something = …

I think there could be a separate rule for preventing local aliasing like that if there's a need for that. The use case is different, and people may want one but not the other.

[jfmengels]

Hey @edgerunner 👋

Thanks for reporting an issue. I think this is a bit of a grey area because, yes, the user can annotate their values, but it's not with the expected type names.

For instance, if you have this code in a separate module

module A exposing (..)

import A

someValue = A.something

and you try to annotate someValue, tools like IntelliJ and the Elm LSP will currently annotate it with something : A.Short, which won't compile because A.Short is not exposed. These could obviously be changed to annotate with something : SomeModule.OnleLongTypeName, but I'd say the current behavior is very reasonable.

I think your scenario is pretty rare (direct alias to another type), but I would argue that it would probably be better in this case to either expose the type (to reduce the amount of confusion for the readers of the code), or not define a type alias at all (I'm a little skeptical still of the value of such a type alias).

I'd say the current behavior does reflect what I think should happen, maybe the docs could be reworded slightly to make that explicit. What do you think?

Also let me know if you have a scenario where this is in practice a pain point for you. Otherwise I'll close the issue because I think the current behavior is reasonable.

[edgerunner]

The thing that got me this error originally was that Lamdera uses local aliasing in its boilerplate, because it has frontend and backend models defined on a single file, but imports and refers to them as Model in their respective files. I also used the same as a local short name in my other projects, especially if it is used a lot, to improve readability.

I understand that this may be an anti-pattern that people want to avoid. But the description of NoMissingTypeExpose explicitly mentions something else that it is targeting, ie. that the type used in an exported function also better be exposed. This, I think, is another (anti-)pattern.

I also understand that the tooling may then come up with unresolved annotations and break things, which is a valid use case, but then from the perspective of the user, what I'd prefer (in order) would be

1. that the tooling resolves the alias to the most concise public definition
2. or the review rules allow me to distinguish between the "hidden alias" and "hidden type" problems.
3. or the documentation (and the error message) reflect that the rule also covers the "hidden alias" case and the rationale

What do you think? Is it at least worth mentioning in the error message to avoid the kind of initial confusion I had?

[jfmengels]

I think that makes sense. For option 1) you'll have to open issues in the different editors/tools (and I can imagine them saying no).
For 2), this could be done through configuration of the rule, but I don't think the frequency nor the brought value of this merits the complexity of a configuration (for the user mostly).

I'm fine with 3) updating the error message and/or documentation. If we go with the error way, then I'd say that ideally this part of the error message should only appear when we're hitting this specific situation.

Would you be happen to want to work on this? If not, I/someone will get to it later :)

[edgerunner]

No idea how simple/complicated that would be, I could take a look at least.

And maybe if it turns out to be simple enough, I'll write an alternative "hidden type" rule that explicitly allows the "hidden alias" case, and as far as I understood (first time using elm-review) it should be easy to supplant NoMissingTypeExpose with that variant for anyone with my specific situation, right?