Add support for annotating types with layouts. #1

goldfirere · 2022-11-21T21:43:18Z

For example, this adds support for [(int : immediate)] and
[(string list : value)].

This is the first part of layout annotations. See the test case for a hint of what's to come! But this can still be merged as is, if we like.

goldfirere · 2022-12-21T03:20:39Z

This is now in an intermediate state as I'm adding more layout annotations. The first two commits are good, but please wait for general review. I will unmark this as a Draft and post here when it's ready for proper review.

goldfirere · 2023-01-06T23:11:04Z

This is now ready for review. There are tests failing in testsuite/tests/typing-layouts/annots.ml, but that's because I think there are type-inference bugs. Want to coordinate with @ccasin to fix those.

ccasin

This looks mostly good. I think there is one issue, which is to do with check_poly_univars and mutually defined types. For example, this currently checks:

type ('a : immediate) t_imm

type s = { f : ('a : value) . 'a -> 'a u }
and 'a u = 'a t_imm

I think we want to reject this. (Or if we want to accept it, s should get the type {f : ('a : immediate). 'a -> 'a u}, but it gets value).

The problem is that both make_poly_univars and check_poly_univars happen inside the call to transl_simple_type in transl_labels, and that happens in an environment where we haven't yet discovered that u's parameter must be immediate. There's a similar problem for gadt constructors.

I don't see a super easy solution, and it's further complicated by the fact that we aren't even doing layout checks when these types are translated (because they are delayed for later). Three possible but annoying solutions:

It might be possible to remember these univar checks and perform them later, after we do all the delayed layout checks (though I'd have to think a little harder about what information ends up in what environment). This seems easiest if possible
We could retranslate these types for the purpose of performing this check once we have an appropriate environment and have performed the circularity checks. This seems like a bad idea for performance reasons.
We could figure out what we're going to do about "lower bounds" for layouts, which we already want for efficiency and I think could solve this (if u's parameter gets a lower bound of value while translating s's body, then when we could get an error when translating u).

I'm happy to think more about this - I really want to get out this review today, so I haven't spent much time on it.

I've put some other comments and design questions throughout, but overall things look good other than this issue. There are a few things that seem a bit awkward, but which I've mostly held off on commenting on because I expect they will change when we eliminate the modifications to the parse tree and move to the new extension mechanism (e.g., there are lots of places where we're using a pair of lists of vars and layout annotations where it seems cleaner to have a list of pairs).

boot/menhir/menhirLib.ml

parsing/ast_helper.ml

testsuite/tests/typing-unboxed-types/test.ml

parsing/location.ml

parsing/ast_mapper.ml

typing/typetexp.mli

typing/typetexp.ml

ccasin · 2023-01-12T18:39:17Z

typing/typedtree.mli

@@ -755,7 +762,7 @@ and constructor_declaration =
    {
     cd_id: Ident.t;
     cd_name: string loc;
-     cd_vars: string loc list;
+     cd_vars: (string * layout_annotation option) list;


I guess since this typechecks the loc information wasn't used anywhere, but I wonder if it wouldn't be nice to keep and use to give better located errors when something fails in layout checking, instead of indicating the whole quantified type. (and in Text_decl below, as well). Though I guess if we do that here we'd want to do it in all places you can have quantifiers like this.

That's about right. I removed the loc to make code reuse easier. Making different decisions about this in different places is annoying. I'm all for adding more locations to report better errors, but I think we should do so separately.

Sounds reasonable.

For example, this adds support for [(int : immediate)], [(string list : value)], and [(type 'a t : immediate)]. The full syntax is at https://docpub/JD/p/surface-syntax-for-unboxed-types/db6b3168-1985-302e-155c-e2b59c8f40bf#layout-annotations Also, fix up pretty-printing to show more layouts.

goldfirere · 2023-01-18T17:05:05Z

OK. I think I've resolved everything here. OK to merge?

goldfirere · 2023-03-01T22:22:09Z

Small update: it was decided not to merge this for now, because we're hoping to release a version of unboxed types without annotation support.

This was taken from ccasin/ocaml-jst#1, but rebased and cleaned. There are a handful of failing tests, still, but they're all newly introduced in this patch.

goldfirere · 2023-06-28T21:31:11Z

Superseded by ocaml-flambda/flambda-backend#1417

Initial implementation of layout annotations This was taken from ccasin/ocaml-jst#1, but rebased and cleaned. There are a handful of failing tests, still, but they're all newly introduced in this patch. Rename ~reason to ~context for annotations (only) Move layout annotations into aliases This now prevents e.g. [(int : immediate)] but allows [int as ('a : immediate)], as discussed with the type-systems group. Add control over printing verbosity Move alias over to jane-syntax architecture Move variable layout annotation into jane-syntax Support for annotations on _ Check layout annotation during conversion Handle type parameters uniformly Convert Ptyp_poly to use jane-syntax architecture Comments from Antal Move annotations on extension constructors into JS Combine unboxed constants with other layouts Newtypes are now in jane-syntax. Constructor declarations in jane-syntax Make testsuite pass Some minor cleanup of old XXXs. Some cleanup from review A little cleanup in Jane_syntax from review Fix compilation with dune-based build Incorporate test case from old PR, improving msg Respond to Antal's review More review fixes Move test that now passes Responses to comments rename test Remove "assert false" in favor of "Misc.fatal_error" Add comment about generic parameters transl_type_param_layout --> get_type_param_layout Rename error constructors Remove module that will interfere with merging

This commit also includes various improvements to jane-syntax. Documentation coming in a separate commit. This commit has been rebased to include the following commits: Pprintast now depends on Printast Initial implementation of layout annotations This was taken from ccasin/ocaml-jst#1, but rebased and cleaned. There are a handful of failing tests, still, but they're all newly introduced in this patch. Rename ~reason to ~context for annotations (only) Move layout annotations into aliases This now prevents e.g. [(int : immediate)] but allows [int as ('a : immediate)], as discussed with the type-systems group. Add control over printing verbosity Move alias over to jane-syntax architecture Move variable layout annotation into jane-syntax Support for annotations on _ Check layout annotation during conversion Handle type parameters uniformly Convert Ptyp_poly to use jane-syntax architecture Comments from Antal Move annotations on extension constructors into JS Combine unboxed constants with other layouts Newtypes are now in jane-syntax. Constructor declarations in jane-syntax Make testsuite pass Some minor cleanup of old XXXs. Some cleanup from review A little cleanup in Jane_syntax from review Fix compilation with dune-based build Incorporate test case from old PR, improving msg Respond to Antal's review More review fixes Move test that now passes Responses to comments rename test Remove "assert false" in favor of "Misc.fatal_error" Add comment about generic parameters transl_type_param_layout --> get_type_param_layout Rename error constructors Remove module that will interfere with merging

* Add layout annotations to the language. This commit also includes various improvements to jane-syntax. Documentation coming in a separate commit. This commit has been rebased to include the following commits: Pprintast now depends on Printast Initial implementation of layout annotations This was taken from ccasin/ocaml-jst#1, but rebased and cleaned. There are a handful of failing tests, still, but they're all newly introduced in this patch. Rename ~reason to ~context for annotations (only) Move layout annotations into aliases This now prevents e.g. [(int : immediate)] but allows [int as ('a : immediate)], as discussed with the type-systems group. Add control over printing verbosity Move alias over to jane-syntax architecture Move variable layout annotation into jane-syntax Support for annotations on _ Check layout annotation during conversion Handle type parameters uniformly Convert Ptyp_poly to use jane-syntax architecture Comments from Antal Move annotations on extension constructors into JS Combine unboxed constants with other layouts Newtypes are now in jane-syntax. Constructor declarations in jane-syntax Make testsuite pass Some minor cleanup of old XXXs. Some cleanup from review A little cleanup in Jane_syntax from review Fix compilation with dune-based build Incorporate test case from old PR, improving msg Respond to Antal's review More review fixes Move test that now passes Responses to comments rename test Remove "assert false" in favor of "Misc.fatal_error" Add comment about generic parameters transl_type_param_layout --> get_type_param_layout Rename error constructors Remove module that will interfere with merging * Remove call to make_params in enter_type * Keep minimizer compatible * Format chameleon

* Add layout annotations to the language. This commit also includes various improvements to jane-syntax. Documentation coming in a separate commit. This commit has been rebased to include the following commits: Pprintast now depends on Printast Initial implementation of layout annotations This was taken from ccasin#1, but rebased and cleaned. There are a handful of failing tests, still, but they're all newly introduced in this patch. Rename ~reason to ~context for annotations (only) Move layout annotations into aliases This now prevents e.g. [(int : immediate)] but allows [int as ('a : immediate)], as discussed with the type-systems group. Add control over printing verbosity Move alias over to jane-syntax architecture Move variable layout annotation into jane-syntax Support for annotations on _ Check layout annotation during conversion Handle type parameters uniformly Convert Ptyp_poly to use jane-syntax architecture Comments from Antal Move annotations on extension constructors into JS Combine unboxed constants with other layouts Newtypes are now in jane-syntax. Constructor declarations in jane-syntax Make testsuite pass Some minor cleanup of old XXXs. Some cleanup from review A little cleanup in Jane_syntax from review Fix compilation with dune-based build Incorporate test case from old PR, improving msg Respond to Antal's review More review fixes Move test that now passes Responses to comments rename test Remove "assert false" in favor of "Misc.fatal_error" Add comment about generic parameters transl_type_param_layout --> get_type_param_layout Rename error constructors Remove module that will interfere with merging * Remove call to make_params in enter_type * Keep minimizer compatible * Format chameleon

* Fix ghost locations in modular extension AST nodes * Add missing ghostification Thank you, Carl! * Comment update about ghostiness (+ word-wrapping) * Add ghostify function (#1) * Add `Location.ghostify` * Update the parser's `make_ghost` to save an allocation in some cases * Promote parser.ml * Mark the inner mutable arrays for iarrays as ghost * Add comment about ghostification for comprehensions * Explain that ppxlib is where the ghostiness requirement is enforced * Use `Ast_helper.default_loc` to default the generated locations * Restore propagating the location, now via `Ast_helper.default_loc` * Drop obsolete comment * Rewrite advisory comment about locations

* Parse `#0`, `-#1`, `#2.7`, and `-#3.1`, treating them as boxed * Fix parsing of unsuffixed unboxed int literals * Rewrite CR comment * Update comment * promote-menhir * Unboxed literal tests * Adjust error for unsuffixed unboxed integers

goldfirere force-pushed the rae/layout-annotations branch from 026aac9 to cb5145e Compare November 23, 2022 03:20

goldfirere changed the base branch from unboxed to unboxed-clean November 23, 2022 03:20

goldfirere mentioned this pull request Nov 23, 2022

Add layout annotations on type parameters ocaml-flambda/ocaml-jst#68

Merged

goldfirere force-pushed the rae/layout-annotations branch from ddd0221 to 80f1849 Compare December 20, 2022 20:46

goldfirere marked this pull request as draft December 21, 2022 03:19

goldfirere force-pushed the rae/layout-annotations branch from cb3ef64 to 7d94fd6 Compare December 30, 2022 03:10

stedolan and others added 8 commits December 30, 2022 16:28

Move type_unboxed.unboxed into type_kind (backport PR #10252)

724026d

Make immediacy part of type_kind (backport PR #10041)

b8946d8

Layouts version 1

a4a23db

bootstrap

7ab71c6

Fix a dependency bug in one dune file, clean up build stuff

33b6e68

Fix scope issue arising from expansion in immediacy check

9dba4e2

can't see any reason these need to be value

dd0d23f

Fields in float record shouldn't be given layout "Any"

c6e8602

goldfirere force-pushed the rae/layout-annotations branch from 7d94fd6 to ba63ad9 Compare January 4, 2023 22:02

Some "fixes" related to a stack overflow

abd5147

goldfirere changed the base branch from unboxed-clean to unboxed-clean-rebase January 6, 2023 22:04

goldfirere force-pushed the rae/layout-annotations branch from ba63ad9 to 335b077 Compare January 6, 2023 23:10

ccasin reviewed Jan 12, 2023

View reviewed changes

goldfirere force-pushed the rae/layout-annotations branch from 335b077 to 30153c6 Compare January 13, 2023 21:47

goldfirere added 2 commits January 17, 2023 13:36

Print out layout annotations like users

bf9b7fd

Rename to use const_layout

4d76dfe

ccasin force-pushed the unboxed-clean-rebase branch from efcac41 to 1ad4ef0 Compare February 28, 2023 09:31

goldfirere mentioned this pull request May 25, 2023

Support layout annotations ocaml-flambda/flambda-backend#1417

Merged

goldfirere closed this Jun 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for annotating types with layouts. #1

Add support for annotating types with layouts. #1

goldfirere commented Nov 21, 2022

goldfirere commented Dec 21, 2022

goldfirere commented Jan 6, 2023

ccasin left a comment •

edited

Loading

ccasin Jan 12, 2023

goldfirere Jan 13, 2023

ccasin Jan 13, 2023

goldfirere commented Jan 18, 2023

goldfirere commented Mar 1, 2023

goldfirere commented Jun 28, 2023

Add support for annotating types with layouts. #1

Add support for annotating types with layouts. #1

Conversation

goldfirere commented Nov 21, 2022

goldfirere commented Dec 21, 2022

goldfirere commented Jan 6, 2023

ccasin left a comment • edited Loading

Choose a reason for hiding this comment

ccasin Jan 12, 2023

Choose a reason for hiding this comment

goldfirere Jan 13, 2023

Choose a reason for hiding this comment

ccasin Jan 13, 2023

Choose a reason for hiding this comment

goldfirere commented Jan 18, 2023

goldfirere commented Mar 1, 2023

goldfirere commented Jun 28, 2023

ccasin left a comment •

edited

Loading