Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Remove direction from declarationVisitor #13

Closed
jfmengels opened this issue May 10, 2020 · 5 comments
Closed

RFC: Remove direction from declarationVisitor #13

jfmengels opened this issue May 10, 2020 · 5 comments
Labels

Comments

@jfmengels
Copy link
Owner

Both declaration and expression visitors are visited twice

  • on "enter", before the children have been visited
  • on "exit", after the children have been visited

This is quite useful to know in which "context" you are: Am I inside a let..in expression, which function declaration am I, etc?

For let..in expression, this can be quite useful, as the direction-less alternative can be quite painful to write.

For declaration visitors though, the point of the "exit" visitor is, most often as I have seen, to remove the "current" function related information that were set in the "enter" visitor. But these will be overridden anyway by the next "enter" visit.

We might avoid a lot of visits by making the declaration visitor "direction-less", thus gaining in performance.

This will likely happen, unless scenarios are found where this simplifies the visit of a module drastically. If you know of any, please write them down as comments.

@jfmengels jfmengels added enhancement New feature or request discussion labels May 10, 2020
@sparksp
Copy link

sparksp commented Jun 1, 2020

NoUnused.Parameters uses OnEnter and OnExit.

OnEnter
Record all the values from the arguments list in the context. The Expression visitor will then remove any values it comes across.
OnExit
Report any values remaining in the context.

If you take away OnExit then I would have to store all the remaining values for final module evaluation instead. Alternatively, if you would provide me with a declarationExitVisitor then that would suit this case. You could also separate the expressionVisitor in the same way.

@jfmengels
Copy link
Owner Author

Yeah, a few of the rules I wrote also use it, but you could do it without.
Instead of doing something on declaration exit, you do it on declaration enter right before you do what you currently do on enter. That would work since the next thing that gets visited after a declaration exit is the next declaration's enter. For the last declaration, you would then do that in the final evaluation (which is not ideal, but doable)

I think having a withDeclarationExitVisitor and equivalent expression visitor makes sense. Considering how often the exit visitors are used (not that much), this would make the visitors simpler and the rules run a bit faster.

The only drawback is that the code for enter and exit, which are related, would be more separated in terms of lines of code than what they are today.

But I think this would be nice for the next major version 👍

@sparksp
Copy link

sparksp commented Jun 1, 2020

What I liked about using OnExit for NoUnused.Parameters was that I could get at the Node again and I didn't need to keep it in the context - this meant that OnEnter was only concerned with Strings (and context could be a Set instead of a Dict too).

Having the (same) code split between entering the next node and a final visitor sounds like a step backward to me - it would introduce 3 states too: 1 where you've not visited anything yet, 2 where you have, and 3 being after the last node. So, I'm glad you like the idea of exit visitors.

@jfmengels
Copy link
Owner Author

That's a very good point 👍

@jfmengels
Copy link
Owner Author

Implemented in v2.2.0 https://github.com/jfmengels/elm-review/releases/tag/2.2.0 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants