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

Ongoing branch for v8 #188

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open

Ongoing branch for v8 #188

wants to merge 44 commits into from

Conversation

jfmengels
Copy link
Collaborator

@jfmengels jfmengels commented Aug 16, 2023

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:

  • Adding all the missing details that some tools need but can't access, such as the position of some elements in the node, or knowing whether a string is using " or """.
  • Prevent more impossible cases. Ex: It's not possible to have a RecordUpdateExpression without any field assigments, so those become a non-empty list.
  • Renaming the different variants to be consistant. Ex: The naming for RecordUpdateExpression is inconsistent with RecordExpr.
  • Performance: It's not the main concern, but we do want to keep this library fast, though not at the expense of losing information about the source code.

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

  • Application
    • is renamed to FunctionCall
    • separates the function with the arguments: Application (List (Node Expression)) --> Application (Node Expression) (List (Node Expression)) (should this be a non empty-list?)
  • RecordUpdateExpression
    • is renamed to RecordUpdate
    • has a non-empty list of elements: RecordUpdate (Node String) (List (Node RecordSetter)) -> RecordUpdateExpression (Node String) (Node RecordSetter) (List (Node RecordSetter))
  • CaseExpression
    • is renamed to Case
    • Its data CaseBlock makes sure there is at least one element: { expression : Node Expression, cases : Cases } -> { expression : Node Expression, firstCase : Case, restOfCases : List Case }
      • Should this be Case (Node Expression) (NonEmptyList ( Node Pattern, Node Expression ))?
  • TupledExpression is renamed to TupleExpression
  • UnitExpr is removed in favor of using TupleExpression
  • ParenthesizedExpression is removed in favor of using TupleExpression
  • Lambda has a non-empty list of arguments: Its field args : List (Node Pattern) becomes firstArg : Node DestructurePattern and restOfArgs : List (Node DestructurePattern).
  • RecordAccessFunction (.field) now doesn't include the . in the string it contains (though the . is still included in the node's range).
  • Literal
  • Floatable is renamed to FloatLiteral
  • Integer is renamed to IntegerLiteral
  • Hex is renamed to HexLiteral
  • ListExpr is renamed to ListLiteral
  • OperatorApplication is renamed to Operation
  • RecordExpr is renamed to Record
  • IfBlock is renamed to If
  • GLSLExpression is renamed to GLSL

Declaration

  • Remove the Destructuring variant (which was impossible for Elm 0.19)
  • Ports now have a documentation field. This removes these comments from the AST's comments list.

Type

  • Custom type declarations have a non-empty list of elements: Type declaration have a firstConstructor and a restOfConstructors field instead of constructors field.

TypeAnnotation

  • GenericType is renamed to Var
  • Typed is renamed to Type
  • Tupled is renamed to Tuple
  • Unit is removed in favor of using Tuple

Pattern

  • FloatPattern has been removed, as that is a syntax error in ELm 0.19.

DestructurePattern

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

  • Expression.FunctionImplementation
  • Expression.LetDestructuring
  • Expression.Lambda

Exposing

  • Explicit has a non-empty list of elements: Explicit (List (Node TopLevelExpose)) -> Explicit (Node TopLevelExpose) (List (Node TopLevelExpose))

Port

New module representing the data for a port.

Range

  • Removes Range.emptyRange function in favor of Range.empty introduced in v7.3.0.

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:

  • Elm.Parser.parseToFile is renamed to Elm.Parser.parse, removing the old Elm.Parser.parse
  • The Elm.Processing module is removed.

Misc

@MartinSStewart
Copy link
Collaborator

MartinSStewart commented Aug 16, 2023

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
Copy link
Contributor

lue-bird commented Aug 16, 2023

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 )

Some variants still seem to have inconsistent names, namely

  • RecordUpdateExpression. why not RecordUpdate?
  • TupleExpression. why not Tuple, same as the tuple type variant?

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
Copy link
Collaborator

MartinSStewart commented Aug 16, 2023

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
Copy link
Contributor

lue-bird commented Aug 16, 2023

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
Copy link
Collaborator Author

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)

Repository owner locked and limited conversation to collaborators Aug 17, 2023
@jfmengels jfmengels force-pushed the breaking-changes-v8 branch 3 times, most recently from cab4987 to fff26eb Compare August 17, 2023 22:05
@jfmengels jfmengels force-pushed the breaking-changes-v8 branch 3 times, most recently from afc4344 to 45f893f Compare July 26, 2024 21:35
@jfmengels jfmengels force-pushed the breaking-changes-v8 branch 4 times, most recently from eff82e1 to 0f43132 Compare August 7, 2024 15:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants