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

New rule: NoUnused.TypeVariables #67

Open jfmengels opened 2 years ago

jfmengels commented 2 years ago

What the rule should do:

Report unused type variables in type definitions

What problems does it solve:

This simplifies code, and/or can help discover that phantom types are not used as expected.

Example of things the rule would report:

type A a = A | B

init : A a -- maybe even if the value is something more specific but then never used? Like `A Int
fn : A a -> A a

Example of things the rule would not report:

-- actually uses the type variable
type WithData a = A | B a

-- The type variable is bound in some places where the type is used
type WithPhantomType a = A | B

--   The type variable has 2 different values
changeType : WithPhantomType a -> WithPhantomType ()

-- The type variable is bound to something now
fn : a -> WithPhantomType a -> WithPhantomType a

When (not) to enable this rule:

I don't see a reason to not enable it :shrug:

I am looking for:

I'm looking for someone to implement this.

I am uncertain as to how many things this will in practice catch, or if in many cases there will be false negatives because the heuristic I suggested is not sufficient to catch everything. I am sure there are more cases we can handle, but having a start of this rule to then discover the rest would be a great start.

lue-bird commented 2 years ago

Hmm. I think it's valid and will generally be helpful. There are use cases when a library supplies/accepts generic phantom tagged values where users have to annotate results and arguments.

Really obscure and a bad pattern IMO (elm-tagged and previous, bad iterations of typed-value use it; elm-unit and current elm-typed use explicit tagging), though I can imagine folks using a similar technique for good :shrug:

jfmengels commented 2 years ago

From the functions that I see available in the packages you linked, none of these libraries would be reported by this rule idea (based on "Example of things the rule would not report").

lue-bird commented 9 months ago

I just found this thread again. One example is Quantity.zero and Quantity.negate:

zero : Quantity number units
negate : Quantity number units -> Quantity number units

type Quantity number units
    = Quantity number

Another example is Tagged.tag and Tagged.retag

tag : value -> Tagged tag value
retag : Tagged oldTag value -> Tagged newTag value

type Tagged tag value
    = Tagged value

But maybe I still don't understand what the rule is trying to do? The suggested name doesn't help.

jfmengels commented 9 months ago

I agree the idea can be specified a bit more.

The aim of the rule is to report unused type variables, unused in the sense that it provides no value whatsoever, in the aim to simplify code. You're right that they can be used for phantom types (it would be too simple otherwise).

I think we can only report a type variable as unused if

  1. It is not used in the definition of the type
  2. It is unbound in all locations: 2.1. it is never bound either to a specific value (A Int) 2.2. it is never bound to another type variable (a -> A a or A a -> A a)

In the conditions above, which I imagine would be rather rare, I think the type variable is completely useless, so we either lost the use of a type variable, or there is a "tagging" system in place that is used unnecessarily.

If the type is exposed as part of a package, then we can't know all locations, and we won't report it. If it's an internal or a type defined in an application, then we can know all the uses, and therefore we could tell whether the type variable provides value or not.

Let me know if this clarifies things for you.


I guess we could also report the type variable if we see it used unnecessarily. For instance, if we have type alias A r = { r | ... } and then we always see A {} or A <unbound>, then we could report it as unused. But this is probably a bit out of scope.