Add elm-review rule for FocusLoop.Lazy

component-catalog/review/elm.json

@@ -1,7 +1,8 @@
 {
     "type": "application",
     "source-directories": [
-        "src"
+        "src",
+        "../../src"
     ],
     "elm-version": "0.19.1",
     "dependencies": {
@@ -33,4 +34,4 @@
         },
         "indirect": {}
     }
-}
+}

component-catalog/review/src/ReviewConfig.elm

@@ -15,6 +15,7 @@ import NoUnused.CustomTypeConstructors
 import NoUnused.Exports
 import NoUnused.Modules
 import NoUnused.Variables
+import Nri.Ui.ElmReview.MemoizedFocusLoopLazy as MemoizedFocusLoopLazy
 import Review.Rule exposing (Rule)
 
 
@@ -37,4 +38,7 @@ config =
         -- , NoUnused.Parameters.rule
         -- , NoUnused.Patterns.rule
         , NoUnused.Variables.rule
+
+        --
+        , MemoizedFocusLoopLazy.rule
         ]

elm.json

@@ -94,7 +94,8 @@
         "Nri.Ui.TextArea.V5",
         "Nri.Ui.TextInput.V7",
         "Nri.Ui.Tooltip.V3",
-        "Nri.Ui.UiIcon.V1"
+        "Nri.Ui.UiIcon.V1",
+        "Nri.Ui.ElmReview.MemoizedFocusLoopLazy"
     ],
     "elm-version": "0.19.0 <= v < 0.20.0",
     "dependencies": {
@@ -114,10 +115,12 @@
         "elm-community/random-extra": "3.2.0 <= v < 4.0.0",
         "elm-community/string-extra": "4.0.1 <= v < 5.0.0",
         "elm-explorations/test": "2.0.0 <= v < 3.0.0",
+        "jfmengels/elm-review": "2.13.1 <= v < 3.0.0",
         "pablohirafuji/elm-markdown": "2.0.5 <= v < 3.0.0",
         "rtfeldman/elm-css": "17.0.1 <= v < 19.0.0",
         "rtfeldman/elm-iso8601-date-strings": "1.1.4 <= v < 2.0.0",
         "rtfeldman/elm-sorter-experiment": "2.1.1 <= v < 3.0.0",
+        "stil4m/elm-syntax": "7.3.2 <= v < 8.0.0",
         "tesk9/accessible-html-with-css": "4.1.0 <= v < 6.0.0",
         "tesk9/palette": "3.0.1 <= v < 4.0.0"
     },
@@ -125,4 +128,4 @@
         "elm/html": "1.0.0 <= v < 2.0.0",
         "tesk9/accessible-html": "5.0.0 <= v < 6.0.0"
     }
-}
+}

review/elm.json

@@ -1,7 +1,8 @@
 {
     "type": "application",
     "source-directories": [
-        "src"
+        "src",
+        "../src"
     ],
     "elm-version": "0.19.1",
     "dependencies": {
@@ -33,4 +34,4 @@
         },
         "indirect": {}
     }
-}
+}

review/src/ReviewConfig.elm

@@ -19,6 +19,7 @@ import NoUnused.Modules
 import NoUnused.Parameters
 import NoUnused.Patterns
 import NoUnused.Variables
+import Nri.Ui.ElmReview.MemoizedFocusLoopLazy as MemoizedFocusLoopLazy
 import Review.Rule exposing (Rule)
 
 
@@ -38,4 +39,7 @@ config =
     -- , NoUnused.Parameters.rule
     -- , NoUnused.Patterns.rule
     , NoUnused.Variables.rule
+
+    --
+    , MemoizedFocusLoopLazy.rule
     ]

src/Nri/Ui/ElmReview/MemoizedFocusLoopLazy.elm

@@ -0,0 +1,266 @@
+module Nri.Ui.ElmReview.MemoizedFocusLoopLazy exposing (rule)
+
+{-| This module is shamelessly copied from <https://github.com/NoRedInk/elm-review-html-lazy/blob/master/src/UseMemoizedLazyLambda.elm> and modified for FocusLoop.Lazy.
+
+See the repo above for more details.
+
+@docs rule
+
+-}
+
+import Elm.Syntax.Declaration exposing (Declaration(..))
+import Elm.Syntax.Exposing exposing (Exposing(..))
+import Elm.Syntax.Expression exposing (Expression(..), LetDeclaration(..))
+import Elm.Syntax.Import exposing (Import)
+import Elm.Syntax.Node as Node exposing (Node(..))
+import Review.ModuleNameLookupTable as ModuleNameLookupTable exposing (ModuleNameLookupTable)
+import Review.Rule as Rule exposing (ContextCreator, Error, Rule)
+import Set exposing (Set)
+
+
+type alias ModuleContext =
+    { importedNames : ModuleNameLookupTable
+    , importedExposingAll : Set String
+    }
+
+
+type alias KnownModule =
+    { name : String
+    , functions : Set String
+    }
+
+
+focusLoopLazyModule : KnownModule
+focusLoopLazyModule =
+    { name = "Nri.Ui.FocusLoop.Lazy.V1"
+    , functions = Set.fromList [ "lazy", "lazy2", "lazy3", "lazy4", "lazy5" ]
+    }
+
+
+{-|
+
+    This rule checks that calls to FocusLoop.lazy, lazy2, ... are memoized at the top level of a view function.
+
+-}
+rule : Rule
+rule =
+    Rule.newModuleRuleSchemaUsingContextCreator "UseMemoizedLambda" initialContext
+        |> Rule.withImportVisitor importVisitor
+        |> Rule.withDeclarationEnterVisitor declarationEnterVisitor
+        |> Rule.fromModuleRuleSchema
+
+
+initialContext : ContextCreator () ModuleContext
+initialContext =
+    Rule.initContextCreator
+        (\importedNames _ ->
+            { importedNames = importedNames
+            , importedExposingAll = Set.empty
+            }
+        )
+        |> Rule.withModuleNameLookupTable
+
+
+findLazyCalls : ModuleContext -> Node Expression -> List (Node Expression)
+findLazyCalls moduleContext expression =
+    fold
+        (\exp accum ->
+            case identifyLazyFunction moduleContext exp of
+                Just _ ->
+                    exp :: accum
+
+                _ ->
+                    accum
+        )
+        []
+        expression
+        |> List.reverse
+
+
+declarationEnterVisitor : Node Declaration -> ModuleContext -> ( List (Error {}), ModuleContext )
+declarationEnterVisitor node moduleContext =
+    case Node.value node of
+        FunctionDeclaration { declaration } ->
+            let
+                decl =
+                    Node.value declaration
+
+                makeLazyError (Node range _) =
+                    Rule.error { message = "Calls to lazy should be memoized at the top level of a view function.", details = [ "See here" ] } range
+
+                errors =
+                    case ( normalizeApplication decl.expression, decl.arguments ) of
+                        ( [ lazyFunc, _ ], [] ) ->
+                            case identifyLazyFunction moduleContext lazyFunc of
+                                Just _ ->
+                                    []
+
+                                Nothing ->
+                                    findLazyCalls moduleContext decl.expression
+                                        |> List.map makeLazyError
+
+                        _ ->
+                            findLazyCalls moduleContext decl.expression
+                                |> List.map makeLazyError
+            in
+            ( errors, moduleContext )
+
+        _ ->
+            ( [], moduleContext )
+
+
+importVisitor :
+    Node Import
+    -> { context | importedExposingAll : Set String }
+    -> ( List (Error {}), { context | importedExposingAll : Set String } )
+importVisitor (Node _ { moduleName, exposingList }) context =
+    case exposingList of
+        Just (Node _ (All _)) ->
+            ( [], { context | importedExposingAll = Set.insert (Node.value moduleName |> String.join ".") context.importedExposingAll } )
+
+        _ ->
+            ( [], context )
+
+
+identifyLazyFunction :
+    { context | importedNames : ModuleNameLookupTable, importedExposingAll : Set String }
+    -> Node Expression
+    -> Maybe String
+identifyLazyFunction { importedNames, importedExposingAll } node =
+    case Node.value node of
+        FunctionOrValue _ functionName ->
+            case ModuleNameLookupTable.moduleNameFor importedNames node of
+                Just ((_ :: _) as moduleNameList) ->
+                    let
+                        moduleName =
+                            moduleNameList |> String.join "."
+
+                        isLazyModule =
+                            moduleName == focusLoopLazyModule.name
+                    in
+                    if isLazyModule then
+                        Just functionName
+
+                    else
+                        Nothing
+
+                _ ->
+                    let
+                        fromHtmlLazy =
+                            Set.member focusLoopLazyModule.name importedExposingAll && Set.member functionName focusLoopLazyModule.functions
+                    in
+                    if fromHtmlLazy then
+                        Just functionName
+
+                    else
+                        Nothing
+
+        _ ->
+            Nothing
+
+
+
+{- https://github.com/NoRedInk/elm-review-html-lazy/blob/master/src/Elm/Syntax/Expression/Extra.elm -}
+
+
+foldHelper : (Node Expression -> a -> a) -> a -> List (Node Expression) -> a
+foldHelper function accum stack =
+    case stack of
+        [] ->
+            accum
+
+        expr :: stackTail ->
+            let
+                newStack =
+                    case Node.value expr of
+                        Application exprs ->
+                            exprs
+
+                        OperatorApplication _ _ leftExp rightExp ->
+                            [ leftExp, rightExp ]
+
+                        IfBlock condExp trueExp falseExp ->
+                            [ condExp, trueExp, falseExp ]
+
+                        Negation exp ->
+                            [ exp ]
+
+                        TupledExpression exps ->
+                            exps
+
+                        ParenthesizedExpression exp ->
+                            [ exp ]
+
+                        LetExpression { declarations, expression } ->
+                            let
+                                mapLetDeclarations (Node _ letDeclaration) =
+                                    case letDeclaration of
+                                        LetFunction { declaration } ->
+                                            (Node.value declaration).expression
+
+                                        LetDestructuring _ exp ->
+                                            exp
+                            in
+                            List.map mapLetDeclarations declarations ++ [ expression ]
+
+                        CaseExpression { expression, cases } ->
+                            expression :: List.map Tuple.second cases
+
+                        LambdaExpression { expression } ->
+                            [ expression ]
+
+                        RecordExpr recordSetters ->
+                            List.map (Node.value >> Tuple.second) recordSetters
+
+                        ListExpr exps ->
+                            exps
+
+                        RecordAccess exp _ ->
+                            [ exp ]
+
+                        RecordUpdateExpression _ recordSetters ->
+                            List.map (Node.value >> Tuple.second) recordSetters
+
+                        _ ->
+                            []
+            in
+            foldHelper function (function expr accum) (newStack ++ stackTail)
+
+
+fold : (Node Expression -> a -> a) -> a -> Node Expression -> a
+fold function accum expr =
+    foldHelper function accum [ expr ]
+
+
+unParenthesize : Node Expression -> Node Expression
+unParenthesize node =
+    case Node.value node of
+        ParenthesizedExpression exp ->
+            unParenthesize exp
+
+        _ ->
+            node
+
+
+normalizeApplicationHelper : Node Expression -> List (Node Expression) -> List (Node Expression)
+normalizeApplicationHelper exp accum =
+    case Node.value exp of
+        Application (func :: args) ->
+            normalizeApplicationHelper func (args ++ accum)
+
+        OperatorApplication "<|" _ func arg ->
+            normalizeApplicationHelper func (arg :: accum)
+
+        OperatorApplication "|>" _ arg func ->
+            normalizeApplicationHelper func (arg :: accum)
+
+        ParenthesizedExpression innerExp ->
+            normalizeApplicationHelper innerExp accum
+
+        _ ->
+            exp :: List.map unParenthesize accum
+
+
+normalizeApplication : Node Expression -> List (Node Expression)
+normalizeApplication exp =
+    normalizeApplicationHelper exp []

tests/Spec/Nri/Ui/FocusLoopLazy.elm

@@ -98,8 +98,8 @@ update msg model =
             }
 
 
-view : State -> List (Html Msg)
-view state =
+view : List String -> List ( String, Html Msg )
+view =
     FocusLoop.lazy
         { focus = Focus
         , toId = identity
@@ -107,8 +107,6 @@ view state =
         , upDown = True
         , view = \arrowKeyHandlers item -> Html.button [ Key.onKeyDownPreventDefault arrowKeyHandlers ] [ Html.text item ]
         }
-        state.foos
-        |> List.map Tuple.second
 
 
 program : TestContext
@@ -119,6 +117,8 @@ program =
             , focused = Nothing
             }
         , update = update
-        , view = view >> Html.div [] >> Html.toUnstyled
+        , view =
+            \state ->
+                Html.toUnstyled (Html.div [] (view state.foos |> List.map Tuple.second))
         }
         |> ProgramTest.start ()

tests/Spec/Nri/Ui/Review/MemoizedFocusLoopLazy.elm

@@ -0,0 +1,55 @@
+module Spec.Nri.Ui.Review.MemoizedFocusLoopLazy exposing (..)
+
+import Nri.Ui.ElmReview.MemoizedFocusLoopLazy exposing (rule)
+import Review.Test
+import Test exposing (Test, describe, test)
+
+
+withHeader : String -> String
+withHeader body =
+    """
+module A exposing (..)
+import Nri.Ui.FocusLoop.Lazy.V1 as Lazy
+import Html exposing (text)
+
+""" ++ body
+
+
+all : Test
+all =
+    describe "MemoizedFocusLoopLazy"
+        [ test "Passes if lazy is memoized" <|
+            \_ ->
+                withHeader """
+f = Lazy.lazy {}
+"""
+                    |> Review.Test.run rule
+                    |> Review.Test.expectNoErrors
+        , test "Fails if lazy application is not the top expression" <|
+            \_ ->
+                withHeader """
+f = let g = Lazy.lazy view
+     in g
+"""
+                    |> Review.Test.run rule
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = "Calls to lazy should be memoized at the top level of a view function."
+                            , details = [ "See here" ]
+                            , under = "Lazy.lazy"
+                            }
+                        ]
+        , test "Fails if lazy application is not point-free" <|
+            \_ ->
+                withHeader """
+f x = Lazy.lazy view x
+"""
+                    |> Review.Test.run rule
+                    |> Review.Test.expectErrors
+                        [ Review.Test.error
+                            { message = "Calls to lazy should be memoized at the top level of a view function."
+                            , details = [ "See here" ]
+                            , under = "Lazy.lazy"
+                            }
+                        ]
+        ]

tests/elm-verify-examples.json

@@ -90,6 +90,7 @@
         "Nri.Ui.TextArea.V5",
         "Nri.Ui.TextInput.V7",
         "Nri.Ui.Tooltip.V3",
-        "Nri.Ui.UiIcon.V1"
+        "Nri.Ui.UiIcon.V1",
+        "Nri.Ui.ElmReview.MemoizedFocusLoopLazy"
     ]
 }