jfmengels / elm-review-unused

Provides elm-review rules to detect unused elements in your Elm project
https://package.elm-lang.org/packages/jfmengels/elm-review-unused/latest/
BSD 3-Clause "New" or "Revised" License
23 stars 12 forks source link

`NoUnused.CustomTypeConstructors`: phantom type constructor name should not have to match #46

Closed lue-bird closed 3 years ago

lue-bird commented 3 years ago

As types to use as a phantom type argument, people can for example use

type APhantomTag
    = APhantomTag Never -- not constructable

a : T APhantomTag

NoUnused.CustomTypeConstructors does not flag this constructor. However, if the constructor name doesn't match the type name, the constructor is reported.

type APhantomTag
    = APhantomTagForT Never

→ Don't check for equal names.

jfmengels commented 3 years ago

Hey! Thanks for the issue :wave:

I have to admit I'm surprised the rule doesn't report an issue for the first case :thinking:

The documentation of the rule has this explanation for handling of phantom types:

This does not prevent you from using phantom types: A constructor won't be reported if

- It is the only constructor of a type that has no type variable
- It has no parameters
- It is used as an argument of a custom type, in the stead of a type variable that is not used in the definition in any of the type's constructors

It seems like it's a bit out of date or at least unprecise, because we do check for Never, contrary to the explanation.

Would it be a good solution for you if the explanation was done better? Or maybe if there was an automatic fix to add Never and/or rename the constructor?

Or do you have a problem with having this convention for phantom types? If so, could you please explain what bothers you about it?

lue-bird commented 3 years ago

Or maybe if there was an automatic fix to add Never

That would be 👍

a problem with having this convention for phantom types?

  1. The constructor is Never used, it's name does not matter → don't check it
  2. It could (and has) lead to name clashes.
type AOrB phantom
    = A
    | B

type A = A Never
--       ^ name clash

a : AOrB A

Nothing should hold you back from naming a phantom type after an existing variant.