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.

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

Lax typing for functional record updates #6608

Closed
vicuna opened this issue Oct 9, 2014 · 11 comments
Closed

Lax typing for functional record updates #6608

vicuna opened this issue Oct 9, 2014 · 11 comments

Comments

@vicuna
Copy link

vicuna commented Oct 9, 2014

Original bug ID: 6608
Reporter: @yallop
Assigned to: @garrigue
Status: resolved (set by @garrigue on 2016-11-13T01:05:07Z)
Resolution: fixed
Priority: normal
Severity: minor
Target version: 4.03.1+dev
Fixed in version: 4.05.0 +dev/beta1/beta2/beta3/rc1
Category: typing
Tags: junior_job
Has duplicate: #7211
Related to: #5311 #6179 #7696
Parent of: #7695

Bug description

If a functional record update lists all the fields of a record then there's sometimes no check that the record expression has the appropriate type:

{ "reference" with contents = 0 };;

Characters 0-33:
{ "reference" with contents = 0 };;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning 23: all the fields are explicitly listed in this record:
the 'with' clause is useless.

  • : int ref = {contents = 0}

The exact behaviour depends on whether the record expression has record type:

type t = { x: int };;

type t = { x : int; }

{{x=3} with contents = 5};;

Characters 12-20:
{{x=3} with contents = 5};;
^^^^^^^^
Error: This record expression is expected to have type t
The field contents does not belong to type t

@vicuna
Copy link
Author

vicuna commented Oct 10, 2014

Comment author: @damiendoligez

I seem to remember that this is what led to the implementation of warning 23, but I can't find the corresponding PR.

As for the inconsistent behaviour, I don't know where it comes from.

@vicuna
Copy link
Author

vicuna commented Jan 15, 2015

Comment author: @garrigue

The inconsistency is due to the fact one first tries to get type information from the expression. If this information can be obtained it is used to type the fields, but if it is unknown, or not a record, this is delayed to later check, and this check never comes if all the fields are there...
Probably we should unify with the type constructor when all the fields are there.

@vicuna
Copy link
Author

vicuna commented Oct 13, 2016

Comment author: @garrigue

Changed the internal behavior in commit c545e04: delete the extended_expression field if the original expression is not used.
This could cause segmentation faults depending on the ulterior compilation scheme.

Whether we need to restrict the typing is left open.

@vicuna
Copy link
Author

vicuna commented Oct 18, 2016

Comment author: @yallop

I'm not sure about deleting the extended expression. What if it has effects? It seems surprising for

{ (raise E) with x = () }

not to raise an exception.

@vicuna
Copy link
Author

vicuna commented Nov 10, 2016

Comment author: @gasche

It also seems strange to me that the expression is not evaluated. I would expect { expr with fields } to be equivalent to let r = expr in { r with fields }.

@vicuna
Copy link
Author

vicuna commented Nov 10, 2016

Comment author: @damiendoligez

I concur. An expression should be strict or lazy, not some strange mix of the two. Especially considering that this construct is intended to make it easier to use some subsets of a record's labels without taking the others into account. Having to know whether the subset is the whole set, really defeats the purpose.

@vicuna
Copy link
Author

vicuna commented Nov 12, 2016

Comment author: @garrigue

Looks like I was sleepy when I did this change.
Fortunately, #901 removes the need for c545e04, so we could revert it.

It seems to be already done in trunk (or is it just that the changes from 4.04 are not yet merged?)
It could also be nice to merge #901 in trunk, and revert the change in typecore.ml, but of course keeping the test.

@vicuna
Copy link
Author

vicuna commented Nov 12, 2016

Comment author: @gasche

I merged #901 in trunk yesterday. Jacques, can I let you revert c545e04?

My gut feeling is that it doesn't need to go in 4.04 -- the behavior of 4.04.0 is reasonable (arguably ignoring the expression could break programs that rely on a side-effect of the expression, but we don't know of any such breakage), and having fewer patches in the 4.04 branch makes it easier to review.

@vicuna
Copy link
Author

vicuna commented Nov 13, 2016

Comment author: @garrigue

I can't, because it's not there: c545e04 is not merged into trunk yet.
So I'm not sure how to proceed: manually merge c545e04, and then revert the change leaving only the test?

@vicuna
Copy link
Author

vicuna commented Nov 13, 2016

Comment author: @garrigue

Merge (and reverted) c545e04 into trunk.
All green for the future.

@xavierleroy
Copy link
Contributor

Nearly 4 years later, c545e04 has NOT been reverted, so we still have the wrong behavior reported in #7696, namely that the base expression is not evaluated if all labels are redefined.

xavierleroy pushed a commit that referenced this issue Apr 8, 2020
…edefined (#9432)

This commit reverts c1a7ace (originally c545e04), which was a
temporary fix that is no longer needed because it was superseded by
#6608.

The temporary fix caused `{expr with lbl1 = e1; ... }` to not evaluate
`expr` if all labels of its type are overriden.  As reported in #7696
this is not desirable.  Reverting the temporary fix causes `expr` to
be evaluated always.

As a consequence, a corner case of value "let rec" is no longer accepted.
The corresponding test was updated.

Closes: #7696
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants