search

stil4m / elm-syntax

Elm syntax in Elm
MIT License
93 stars 28 forks source link

Operator table for built in operators #37

Open ollef opened 4 years ago

ollef commented 4 years ago

Since the set of infix operators is static in Elm 0.19, would it make sense to just always use a predefined operator table containing these (plus maybe the ones from Parser and List.::)?

I was working on creating some elm-review rules, but couldn't quite get them to work because infix expressions weren't parsed taking operator precedence into account.

ollef commented 4 years ago

I started working on this here, but I ran into trouble because the infix operator processing isn't producing the results I'm expecting. See e.g. the test I added. I also can't find where it uses the InfixDirection in the code?

jfmengels commented 4 years ago

@ollef I have hardcoded the operator table in elm-review's file parsing (by hardcoding the dependencies that define operators). Expect a new (major) version containing that change in a few weeks.

That said, I agree it would make sense to have elm-syntax handle it automatically.

ollef commented 4 years ago

Nice, that'll save me some work!

Did you do your own processing or are you passing the operators to elm-syntax?

Unless I'm reading the code wrong, the operator processing of elm-syntax doesn't take the direction into account, so it might not be quite right.

jfmengels commented 4 years ago

I am passing hardcoded (and simplified) dependencies to elm-syntax before it parses the files. The code for that is here: https://github.com/jfmengels/elm-review/blob/b0524fc0f11b3769a3ba517b37507500556fb4d9/src/Review/Dependencies.elm

Basically, it's all the operators in elm/core, elm/parser and elm/url. There aren't any others, as far as I have looked (not even in elm-explorations).

The point of doing that was to get the correct AST at the end. I haven't actually tested it in practice, as I haven't had to write any rules sensitive to this. I haven't looked at elm-syntax's code, so I can't tell whether you read the code wrong or not.

jfmengels commented 4 years ago

@ollef The work I mentioned has been released in elm-review v2. I haven't looked further into whether the AST is correct though.

ollef commented 4 years ago

Great, thanks for letting me know!

stil4m commented 4 years ago

I've just implemented this: https://github.com/stil4m/elm-syntax/pull/66

@ollef Would this fit your need?

@jfmengels Would this allow you to reduce your code base on elm-review a bit? Did I miss anything?

ollef commented 4 years ago

I don't have a direct need for this, but it should be good if elm-review can use it. :+1: