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

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator 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

This comment has been minimized.

Copy link
Collaborator 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

This comment has been minimized.

Copy link
Collaborator 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

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 2, 2018

Comment author: tbrk

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

@vicuna

This comment has been minimized.

Copy link
Collaborator 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

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 2, 2018

Comment author: @nojb

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

@vicuna

This comment has been minimized.

Copy link
Collaborator 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

This comment has been minimized.

Copy link
Collaborator 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

This comment has been minimized.

Copy link
Collaborator 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

This comment has been minimized.

Copy link
Collaborator 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 May 2, 2018

@vicuna

This comment has been minimized.

Copy link
Collaborator 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);;

@vicuna vicuna added the bug label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.