jfmengels / elm-review

Analyzes Elm projects, to help find mistakes before your users find them.
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
Other
255 stars 13 forks source link

elm-review shouldn't care about indirect dependencies #134

Closed hanshenrik closed 2 years ago

hanshenrik commented 2 years ago

Describe the bug We just encountered a problem with an indirect dependency that shares module name with a direct dependency (installed as a local source directory, not as a normal direct depencensy). Here's the case:

// elm.json

{
    "source-directories": [
        "src",
        "lib/elm-ui/src" // We use a local fork of elm-ui
    ],
    "dependencies": {
        "direct": {
            "kuon/elm-string-normalize": "1.0.4",
        },
        "indirect": {
            "elm-explorations/benchmark": "1.0.2",
            "mdgriffith/style-elements": "5.0.2",
        }
    }
}

Both elm-ui and style-elements (indirect dependency of elm-explorations/benchmark, which is an indirect dependency of kuon/elm-string-normalize) exposes a module called Element. The Elm app runs perfectly fine, it does not complain about multiple modules with the same name. However, elm-review recognises import Element as style-elements rather than elm-ui.

Maybe because we don't have elm-ui as a regular direct dependency, but rather as a local source directory? Because of this, elm-review complains about unused variables (since we're not using anything from style-elements). Is there a way of telling elm-review which dependency a module is from, in cases where multiple modules share the same name? Is it correct that elm-review should consider indirect dependencies at all, or should it ignore all of those?

Note: We have told elm-review to ignore the code in lib/elm-ui (since it might not follow our code conventions) by adding Review.Rule.ignoreErrorsForDirectories [ "lib" ] to the rules in ReviewConfig.elm. Could this affect the outcome perhaps?

SSCCE (Short Self-Contained Correct Example) I didn't get time to set up a working example in a public repo unfortunately, so I might have to fix this into a proper SSCCE in the future. But if you have an existing project with elm-review already set up (which I guess you have 🙃), I think you should be able to reproduce the problem with these steps:

  1. Fork elm-ui into the root folder in your repo lib/elm-ui
  2. Add it as a source directory in your elm.json
"source-directories": [
        "src",
        "lib/elm-ui/src"
    ],
  1. elm install kuon/elm-string-normalize
  2. Import Element in Main.elm
import Element exposing (..)
import String.Normalize

main =
  el [ Background.color <| rgb 0 0 0 ] [ text String.Normalize.removeDiacritics "Hello, Élm!" ]
  1. Make sure ReviewConfig.elm includes the following
    
    module ReviewConfig exposing (config)

import NoUnused.Variables import Review.Rule exposing (Rule)

config : List Rule config = [ NoUnused.Variables.rule ] |> List.map (Review.Rule.ignoreErrorsForDirectories [ "lib" ])


6. `npx elm-review`
7. Should cause an error like this:

-- ELM-REVIEW ERROR -------------------------------------- src/Main.elm:3:16

(fix) NoUnused.Variables: No imported elements from Element are used

3| import Element exposing (..)



**Expected behavior**
`elm-review` shouldn't report this as an error, since `Color` and `rgb` from `elm-ui`'s `Element` module is used.

**Additional context**
[Slack discussion](https://elmlang.slack.com/archives/C010RT4D1PT/p1660905159659139).