visit builders

[lue-bird]

Instead of withSimpleStructureVisitor & withStructureVisitor, builders could cover both cases:

visitErrors : List (Error errorScope) -> VisitResult context_ (Error errorScope)
visitToContext : context -> VisitResult context error -> VisitResult context error

type VisitResult context error
    = VisitResult
        { alteredContext : context
        , errors : List error
        }

up for debate

• creating a separate module Review.Visit exposing (errors, withErrors, toContext, VisitResult(..), ...)
• changing the order to supply the altered context first

  visitErrors : List (Error errorScope) -> VisitResult context_ errorScope
  visitToContext : context -> VisitResult context errorScope_
  visitWithErrors :
    List (Error errorScope)
    -> VisitResult context (Error errorScope)
    -> VisitResult context (Error errorScope)

• replacing VisitResult's errors : List error → List (Error errorScope)
• renaming VisitResult → Review.Rule.StepResult

advantages

• the duplicate with...Visitor pairs pollute the docs and makes the other go out of sync easily (there's no simple variant of withDeclarationListVisitor for example, though I guess this is on purpose)
• withStructureVisitor's undescriptive tuples now have names

disadvantages

• It's marginally more verbose when creating a simple rule
• can only replace the current way through a breaking change

[jfmengels]

I absolutely agree that the different variants of visitors (and other functions) make the API more confusing than necessary. The Simple visitors are in practice rarely useful anyway, since you very often need the context as an input :shrug:

I would also like to remove the confusion for some visitors which are available for both module and project rules, but that's for a different issue.

Just the clarify: the idea is that we change visitors to be like

expressionVisitor : Node Expression -> Context -> VisitResult errorScope Context

I like the idea. I was actually thinking of doing something like this way back before releasing elm-review. A nice addition to this would be that I wouldn't necessarily concatenate the list of errors that was returned before with the list of errors that we already know about. I was wondering just today whether that is something that slows down the process or not (probably not, but to be benchmarked. Again, for a different issue).

I think that maybe we could have a slightly different API, without a builder pattern, like this maybe:

type VisitResult context errorScope
    = VisitResult
        -- If context is Nothing, the underlying mechanism
        -- will make sure that we re-use the previous context
        { context : Maybe context
        , errors : List (Error errorScope)
        }

visitErrors : List (Error errorScope) -> VisitResult context_ errorScope
visitToContext : context -> VisitResult context errorScope_
visitBoth : List (Error errorScope) -> context -> VisitResult context errorScope_

I think the visitX naming for these functions is super confusing (and the Both oart could be improved as well). Maybe returnX would be slightly better, not sure yet.

My proposal is a bit simpler. My biggest worry with this would be that people forget to return the updated context (or forget to update it in the first place) and that that becomes somewhat hard to debug. But it's hard to say whether this would impact.

can only replace the current way through a breaking change

Maybe there's a way to support both and deprecate the old ones, to make the transition easier until we release a major version. It would make for even more confusing API/docs but maybe worth it? :man_shrugging:

[lue-bird]

My biggest worry with this would be that people forget to return the updated context (or forget to update it in the first place)

elm-review-unused to everyones rescue! Good thing it comes by default with every initialized rule! (catches both unused context lambda arguments and unused updated contexts in lets etc.)

context : Maybe context

is a great change for context re-use! This would help in cases like

expressionVisit ... =
    \context ->
        case context of
            DependencyModuleContext dependencyModuleContext ->
                dependencyModuleContext |> DependencyModuleContext

            GenerationModuleContext generationModuleContext ->
                generationModuleContext |> GenerationModuleContext

            PossibleUseModuleContext notGenerationModuleContext ->
                ..the actual logic..

visitBoth

as you said is bad regarding descriptiveness. If anything, visitErrorsAndContext. The change makes sense to disallow multiple visitToContext calls while keeping a simple API (w/out phantoms and the like) :+1:

visitX naming for these functions is super confusing

it should be read as a noun/context, which is the reason I suggested Review.Rule.Visit.errors which makes this more clear in my eyes at least. Hope we find nicer names, though!

---

What's your current preference choosing between

• errors : List error and VisitResult Context (Error errorScope)
• errors : List (Error error) and VisitResult Context errorScope

I think VisitResult Context (Error errorScope) is more clear but could lead to worse error messages when functions are defined broadly:

visitErrors : List error -> VisitResult context_ error -- don't do this

I have made good experiences with leaving the descriptive types for users to supply to the type as a whole instead of its inner type variables. Here, I don't think it makes much difference either way

---

As you mentioned, any solution would improve this very common pattern :+1:

withStructureVisitor
    (\structureNode context ->
        ( []
        , context |> theActualLogic structureNode
        )
    )

[jfmengels]

What's your current preference choosing between

I think I would prefer errors : List (Error errorScope) and VisitResult Context errorScope, as that will be less typing, and mentioning Error doesn't give you much. Also allowing it be anything else than Error errorScope would imply that you could have VisitResult Context String or something, which in practice would not work.

it should be read as a noun/context

Hmm... sure, but it doesn't read as such in my mind. Also, the visitPattern will add some confusion if we add it.

which is the reason I suggested Review.Rule.Visit.errors

Moving to a separate module would be a possibility yes.

[jfmengels]

Some additional thought: For performance reasons, I have always figured it would be interesting to skip sections of the AST. For instance in a expression visitor, you could tell elm-review to

• skip the sub-expressions of the current expression
• skip the remainder of the declaration and go to the next declaration
• skip the remainder of the file

I don't know if that is a good idea and whether it would in practice help performance (a lot of overhead if only few rules use it), as it is kind of like a "goto" instruction. It has some interesting performance implications but also a lot of the same drawbacks, such as skipping all the "exit" visitors and missing some nodes that should have been visited.

Anyway, it would be an interesting but quite complex API with a lot of things to figure out (what can you skip, and from where?). But I'm pretty sure that it could re-use the same API suggested here using a builder pattern.

xyzVisitor node context =
   -- ...
   Rule.visitErrorsAndContext newContext errors
    |> Rule.skipRemainderOfTheFile

Potentially, there would have to be an additional phantom type to VisitResult.