elm-review/tests/NoPrematureLetComputationTest.elm

module NoPrematureLetComputationTest exposing (all)

import NoPrematureLetComputation exposing (rule)
import Review.Test
import Test exposing (Test, describe, test)



-- TODO Handle destructuring lets with multiple variables
-- TODO Incorporate https://github.com/jfmengels/elm-review/discussions/93 ? As an option?
--      Not needed because this rule currently only targets is only for functions


message : String
message =
    "Let value was declared prematurely"


details : Int -> List String
details letInsertLine =
    [ "This value is only used in some code paths, and it can therefore be computed unnecessarily."
    , "Try moving it closer to where it is needed, I recommend to move it to line " ++ String.fromInt letInsertLine ++ "."
    ]


all : Test
all =
    describe "NoPrematureLetComputation"
        [ baseTests
        , ignoreFixTests
        , letDestructuringTests
        , novingIntoFunctionTests
        ]


baseTests : Test
baseTests =
    describe "Base tests"
        [ test "should report a let declaration that could be computed in a if branch" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z : Int
    z = 1
  in
  if b then
    z
  else
    1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 8
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 5, column = 5 }, end = { row = 5, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a b c d =
  if b then
    let
        z : Int
        z = 1
    in
    z
  else
    1
"""
                        ]
        , test "should report a let declaration that could be computed in a if branch (referenced by record update expression)" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = {a = 1}
  in
  if b then
    {z | a = 2}
  else
    {a = 3}
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 7
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a b c d =
  if b then
    let
        z = {a = 1}
    in
    {z | a = 2}
  else
    {a = 3}
"""
                        ]
        , test "should add to an existing let block instead of inserting a new let block" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = {a = 1}
  in
  if b then
    let
      y = 1
    in
    {z | a = 2}
  else
    {a = 3}
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 8
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a b c d =
  if b then
    let
      z = {a = 1}
      y = 1
    in
    {z | a = 2}
  else
    {a = 3}
"""
                        ]
        , test "should not report let functions" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z n = 1
  in
  if b then
    z 1
  else
    1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should not report a let declaration is used in multiple if branches" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = 1
  in
  if b then
    z
  else
    z + 1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should not report a let declaration if it's unused" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = 1
  in
  if b then
    1
  else
    2
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should not report a let declaration is used next to where it was declared" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = 1
    y = z * 2
  in
  if b then
    y
  else
    y + 1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should not report a let declaration without branches" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = 1
  in
  z
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should report a let declaration that could be computed in a case branch" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = 1
  in
  case b of
    A ->
        1
    B ->
        z
    C ->
        1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 10
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a b c d =
  case b of
    A ->
        1
    B ->
        let
            z = 1
        in
        z
    C ->
        1
"""
                        ]
        , test "should not report a let declaration is used in multiple case branches" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = 1
  in
  case b of
    A ->
        z
    B ->
        z
    C ->
        1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should only remove the moved let declaration when there are other let declarations there" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    y = 1
    z = {a = 1}
  in
  if b then
    {z | a = 2}
  else
    {a = 3}
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 8
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 5, column = 5 }, end = { row = 5, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a b c d =
  let
    y = 1

  in
  if b then
    let
        z = {a = 1}
    in
    {z | a = 2}
  else
    {a = 3}
"""
                        ]
        , test "should not be confused by what happens in other declarations" <|
            \() ->
                """module A exposing (..)
first =
  case A of
    A -> z

second =
    let
        z = let x = 1 in x
        y = z
    in
    y
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should not report when a declaration is used in multiple sibling's declarations" <|
            \() ->
                """module A exposing (..)
fun =
    let
        z = 1
        y = z
        x = z
    in
    1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should not report when a declaration is used in multiple sibling's branches" <|
            \() ->
                """module A exposing (..)
fun =
    let
        z = 1
        y = if True then z else 1
        x = if True then z else 1
    in
    1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should not report when a declaration is used in a branch of a sibling's declaration" <|
            \() ->
                """module A exposing (..)
a =
    let
        z = 1

        viewInput =
            let
                submitOnEnter =
                    if True then
                        z

                    else
                        1
            in
            submitOnEnter

        viewButtonRow =
                z
    in
    1

a = 1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should move a let declaration from one let to the other" <|
            \() ->
                """module A exposing (..)
a =
  let
    z = 1
  in
  let
    y = 1
    x = 1
  in
  y + z
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 7
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a =
  let
    z = 1
    y = 1
    x = 1
  in
  y + z
"""
                        ]
        , test "should properly indent moved let declaration defined on multiple lines" <|
            \() ->
                """module A exposing (..)
a o =
    let
        z =
            foo
                bar
                baz
    in
    thing
        (if o.expDetail then
            let
                y =
                    foo2
                        bar2
                        baz2
            in
            [ y
            , z
            ]

        else
            []
       )
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 12
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 9 }, end = { row = 4, column = 10 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a o =
    thing
        (if o.expDetail then
            let
                z =
                    foo
                        bar
                        baz
                y =
                    foo2
                        bar2
                        baz2
            in
            [ y
            , z
            ]

        else
            []
       )
"""
                        ]
        ]


ignoreFixTests : Test
ignoreFixTests =
    describe "Ignoring automatic fixes"
        [ test "should not suggest a fix for let declarations that introduce variables in their implementation (lambda)" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z : Int
    z = \\y -> y + 1
  in
  case b of
    A y ->
      if b then
        z
      else
        1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 10
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 5, column = 5 }, end = { row = 5, column = 6 } }
                        ]
        , test "should suggest a fix for lambda that does not introduce variables" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = \\() _ -> 1
  in
  case b of
    A y ->
      if b then
        z
      else
        1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 9
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed
                                """module A exposing (..)
a b c d =
  case b of
    A y ->
      if b then
        let
            z = \\() _ -> 1
        in
        z
      else
        1
"""
                        ]
        , test "should not suggest a fix for let declarations that introduce variables in their implementation (let block)" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z : Int
    z = let y = 1
        in y
  in
  case b of
    A y ->
      if b then
        z
      else
        1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 11
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 5, column = 5 }, end = { row = 5, column = 6 } }
                        ]
        , test "should not suggest a fix for let declarations that introduce variables in their implementation but still suggest fixes for others" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z : Int
    z = \\y -> y + 1
    x = 1
  in
  case b of
    A y ->
      if b then
        z
      else
        x
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 11
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 5, column = 5 }, end = { row = 5, column = 6 } }
                        , Review.Test.error
                            { message = message
                            , details = details 13
                            , under = "x"
                            }
                            |> Review.Test.atExactly { start = { row = 6, column = 5 }, end = { row = 6, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a b c d =
  let
    z : Int
    z = \\y -> y + 1

  in
  case b of
    A y ->
      if b then
        z
      else
        let
            x = 1
        in
        x
"""
                        ]
        , test "should not suggest a fix for let declarations that introduce variables in their implementation (case expression)" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z : Int
    z = case c of
      B y -> y + 1
  in
  case b of
    A y ->
      if b then
        z
      else
        1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 11
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 5, column = 5 }, end = { row = 5, column = 6 } }
                        ]
        , test "should suggest a fix for let declarations that do not introduce variables in their implementation (case expression)" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = case c of
      B -> 1
  in
  case b of
    A y ->
      if b then
        z
      else
        1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 10
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a b c d =
  case b of
    A y ->
      if b then
        let
            z = case c of
              B -> 1
        in
        z
      else
        1
"""
                        ]
        ]


letDestructuringTests : Test
letDestructuringTests =
    describe "Let destructuring"
        [ test "should report a let destructuring with a single value (record destructuring)" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    {z} = {z = 1}
  in
  if b then
    z
  else
    1
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 7
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 6 }, end = { row = 4, column = 7 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a b c d =
  if b then
    let
        {z} = {z = 1}
    in
    z
  else
    1
"""
                        ]
        , test "should report a let destructuring with a single value (named pattern destructuring)" <|
            \() ->
                """module A exposing (..)
a =
    let
        (Foo z) =
            point
    in
    if condition then
        let
            b =
                1
        in
        z

    else
        []
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 9
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 14 }, end = { row = 4, column = 15 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a =
    if condition then
        let
            (Foo z) =
                point
            b =
                1
        in
        z

    else
        []
"""
                        ]
        ]


novingIntoFunctionTests : Test
novingIntoFunctionTests =
    describe "Moving into functions"
        [ test "should not report let declaration that would be moved to inside a lambda" <|
            \() ->
                """module A exposing (..)
a =
  let
    z = 1
  in
  (\\b ->
      if b then
        z
      else
        1
  )
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should try to move as close as possible to a lambda but not inside" <|
            \() ->
                """module A exposing (..)
a =
  let
    z = 1
  in
  if c then
    (\\b ->
        if b then
          z
        else
          1
    )
  else
    identity
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 7
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a =
  if c then
    let
        z = 1
    in
    (\\b ->
        if b then
          z
        else
          1
    )
  else
    identity
"""
                        ]
        , test "should not move declaration inside of a let function" <|
            \() ->
                """module A exposing (..)
a b c d =
  let
    z = 1
    y () =
      if cond then
        z
      else
        1
  in
  y ()
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should report let declaration that could be moved to inside a lambda passed to Maybe.map" <|
            \() ->
                """module A exposing (..)
a =
  let
    z = 1
  in
  Maybe.map
      (\\b ->
          z
      )
      x
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 8
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a =
  Maybe.map
      (\\b ->
          let
              z = 1
          in
          z
      )
      x
"""
                        ]
        , test "should report let declaration that could be moved to inside a lambda passed to Maybe.map2" <|
            \() ->
                """module A exposing (..)
a =
  let
    z = 1
  in
  Maybe.map2
      (\\b c ->
          z
      )
      x
      y
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 8
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a =
  Maybe.map2
      (\\b c ->
          let
              z = 1
          in
          z
      )
      x
      y
"""
                        ]
        , test "should not report reference to inside a lambda passed to Maybe.map if there are missing arguments to Maybe.map" <|
            \() ->
                """module A exposing (..)
a =
    let
        z = 1
        maybeZ =
            Maybe.map
                (\\b ->
                    z
                )
    in
    Maybe.map2
        Tuple.pair
        (maybeZ (Just x))
        (maybeZ (Just y))
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should not report reference to inside a lambda passed to Maybe.map2 if there are missing arguments to Maybe.map2" <|
            \() ->
                """module A exposing (..)
a =
    let
        z = 1
        maybeZ =
            Maybe.map2
                (\\b c ->
                    z
                )
                x
    in
    Maybe.map2
        Tuple.pair
        (maybeZ (Just x))
        (maybeZ (Just y))
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should not report reference to inside a lambda passed to List.map" <|
            \() ->
                """module A exposing (..)
a =
  let
    z = 1
  in
  List.map
      (\\b ->
          z
      )
      x
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should report let declaration that could be moved to inside a lambda passed to Maybe.map using |>" <|
            \() ->
                """module A exposing (..)
a =
  let
    z = 1
  in
  x
  |> Maybe.map
      (\\b ->
          z
      )
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 9
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a =
  x
  |> Maybe.map
      (\\b ->
          let
              z = 1
          in
          z
      )
"""
                        ]
        , test "should not report reference to inside a lambda passed to Maybe.map2 if there are missing arguments to Maybe.map (using |>)" <|
            \() ->
                """module A exposing (..)
a =
    let
        z = 1
        maybeZ =
            x
            |> Maybe.map2
                (\\b ->
                    z
                )
    in
    Maybe.map2
        Tuple.pair
        (maybeZ (Just x))
        (maybeZ (Just y))
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        , test "should report let declaration that could be moved to inside a lambda passed to Maybe.map using <|" <|
            \() ->
                """module A exposing (..)
a =
  let
    z = 1
  in
  Maybe.map
      (\\b ->
          z
      ) <| x
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectErrors
                        [ Review.Test.error
                            { message = message
                            , details = details 8
                            , under = "z"
                            }
                            |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 6 } }
                            |> Review.Test.whenFixed """module A exposing (..)
a =
  Maybe.map
      (\\b ->
          let
              z = 1
          in
          z
      ) <| x
"""
                        ]
        , test "should not report reference to inside a lambda passed to Maybe.map2 if there are missing arguments to Maybe.map (using <|)" <|
            \() ->
                """module A exposing (..)
a =
    let
        z = 1
        maybeZ =
           Maybe.map2
            (\\b ->
                z
            ) <| x
    in
    Maybe.map2
        Tuple.pair
        (maybeZ (Just x))
        (maybeZ (Just y))
"""
                    |> Review.Test.run rule
                    |> Review.Test.expectNoErrors
        ]