False positive on `NoUnused.CustomTypeConstructorArg`

[jpagex]

False positive on NoUnused.CustomTypeConstructorArg when the type is only use in equality comparisons.

SSCCE (Short Self-Contained Correct Example) When running npx elm-review --template jfmengels/elm-review-unused/example on the following code:

module Main exposing (main)

import Html exposing (Html)

type Id
    = Id String

main : Html msg
main =
    Html.text <|
        if Id "A" == Id "A" then
            "OK"

        else
            "NOK"

I get:

-- ELM-REVIEW ERROR ------------------------------------------ src/Main.elm:7:10

NoUnused.CustomTypeConstructorArgs: Argument is never extracted and therefore
never used.

6| type Id
7|     = Id String
            ^^^^^^

This argument is never used. You should either use it somewhere, or remove it at
the location I pointed at.

I found 1 error in 1 file.

Even though I need the String parameter.

Expected behavior No error reported.

Additional context elm-review CLI version: 2.3.3 "jfmengels/elm-review": "2.3.8" "jfmengels/elm-review-unused": "1.1.4"

[jfmengels]

Hi @jpagex

Thanks for creating the issue :slightly_smiling_face:

I am not sure what the correct approach is here. In the example you gave here, the condition Id a == Id b is equivalent to a == b, which makes the usage of the custom type constructor not useful, and something we should aim to remove (as will be the case with the next release for NoUnused.CustomTypeConstructor).

But if we take a == Id b, then it does serve some purpose, and I agree the rule should not report an error.

I think to handle this case well we need to check for comparisons of values of this type and count all parameters as used.

The annoying thing is that we would need type inference for that, and even that might not be enough on its own... :/

I fear that this might a very tricky issue to solve, though fortunately for few situations. If someone has a bright idea for this, let me know.

In the meantime, a workaround is to extract the string out of an Id at least once somewhere, for instance in something like

equals : Id a -> Id a -> Bool
equals (Id a) (Id b) = a == b

[jpagex]

Actually, the real use case is more of the following:

type AgendaEventId
    = ScheduleItemId Time.Weekday Hour Hour
    | LessonId Id
    | EventId Id

In this case, the custom type is quite useful. In my use case, I only need to compare the IDs, which is why I used the == operator and did not need case statements.

The proposition you made was exactly how I solved it:

areSameIds : AgendaEventId -> AgendaEventId -> Bool
areSameIds id1 id2 =
    case ( id1, id2 ) of
        ( ScheduleItemId w1 s1 e1, ScheduleItemId w2 s2 e2 ) ->
            w1 == w2 && s1 == s2 && e1 == e2

        ( LessonId l1, LessonId l2 ) ->
            l1 == l2

        ( EventId e1, EventId e2 ) ->
            e1 == e2

        _ ->
            False

It is indeed an edge case and there is a workaround. I would consider this a bug. Nevertheless, being forced to create a function to be explicit on the comparison is an improvement in my opinion. I do not have a case in mind where it would be preferred to use the == operator instead (except for simplicity).

[jfmengels]

I'm not sure I agree that areSameIds is an improvement over simply using == in this case. I'll try and come back to this issue once we have more type information available in elm-review.

[jpagex]

In this precise case I agree, I am not so sure either. For more complex opaque types, it could still be better to make it clear/explicit how to compare two objects instead of relying on the == operator. (Sometimes, some parameters should not be taken into account for the comparison.)

Nevertheless, making elm-review handling this case would be surely appreciated.

Thanks for your work.

[r-k-b]

Another data point: we're using pzp1997/assoc-list where the keys are custom types where some constructors have arguments. The rule incorrectly treats the custom type constructor arguments as unused.

[jfmengels]

@r-k-b could you provide an SSCCE so that we're both on the same page?

[r-k-b]

Sure, here's one:

module Main exposing (..)

import AssocList exposing (fromList)

type Route
    = UserInfo Int

routes : AssocList.Dict Route String
routes =
    fromList [ ( UserInfo 1, "foo" ), ( UserInfo 2, "bar" ) ]

{-| routeA == Just "bar"
-}
routeA : Maybe String
routeA =
    {- The type constructor argument is indirectly "used" as a key to extract
       the value, but the Rule warns that the argument is never extracted.
    -}
    AssocList.get (UserInfo 2) routes

[jfmengels]

Fixed in v1.1.7 :) There will be false positives still, but I imagine a lot less so

[jpagex]

Thank you for the work. Unfortunately it does not solve my use case. Here is another SSCCE:

• elm-review: 2.4.6
• elm-review-unused: 1.1.7

module Main exposing (..)

import Html exposing (Html)

type Id
    = Id String

type Hour
    = Hour Int

type AgendaEventId
    = ScheduleItemId Time.Weekday Hour Hour
    | LessonId Id
    | EventId Id

type alias AgendaEvent =
    { id : AgendaEventId
    , title : String
    }

main : Html msg
main =
    let
        event =
            { id = EventId (Id "1")
            , title = "Event 1"
            }

        simultaneousAgendaEvents =
            [ { id = ScheduleItemId Time.Mon (Hour 480) (Hour 600)
              , title = "Schedule 1"
              }
            , { id = LessonId (Id "2")
              , title = "Lesson 1"
              }
            ]

        simultaneousPosition =
            getSimultaneousPosition event simultaneousAgendaEvents
    in
    Html.text (String.fromInt simultaneousPosition)

getSimultaneousPosition : AgendaEvent -> List AgendaEvent -> Int
getSimultaneousPosition event events =
    events
        |> List.indexedMap (\a b -> ( a, b ))
        |> List.filter (\( _, e ) -> e.id == event.id)
        |> List.map (\( idx, _ ) -> idx)
        |> List.head
        |> Maybe.withDefault 0

[jfmengels]

Yeah... this is a part where you're doing something too clever for the rule.

I think that for this we'd need type inference, which we won't have for quite some time, and even then I am not sure we'll be able tell in all cases. I'd say in this case we could?

I'll re-open the issue, but don't expect this to be fixed anytime soon, it's going to be a lot of work to get there :confused:

[jpagex]

Thanks for this fast response!

Do not worry at all! You are doing a great job and I love the tool! This is not blocking me. I just wanted to let you know about my case and give you a feedback. I am really grateful for the time you are investing.

[lydell]

@jfmengels I might be missing something, but I don’t make the connection between this issue an the PR (#40) that closed this?

[jfmengels]

Ugh. Someone opened an issue on review-unused (instead of elm-review-unused) with the same issue number (https://github.com/jfmengels/review-unused/issues/25), and I made a commit saying that it fixes #25, and I only noticed the problem later. This should not have been closed.

[lydell]

I see. Maybe you can add an issue template to the deprecated repos that repeats that stuff has moved to the new ones.