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

Mutable field update ignored in closure #7789

Closed
vicuna opened this issue May 2, 2018 · 11 comments
Closed

Mutable field update ignored in closure #7789

vicuna opened this issue May 2, 2018 · 11 comments
Assignees

Comments

@vicuna
Copy link

vicuna commented May 2, 2018

Original bug ID: 7789
Reporter: tbrk
Assigned to: @gasche
Status: resolved (set by @gasche on 2018-05-02T20:17:55Z)
Resolution: not a bug
Priority: high
Severity: major
Platform: x86_64
OS: MacOS / Debian Linux
OS Version: 10.11.6 / 9.4
Version: 4.06.1
Fixed in version: 4.06.0
Category: misc
Monitored by: @nojb @yallop

Bug description

Consider the following program:

type data = { mutable n  : int; }

let f { n } t = Printf.printf "n=%d\n%!" n

let go udata g =
  g 1.0;
  udata.n <- 10;
  g 1.0
;;

let udata = { n = 0 } in
go udata (f udata);;

In OCaml <= 4.05.0, it prints (correctly):
n=0
n=10

In OCaml 4.06.0 and 4.07.0+beta2 (at least), it prints (incorrectly):
n=0
n=0

Steps to reproduce

Compile the above program with ocamlc or ocamlopt, or enter it into the top-level.

Additional information

The given code is a reduced form from a larger example:
https://github.com/inria-parkas/sundialsml/blob/55a05d3794e98e1d4b3f9953088e41ef6f359bb2/examples/arkode/C_serial/ark_heat1D_adapt.ml#L155

@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: @xavierleroy

Ouch, this looks bad.

Are you using the flambda optimizer? To answer this question you can do
ocamlopt -config|grep flambda

@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: tbrk

Yes, it surprises me that no one else hit it yet. I hope it's not a bêtise on my part.

flambda: false (for both 4.06.0 and 4.07.0+beta2).

I've just started a git bisect.

@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: @nojb

The code for f in 4.06 is:

  (function param/1035
     (funct-body //toplevel//(3)<ghost>:44-80
       (let (n/1012 =o (field 0 param/1035))
         (before //toplevel//(3)<ghost>:50-80
           (function t/1013
             (funct-body //toplevel//(3)<ghost>:50-80
               (before //toplevel//(3):54-80
                 (after //toplevel//(3):54-80
                   (apply (field 1 (global Printf!))
                     [0:
                      [11: "n=" [4: 0a 0a 0a [12: '\n' [10: 0a]]]]
                      "n=%d\n%!"]
                     n/1012)))))))))

In 4.05 it is:

  (function param/1238 t/1216
     (funct-body //toplevel//(3)<ghost>:48-84
       (let (n/1215 =a (field 0 param/1238))
         (before //toplevel//(3):58-84
           (after //toplevel//(3):58-84
             (apply (field 1 (global Printf!))
               [0:
                [11: "n=" [4: 0a 0a 0a [12: '\n' [10: 0a]]]] "n=%d\n%!"]
               n/1215))))))

Somehow f is curried in 4.06 which leads to an early extraction of the value of the udata.n field in 4.06, while it is correctly delayed until f is fully applied in 4.05.

@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: tbrk

(I don't know why the status changed. I only clicked refresh...)

@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: @nojb

Seems to be related to the compilation of pattern matching as it looks OK if one does not use a pattern to access the field n.

@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: @nojb

I haven't gone through the details, but seems related to #1308

@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: tbrk

I'm still chugging through the bisect. Is there are faster way to rebuild than "make clean; ./configure && make world.opt"?

@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: @nojb

No need to waste your time. The commit responsible is #1308. Behaviour being discussed here is an intended consequence of that PR.

@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: tbrk

Ah, I see. I'm supposed to write:

let f state t = Printf.printf "n=%d\n%!" state.n

I get it now. A bêtise on my part, after all! Thanks for your help. Sorry for the noise.

@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: @gasche

This is the second user report that is surprised by the change of behavior. Maybe we should communicate more clearly about this specific change -- I could write a message to the caml-list.

@vicuna vicuna closed this as completed May 2, 2018
@vicuna
Copy link
Author

vicuna commented May 2, 2018

Comment author: @gasche

Ah, I see. I'm supposed to write:

let f state t = Printf.printf "n=%d\n%!" state.n

Well, I would rather recommend to change

go udata (f udata);;

into

go udata (fun t -> f udata t);;

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

2 participants