这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@ccasin
Copy link

@ccasin ccasin commented Dec 6, 2023

This is, unfortunately, a largeish PR. I think it's not worth reviewing line-by-line in detail, but suggest instead looking at whether the tests are sufficient and doing a quick style review on the rest. (If it's wrong enough to break styling of current features, we'll find that out quickly before we deploy, and if the styling of labeled tuples isn't perfect we can just improve it later).

A few things that make it tricky (but, which, again, I suggest not actually reviewing the implementation of in detail):

  • parser-standard is in a very bad state right now. As discussed offline, it's using a version of jane syntax that can't easily be upgraded, so I had to rewrite parts of the labeled tuples jane syntax modules to be compatible.
  • Further, the parser-standard parser itself is out of date (and can't be fully updated now), and copying in the labeled tuples parsing from the flambda-backend parser resulted in new parser conflicts (this is why there are changes to make the parsing of local_ in function types to be a little more like the current upstream version - this fixed the conflicts).
  • The new terms, patterns and (especially) types have complicated precedence/parenthesization rules due to similarity with existing syntax (e.g., the parameter of (x:int * bool) -> unit needs parens but the parameter of int * x:bool -> unit doesn't).
  • Care must be taken with comments and attributes around puns. Following the formatting of labeled arguments, let _ = ~x:x, ~y:y in ... is rewritten as a pun let _ = ~x, ~y in .... These two actually parse the same, so we must do this. But we can't rewrite let _ = ~x:(x [@attr]), ~y in ... because there'd be no place to put the attribute.
  • The labels in flambda-backend's parser are just strings, but here they need to be augmented with locations to support accurate comment placement.

@ccasin
Copy link
Author

ccasin commented Dec 7, 2023

@alanechang - you've been nominated to review this. See the note above about a suggested review path.

@ccasin ccasin requested a review from alanechang December 7, 2023 15:56
Copy link

@alanechang alanechang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed this following the suggested path. Mainly check for test completeness and correctness. Also added a few notes on style. Did not find any big issues and agree that an error here will probably be easy to catch when deploying.

@ccasin
Copy link
Author

ccasin commented Dec 10, 2023

(This is ready to be merged, but I am going to hold off on clicking the button until the relevant compiler has been released so we don't accidentally ship it before then.)

@goldfirere
Copy link

Nothing bad happens if ocamlformat gets ahead of the compiler, right? Not necessarily disagreeing with your plan to wait -- more checking my own understanding.

@ccasin
Copy link
Author

ccasin commented Dec 10, 2023

Nothing bad happens if ocamlformat gets ahead of the compiler, right? Not necessarily disagreeing with your plan to wait -- more checking my own understanding.

Shipping a new ocamlformat ahead of the relevant feature has caused problems for apply style in the past, but I confess I do not recall the details.

(I'll try to figure out what that issue was and write it down somewhere tomorrow)

@ccasin ccasin merged commit 5193c04 into oxcaml:jane Dec 18, 2023
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.

3 participants