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.

Sign up for GitHub

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

Jump to bottom

Add layout annotations #42

Merged
merged 6 commits into from
Nov 28, 2023
Merged

Add layout annotations #42

merged 6 commits into from
Nov 28, 2023

Conversation

alanechang
Copy link

No description provided.

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

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 alanechang marked this pull request as ready for review November 2, 2023 17:16
@alanechang
Copy link
Author

Rebased this PR over all the new changes

@tdelvecchio-jsc
Copy link

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

goldfirere
goldfirere reviewed Nov 21, 2023
View reviewed changes
vendor/parser-extended/asttypes.mli Show resolved Hide resolved
test/passing/tests/layout_annotation.ml Show resolved Hide resolved
lib/Fmt_ast.ml Outdated Show resolved Hide resolved
lib/Fmt_ast.ml Outdated
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
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

lib/Fmt_ast.ml Outdated
@@ -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)
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
goldfirere
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
parser diff
c4db5d5 
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>
@alanechang
tests for multiple newtypes
197701e 
Signed-off-by: alanechang <alanechang@janestreet.com>
@alanechang
tyvar name string to string option
bf98ce2 
Signed-off-by: alanechang <alanechang@janestreet.com>
@alanechang
remove support for legacy layout attr
39d4cad 
Signed-off-by: alanechang <alanechang@janestreet.com>
@alanechang
reduce diff
cb254b9 
Signed-off-by: alanechang <alanechang@janestreet.com>
@alanechang
refactor fmt_type_var
c9ccbb2 
Signed-off-by: alanechang <alanechang@janestreet.com>
@alanechang alanechang force-pushed the aec/add-layout-annotations branch 2 times, most recently from 4387993 to c9ccbb2 Compare November 27, 2023 21:42
@alanechang
Copy link
Author

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

@goldfirere goldfirere merged commit 0e78ddc into janestreet:jane Nov 28, 2023
8 checks passed
@alanechang alanechang mentioned this pull request Nov 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goldfirere goldfirere goldfirere left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alanechang @tdelvecchio-jsc @goldfirere