NoUnused.CustomTypeConstructors is reported for types used in phantom builder annotations

[dillonkearns]

Describe the bug NoUnused.CustomTypeConstructors reports an unused type constructor when it is used within a Phantom Builder type annotation.

SSCCE (Short Self-Contained Correct Example)

module Example exposing (Builder, init)

type OnlyUsedInPhantom
    = OnlyUsedInPhantom

type Builder constraints
    = Builder

init : Builder { constraints | phantom : OnlyUsedInPhantom } -> Builder constraints
init _ =
    Builder

Make sure "jfmengels/elm-review-unused": "1.1.20" is installed.

Run the NoUnused.CustomTypeConstructors rule.

See the error:

NoUnused.CustomTypeConstructors: Type constructor `OnlyUsedInPhantom` is not
used.

156| type OnlyUsedInPhantom
157|     = OnlyUsedInPhantom
           ^^^^^^^^^^^^^^^^^

This type constructor is never used. It might be handled everywhere it might
appear, but there is no location where this value actually gets created.

Expected behavior

I expect no error to be reported here because the type itself is used, and though the constructor is not used as the error message indicates, there's no way to define a type with no constructors.

I'm not sure what the exact method would be for avoiding errors like these. The two approaches I can think of are

1) Static analysis to see that it shows up in a phantom style and therefore needs to be defined. 2) A naming convention for variant names that are not intended to be constructed for cases like this.

[jfmengels]

Thanks for the issue :)

Static analysis to see that it shows up in a phantom style and therefore needs to be defined.

The rule already does that, but I guess it doesn't handle look at the contents of what is used for the phantom argument. This could be improved I agree :+1:

A naming convention for variant names that are not intended to be constructed for cases like this.

An easy fix/workaround here would be to add a Never, as indicated in the rule's documentation

type OnlyUsedInPhantom
    = OnlyUsedInPhantom Never

[dillonkearns]

Oh, I totally missed that comment! Yes, that seems super reasonable. I like that it's not a naming convention but rather a proof that it can't be constructed.

So maybe the rule could point that out instead? Seems like there's already a sufficient way to handle it, which arguably leads to more expressive code.

[jfmengels]

D you mean to say something like:

X is unused, please remove it.

If X is used as a phantom type, consider adding a Never to make that explicit, like this: "..."

It could become quite long (not necessarily a problem), but especially confusing for beginners who don't know what a :ghost: type is :man_shrugging:

[dillonkearns]

Yeah, it definitely could be a red herring for beginners that could take them down the wrong path. Having a link to some notes about the phantom case could hide some of those details, but still a lot of information that could be confusing if it's not relevant.

I guess another option could be a combination of the static analysis and the error message? For example, if the type is used in type annotations but the constructor is never used, only in those cases show the extra message with a link to phantom type instructions?