sparksp / elm-review-imports

elm-review rule to enforce consistent import aliases
https://package.elm-lang.org/packages/sparksp/elm-review-imports/latest/
MIT License
14 stars 3 forks source link

Collision detection with preferred aliases #3

Open sparksp opened 4 years ago

sparksp commented 4 years ago

What should we do if there's a collision of aliases within a module? Consider the following imports:

--[ src/Somefile.elm ]
import Html.Attributes as A
import Svg.Attributes as Attr

Scenario 1 - Pass

--[ review/src/ReviewConfig.elm ]
NoInconsistentAliases.config
    [ ( "Svg.Attributes", "Attr" )
    ]
    |> NoInconsistentAliases.rule

There's nothing to do here, we already meet the config.

Scenario 2 - Collision

--[ review/src/ReviewConfig.elm ]
NoInconsistentAliases.config
    [ ( "Html.Attributes", "Attr" )
    ]
    |> NoInconsistentAliases.rule

The rule says that A is wrong for Html.Attributes, but our preferred alias Attr is already taken. We don't have a preference for Svg.Attributes so we could report Html.Attributes and mention that the preferred alias is already taken. We would not be able to provide an automated fix for this.

Solution: Report Html.Attributes with no fix.

Scenario 3 - Fail

--[ review/src/ReviewConfig.elm ]
NoInconsistentAliases.config
    [ ( "Html.Attributes", "Attr" )
    , ( "Svg.Attributes", "SvgAttr" )
    ]
    |> NoInconsistentAliases.rule

The rule says that both aliases are wrong, however we can't offer a fix for Html.Attributes because the preferred alias is already taken. We can offer a fix for Svg.Attributes, which would clear the way to fix Html.Attributes.

Solution: Report Html.Attributes with no fix (as Scenario 2), and report Svg.Attributes with a fix (as normal).

Scenario 4 - Double Collision

--[ review/src/ReviewConfig.elm ]
NoInconsistentAliases.config
    [ ( "Html.Attributes", "Attr" )
    , ( "Svg.Attributes", "Attr" )
    ]
    |> NoInconsistentAliases.rule

The rule says that A is wrong for Html.Attributes, but our preferred alias Attr is already taken by another preferred alias. If the user fixes Html.Attributes then we will have a problem with Svg.Attributes instead.

Solution: No Report

Scenario 5 - Double Fail

--[ review/src/ReviewConfig.elm ]
NoInconsistentAliases.config
    [ ( "Html.Attributes", "Attributes" )
    , ( "Svg.Attributes", "Attributes" )
    ]
    |> NoInconsistentAliases.rule

The rule says that both aliases are wrong. We can report both of them and offer fixes for each - when a fix is applied for either then we'll be in the same situation as Scenario 4.

Solution: Report both aliases as normal.

Scenario 6 - Collision with bare module

--[ src/Somefile.elm ]
import Attr
import Html.Attributes as A
--[ review/src/ReviewConfig.elm ]
NoInconsistentAliases.config
    [ ( "Html.Attributes", "Attr" )
    ]
    |> NoInconsistentAliases.rule

The rule says that A is wrong for Html.Attributes, but our preferred alias Attr is already taken by a bare module import (no alias). If the user fixes Html.Attributes then they will have to alias module Attr instead.

Solution: No Report

Scenario 7 - Collision with bare module

--[ src/Somefile.elm ]
import Attributes
import Html.Attributes as A
--[ review/src/ReviewConfig.elm ]
NoInconsistentAliases.config
    [ ( "Attributes", "Attr" )
    , ( "Html.Attributes", "Attributes" )
    ]
    |> NoInconsistentAliases.rule

This is Scenario 3 again - the only difference is that Attributes is a bare module. We don't need to do anything different here - when Attributes is correctly aliased then a fix can be offered for Html.Attributes.

Solution: Report Attributes with a fix (as normal), and report Html.Attributes with no fix (as Scenario 2).

Summary

1) Do not report a bad alias if the preferred alias is taken by another matching preferred alias. 2) Do not report a bad alias if the preferred alias is taken by an import with no alias (singular, bare module). 3) Report a bad alias if the preferred alias is taken by an aliased module with no preferred alias, but do not offer a fix.

dillonkearns commented 3 years ago

Another use case to consider:

In elm-pages I'm using this pattern which some package authors use to keep some internals accessible within the package. I have a type alias that is in an "exposed module" in the package. It has the same name as the actual internal definition.

import DataSource.Internal.Glob exposing (Glob(..))

type alias Glob a =
    DataSource.Internal.Glob.Glob a

I get this error

(fix) NoModuleOnExposedNames: Module used on exposed type `Glob`.

224| type alias Glob a =
225|     DataSource.Internal.Glob.Glob a
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It is not necessary to use the module here as `Glob` was exposed on import.

You should remove the module from this call, or remove the name from the import
.. exposing list.

However, that would result in an invalid Elm module if I followed that fix:

-- ALIAS PROBLEM - elm-pages/src/DataSource/Glob.elm

This type alias is recursive, forming an infinite type!

224| type alias Glob a =
                ^^^^
When I expand a recursive type alias, it just keeps getting bigger and bigger.
So dealiasing results in an infinitely large type! Try this instead:

    type Glob a =
        Glob
            (Glob a)

Hint: This is kind of a subtle distinction. I suggested the naive fix, but I
recommend reading <https://elm-lang.org/0.19.1/recursive-alias> for ideas on how
to do better.