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
Comments
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. |
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... |
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. |
Comment author: @gasche It also seems strange to me that the expression is not evaluated. I would expect |
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. |
Comment author: @garrigue Looks like I was sleepy when I did this change. It seems to be already done in trunk (or is it just that the changes from 4.04 are not yet merged?) |
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. |
…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
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.
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
The text was updated successfully, but these errors were encountered: