Nested record definitions #7241

zth · 2025-01-10T22:57:09Z

type uploadOptions = {
  file: {
    path: string,
    filename: string,
    metadata?: {description?: string, tags?: array<string>},
  },
  processing?: {
    resize?: {width: int, height: int},
    compress?: {level: int, format: string},
  },
  security?: {
    encryption?: {algorithm: string, key: string},
    accessControl?: {public: bool, expiration?: int},
  },
}

let uploadOptions = {
  file: {
    path: "/path/to/file.png",
    filename: "file.png",
    metadata: {description: "Sample file", tags: ["image", "upload"]},
  },
  processing: {
    resize: {width: 1920, height: 1080},
    compress: {level: 7, format: "jpg"},
  },
  security: {
    encryption: {algorithm: "AES-256", key: "secure-key"},
    accessControl: {public: false, expiration: 3600},
  },
}

Things missing:

Support for type params
Support in the outcome printer so hovers etc include the full nested type.
Make sure locs are good and that the editor experience is good. Update: Now good enough for an initial version

Things considered but dropped:

~~Support for @as for renaming the generated type (do we want it?)~~ Make the definition explicit if it should be reused.
~~First class representation in the AST~~ This might be something to pick up a bit later, but for now we can let it be driven by the single attribute added.

zth · 2025-03-12T15:31:25Z

Turns out it's quite tricky to make these inline records actually print inline.

zth · 2025-03-13T10:21:15Z

compiler/ml/printtyp.ml

+let filter_params tyl =
+  let params =
+    List.fold_left
+      (fun tyl ty ->
+        let ty = repr ty in
+        if List.memq ty tyl then Btype.newgenty (Tsubst ty) :: tyl
+        else ty :: tyl)
+      [] tyl
+  in
+  List.rev params
+
+let mark_loops_constructor_arguments = function
+  | Cstr_tuple l -> List.iter mark_loops l
+  | Cstr_record l -> List.iter (fun l -> mark_loops l.ld_type) l
+
+let find_inlined_type name (printing_context : printing_context option) =
+  match printing_context with
+  | None -> None
+  | Some {inlined_types} ->
+    inlined_types
+    |> List.find_opt (fun inlined_type ->
+           match inlined_type with
+           | Record {type_name} -> type_name = name)
+


These are just moved, not changed.

zth · 2025-03-13T10:21:37Z

compiler/ml/printtyp.ml

+      | Ttuple tyl -> Otyp_tuple (tree_of_typlist ?printing_context sch tyl)
+      | Tconstr (p, _tyl, _abbrev)
+        when printing_context
+             |> find_inlined_type (Path.name p)
+             |> Option.is_some -> (
+        match
+          find_inlined_type (Path.name p) printing_context |> Option.get
+        with
+        | Record {labels} ->
+          Otyp_record (List.map (tree_of_label ?printing_context) labels))


This is what does the actual inlining in the outcome printer.

zth · 2025-03-13T10:22:44Z

compiler/ml/printtyp.ml

+and tree_of_constraints ?printing_context params =
+  List.fold_right
+    (fun ty list ->
+      let ty' = unalias ty in
+      if proxy ty != proxy ty' then
+        let tr = tree_of_typexp ?printing_context true ty in
+        (tr, tree_of_typexp ?printing_context true ty') :: list
+      else list)
+    params []
+
+let typexp ?printing_context sch ppf ty =
+  !Oprint.out_type ppf (tree_of_typexp ?printing_context sch ty)
+
+let type_expr ppf ty = typexp false ppf ty
+
+and type_sch ppf ty = typexp true ppf ty
+
+and type_scheme ppf ty =
+  reset_and_mark_loops ty;
+  typexp true ppf ty
+
+(* Maxence *)
+let type_scheme_max ?(b_reset_names = true) ppf ty =
+  if b_reset_names then reset_names ();
+  typexp true ppf ty
+(* End Maxence *)
+
+let tree_of_type_scheme ty =
+  reset_and_mark_loops ty;
+  tree_of_typexp true ty
+
+(* Print one type declaration *)


Just moved.

tests/analysis_tests/tests-reanalyze/deadcode/expected/deadcode.txt

tests/analysis_tests/tests-reanalyze/deadcode/expected/exception.txt

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Syntax Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite	Current: `c33f978`	Previous: `80744a5`	Ratio
`Parse RedBlackTree.res - time/run`	`2.2335862533333333` ms	`1.87223692` ms	`1.19`
`Print RedBlackTree.res - time/run`	`3.43393426` ms	`2.9391152133333334` ms	`1.17`
`Print RedBlackTreeNoComments.res - time/run`	`3.1161356999999996` ms	`2.693127673333333` ms	`1.16`
`Parse Napkinscript.res - time/run`	`77.13125236666667` ms	`60.98575317999999` ms	`1.26`
`Print Napkinscript.res - time/run`	`119.29209173333334` ms	`102.03488166` ms	`1.17`
`Parse HeroGraphic.res - time/run`	`8.86553774` ms	`7.62831228` ms	`1.16`
`Print HeroGraphic.res - time/run`	`14.078760946666664` ms	`12.308145193333333` ms	`1.14`

This comment was automatically generated by workflow using github-action-benchmark.

zth · 2025-03-13T20:52:49Z

@cristianoc type params left to solve, but I don't think that'll necessary be super complicated, so I'd be happy to get an initial review from you on the approach.

cristianoc · 2025-03-14T07:36:33Z

tests/syntax_tests/data/parsing/errors/typeDef/expected/inlineRecord.res.txt

  {
  name: string ;
-  address: < street: string  ;country: string   >  }
+  address: ((user.address)[@res.inlineRecordReference ]) }


Why use both res.inlineRecordReference and res.inlineRecordDefinition?
Presumably if one can find user.address, one can also determine that it is an inline record definition. Or perhaps there's some convenience given by this.

Would you add some parsing and printing tests too.

Thinking about parsing and printing -- this representation makes you search for ids such as user.address.
Wondering whether going directly with a new AST representation could actually simplify this.
The compilation to usual type definitions could perhaps be done after parsing/printing, as for JSX.
On the fence on whether it's simpler to investigate this sooner or later.

Reason for having an explicit attribute is because I didn't want to hijack having . in the name meaning that it's an inline record. But perhaps that is OK, given that it's still an illegal identifier, and it only takes effect if there's also an inlined type named that available.

I think an actual AST representation makes sense as well. Not sure how to best structure it though.

Reason for having an explicit attribute is because I didn't want to hijack having . in the name meaning that it's an inline record. But perhaps that is OK, given that it's still an illegal identifier, and it only takes effect if there's also an inlined type named that available.

I think an actual AST representation makes sense as well. Not sure how to best structure it though.

Does not matter much. The observation was: if the printer is able to inline user.address, then it should be able to find that user.address is in fact a type definition decorated with res.inlineRecordDefinition, which should be all the info one needs. But maybe this makes things slower or more complicated, if every type reference needs look up to see if it's inlined.

Ahh I see what you mean now. Perf wise it shouldn't be much of a hit - it'll only attempt lookup in the cases when there are actual inlined types, and here we could safely look for a . in the name before attempting as well. I'll see if I can change it to work like that real quick.

@cristianoc good catch, was able to get rid of res.inlineRecordReference here: 82fc9c8

cristianoc · 2025-03-14T08:20:50Z

compiler/syntax/src/res_core.ml

+        ~params ~start_pos p
+    in
+    let rec_flag =
+      if List.length !inline_types > 0 then Asttypes.Recursive else rec_flag


Is this turning non-recursive types into recursive ones?
That would break the usual binding rules. Does one need recursion here to represent the new type, even though there's a nesting structure in inner types?

Yes it does, but only if there are also inline types in the type itself. I wasn't able to make this work reliably without the rec flag when trying.

Yes it does, but only if there are also inline types in the type itself. I wasn't able to make this work reliably without the rec flag when trying.

In this example:

type options = { extra: { superExtra: {age: int}, }, }

the parser generates type definitions in this order:

options.extra

options.extra.superExtra

options

so the order is a bit inconsistent with nested types. If deeply nested types come first, then one won't need to add rec.

Still doesn't work. I suspect this might be because it's a list of type defs we produce here in the same Pstr_type, and not separate Pstr_type definitions. Therefore it needs to be rec..?

Yes it'd have to be a sequence of type defs so later ones can see earlier ones.

That would require us to change the parser a bit I think, to support returning lists of nodes everywhere for structure/signatures, rather than a single node. Can do that of course. What type of issues do you see with using the rec flag like this?

compiler/syntax/src/res_core.ml

cristianoc · 2025-03-14T14:43:18Z

compiler/syntax/src/res_core.ml

  Parser.leave_breadcrumb p Grammar.RecordDecl;
  Parser.expect Lbrace p;
  let rows =
    parse_comma_delimited_region ~grammar:Grammar.RecordDecl ~closing:Rbrace
-      ~f:parse_field_declaration_region p
+      ~f:(parse_field_declaration_region ?current_type_name_path ?inline_types)


Reanalyze gives here Call must have named argument f in CI.
One possibility is to remove the termination analysis as reanalyze won't be supported starting from ocaml 5.3 anyway. Simplifies the coding as one does not need to respect its syntactic restrictions, the downside is one needs to be careful about not introducing infinite loops.

@cristianoc sounds reasonable to turn it off. Can I turn off just the termination analysis?

That's the only thing used in tests, the termination analysis.
I think there's a single target in CI where it's active CC @cknitt

cristianoc

This could go ahead as experimental feature.
There's a small comment about reanalyze restrictions.

zth · 2025-03-19T13:04:57Z

@cristianoc I made some additional changes, including supporting type params and adding a few more tests. Another look please, and then this should be ready to go as an experimental feature.

cristianoc · 2025-03-20T07:47:40Z

.github/workflows/ci.yml

@@ -110,6 +110,7 @@ jobs:
          - os: ubuntu-24.04
            ocaml_compiler: ocaml-variants.5.2.1+options,ocaml-option-static
            # Reanalyze does not work on OCaml 5.3.0 anymore, therefore run it on 5.2.1
+            # disabled for now, since Reanalyze will stop working going forward. Can likely be removed entirely soon


Since this is the only use of reanalyze, how about removing it entirely.
No point in bringing it back later, as it would immediately give an error.
CC @cknitt

@cknitt you agree? I'll remove it before merging this, if so.

Yes, go ahead!

zth force-pushed the nested-record-definitions branch from 10634fe to 2f8d81b Compare January 11, 2025 20:00

zth added this to the v12 milestone Jan 11, 2025

zth self-assigned this Jan 11, 2025

zth force-pushed the nested-record-definitions branch from 2f8d81b to 3a89ab3 Compare March 12, 2025 14:11

zth commented Mar 13, 2025

View reviewed changes

tests/analysis_tests/tests-reanalyze/deadcode/expected/deadcode.txt Outdated Show resolved Hide resolved

zth commented Mar 13, 2025

View reviewed changes

tests/analysis_tests/tests-reanalyze/deadcode/expected/exception.txt Outdated Show resolved Hide resolved

github-actions bot reviewed Mar 13, 2025

View reviewed changes

zth changed the title ~~PoC of nested record definitions~~ Nested record definitions Mar 13, 2025

zth marked this pull request as ready for review March 13, 2025 20:52

zth requested a review from cristianoc March 13, 2025 20:52

cristianoc reviewed Mar 14, 2025

View reviewed changes

compiler/syntax/src/res_core.ml Outdated Show resolved Hide resolved

cristianoc reviewed Mar 14, 2025

View reviewed changes

zth force-pushed the nested-record-definitions branch from 4528601 to c33f978 Compare March 18, 2025 17:13

zth added 9 commits March 19, 2025 13:54

poc of nested record definitions

42fac3a

tests

0706f6c

fix reanalyze suggestions

bee6305

allow inline records in constrs

c118a04

basic inline printing of nested records

f1a8a5c

update errors

9fe845e

output

77c6295

make hovers a bit nicer

6560a83

refactor away the need for res.inlineRecordReference

0f88be3

zth added 8 commits March 19, 2025 13:54

refactor away unecessary dot check

c98809e

refactor inline types context

f6129ac

support type params

35d95b5

add tests

4097443

try disabling reanalyze

653c81f

remove file

e6598ee

changelog

04847f7

comments

ed4cd0f

zth requested a review from cristianoc March 19, 2025 13:04

zth force-pushed the nested-record-definitions branch from b902d46 to ed4cd0f Compare March 19, 2025 13:05

cristianoc approved these changes Mar 20, 2025

View reviewed changes

remove reanalyze from ci

ce287c4

zth merged commit 20fea08 into master Mar 20, 2025
20 checks passed

zth deleted the nested-record-definitions branch March 20, 2025 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested record definitions #7241

Nested record definitions #7241

zth commented Jan 10, 2025 •

edited

Loading

zth commented Mar 12, 2025

zth Mar 13, 2025

zth Mar 13, 2025 •

edited

Loading

zth Mar 13, 2025

github-actions bot left a comment •

edited

Loading

zth commented Mar 13, 2025

cristianoc Mar 14, 2025

cristianoc Mar 14, 2025

zth Mar 14, 2025

cristianoc Mar 14, 2025

zth Mar 14, 2025

zth Mar 14, 2025

cristianoc Mar 14, 2025

zth Mar 14, 2025

cristianoc Mar 14, 2025

zth Mar 14, 2025

cristianoc Mar 14, 2025

zth Mar 14, 2025

cristianoc Mar 14, 2025

zth Mar 18, 2025

cristianoc Mar 18, 2025

cristianoc left a comment

zth commented Mar 19, 2025

cristianoc Mar 20, 2025

zth Mar 20, 2025

cknitt Mar 20, 2025

Nested record definitions #7241

Nested record definitions #7241

Conversation

zth commented Jan 10, 2025 • edited Loading

zth commented Mar 12, 2025

Choose a reason for hiding this comment

zth Mar 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

zth commented Mar 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc left a comment

Choose a reason for hiding this comment

zth commented Mar 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zth commented Jan 10, 2025 •

edited

Loading

zth Mar 13, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading