Add Review.Rule.with...Pattern...Visitors

[lue-bird]

Including Pattern visitors would be useful to for example forbid tuple patterns, specific named patterns, ...:

• Review.Rule.withSimplePatternVisitor
• Review.Rule.withPatternEnterVisitor
• Review.Rule.withPatternExitVisitor

It is currently quite a lot of code and therefore error-prone.
Maybe the code I wrote can help with the implementation just a little 🤷:

import Elm.Syntax.Pattern as Pattern exposing (Pattern)
import Elm.Syntax.Expression as Expression exposing (Expression)

reviewExpression : Expression -> ...
reviewExpression =
    \expression ->
        case expression of
            Expression.CaseExpression { cases } ->
                cases
                    |> List.concatMap
                        (\( casePattern, _ ) ->
                            casePattern |> ...
                        )

            Expression.LambdaExpression { args } ->
                args |> List.concatMap ...

            Expression.LetExpression { declarations } ->
                let
                    declarationLetNamedThings =
                        \(Node _ declarationLet) ->
                            case declarationLet of
                                Expression.LetFunction { declaration, signature } ->
                                    declaration
                                        |> Node.value
                                        |> .arguments
                                        |> List.concatMap ...

                                Expression.LetDestructuring destructuringPattern _ ->
                                    destructuringPattern |> ...
                in
                declarations
                    |> List.concatMap declarationLetNamedThings

            _ ->
                []


reviewDeclarationModuleScope : Declaration -> ...
reviewDeclarationModuleScope =
    \declaration ->
        case declaration of
            Declaration.FunctionDeclaration function ->
                function.declaration
                    |> Node.value
                    |> .arguments
                    |> List.concatMap ...

            Declaration.AliasDeclaration _ ->
                []

            Declaration.CustomTypeDeclaration _->
                []

            Declaration.PortDeclaration _ ->
                []

            -- not possible in modules outside elm/...
            Declaration.InfixDeclaration _ ->
                []

            -- Such a declaration doesn't exist in elm.
            Declaration.Destructuring _ _ ->
                []

{-| All patterns that are itself part of the whole pattern.

Doesn't collect pattern literals, just looks one step deeper into the whole pattern.

-}
patternChildren : Pattern -> List (Node Pattern)
patternChildren =
    \pattern ->
        case pattern of
            Pattern.UnitPattern ->
                []

            Pattern.CharPattern _ ->
                []

            Pattern.IntPattern _ ->
                []

            Pattern.FloatPattern _ ->
                []

            Pattern.HexPattern _ ->
                []

            Pattern.StringPattern _ ->
                []

            Pattern.RecordPattern _ ->
                []

            Pattern.AllPattern ->
                []

            Pattern.VarPattern _ ->
                []

            Pattern.ParenthesizedPattern innerPattern ->
                [ innerPattern ]

            Pattern.AsPattern innerPattern _ ->
                [ innerPattern ]

            Pattern.UnConsPattern headPattern tailPattern ->
                [ headPattern, tailPattern ]

            Pattern.NamedPattern _ argumentPatterns ->
                argumentPatterns

            Pattern.TuplePattern partPatterns ->
                partPatterns

            Pattern.ListPattern elementPatterns ->
                elementPatterns

[jfmengels]

Having a specific visitor for pattern would I think only be useful to detect usages of a type (to find unused ones or to forbid some).

Apart from that, I have a hard time finding good use-cases for a generic visitor, because I feel like often the reason why I'm visiting is contextual, and contextual to the expression, or declaration I'm looking at. For instance, I may not visit patterns the same way when destructuring arguments of a function and when I'm looking at the patterns of a case expression.

Therefore, I think it could make sense for us to provide something like a Review.Rule.visitPattern, which would be kind of like a "fold"

visitPattern : (Node Pattern -> a) -> List (Node Pattern) -> a -> a
visitPattern fn nodes acc =
  case nodes of
    [] -> acc
    pattern :: restOfPatterns ->
      -- Using your implementation of `patternChildren`
      visitPattern fn (patternChildren children ++ restOfPatterns) (fn pattern)

and then you can call this function in all the relevant places, depending on the context that you're in (looking at arguments, pattern matches, etc.)

I think we could provide something similar for type annotations as well, instead of the proposal in #122.

I'd be curious to see whether in practice this fits with what I've done in some of my rules. Because I feel like in some cases the way I visited was different than this proposal or not. I think that would be worth checking when implementing this feature.

Then comes the question: And then comes the question: should we provide similar functions for expressions and declarations for consistency?

What do you think of this?

[lue-bird]

Very neat proposal, might even fit into elm-syntax as it is unrelated to elm-review!

Doesn't this apply (or doesn't apply) for expressions as well?
Why give expressions special treatment vs patterns, types, ...

should we provide similar functions for expressions and declarations for consistency?

hell yes, I've written (or copied) similar functions lots of times

For expressions

import Elm.Syntax.Expression as Expression exposing (Expression)
import Elm.Syntax.Node as Node exposing (Node(..))

{-| Get all immediate child expressions of an expression.
-}
expressionsInner : Expression -> List (Node Expression)
expressionsInner expression =
    case expression of
        Expression.UnitExpr ->
            []

        Expression.Integer _ ->
            []

        Expression.Hex _ ->
            []

        Expression.Floatable _ ->
            []

        Expression.Literal _ ->
            []

        Expression.CharLiteral _ ->
            []

        Expression.GLSLExpression _ ->
            []

        Expression.RecordAccessFunction _ ->
            []

        Expression.FunctionOrValue _ _ ->
            []

        Expression.Operator _ ->
            []

        Expression.PrefixOperator _ ->
            []

        Expression.LambdaExpression lambda ->
            [ lambda.expression ]

        Expression.RecordAccess record _ ->
            [ record ]

        Expression.ParenthesizedExpression expression_ ->
            [ expression_ ]

        Expression.Negation expression_ ->
            [ expression_ ]

        Expression.OperatorApplication _ _ leftExpression rightExpression ->
            [ leftExpression, rightExpression ]

        Expression.IfBlock predExpr thenExpr elseExpr ->
            [ predExpr, thenExpr, elseExpr ]

        Expression.ListExpr expressions ->
            expressions

        Expression.TupledExpression expressions ->
            expressions

        Expression.Application expressions ->
            expressions

        Expression.RecordExpr fields ->
            fields |> List.map (\(Node _ ( _, value )) -> value)

        Expression.RecordUpdateExpression record updaters ->
            (record |> Node.map (FunctionOrValue []))
                :: (updaters |> List.map (\(Node _ ( _, newValue )) -> newValue))

        Expression.CaseExpression caseBlock ->
            caseBlock.expression
                :: (caseBlock.cases |> List.map (\( _, expression_ ) -> expression_))

        Expression.LetExpression letBlock ->
            letBlock.declarations
                |> List.map
                    (\(Node _ letDeclaration) ->
                        case letDeclaration of
                            LetFunction { declaration } ->
                                declaration |> Node.value |> .expression

                            LetDestructuring _ expression_ ->
                                expression_
                    )
                |> (::) letBlock.expression

[jfmengels]

might even fit into elm-syntax as it is unrelated to elm-review!

Potentially yes. It's not something it usually provides though.

similar functions lots of times

I'm really curious why you have felt the need to do that. In my experience, there's very rarely the need to reimplement your own expression visitor. Could you provide some examples for this?

Why give expressions special treatment vs patterns, types, ...

It doesn't need to have special treatment, but there a few reasons among which:

• Expressions are a lot more often context insensitive, in the sense that their handling doesn't depend in which declaration they appear. For patterns, it usually matters more, as explained in my previous comment.
• There are a lot of expressions variants, letting users handle them would be annoying. It's slightly more okay for patterns/types (though I agree it's still annoying, which is why I'm in favor of adding something like this)
• Expressions often create errors, and those would be annoying to have to accumulate manually.
• Patterns and types rarely create errors on their own. It's usually errors that depend on the expression/declaration being visited.