Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0006608OCamltypingpublic2014-10-09 14:012017-12-22 07:06
Assigned Togarrigue 
PrioritynormalSeverityminorReproducibilityhave not tried
PlatformOSOS Version
Product Version 
Target Version4.03.1+devFixed in Version4.05.0 +dev/beta1/beta2/beta3/rc1 
Summary0006608: Lax typing for functional record updates
DescriptionIf 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
Attached Files

- Relationships
related to 0006179closed Warning 23 is dubious 
related to 0005311closeddoligez Warning 23 message is confusing 
parent of 0007695resolvedgarrigue Fatal error: exception Ctype.Unify(_) 
has duplicate 0007211resolvedgasche Strange behavior of exhaustive record copy with disambiguation 
related to 0007696new record expression deleted when all fields specified 

-  Notes
doligez (administrator)
2014-10-10 15:34

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.
garrigue (manager)
2015-01-15 23:56

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.
garrigue (manager)
2016-10-13 14:07

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.
yallop (developer)
2016-10-18 15:38

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.
gasche (administrator)
2016-11-10 16:12

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 }`.
doligez (administrator)
2016-11-10 17:33

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.
garrigue (manager)
2016-11-12 11:09

Looks like I was sleepy when I did this change.
Fortunately, GPR#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 GPR#901 in trunk, and revert the change in, but of course keeping the test.
gasche (administrator)
2016-11-12 15:33

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.
garrigue (manager)
2016-11-13 01:47

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?
garrigue (manager)
2016-11-13 02:05

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

- Issue History
Date Modified Username Field Change
2014-10-09 14:01 yallop New Issue
2014-10-10 15:33 doligez Relationship added related to 0006179
2014-10-10 15:33 doligez Relationship added related to 0005311
2014-10-10 15:34 doligez Note Added: 0012344
2014-10-10 15:34 doligez Status new => acknowledged
2014-10-10 15:34 doligez Target Version => 4.02.2+dev / +rc1
2014-10-28 09:51 gasche Tag Attached: junior_job
2015-01-15 23:28 doligez Target Version 4.02.2+dev / +rc1 => 4.03.0+dev / +beta1
2015-01-15 23:56 garrigue Note Added: 0013113
2015-01-15 23:56 garrigue Assigned To => garrigue
2015-01-15 23:56 garrigue Status acknowledged => assigned
2016-04-12 15:14 doligez Target Version 4.03.0+dev / +beta1 => 4.03.1+dev
2016-10-13 14:07 garrigue Note Added: 0016402
2016-10-13 14:07 garrigue Status assigned => acknowledged
2016-10-18 15:38 yallop Note Added: 0016416
2016-11-10 16:12 gasche Note Added: 0016538
2016-11-10 17:33 doligez Note Added: 0016539
2016-11-12 11:09 garrigue Note Added: 0016560
2016-11-12 15:33 gasche Note Added: 0016561
2016-11-12 16:22 gasche Relationship added has duplicate 0007211
2016-11-13 01:47 garrigue Note Added: 0016569
2016-11-13 02:05 garrigue Note Added: 0016570
2016-11-13 02:05 garrigue Status acknowledged => resolved
2016-11-13 02:05 garrigue Fixed in Version => 4.05.0 +dev/beta1/beta2/beta3/rc1
2016-11-13 02:05 garrigue Resolution open => fixed
2017-02-23 16:45 doligez Category OCaml typing => typing
2017-12-22 01:27 yallop Relationship added related to 0007696
2017-12-22 07:06 nojebar Relationship added parent of 0007695

Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker