Make type_expr private #9994

garrigue · 2020-10-30T08:26:46Z

This PR makes Types.type_expr a private record, so that one cannot directly create or modify it.
The public definition is in submodule Internal, with conversion (identity) functions.

To make this definition easier to use, Btype.mark_type_node is extended with two new optional arguments, guard and after, which are called with the repred type, respectively before marking it (returning whether to mark it or not) and after marking it.

We have plans for further abstraction, but this seems a good first step.

@t6s co-authored this PR.

gasche · 2020-10-30T09:32:52Z

This looks interesting, thanks! (It's also joint work with @t6s, one of the first such contribution: welcome!)

I have the impression that mark_type_node is almost always passed an after parameter. To me this suggests that the API could be

val mark_type_node: type_expr -> ?guard:(type_expr -> bool) -> (type_expr -> unit) -> unit

to be used as

mark_type_node ty @@ fun ty ->
...

(or with just ignore in the cases that currently use the default after value; but I think that they could all be rewritten to make meaningful use of their parameter.)

garrigue · 2020-11-06T09:25:26Z

I have no strong opinion as to whether after should be optional or not, but do you really think that this @@ syntax is going to be more readable?
It's starting to look Haskellish.

gasche · 2020-11-06T10:34:19Z

Don't use @@ if you don't like it, but I think after should be mandatory.

trefis · 2020-11-06T11:37:23Z

On a not-exactly-related-but-still-somewhat-relevant note: is there some reason to keep using the types marking mechanism instead of a table indexed by the type expr id (as is done in Ctype.lower_contravariant for instance)?

t6s · 2020-11-06T12:27:53Z

Ctype.closed_class is the only place where mark_type_node is called without after, but its code is complicated by try .. with. Can we fit this logic into a use of after?

gasche · 2020-11-06T12:39:57Z

I would try

let closed_class params sign =
  let ty = object_fields (repr sign.csig_self) in
  let (fields, rest) = flatten_fields ty in
  List.iter mark_type params;
  mark_type rest;
  List.iter
    (fun (lab, _, ty) -> if lab = dummy_method then mark_type ty)
    fields;
  try
-    mark_type_node (repr sign.csig_self);
+    mark_type_node (repr sign.csig_self) @@ fun csig_self ->
    List.iter
      (fun (lab, kind, ty) ->
        if field_kind_repr kind = Fpresent then
        try closed_type ty with Non_closed (ty0, real) ->
          raise (CCFailure (CC_Method (ty0, real, lab, ty))))
      fields;
-    mark_type_params (repr sign.csig_self);
+    mark_type_params csig_self;
    List.iter unmark_type params;
    unmark_class_signature sign;
    None
  with CCFailure reason ->
    mark_type_params (repr sign.csig_self);
    List.iter unmark_type params;
    unmark_class_signature sign;
    Some reason

gasche · 2020-11-06T12:46:05Z

We could also factorize as

let closed_class params sign =
  let ty = object_fields (repr sign.csig_self) in
  let (fields, rest) = flatten_fields ty in
  List.iter mark_type params;
  mark_type rest;
  List.iter
    (fun (lab, _, ty) -> if lab = dummy_method then mark_type ty)
    fields;
  mark_type_node (repr sign.csig_self) @@ fun csig_self ->
  let reason =
    try
      List.iter
      (fun (lab, kind, ty) ->
        if field_kind_repr kind = Fpresent then
        try closed_type ty with Non_closed (ty0, real) ->
          raise (CCFailure (CC_Method (ty0, real, lab, ty))))
      fields;
      None
    with CCFailure reason ->
      Some reason in
  mark_type_params csig_self;
  List.iter unmark_type params;
  unmark_class_signature sign;
  reason

gasche · 2020-11-06T12:48:22Z

Note: we would not need an exception at all if we had my baby List.map_option from #9630.

garrigue · 2020-11-10T01:03:07Z

@gasche It looks like you misunderstood the role of after.
It is only called when the node has to be marked (i.e. level >= lowest_level).
So your suggested changes for closed_class would be incorrect.

My temporary conclusion is that the current proposed API of using an optional argument is better, because it does not invite people to always add some post-processing, which could be wrongly scoped.
Same thing for @@, which changes the execution path without parentheses (have we learnt nothing from the if then debacle :-)

As for @trefis 's comment, the use of a hash table in lower_contravariant comes from the possibility of expansion, which would invalidate the invariants of marking. But I agree that mark_type goes against the goal of abstracting the type representation (and requires exclusive access to the type graph, which is hard to enforce statically), and may eventually have to be replaced by something else. Not in this PR, though.

gasche · 2020-11-10T05:53:36Z

Good catch; in this case mark_type_node (repr sign.csig_self) ignore would be fine?

gasche · 2020-11-10T07:12:55Z

My temporary conclusion is that the current proposed API of using an optional argument is better, because it does not invite people to always add some post-processing, which could be wrongly scoped.

After thinking about this, I think that either APIs are just as error-prone as each other, and I am still in favor of a mandatory and non-labelled after parameter. If the error in my newcomer suggestion above can be attributed to something else than carelessness, it would dig in the following direction: why is this function using mark_type_node in such a different way than all other callsites? Should it not be using another marking function?

garrigue · 2020-11-10T07:33:29Z

So you want use to re-split it in mark_type_node (as before) and mark_type_node_with ?
This makes all the other calls longer, but if you think so...

Note that this call is not that different: fields being a list of subnodes of sign.csig_self, the List.iter just after could be inside after. But since we do not refer directly to csig_self in it, there is little incentive to do that.

I think the real choice is whether it is ok to use optional arguments to factorize APIs.
This is certainly the case in libraries, but I understand if there is some resistance inside the compiler.

t6s · 2020-11-24T05:38:05Z

The last commit makes after mandatory and separates the particular use-case of mark_type_node in closed_class into a new function mark_type_node_only. Do you think this makes the changes look better? @gasche

typing/btype.ml

lpw25 · 2020-11-24T10:31:03Z

typing/btype.ml

 function
   {desc = Tlink t' as d'} ->
     repr_link true t d' t'
 | {desc = Tfield (_, k, _, t') as d'} when field_kind_repr k = Fabsent ->
     repr_link true t d' t'
 | t' ->
     if compress then begin
-       log_change (Ccompress (t, t.desc, d)); t.desc <- d
+       log_change (Ccompress (t, t.desc, d)); (Internal.unlock t).desc <- d


There are a few lines like this. Maybe add an Internal.set_desc to make them easier to read.

The idea here is that one should not directly modify types, so providing a function that does it in types.mli seems a mixed message. Or change the name Internal to something more scary like Unsafe_access.

lpw25 · 2020-11-24T10:31:39Z

typing/btype.ml

 let rec mark_type ty =
  let ty = repr ty in
  if ty.level >= lowest_level then begin
-    ty.level <- pivot_level - ty.level;
+    (* type nodes with negative levels are "marked" *)
+    (Internal.unlock ty).level <- mirror_level ty.level;


And an Internal.set_level to use here too.

typing/types.mli

lpw25 · 2020-11-24T11:05:06Z

typing/datarepr.ml

@@ -182,7 +180,8 @@ let extension_descr ~current_unit path_ext ext =
      cstr_uid = ext.ext_uid;
    }

-let none = {desc = Ttuple []; level = -1; scope = Btype.generic_level; id = -1}
+let none = Internal.lock


Maybe move this into Btype with a name like dummy_type.

typing/btype.mli

lpw25

This is a good idea. I think the code could be cleaner though, so I've left some comments.

lpw25

I've reviewed the latest changes. They look good.

typing/ctype.ml

t6s · 2020-12-11T06:37:57Z

The last commit replaces Internal module by Private_type_expr module, which provides a more modular interface with setters.
The name is also changed since Internal was sounding too generic.

t6s · 2020-12-11T06:38:43Z

Btw, I am seeing github's complaints about conflicts while the branch builds successfully at my machine. Why?

trefis · 2020-12-11T07:37:59Z

The conflicts that github is talking about is those you'd encounter if you were to rebase on a recent trunk.
Which you will want to do before we can merge your PR.

lpw25

Latest changes look good. I think this is good to merge now.

gasche

Looks nice, thanks!

typing/btype.ml

…tion

…vate_type_expr.(create|set_*)

…typeset

garrigue · 2020-12-14T03:53:08Z

Thanks for you reviews.
I removed the commented out dead code, and rebased.
Waiting for the CI.

trefis · 2021-02-09T20:37:27Z

typing/ctype.ml

@@ -730,8 +727,10 @@ let duplicate_class_type ty =
 *)
 let rec generalize ty =
  let ty = repr ty in
+  (* generalize the type iff ty.level <= !current_level *)


I'm a bit late to the party, but I think that comment is incorrect (and also, vaguely useless?).

Introduced in ocaml#9994

trefis · 2021-02-10T14:51:09Z

typing/ctype.ml

  if (ty.level > !current_level) && (ty.level <> generic_level) then begin
    set_level ty generic_level;
+    (* recur into abbrev for the speed *)


Also, I think this comment (apart from being obscure to me) is at odds with the comment above the function.
So, which is it?

Introduced in ocaml#9994

garrigue added the no-change-entry-needed label Oct 30, 2020

lpw25 reviewed Nov 24, 2020

View reviewed changes

typing/btype.ml Outdated Show resolved Hide resolved

lpw25 reviewed Nov 24, 2020

View reviewed changes

typing/types.mli Show resolved Hide resolved

lpw25 reviewed Nov 24, 2020

View reviewed changes

typing/btype.mli Outdated Show resolved Hide resolved

lpw25 reviewed Nov 24, 2020

View reviewed changes

lpw25 reviewed Dec 8, 2020

View reviewed changes

typing/ctype.ml Show resolved Hide resolved

lpw25 approved these changes Dec 11, 2020

View reviewed changes

gasche reviewed Dec 11, 2020

View reviewed changes

typing/btype.ml Outdated Show resolved Hide resolved

t6s and others added 3 commits December 14, 2020 12:38

privatize type_expr

b9a07df

fixing btype.ml

3576906

do not open Internal

e859212

t6s and others added 17 commits December 14, 2020 12:40

finished private-ization, mark_type_node_with and set_type_desc'ifica…

031f0e2

…tion

integrate mark_type_node_with into mark_type_node

1725c7f

use mark_type_node as many as possible

6859ed2

check-typo

d4dd1b3

remove tabs

9e6b0b8

separate mark_type_node and mark_type_node_only

356867d

make mark_type_node 1st order

9e9b468

finish switching to 1st order mark_type_node

78e9b8e

new interface for mark_node (previously mark_type_node)

cc2bf4b

remove mirror_level for better abstraction

dff7a80

hide Btype.pivot_level

0357c81

experiment with setters

c06dd24

change module name to Private_type_expr

f8c455f

create

efb811e

kill remaining pivot_level

4dfc402

switch the interface for type_expr from Internal.(lock|unlock) to Pri…

3681071

…vate_type_expr.(create|set_*)

restore optimization in Ctype.deep_occur + remove obsolete Btype.set_…

273bf4c

…typeset

garrigue force-pushed the private_type_expr branch from 3ff9bbd to 273bf4c Compare December 14, 2020 03:51

garrigue added 3 commits December 14, 2020 12:57

forgotten line

efccff7

long lines

677e81d

forgotten line

f1893c6

garrigue merged commit 9f128b2 into ocaml:trunk Dec 14, 2020

trefis reviewed Feb 9, 2021

View reviewed changes

trefis added a commit to trefis/ocaml that referenced this pull request Feb 10, 2021

generalize: remove incorrect comment

fc45fc1

Introduced in ocaml#9994

trefis mentioned this pull request Feb 10, 2021

Ctype nitpicks #10211

Merged

trefis reviewed Feb 10, 2021

View reviewed changes

garrigue pushed a commit to garrigue/ocaml that referenced this pull request Mar 3, 2021

generalize: remove incorrect comment

ad41578

Introduced in ocaml#9994

dbuenzli pushed a commit to dbuenzli/ocaml that referenced this pull request Mar 25, 2021

Make type_expr private (ocaml#9994)

65ffab0

smuenzel pushed a commit to smuenzel/ocaml that referenced this pull request Mar 30, 2021

generalize: remove incorrect comment

5e12afe

Introduced in ocaml#9994

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make type_expr private #9994

Make type_expr private #9994

garrigue commented Oct 30, 2020 •

edited

gasche commented Oct 30, 2020

garrigue commented Nov 6, 2020

gasche commented Nov 6, 2020

trefis commented Nov 6, 2020

t6s commented Nov 6, 2020

gasche commented Nov 6, 2020

gasche commented Nov 6, 2020

gasche commented Nov 6, 2020

garrigue commented Nov 10, 2020

gasche commented Nov 10, 2020

gasche commented Nov 10, 2020

garrigue commented Nov 10, 2020

t6s commented Nov 24, 2020

lpw25 Nov 24, 2020

garrigue Nov 27, 2020

lpw25 Nov 24, 2020

lpw25 Nov 24, 2020

lpw25 left a comment

lpw25 left a comment

t6s commented Dec 11, 2020

t6s commented Dec 11, 2020 •

edited

trefis commented Dec 11, 2020

lpw25 left a comment

gasche left a comment

garrigue commented Dec 14, 2020

trefis Feb 9, 2021

trefis Feb 10, 2021

Make type_expr private #9994

Make type_expr private #9994

Conversation

garrigue commented Oct 30, 2020 • edited

gasche commented Oct 30, 2020

garrigue commented Nov 6, 2020

gasche commented Nov 6, 2020

trefis commented Nov 6, 2020

t6s commented Nov 6, 2020

gasche commented Nov 6, 2020

gasche commented Nov 6, 2020

gasche commented Nov 6, 2020

garrigue commented Nov 10, 2020

gasche commented Nov 10, 2020

gasche commented Nov 10, 2020

garrigue commented Nov 10, 2020

t6s commented Nov 24, 2020

lpw25 Nov 24, 2020

Choose a reason for hiding this comment

garrigue Nov 27, 2020

Choose a reason for hiding this comment

lpw25 Nov 24, 2020

Choose a reason for hiding this comment

lpw25 Nov 24, 2020

Choose a reason for hiding this comment

lpw25 left a comment

Choose a reason for hiding this comment

lpw25 left a comment

Choose a reason for hiding this comment

t6s commented Dec 11, 2020

t6s commented Dec 11, 2020 • edited

trefis commented Dec 11, 2020

lpw25 left a comment

Choose a reason for hiding this comment

gasche left a comment

Choose a reason for hiding this comment

garrigue commented Dec 14, 2020

trefis Feb 9, 2021

Choose a reason for hiding this comment

trefis Feb 10, 2021

Choose a reason for hiding this comment

garrigue commented Oct 30, 2020 •

edited

t6s commented Dec 11, 2020 •

edited