From e939327cc471eddbfd85d00c9fec9a15ec11f1c8 Mon Sep 17 00:00:00 2001 From: Casey Webb Date: Fri, 22 Sep 2023 20:20:00 -0500 Subject: [PATCH] Add elm-review rule for FocusLoop.Lazy --- component-catalog/review/elm.json | 5 +- component-catalog/review/src/ReviewConfig.elm | 2 + elm.json | 7 +- .../Ui/ElmReview/MemoizedFocusLoopLazy.elm | 266 ++++++++++++++++++ tests/Spec/Nri/Ui/FocusLoopLazy.elm | 10 +- .../Nri/Ui/Review/MemoizedFocusLoopLazy.elm | 55 ++++ tests/elm-verify-examples.json | 3 +- 7 files changed, 338 insertions(+), 10 deletions(-) create mode 100644 src/Nri/Ui/ElmReview/MemoizedFocusLoopLazy.elm create mode 100644 tests/Spec/Nri/Ui/Review/MemoizedFocusLoopLazy.elm diff --git a/component-catalog/review/elm.json b/component-catalog/review/elm.json index aebec6e486..7493ac47d3 100644 --- a/component-catalog/review/elm.json +++ b/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": {} } -} +} \ No newline at end of file diff --git a/component-catalog/review/src/ReviewConfig.elm b/component-catalog/review/src/ReviewConfig.elm index 95a49c5b4e..48a68da41c 100644 --- a/component-catalog/review/src/ReviewConfig.elm +++ b/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,5 @@ config = -- , NoUnused.Parameters.rule -- , NoUnused.Patterns.rule , NoUnused.Variables.rule + , MemoizedFocusLoopLazy.rule ] diff --git a/elm.json b/elm.json index 5d9779edf5..78ea87af22 100644 --- a/elm.json +++ b/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" } -} +} \ No newline at end of file diff --git a/src/Nri/Ui/ElmReview/MemoizedFocusLoopLazy.elm b/src/Nri/Ui/ElmReview/MemoizedFocusLoopLazy.elm new file mode 100644 index 0000000000..36aa61cef7 --- /dev/null +++ b/src/Nri/Ui/ElmReview/MemoizedFocusLoopLazy.elm @@ -0,0 +1,266 @@ +module Nri.Ui.ElmReview.MemoizedFocusLoopLazy exposing (rule) + +{-| This module is shamelessly copied from 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 [] diff --git a/tests/Spec/Nri/Ui/FocusLoopLazy.elm b/tests/Spec/Nri/Ui/FocusLoopLazy.elm index c4c6708a0d..5d61aa4733 100644 --- a/tests/Spec/Nri/Ui/FocusLoopLazy.elm +++ b/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 () diff --git a/tests/Spec/Nri/Ui/Review/MemoizedFocusLoopLazy.elm b/tests/Spec/Nri/Ui/Review/MemoizedFocusLoopLazy.elm new file mode 100644 index 0000000000..1f687c3e21 --- /dev/null +++ b/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" + } + ] + ] diff --git a/tests/elm-verify-examples.json b/tests/elm-verify-examples.json index 60914c4292..05023238d1 100644 --- a/tests/elm-verify-examples.json +++ b/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" ] }