Add layout annotations #42

alanechang · 2023-11-01T19:05:46Z

No description provided.

alanechang · 2023-11-02T17:16:33Z

This PR is ready for review. It consists of the following changes:

Add layout annotation to parser-standard: This is done by first applying #1417. Then for the jane syntax bits, jane_syntax_parsing is completely copy-pasted from #1586 then had the match_jane_syntax_piece function added in, jane_syntax is changed to include the Layouts module which is copied from the PR mentioned but the handling of attrs is changed to match all other existing modules.
Add layout annotation to parser-extended: Done by changing the parsetree. Introduced the type of ty_var and replaced many occurrences of string with the new type.
Fmt_ast is changed to format the new layout annotation. This is mainly done by a rework of the fmt_type_var function. Some extra logic are added around comment relocation and adding parentheses.
Tests are added for the new syntax. Most are taken from the flambda backend annotation tests.

Review suggestion: @goldfirere?

alanechang · 2023-11-06T21:20:36Z

Rebased this PR over all the new changes

tdelvecchio-jsc · 2023-11-15T19:06:33Z

Tip of jane was force-pushed to again. I rebased your PR on the new tip: https://github.com/janestreet/ocamlformat/tree/aec-add-layout-annotations-rebase

vendor/parser-extended/asttypes.mli

test/passing/tests/layout_annotation.ml

lib/Fmt_ast.ml

goldfirere · 2023-11-21T22:01:01Z

lib/Fmt_ast.ml

-          Cmts.relocate c.cmts ~src:pexp_loc ~before ~after:e.pexp_loc ;
+          Cmts.relocate c.cmts ~src:pexp_loc ~before:before_name
+            ~after:e.pexp_loc ;
+          Option.iter


This new Cmts.relocate supersedes the previous one, right? Then I think it would be cleaner to just have a match before_layout_opt with ... that either does one relocate call or the other. As it is, I've stared at this for 5 minutes and I'm still not sure exactly what's going on here.

I think I over-complicated the logic here and there's no need for the call to Cmts.relocate inside the Option.iter. Relocating the comments to before the tyvar name should be enough.

goldfirere · 2023-11-21T22:06:23Z

lib/Fmt_ast.ml

@@ -3525,17 +3563,17 @@ and fmt_tydcl_param c ctx ty =
     here. *)
  match ty.ptyp_attributes with
  | [] | _ :: _ :: _ -> noop
-  | [attr] -> fmt_if_k (is_layout attr) (fmt "@ :@ " $ str attr.attr_name.txt)
+  | [attr] -> fmt_if_k (is_layout attr) (fmt_layout_attr c attr)


I think (?) this shouldn't be necessary any more. It rewrites something like

type 'a [@immediate] t

to

type ('a : immediate) t

But we've stopped accepting the previous syntax for a little while. So I think this extra logic can be removed.

I removed this extra logic together with the bit in fmt_core_type. It doesn't seem to break any of the tests and a quick grep search doesn't show any occurrences of the old syntax in jane.

vendor/parser-extended/parser.mly

lib/Fmt_ast.ml

vendor/parser-extended/parser.mly

Signed-off-by: alanechang <alanechang@janestreet.com> other diffs Signed-off-by: alanechang <alanechang@janestreet.com> jane syntax diff Signed-off-by: alanechang <alanechang@janestreet.com> ast helper diff Signed-off-by: alanechang <alanechang@janestreet.com> wip Signed-off-by: alanechang <alanechang@janestreet.com> wip Signed-off-by: alanechang <alanechang@janestreet.com> wip Signed-off-by: alanechang <alanechang@janestreet.com> copy paste Signed-off-by: alanechang <alanechang@janestreet.com> add match_jane_syntax_piece Signed-off-by: alanechang <alanechang@janestreet.com> wip Signed-off-by: alanechang <alanechang@janestreet.com> parser standard updated Signed-off-by: alanechang <alanechang@janestreet.com> fix parser Signed-off-by: alanechang <alanechang@janestreet.com> wip Signed-off-by: alanechang <alanechang@janestreet.com> wip Signed-off-by: alanechang <alanechang@janestreet.com> wip Signed-off-by: alanechang <alanechang@janestreet.com> wip Signed-off-by: alanechang <alanechang@janestreet.com> building now Signed-off-by: alanechang <alanechang@janestreet.com> fix format Signed-off-by: alanechang <alanechang@janestreet.com> fix format Signed-off-by: alanechang <alanechang@janestreet.com> fix up paren Signed-off-by: alanechang <alanechang@janestreet.com> new layout annot for type decl Signed-off-by: alanechang <alanechang@janestreet.com> add new test Signed-off-by: alanechang <alanechang@janestreet.com> new layout annot on type decl Signed-off-by: alanechang <alanechang@janestreet.com> fix paren Signed-off-by: alanechang <alanechang@janestreet.com> remove extra paren Signed-off-by: alanechang <alanechang@janestreet.com> test changes Signed-off-by: alanechang <alanechang@janestreet.com> handle layout annot and attr differently Signed-off-by: alanechang <alanechang@janestreet.com> test changes Signed-off-by: alanechang <alanechang@janestreet.com> diff changes Signed-off-by: alanechang <alanechang@janestreet.com> parenize in more ctx Signed-off-by: alanechang <alanechang@janestreet.com> remove cr Signed-off-by: alanechang <alanechang@janestreet.com> clean up newtype Signed-off-by: alanechang <alanechang@janestreet.com> test and diff Signed-off-by: alanechang <alanechang@janestreet.com> fix comment bug Signed-off-by: alanechang <alanechang@janestreet.com> add missing map Signed-off-by: alanechang <alanechang@janestreet.com> refactor Signed-off-by: alanechang <alanechang@janestreet.com> more clean up Signed-off-by: alanechang <alanechang@janestreet.com> test change Signed-off-by: alanechang <alanechang@janestreet.com> reduce diff Signed-off-by: alanechang <alanechang@janestreet.com> less diff Signed-off-by: alanechang <alanechang@janestreet.com> new patch Signed-off-by: alanechang <alanechang@janestreet.com> add new line Signed-off-by: alanechang <alanechang@janestreet.com> patch diff Signed-off-by: alanechang <alanechang@janestreet.com> more tests Signed-off-by: alanechang <alanechang@janestreet.com>

Signed-off-by: alanechang <alanechang@janestreet.com>

alanechang · 2023-11-27T21:45:26Z

Had to rebase to fix the CI. I added two new commits:

reduce diff to remove some unnecessary indents
refactor fmt_type_var: refactors fmt_type_var

alanechang force-pushed the aec/add-layout-annotations branch 2 times, most recently from 8bee6d5 to 302a440 Compare November 2, 2023 17:14

alanechang marked this pull request as ready for review November 2, 2023 17:16

tdelvecchio-jsc force-pushed the jane branch from 62a1097 to 21fd82a Compare November 3, 2023 20:45

alanechang force-pushed the aec/add-layout-annotations branch from ec7fac8 to 7f5f434 Compare November 6, 2023 21:19

tdelvecchio-jsc force-pushed the jane branch from 21fd82a to 4bb9282 Compare November 13, 2023 22:22

alanechang force-pushed the aec/add-layout-annotations branch from 7f5f434 to f82adb5 Compare November 20, 2023 15:43

goldfirere reviewed Nov 21, 2023

View reviewed changes

goldfirere reviewed Nov 27, 2023

View reviewed changes

lib/Fmt_ast.ml Outdated Show resolved Hide resolved

vendor/parser-extended/parser.mly Show resolved Hide resolved

alanechang added 6 commits November 27, 2023 16:34

tests for multiple newtypes

197701e

Signed-off-by: alanechang <alanechang@janestreet.com>

tyvar name string to string option

bf98ce2

Signed-off-by: alanechang <alanechang@janestreet.com>

remove support for legacy layout attr

39d4cad

Signed-off-by: alanechang <alanechang@janestreet.com>

reduce diff

cb254b9

Signed-off-by: alanechang <alanechang@janestreet.com>

refactor fmt_type_var

c9ccbb2

Signed-off-by: alanechang <alanechang@janestreet.com>

alanechang force-pushed the aec/add-layout-annotations branch 2 times, most recently from 4387993 to c9ccbb2 Compare November 27, 2023 21:42

goldfirere merged commit 0e78ddc into janestreet:jane Nov 28, 2023

alanechang mentioned this pull request Nov 28, 2023

Fix local syntax normalization #46

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add layout annotations #42

Add layout annotations #42

alanechang commented Nov 1, 2023

alanechang commented Nov 2, 2023

alanechang commented Nov 6, 2023

tdelvecchio-jsc commented Nov 15, 2023

goldfirere Nov 21, 2023

alanechang Nov 23, 2023

goldfirere Nov 21, 2023

alanechang Nov 23, 2023

alanechang commented Nov 27, 2023

Add layout annotations #42

Add layout annotations #42

Conversation

alanechang commented Nov 1, 2023

alanechang commented Nov 2, 2023

alanechang commented Nov 6, 2023

tdelvecchio-jsc commented Nov 15, 2023

goldfirere Nov 21, 2023

Choose a reason for hiding this comment

alanechang Nov 23, 2023

Choose a reason for hiding this comment

goldfirere Nov 21, 2023

Choose a reason for hiding this comment

alanechang Nov 23, 2023

Choose a reason for hiding this comment

alanechang commented Nov 27, 2023