NoUnused.Variables falsely reporting unused constructor

[erlandsona]

Describe the bug A clear and concise description of what the bug is.

╰  1  ../node_modules/.bin/elm-review --compiler ../node_modules/.bin/elm --fix
y-- ELM-REVIEW ERROR ---------------------- src/Page/NoteList/Tearsheet.elm:67:65

NoUnused.Variables: Imported constructors for `Millions` are not used

66| import Stuff exposing (Stuff)
67| import Things exposing (Data, Millions(..))
                                                           ^^^^^^^^^^^^

You should either use this value somewhere, or remove it at the location I
pointed at.

I think I can fix this. Here is my proposal:

  66| import Stuff exposing (Stuff)
  67| import Things exposing (Data, Millions(..))
  68| import Things exposing (Data, Millions)

Not entirely sure why this is happening as the Constructor is being used in another file to unpack a decoded value. The fix causes a compile error but I don't want to remove the rule for the file?

SSCCE (Short Self-Contained Correct Example) Steps to reproduce the behavior:

1. Create a custom type with a single constructor
2. Write a decoder for it in the same file like Decode.int |> Decode.map Ctor
3. In another file import Stuff exposing (TheType(..))
4. use the Ctor in a function head pattern match but not to construct a value.

Expected behavior This should NOT cause the rule to activate, let me know if you need anything else but hope this silently helps someone else before they hit this as well...

Additional Context src/review/elm.json

{
    "type": "application",
    "source-directories": [
        "src"
    ],
    "elm-version": "0.19.1",
    "dependencies": {
        "direct": {
            "elm/core": "1.0.5",
            "jfmengels/elm-review": "2.6.1",
            "jfmengels/elm-review-common": "1.2.0",
            "jfmengels/elm-review-performance": "1.0.1",
            "jfmengels/elm-review-simplify": "2.0.7",
            "jfmengels/elm-review-unused": "1.1.19",
            "lue-bird/elm-review-missing-record-field-lens": "1.0.2",
            "stil4m/elm-syntax": "7.2.8"
        },
        "indirect": {
            "Chadtech/elm-bool-extra": "2.4.2",
            "elm/html": "1.0.0",
            "elm/json": "1.1.3",
            "elm/parser": "1.1.0",
            "elm/project-metadata-utils": "1.0.2",
            "elm/random": "1.0.0",
            "elm/time": "1.0.0",
            "elm/virtual-dom": "1.0.2",
            "elm-community/basics-extra": "4.1.0",
            "elm-community/list-extra": "8.5.1",
            "elm-community/maybe-extra": "5.2.1",
            "elm-explorations/test": "1.2.2",
            "miniBill/elm-unicode": "1.0.2",
            "rtfeldman/elm-hex": "1.0.0",
            "stil4m/structured-writer": "1.0.3",
            "the-sett/elm-pretty-printer": "2.2.3",
            "the-sett/elm-syntax-dsl": "5.3.0"
        }
    },
    "test-dependencies": {
        "direct": {
            "elm-explorations/test": "1.2.2"
        },
        "indirect": {}
    }
}

[jfmengels]

@erlandsona Just want to check, but are you sure you don't pattern match using Stuff.Ctor ->, right? That sounds unlikely seeing as applying the fix creates a compiler error.

I have trouble reproducing it. This is what I tried

Given

module B exposing (C(..))
type C = D

Then I tried:

module A exposing (a)
import B exposing (C(..))
a =
  case x of
    D -> 1

and also

module A exposing (a)
import B exposing (C(..))
a (D x) = 1

I must be missing something, but I feel a bit blind without the part where you use the constructor.

[erlandsona]

In ModuleA.elm

module ModuleA exposing (Millions(..), decoder)

type Millions
    = Millions Float

decoder : Decoder Millions
decoder = Decode.float |> Decode.map Millions

Then in ModuleB.elm

import ModuleA exposing (Millions(..))

fn : Millions -> Float
fn (Millions f) = f

That should trigger it... the issue here is that we're providing the runtime instances of the Millions via the decoder so we never actually "construct" one of them outside of ModuleA but ModuleB is definitely using the constructor.

[jfmengels]

Hmm, it's a bit embarrassing but I'm still unable to reproduce this. What I'm trying is to add a test with your examples to the test suite for the rule, and they all behave as expected, not returning any error.

Would it be possible for you to create an SSCCE? Maybe you can re-use someone else's SSCCE like this one or use it as inspiration to get you started.

Are you perhaps running elm-review with arguments, like elm-review src/, thereby ignoring some folders (like src-gen/ which would contain ModuleA for instance)? That's one of the few known ways where people can artificially create false positives :thinking:

[erlandsona]

So I just forked the sscce & tried reproducing the issue locally myself and no bones 🤔 What's weird is nearly 1-to-1 with what the code looks like in production but maybe there's something I'm just not seeing. We don't really have much/any codegen (yet 😏) so I that's likely not the culprit. And far as running the cli here's our Make task...

ELM_REVIEW := node_modules/.bin/elm-review
ELM_DIR := elm

.PHONY : elm-review
elm-review : $(ELM_REVIEW)
> @echo "Running elm-review"
> cd $(ELM_DIR) && ../$(ELM_REVIEW) --compiler ../node_modules/.bin/elm

Here's the link to my repro attempt... https://github.com/erlandsona/elm-review-unused-sscce

Guess this is gonna be more nefarious trackin' down than something reproducible in the small :/

[jfmengels]

Since we can't reproduce, I'm going to close the issue. Please comment back if you're able to do it again (or open a different issue).

[erlandsona]

Fair enough... Also with suppress it's generally been a non-issue anyways so woooooo!

[jfmengels]

Wait? Do you still have the issue in your codebase?

[erlandsona]

Haven't checked in a while 😅 I think so?

[jfmengels]

Well, if you ever figure that out again, you know what to do :smile: