stil4m / elm-syntax

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

Ongoing branch for v8 #188

Open jfmengels opened 1 year ago

jfmengels commented 1 year ago

This PR contains a pull request containing all breaking changes for the next major release v8 and all subsequent work we do that can't or shouldn't target v7.

This is itself a rebase and cherry-pick of all the changes done on the previous v8 branch, which should probably not be touched anymore.

The intent is to get as many breaking changes into this version so that we can avoid needing to release a v9 in the future, as that will impact all the tools that directly or indirectly depend on this project, including but not limited to elm-review.

Main goals

Here are a few of the main intentions of the tool:

Proposals for changes to come in v8 can be created and found with the breaking change label. Feel free to suggest your own!

Note: We are still maintaining v7 on the master branch. If we have changes to apply to v7, we will likely rebase this branch often (leading to potential conflicts for branches that target this branch).

Summary of changes so far

Note that there is still plenty of work to be done and nothing here is set in stone. If you have any suggestions or concerns, please let us know by opening a new issue.

You can view the CHANGELOG here (under Unreleased): https://github.com/stil4m/elm-syntax/blob/breaking-changes-v8/CHANGELOG.md, which should be the most up to date list of changes.

Breaking changes

Expression

Declaration

Type

TypeAnnotation

Pattern

DestructurePattern

New module representing the patterns available in a destructuring pattern (in function arguments for instance). This is used in:

Exposing

Port

New module representing the data for a port.

Range

Parsing and processing

v7.3.0 introduced Elm.Parser.parseToFile to combine Elm.Parser.parse and Elm.Processing.process to avoid having post-processing the resulting RawFile (to fix the documention comments and to re-balance the AST based on operator precedence).

In v8 we don't need any post-processing anymore, so:

Misc

MartinSStewart commented 1 year ago

After working on an elm-review rule with a lot of AST manipulation, I'm convinced that elm-syntax should track line breaks and comments within the AST. With that it should be possible to generate elm-review rule fixes by modifying the AST instead of needing to edit strings. This would make it much harder (maybe impossible) to make fixes that cause syntax errors, and probably will improve performance too since the AST doesn't need to be parsed again or require elm-format.

lue-bird commented 1 year ago

The tuple one seems really strange to me. tuples, units and parenthesizing are semantically very different and it's still not exhaustive since tuples can't have more than three elements. I'd much prefer separate variants

Some variants still seem to have inconsistent names, namely

For all the firstX and a restOfX fields, it seems much simpler and easier to handle to me to consistently use one xs field with a non-empty list. Same goes for making first and rest separate variant arguments.

Application (Node Expression) (List (Node Expression)) ← should this be a non empty-list?

yes.

All the other changes sound awesome!

MartinSStewart commented 1 year ago

The tuple one seems really strange to me. tuples, units and parenthesizing are semantically very different and it's still not exhaustive since tuples can't have more than three elements. I'd much prefer separate variants

* `Unit`

* `Parenthesized inParens`

* `Tuple2 ( first, second )`

* `Tuple3 ( first, second, third )`

I'm conflicted about this. Having Unit, Parenthesized, Tuple2, and Tuple3 does make the usage more clear and prevents the user from creating larger tuples that the compiler will reject.

On the other hand, very often when working with the AST, I just want to do Tuples tuples -> List.map expressionHandler tuples and having 4 different cases just complicates that.

Also, while 4+ elements is not a valid tuple. Should it be considered a syntax error? Maybe it's in the same class of errors as defining two top level functions with the same name. Or referencing a function that doesn't exist.

lue-bird commented 1 year ago

very often when working with the AST, I just want to do Tuples tuples -> List.map expressionHandler tuples and having 4 different cases just complicates that.

I feel like this extra convenience doesn't have to be baked into the type at the cost of safety and understandability. It's probably better to use helper functions that operate on all parts of 2-tuples and another one for 3-tuples to make this simpler.

From my experience from the perspective of an elm-review user, the list representation is (almost) never helpful, even as a convenience feature

jfmengels commented 1 year ago

These are all valid points that I want to discuss (I think I also disagree with some of the changes already in the branch), but I don't want this PR to become the place for discussion, considering how many changes it includes and will include. I will lock the discussion for this issue.

@lue-bird @MartinSStewart Do you mind creating new issues to discuss these? I'll do the same (I'll probably remove the existing comments once we have dedicated issues in order to reduce confusion)