Backport work from review-unused

tests/NoUnused/Exports.elm

@@ -277,20 +277,29 @@ exposedElements nodes =
             (\node ->
                 case Node.value node of
                     Exposing.FunctionExpose name ->
-                        Just <| ( name, { range = Node.range node, exposedElement = Function } )
+                        Just ( name, { range = untilEndOfVariable name (Node.range node), exposedElement = Function } )
 
                     Exposing.TypeOrAliasExpose name ->
-                        Just <| ( name, { range = Node.range node, exposedElement = TypeOrTypeAlias } )
+                        Just ( name, { range = untilEndOfVariable name (Node.range node), exposedElement = TypeOrTypeAlias } )
 
                     Exposing.TypeExpose { name } ->
-                        Just <| ( name, { range = Node.range node, exposedElement = ExposedType } )
+                        Just ( name, { range = untilEndOfVariable name (Node.range node), exposedElement = ExposedType } )
 
-                    Exposing.InfixExpose name ->
+                    Exposing.InfixExpose _ ->
                         Nothing
             )
         |> Dict.fromList
 
 
+untilEndOfVariable : String -> Range -> Range
+untilEndOfVariable name range =
+    if range.start.row == range.end.row then
+        range
+
+    else
+        { range | end = { row = range.start.row, column = range.start.column + String.length name } }
+
+
 
 -- DECLARATION LIST VISITOR
 

tests/NoUnused/ExportsTest.elm

@@ -592,6 +592,34 @@ type alias B = A.OtherType
                             ]
                           )
                         ]
+        , test "should report the correct range when exports are on multiple lines" <|
+            \() ->
+                [ """module A
+             exposing ( Card
+    , Link
+    , init
+    , toElement
+    )
+type Card = Card
+type Link = Link
+init = 1
+""", """
+module Exposed exposing (..)
+import A
+a = A.Card A.init A.toElement
+""" ]
+                    |> Review.Test.runOnModulesWithProjectData package_ rule
+                    |> Review.Test.expectErrorsForModules
+                        [ ( "A"
+                          , [ Review.Test.error
+                                { message = "Exposed type or type alias `Link` is never used outside this module."
+                                , details = details
+                                , under = "Link"
+                                }
+                                |> Review.Test.atExactly { start = { row = 3, column = 7 }, end = { row = 3, column = 11 } }
+                            ]
+                          )
+                        ]
         ]
 
 

tests/NoUnused/Variables.elm

@@ -10,15 +10,15 @@ module NoUnused.Variables exposing (rule)
 -}
 
 import Dict exposing (Dict)
-import Elm.Syntax.Declaration exposing (Declaration(..))
-import Elm.Syntax.Exposing exposing (Exposing(..), TopLevelExpose(..))
-import Elm.Syntax.Expression exposing (Expression(..), Function, FunctionImplementation, LetDeclaration(..))
+import Elm.Syntax.Declaration as Declaration exposing (Declaration)
+import Elm.Syntax.Exposing as Exposing exposing (Exposing)
+import Elm.Syntax.Expression as Expression exposing (Expression, Function, FunctionImplementation)
 import Elm.Syntax.Import exposing (Import)
-import Elm.Syntax.Module as Module exposing (Module(..))
+import Elm.Syntax.Module as Module exposing (Module)
 import Elm.Syntax.Node as Node exposing (Node(..))
 import Elm.Syntax.Pattern as Pattern exposing (Pattern)
 import Elm.Syntax.Range exposing (Range)
-import Elm.Syntax.TypeAnnotation exposing (TypeAnnotation(..))
+import Elm.Syntax.TypeAnnotation as TypeAnnotation exposing (TypeAnnotation)
 import NoUnused.NonemptyList as NonemptyList exposing (Nonempty)
 import Review.Fix as Fix exposing (Fix)
 import Review.Rule as Rule exposing (Direction, Error, Rule)
@@ -27,10 +27,6 @@ import Set exposing (Set)
 
 {-| Report variables or types that are declared or imported but never used.
 
-**NOTE**: Since `elm-review` only works in the scope of a single file, this rule
-will not report variables that are exposed but not used anywhere in the project.
-If you wish those to be reported, check out [`elm-xref`](https://github.com/zwilias/elm-xref).
-
     config =
         [ NoUnused.Variables.rule
         ]
@@ -232,25 +228,25 @@ fix declaredModules { variableType, rangeToRemove } =
 moduleDefinitionVisitor : Node Module -> Context -> ( List nothing, Context )
 moduleDefinitionVisitor (Node _ moduleNode) context =
     case Module.exposingList moduleNode of
-        All _ ->
+        Exposing.All _ ->
             ( [], { context | exposesEverything = True } )
 
-        Explicit list ->
+        Exposing.Explicit list ->
             let
                 names =
                     List.filterMap
                         (\(Node _ node) ->
                             case node of
-                                FunctionExpose name ->
+                                Exposing.FunctionExpose name ->
                                     Just name
 
-                                TypeOrAliasExpose name ->
+                                Exposing.TypeOrAliasExpose name ->
                                     Just name
 
-                                TypeExpose { name } ->
+                                Exposing.TypeExpose { name } ->
                                     Just name
 
-                                InfixExpose name ->
+                                Exposing.InfixExpose name ->
                                     -- Just name
                                     Nothing
                         )
@@ -260,13 +256,33 @@ moduleDefinitionVisitor (Node _ moduleNode) context =
 
 
 importVisitor : Node Import -> Context -> ( List (Error {}), Context )
-importVisitor ((Node _ { exposingList, moduleAlias, moduleName }) as node) context =
-    case exposingList of
+importVisitor ((Node _ import_) as node) context =
+    let
+        errors : List (Error {})
+        errors =
+            case import_.moduleAlias of
+                Just moduleAlias ->
+                    if Node.value moduleAlias == Node.value import_.moduleName then
+                        [ Rule.errorWithFix
+                            { message = "Module `Html` is aliased as `Html`"
+                            , details = [ "The alias is the same as the module name, and brings no useful value" ]
+                            }
+                            (Node.range moduleAlias)
+                            [ Fix.removeRange <| moduleAliasRange node (Node.range moduleAlias) ]
+                        ]
+
+                    else
+                        []
+
+                Nothing ->
+                    []
+    in
+    case import_.exposingList of
         Nothing ->
-            ( [], registerModuleNameOrAlias node context )
+            ( errors, registerModuleNameOrAlias node context )
 
         Just declaredImports ->
-            ( []
+            ( errors
             , List.foldl
                 (\( name, variableInfo ) context_ -> register variableInfo name context_)
                 (registerModuleAlias node context)
@@ -324,19 +340,19 @@ moduleAliasRange (Node _ { moduleName }) range =
 expressionVisitor : Node Expression -> Direction -> Context -> ( List (Error {}), Context )
 expressionVisitor (Node range value) direction context =
     case ( direction, value ) of
-        ( Rule.OnEnter, FunctionOrValue [] name ) ->
+        ( Rule.OnEnter, Expression.FunctionOrValue [] name ) ->
             ( [], markAsUsed name context )
 
-        ( Rule.OnEnter, FunctionOrValue moduleName name ) ->
+        ( Rule.OnEnter, Expression.FunctionOrValue moduleName name ) ->
             ( [], markModuleAsUsed (getModuleName moduleName) context )
 
-        ( Rule.OnEnter, OperatorApplication name _ _ _ ) ->
+        ( Rule.OnEnter, Expression.OperatorApplication name _ _ _ ) ->
             ( [], markAsUsed name context )
 
-        ( Rule.OnEnter, PrefixOperator name ) ->
+        ( Rule.OnEnter, Expression.PrefixOperator name ) ->
             ( [], markAsUsed name context )
 
-        ( Rule.OnEnter, LetExpression { declarations, expression } ) ->
+        ( Rule.OnEnter, Expression.LetExpression { declarations, expression } ) ->
             let
                 letBlockContext : LetBlockContext
                 letBlockContext =
@@ -351,7 +367,7 @@ expressionVisitor (Node range value) direction context =
                     List.foldl
                         (\declaration context_ ->
                             case Node.value declaration of
-                                LetFunction function ->
+                                Expression.LetFunction function ->
                                     let
                                         namesUsedInArgumentPatterns : { types : List String, modules : List String }
                                         namesUsedInArgumentPatterns =
@@ -365,7 +381,7 @@ expressionVisitor (Node range value) direction context =
                                         |> registerFunction letBlockContext function
                                         |> markUsedTypesAndModules namesUsedInArgumentPatterns
 
-                                LetDestructuring pattern _ ->
+                                Expression.LetDestructuring pattern _ ->
                                     context_
                         )
                         { context | scopes = NonemptyList.cons emptyScope context.scopes }
@@ -373,7 +389,7 @@ expressionVisitor (Node range value) direction context =
             in
             ( [], newContext )
 
-        ( Rule.OnEnter, LambdaExpression { args } ) ->
+        ( Rule.OnEnter, Expression.LambdaExpression { args } ) ->
             let
                 namesUsedInArgumentPatterns : { types : List String, modules : List String }
                 namesUsedInArgumentPatterns =
@@ -383,10 +399,10 @@ expressionVisitor (Node range value) direction context =
             in
             ( [], markUsedTypesAndModules namesUsedInArgumentPatterns context )
 
-        ( Rule.OnExit, RecordUpdateExpression expr _ ) ->
+        ( Rule.OnExit, Expression.RecordUpdateExpression expr _ ) ->
             ( [], markAsUsed (Node.value expr) context )
 
-        ( Rule.OnExit, CaseExpression { cases } ) ->
+        ( Rule.OnExit, Expression.CaseExpression { cases } ) ->
             let
                 usedVariables : { types : List String, modules : List String }
                 usedVariables =
@@ -401,7 +417,7 @@ expressionVisitor (Node range value) direction context =
             , markUsedTypesAndModules usedVariables context
             )
 
-        ( Rule.OnExit, LetExpression _ ) ->
+        ( Rule.OnExit, Expression.LetExpression _ ) ->
             let
                 ( errors, remainingUsed ) =
                     makeReport (NonemptyList.head context.scopes)
@@ -545,7 +561,7 @@ getUsedModulesFromPattern patternNode =
 declarationVisitor : Node Declaration -> Direction -> Context -> ( List nothing, Context )
 declarationVisitor node direction context =
     case ( direction, Node.value node ) of
-        ( Rule.OnEnter, FunctionDeclaration function ) ->
+        ( Rule.OnEnter, Declaration.FunctionDeclaration function ) ->
             let
                 functionImplementation : FunctionImplementation
                 functionImplementation =
@@ -579,7 +595,7 @@ declarationVisitor node direction context =
             in
             ( [], newContext )
 
-        ( Rule.OnEnter, CustomTypeDeclaration { name, documentation, constructors } ) ->
+        ( Rule.OnEnter, Declaration.CustomTypeDeclaration { name, documentation, constructors } ) ->
             let
                 variablesFromConstructorArguments : { types : List String, modules : List String }
                 variablesFromConstructorArguments =
@@ -610,7 +626,7 @@ declarationVisitor node direction context =
                 |> markUsedTypesAndModules variablesFromConstructorArguments
             )
 
-        ( Rule.OnEnter, AliasDeclaration { name, typeAnnotation, documentation } ) ->
+        ( Rule.OnEnter, Declaration.AliasDeclaration { name, typeAnnotation, documentation } ) ->
             let
                 namesUsedInTypeAnnotation : { types : List String, modules : List String }
                 namesUsedInTypeAnnotation =
@@ -627,7 +643,7 @@ declarationVisitor node direction context =
                 |> markUsedTypesAndModules namesUsedInTypeAnnotation
             )
 
-        ( Rule.OnEnter, PortDeclaration { name, typeAnnotation } ) ->
+        ( Rule.OnEnter, Declaration.PortDeclaration { name, typeAnnotation } ) ->
             let
                 namesUsedInTypeAnnotation : { types : List String, modules : List String }
                 namesUsedInTypeAnnotation =
@@ -644,10 +660,10 @@ declarationVisitor node direction context =
                     (Node.value name)
             )
 
-        ( Rule.OnEnter, InfixDeclaration _ ) ->
+        ( Rule.OnEnter, Declaration.InfixDeclaration _ ) ->
             ( [], context )
 
-        ( Rule.OnEnter, Destructuring _ _ ) ->
+        ( Rule.OnEnter, Declaration.Destructuring _ _ ) ->
             ( [], context )
 
         ( Rule.OnExit, _ ) ->
@@ -764,10 +780,10 @@ registerFunction letBlockContext function context =
 collectFromExposing : Node Exposing -> List ( String, VariableInfo )
 collectFromExposing exposingNode =
     case Node.value exposingNode of
-        All _ ->
+        Exposing.All _ ->
             []
 
-        Explicit list ->
+        Exposing.Explicit list ->
             let
                 listWithPreviousRange : List (Maybe Range)
                 listWithPreviousRange =
@@ -807,27 +823,60 @@ collectFromExposing exposingNode =
                                             { range | start = previousRange.end }
                         in
                         case value of
-                            FunctionExpose name ->
-                                Just ( name, { variableType = ImportedItem ImportedVariable, under = range, rangeToRemove = rangeToRemove } )
+                            Exposing.FunctionExpose name ->
+                                Just
+                                    ( name
+                                    , { variableType = ImportedItem ImportedVariable
+                                      , under = untilEndOfVariable name range
+                                      , rangeToRemove = rangeToRemove
+                                      }
+                                    )
 
-                            InfixExpose name ->
-                                Just ( name, { variableType = ImportedItem ImportedOperator, under = range, rangeToRemove = rangeToRemove } )
+                            Exposing.InfixExpose name ->
+                                Just
+                                    ( name
+                                    , { variableType = ImportedItem ImportedOperator
+                                      , under = untilEndOfVariable name range
+                                      , rangeToRemove = rangeToRemove
+                                      }
+                                    )
 
-                            TypeOrAliasExpose name ->
-                                Just ( name, { variableType = ImportedItem ImportedType, under = range, rangeToRemove = rangeToRemove } )
+                            Exposing.TypeOrAliasExpose name ->
+                                Just
+                                    ( name
+                                    , { variableType = ImportedItem ImportedType
+                                      , under = untilEndOfVariable name range
+                                      , rangeToRemove = rangeToRemove
+                                      }
+                                    )
 
-                            TypeExpose { name, open } ->
+                            Exposing.TypeExpose { name, open } ->
                                 case open of
                                     Just openRange ->
                                         -- TODO Change this behavior once we know the contents of the open range, using dependencies or the interfaces of the other modules
                                         Nothing
 
                                     Nothing ->
-                                        Just ( name, { variableType = ImportedItem ImportedType, under = range, rangeToRemove = rangeToRemove } )
+                                        Just
+                                            ( name
+                                            , { variableType = ImportedItem ImportedType
+                                              , under = range
+                                              , rangeToRemove = rangeToRemove
+                                              }
+                                            )
                     )
                 |> List.filterMap identity
 
 
+untilEndOfVariable : String -> Range -> Range
+untilEndOfVariable name range =
+    if range.start.row == range.end.row then
+        range
+
+    else
+        { range | end = { row = range.start.row, column = range.start.column + String.length name } }
+
+
 collectNamesFromTypeAnnotation : Node TypeAnnotation -> { types : List String, modules : List String }
 collectNamesFromTypeAnnotation node =
     { types = collectTypesFromTypeAnnotation node
@@ -838,10 +887,10 @@ collectNamesFromTypeAnnotation node =
 collectTypesFromTypeAnnotation : Node TypeAnnotation -> List String
 collectTypesFromTypeAnnotation node =
     case Node.value node of
-        FunctionTypeAnnotation a b ->
+        TypeAnnotation.FunctionTypeAnnotation a b ->
             collectTypesFromTypeAnnotation a ++ collectTypesFromTypeAnnotation b
 
-        Typed nameNode params ->
+        TypeAnnotation.Typed nameNode params ->
             let
                 name : List String
                 name =
@@ -854,34 +903,34 @@ collectTypesFromTypeAnnotation node =
             in
             name ++ List.concatMap collectTypesFromTypeAnnotation params
 
-        Record list ->
+        TypeAnnotation.Record list ->
             list
                 |> List.map (Node.value >> Tuple.second)
                 |> List.concatMap collectTypesFromTypeAnnotation
 
-        GenericRecord name list ->
+        TypeAnnotation.GenericRecord name list ->
             list
                 |> Node.value
                 |> List.map (Node.value >> Tuple.second)
                 |> List.concatMap collectTypesFromTypeAnnotation
 
-        Tupled list ->
+        TypeAnnotation.Tupled list ->
             List.concatMap collectTypesFromTypeAnnotation list
 
-        GenericType _ ->
+        TypeAnnotation.GenericType _ ->
             []
 
-        Unit ->
+        TypeAnnotation.Unit ->
             []
 
 
 collectModuleNamesFromTypeAnnotation : Node TypeAnnotation -> List String
 collectModuleNamesFromTypeAnnotation node =
     case Node.value node of
-        FunctionTypeAnnotation a b ->
+        TypeAnnotation.FunctionTypeAnnotation a b ->
             collectModuleNamesFromTypeAnnotation a ++ collectModuleNamesFromTypeAnnotation b
 
-        Typed nameNode params ->
+        TypeAnnotation.Typed nameNode params ->
             let
                 name : List String
                 name =
@@ -894,24 +943,24 @@ collectModuleNamesFromTypeAnnotation node =
             in
             name ++ List.concatMap collectModuleNamesFromTypeAnnotation params
 
-        Record list ->
+        TypeAnnotation.Record list ->
             list
                 |> List.map (Node.value >> Tuple.second)
                 |> List.concatMap collectModuleNamesFromTypeAnnotation
 
-        GenericRecord name list ->
+        TypeAnnotation.GenericRecord name list ->
             list
                 |> Node.value
                 |> List.map (Node.value >> Tuple.second)
                 |> List.concatMap collectModuleNamesFromTypeAnnotation
 
-        Tupled list ->
+        TypeAnnotation.Tupled list ->
             List.concatMap collectModuleNamesFromTypeAnnotation list
 
-        GenericType _ ->
+        TypeAnnotation.GenericType _ ->
             []
 
-        Unit ->
+        TypeAnnotation.Unit ->
             []
 
 

tests/NoUnused/VariablesTest.elm

@@ -555,7 +555,7 @@ import Foo
                     [ Review.Test.error
                         { message = "Imported type `C` is not used"
                         , details = details
-                        , under = "C\n        ,"
+                        , under = "C"
                         }
                         |> Review.Test.whenFixed """module SomeModule exposing (d)
 import Foo
@@ -591,6 +591,41 @@ import Foo
         , a
         )"""
                     ]
+    , test "should report unused imported functions (multiple imports on several lines, function first)" <|
+        \() ->
+            """module SomeModule exposing (d)
+import Foo
+    exposing
+        ( a
+        , b
+        )
+d = 1"""
+                |> Review.Test.run rule
+                |> Review.Test.expectErrors
+                    [ Review.Test.error
+                        { message = "Imported variable `a` is not used"
+                        , details = details
+                        , under = "a"
+                        }
+                        |> Review.Test.whenFixed
+                            """module SomeModule exposing (d)
+import Foo
+    exposing
+        ( b
+        )
+d = 1"""
+                    , Review.Test.error
+                        { message = "Imported variable `b` is not used"
+                        , details = details
+                        , under = "b"
+                        }
+                        |> Review.Test.whenFixed """module SomeModule exposing (d)
+import Foo
+    exposing
+        ( a
+        )
+d = 1"""
+                    ]
     , test "should report unused operator import" <|
         \() ->
             """module SomeModule exposing (a)
@@ -738,6 +773,23 @@ import Html.Styled exposing (Html)
 a : Html
 a = ()"""
                     ]
+    , test "should report import alias if the name is the same thing as the module name" <|
+        \() ->
+            """module SomeModule exposing (a)
+import Html as Html
+a = Html.div"""
+                |> Review.Test.run rule
+                |> Review.Test.expectErrors
+                    [ Review.Test.error
+                        { message = "Module `Html` is aliased as `Html`"
+                        , details = [ "The alias is the same as the module name, and brings no useful value" ]
+                        , under = "Html"
+                        }
+                        |> Review.Test.atExactly { start = { row = 2, column = 16 }, end = { row = 2, column = 20 } }
+                        |> Review.Test.whenFixed """module SomeModule exposing (a)
+import Html
+a = Html.div"""
+                    ]
     , test "should not report import that exposes a used exposed type" <|
         \() ->
             """module SomeModule exposing (a)