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

Add context to visitors #20

Merged
merged 10 commits into from
Apr 13, 2024
Merged

Conversation

jfmengels
Copy link
Collaborator

@jfmengels jfmengels commented Apr 13, 2024

This is a bit of a big bang PR, sorry. I tried cutting it into chunks but that was a bit too hard for me.

This adds the ability for rules to collect context, which makes them infinitely more powerful.
I made the no_deprecated dummy rule to show how it can be used.

Also, I made it so that the errors have to provide the location themselves. That is currently done by storing the current function in the context, but later when we have locations in the AST, this will be a lot simpler.

I fail to update the snapshot tests, but if I'm not mistaken they should work. If someone wants to generate those files, that'd be great.


The main point of the change is to add context to the rule and pass it to the next visitor which returns both a list of errors and the updated context (with the updated information of what was collected). Since the type for the context is different for each rule, it has to be a type variable, like Rule(context). The problem when doing that is that we then can't store rules in a single list.

So what I did here, is to hide that type variable inside of the ModuleVisitorOperations type and in function closures, thanks to a technique described (not so shortly) in https://discourse.elm-lang.org/t/demystifying-jeremys-interfaces/8834/

Basically, we're turning rules into "objects", where rule.expresion_visitor(node) returns the rule itself but with an updated internal state. The internal state contains the list of errors and the current context.

Once the entire file is visited, we get the errors using rule.get_errors() and we discard the rest.

@bcpeinhardt
Copy link
Owner

Weird, gleam test runs fine and then birdie says it fails to compile. Not sure what's going on there, maybe it's a known issue. @giacomocavalieri

@bcpeinhardt
Copy link
Owner

bcpeinhardt commented Apr 13, 2024

It feels a touch complicated, I'm gonna have to clone it and wrap my head around it.
Okay it makes sense now, yeah this is great. I think we should use proper Never types. I'm also not a fan of tuples in function signatures, I'd prefer a type with named fields for readability.

Once we get the test situation fixed I'm fine with merging as is and addressing these comments in a follow up PR.

@bcpeinhardt bcpeinhardt merged commit 4fe79fc into bcpeinhardt:main Apr 13, 2024
7 checks passed
@jfmengels jfmengels deleted the add-context branch April 13, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants