Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collision detection with preferred aliases #3

Open
sparksp opened this issue May 14, 2020 · 1 comment
Open

Collision detection with preferred aliases #3

sparksp opened this issue May 14, 2020 · 1 comment
Labels
NoInconsistentAliases Relates to the NoInconsistentAliases rule

Comments

@sparksp
Copy link
Owner

sparksp commented May 14, 2020

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.
@sparksp sparksp added the help wanted Extra attention is needed label May 14, 2020
@sparksp sparksp mentioned this issue May 15, 2020
19 tasks
@sparksp sparksp added documentation Improvements or additions to documentation and removed help wanted Extra attention is needed labels May 15, 2020
@sparksp sparksp added this to To do in NoInconsistentAliases May 15, 2020
@sparksp sparksp added NoInconsistentAliases Relates to the NoInconsistentAliases rule and removed documentation Improvements or additions to documentation labels May 17, 2020
@sparksp sparksp mentioned this issue Jun 2, 2020
6 tasks
@dillonkearns
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NoInconsistentAliases Relates to the NoInconsistentAliases rule
Projects
Development

No branches or pull requests

2 participants