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.

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

PR#6608: Unify record types when overriding all fields #901

Merged
merged 2 commits into from Nov 11, 2016

Conversation

Projects
None yet
6 participants
@tadeuzagallo
Contributor

tadeuzagallo commented Nov 9, 2016

This PR fixes the issues listed on https://caml.inria.fr/mantis/view.php?id=6608:

Functional record updates, when overriding all the fields and the type of the original record was not known statically, wouldn't be checked, e.g. the following snippet would typecheck:

{ "" with contents=1 }

The fix simply move the unification up: it was only done when a field was kept, now it's done regardless.

Show outdated Hide outdated testsuite/tests/regression/pr6608/Makefile
#* *
#* OCaml *
#* *
#* Thomas Refis, Jane Street Europe *

This comment has been minimized.

@dra27

dra27 Nov 9, 2016

Contributor

No need for a copyright header in a testsuite Makefile (apparently forgotten where this one came from!)

@dra27

dra27 Nov 9, 2016

Contributor

No need for a copyright header in a testsuite Makefile (apparently forgotten where this one came from!)

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 9, 2016

Contributor

It's actually highlighted an error in ocamldoc too! (see the bottom of the AppVeyor log)

Contributor

dra27 commented Nov 9, 2016

It's actually highlighted an error in ocamldoc too! (see the bottom of the AppVeyor log)

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 9, 2016

Member

The patch is not quite right. Notice the comment that says (* do not connect overridden labels *). I think it refers to the fact that, when the value of a field is replaced, the type of this field should be allowed change. Consider the following code for example:

# type ('a, 'b) t = { fst : 'a; snd : 'b };;
# let with_fst r fst = { r with fst };;
val with_fst : ('a, 'b) t -> 'c -> ('c, 'b) t = <fun>

With your patch, with_fst gets a less polymorphic type ('a, 'b) t -> 'a -> ('a, 'b) t.

Member

gasche commented Nov 9, 2016

The patch is not quite right. Notice the comment that says (* do not connect overridden labels *). I think it refers to the fact that, when the value of a field is replaced, the type of this field should be allowed change. Consider the following code for example:

# type ('a, 'b) t = { fst : 'a; snd : 'b };;
# let with_fst r fst = { r with fst };;
val with_fst : ('a, 'b) t -> 'c -> ('c, 'b) t = <fun>

With your patch, with_fst gets a less polymorphic type ('a, 'b) t -> 'a -> ('a, 'b) t.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 9, 2016

Member

The following change would look better to my non-specialist eyes:

diff --git a/typing/typecore.ml b/typing/typecore.ml
index aec18af..45057b2 100644
--- a/typing/typecore.ml
+++ b/typing/typecore.ml
@@ -2330,16 +2330,16 @@ and type_expect_ ?in_function ?(recarg=Rejected) env sexp ty_expected =
         | Some exp ->
             let ty_exp = instance env exp.exp_type in
             let unify_kept lbl =
+              let _, ty_arg1, ty_res1 = instance_label false lbl in
+              unify_exp_types exp.exp_loc env ty_exp ty_res1;
               match matching_label lbl with
               | lid, _lbl, lbl_exp ->
+                  (* do not connect argument types for overridden labels *)
                   Overridden (lid, lbl_exp)
               | exception Not_found -> begin
-                (* do not connect overridden labels *)
-                  let _, ty_arg1, ty_res1 = instance_label false lbl
-                  and _, ty_arg2, ty_res2 = instance_label false lbl in
+                  let _, ty_arg2, ty_res2 = instance_label false lbl in
                   unify env ty_arg1 ty_arg2;
                   unify env (instance env ty_expected) ty_res2;
-                  unify_exp_types exp.exp_loc env ty_exp ty_res1;
                   Kept ty_arg1
                 end
             in

One thing that made this code difficult to read is that _arg means "the type of the field expression" and _res means "the type of the record". I think the names come from typing patterns of the form { f1 = p1; f2 = p2; ...}, where _arg is the type of the argument pattern, and _res is the resulting pattern type. As far as I'm concerned, ty_field and ty_record would be massively easier to read, but _arg/_res is consistently used in the codebase so I didn't want to change it -- that would be @garrigue's choice to make.

Member

gasche commented Nov 9, 2016

The following change would look better to my non-specialist eyes:

diff --git a/typing/typecore.ml b/typing/typecore.ml
index aec18af..45057b2 100644
--- a/typing/typecore.ml
+++ b/typing/typecore.ml
@@ -2330,16 +2330,16 @@ and type_expect_ ?in_function ?(recarg=Rejected) env sexp ty_expected =
         | Some exp ->
             let ty_exp = instance env exp.exp_type in
             let unify_kept lbl =
+              let _, ty_arg1, ty_res1 = instance_label false lbl in
+              unify_exp_types exp.exp_loc env ty_exp ty_res1;
               match matching_label lbl with
               | lid, _lbl, lbl_exp ->
+                  (* do not connect argument types for overridden labels *)
                   Overridden (lid, lbl_exp)
               | exception Not_found -> begin
-                (* do not connect overridden labels *)
-                  let _, ty_arg1, ty_res1 = instance_label false lbl
-                  and _, ty_arg2, ty_res2 = instance_label false lbl in
+                  let _, ty_arg2, ty_res2 = instance_label false lbl in
                   unify env ty_arg1 ty_arg2;
                   unify env (instance env ty_expected) ty_res2;
-                  unify_exp_types exp.exp_loc env ty_exp ty_res1;
                   Kept ty_arg1
                 end
             in

One thing that made this code difficult to read is that _arg means "the type of the field expression" and _res means "the type of the record". I think the names come from typing patterns of the form { f1 = p1; f2 = p2; ...}, where _arg is the type of the argument pattern, and _res is the resulting pattern type. As far as I'm concerned, ty_field and ty_record would be massively easier to read, but _arg/_res is consistently used in the codebase so I didn't want to change it -- that would be @garrigue's choice to make.

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Nov 9, 2016

Contributor

@gasche Thanks for reviewing, that makes sense, I hadn't looked at the type system yet, was mostly trying to familiarise myself with it and thought it was worth a shot. I was mostly confused by instance_label actually, but from re-reading it I think I get it.

Contributor

tadeuzagallo commented Nov 9, 2016

@gasche Thanks for reviewing, that makes sense, I hadn't looked at the type system yet, was mostly trying to familiarise myself with it and thought it was worth a shot. I was mostly confused by instance_label actually, but from re-reading it I think I get it.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 9, 2016

Member

Wait, my message above contains a variant of your change that I believe may be correct. Wouldn't you be interested in taking it and running with it, to see if people that actually know the type-system would agree this is a reasonable change? With your test, plus a regression test on my polymorphic example, this could make fine pull-request and maybe it works.

Member

gasche commented Nov 9, 2016

Wait, my message above contains a variant of your change that I believe may be correct. Wouldn't you be interested in taking it and running with it, to see if people that actually know the type-system would agree this is a reasonable change? With your test, plus a regression test on my polymorphic example, this could make fine pull-request and maybe it works.

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Nov 9, 2016

Contributor

Sure, just didn't mean to take over your fix, but if you're ok with it I'll update it and add the tests you suggested and we'll see how it goes then.

Contributor

tadeuzagallo commented Nov 9, 2016

Sure, just didn't mean to take over your fix, but if you're ok with it I'll update it and add the tests you suggested and we'll see how it goes then.

@tadeuzagallo tadeuzagallo reopened this Nov 9, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 9, 2016

Member

You should also add a Changes entry (see the contribution guide) and you can mention people involved there (you, me, Jeremy Yallop as the requester, and the reviewer(s) if there is substantial review work involved).

Member

gasche commented Nov 9, 2016

You should also add a Changes entry (see the contribution guide) and you can mention people involved there (you, me, Jeremy Yallop as the requester, and the reviewer(s) if there is substantial review work involved).

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 9, 2016

Contributor

@gasche's tweak to your patch allows ocamldoc to build again - I didn't try running the testsuite.

Contributor

dra27 commented Nov 9, 2016

@gasche's tweak to your patch allows ocamldoc to build again - I didn't try running the testsuite.

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Nov 9, 2016

Contributor

@gasche 's code seems correct to me.
Strange I couldn't come up with something like that when I read the MPR.
The _arg /_res naming comes from the label description definition. You should ask Xavier about that, but I see little point in changing it.

Contributor

garrigue commented Nov 9, 2016

@gasche 's code seems correct to me.
Strange I couldn't come up with something like that when I read the MPR.
The _arg /_res naming comes from the label description definition. You should ask Xavier about that, but I see little point in changing it.

Show outdated Hide outdated testsuite/tests/regression/pr6608/Makefile
test_2:
@printf " ... testing 'pr6608_2.reference':"
@$(OCAMLC) -c pr6608_2.ml && echo " => passed" || echo " => failed"

This comment has been minimized.

@gasche

gasche Nov 10, 2016

Member

Instead of writing your own testing Makefile, you could simply reuse the toplevel one that runs both of them through the toplevel, as a way to check either typing errors or the results of type inference. testsuite/tests/typing-unboxed would be an example of that, the Makefile is very simple, and you just have to create a .reference file for each .ml file in the test directory.

@gasche

gasche Nov 10, 2016

Member

Instead of writing your own testing Makefile, you could simply reuse the toplevel one that runs both of them through the toplevel, as a way to check either typing errors or the results of type inference. testsuite/tests/typing-unboxed would be an example of that, the Makefile is very simple, and you just have to create a .reference file for each .ml file in the test directory.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Nov 10, 2016

Member
{ "" with contents=1 }

Warning: I seem to remember that @xavierleroy once told me this was on purpose.

Member

damiendoligez commented Nov 10, 2016

{ "" with contents=1 }

Warning: I seem to remember that @xavierleroy once told me this was on purpose.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 10, 2016

Member

@tadeuzagallo could you squah all your commits together? If/when we merge we don't need the back-and-forth.

After a squash, I would personally be willing to merge the change.

@damiendoligez, @xavierleroy: is this on purpose? Would you have an opinion on @yallop's question in the issue report, of whether { (raise Exit) with contents = 1 } should raise or not?

Member

gasche commented Nov 10, 2016

@tadeuzagallo could you squah all your commits together? If/when we merge we don't need the back-and-forth.

After a squash, I would personally be willing to merge the change.

@damiendoligez, @xavierleroy: is this on purpose? Would you have an opinion on @yallop's question in the issue report, of whether { (raise Exit) with contents = 1 } should raise or not?

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Nov 10, 2016

Member

I totally agree with @yallop. I'm afraid we'll have to undo commit c545e04.

Member

damiendoligez commented Nov 10, 2016

I totally agree with @yallop. I'm afraid we'll have to undo commit c545e04.

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Nov 11, 2016

Contributor

I have no problem with reverting c545e04 once this PR is merged, since it solves the segmentation fault my commit intended to avoid.
Just a small remark: I would rather put the test in typing-misc/records.ml, since this is a typing test, and record related.

Contributor

garrigue commented Nov 11, 2016

I have no problem with reverting c545e04 once this PR is merged, since it solves the segmentation fault my commit intended to avoid.
Just a small remark: I would rather put the test in typing-misc/records.ml, since this is a typing test, and record related.

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Nov 11, 2016

Contributor

By the way, there is no need to squash in advance: you can do Squash and merge with the web interface of github.

Contributor

garrigue commented Nov 11, 2016

By the way, there is no need to squash in advance: you can do Squash and merge with the web interface of github.

@tadeuzagallo tadeuzagallo changed the title from Fix: Unify record types for overriden fields to PR#6608: Unify record types when overriding all fields Nov 11, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 11, 2016

Member

If you squash from the github interface, the commit message of the result is just the concatenation of all commit messages. This means the back-and-forth is elimited from the commit history, but not from the commit message. It's acceptable in some circumstances, but given that this is a small change I may as well as for all the nice things.

Member

gasche commented Nov 11, 2016

If you squash from the github interface, the commit message of the result is just the concatenation of all commit messages. This means the back-and-forth is elimited from the commit history, but not from the commit message. It's acceptable in some circumstances, but given that this is a small change I may as well as for all the nice things.

@gasche gasche merged commit ebdb120 into ocaml:trunk Nov 11, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 11, 2016

Member

Merged! Thanks a lot for pushing things forward.

Member

gasche commented Nov 11, 2016

Merged! Thanks a lot for pushing things forward.

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Nov 11, 2016

Contributor

Shouldn't we also merge this to 4.04, so that we can roll-back c545e04 ?
I'm sorry, but I don't seem to be able to do that (cherry-pick leaves me in a broken state).

Contributor

garrigue commented Nov 11, 2016

Shouldn't we also merge this to 4.04, so that we can roll-back c545e04 ?
I'm sorry, but I don't seem to be able to do that (cherry-pick leaves me in a broken state).

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Nov 11, 2016

Member

If you squash from the github interface, the commit message of the result is just the concatenation of all commit messages.

For what it's worth, there's an opportunity after pressing 'Squash and merge' to edit the commit message before actually merging.

Member

yallop commented Nov 11, 2016

If you squash from the github interface, the commit message of the result is just the concatenation of all commit messages.

For what it's worth, there's an opportunity after pressing 'Squash and merge' to edit the commit message before actually merging.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 11, 2016

Member

Ah, I missed it then, sorry.

@garrigue do you really think this is needed in 4.04.x? It's not a bugfix, if I understand correctly -- even including the revert of the other patch.

Member

gasche commented Nov 11, 2016

Ah, I missed it then, sorry.

@garrigue do you really think this is needed in 4.04.x? It's not a bugfix, if I understand correctly -- even including the revert of the other patch.

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Nov 11, 2016

Contributor

Merged! Thanks a lot for pushing things forward.

Thank you all for the reviews and explanations. I appreciate that sometimes dealing with pull requests is more work than fixing it yourself. Hopefully I'll find something to contribute more soon.

Contributor

tadeuzagallo commented Nov 11, 2016

Merged! Thanks a lot for pushing things forward.

Thank you all for the reviews and explanations. I appreciate that sometimes dealing with pull requests is more work than fixing it yourself. Hopefully I'll find something to contribute more soon.

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Nov 12, 2016

Contributor

@gasche The discussion in PR#6608 seems to imply that my fix of the problem was a bug. But it was needed to protect about a potential segmentation fault if types differed. So if we want to fix this in the 4.04 branch we also need to merge this PR there. Of course, the problem with c545e04 being rather far-fetched (change in evaluation behavior if one ignored warning 23), and this PR restricts typing, so we could also decide that it is safe to wait until 4.05.

Contributor

garrigue commented Nov 12, 2016

@gasche The discussion in PR#6608 seems to imply that my fix of the problem was a bug. But it was needed to protect about a potential segmentation fault if types differed. So if we want to fix this in the 4.04 branch we also need to merge this PR there. Of course, the problem with c545e04 being rather far-fetched (change in evaluation behavior if one ignored warning 23), and this PR restricts typing, so we could also decide that it is safe to wait until 4.05.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #901 from tadeuzagallo/pr6608
PR#6608: Unify record types when overriding all fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment