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

Fix MPR#7414 #929

Merged
merged 2 commits into from Nov 21, 2016

Conversation

Projects
None yet
3 participants
@garrigue
Contributor

garrigue commented Nov 19, 2016

Fix MPR#7414.
The problem was that non-generalizable variable lowering, introduced in 4.04 for modules, didn't work for functors.
The first attempt uses Subst.modtype for its side-effect.

garrigue added some commits Nov 19, 2016

@garrigue garrigue merged commit 2bd2296 into ocaml:trunk Nov 21, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 24, 2017

Member

@vprevosto linked this fix to a typing regression for his (safe) codebase on the caml-list:

module Foo : sig
  module M(E: sig type t end) : sig
  type t
  val u: t -> t -> t
  end
end = struct
  module M(E: sig type t end) = struct
  type t = (E.t, unit) Hashtbl.t
  let u = Hashtbl.fold (fun x () h -> Hashtbl.add h x (); h)
  end
end

If I understand correctly, this regression is expected and users have to change their code -- this is what we concluded for the non-functor case in MPR#7313, MP#7401.

Member

gasche commented Feb 24, 2017

@vprevosto linked this fix to a typing regression for his (safe) codebase on the caml-list:

module Foo : sig
  module M(E: sig type t end) : sig
  type t
  val u: t -> t -> t
  end
end = struct
  module M(E: sig type t end) = struct
  type t = (E.t, unit) Hashtbl.t
  let u = Hashtbl.fold (fun x () h -> Hashtbl.add h x (); h)
  end
end

If I understand correctly, this regression is expected and users have to change their code -- this is what we concluded for the non-functor case in MPR#7313, MP#7401.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 25, 2017

Member

For the record, other OCaml software that have to be fixed after this change are camomile (opam-builder report) and fury-puyo (opam-builder report). I am planning on sending patches to the affected packages.

Member

gasche commented Feb 25, 2017

For the record, other OCaml software that have to be fixed after this change are camomile (opam-builder report) and fury-puyo (opam-builder report). I am planning on sending patches to the affected packages.

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Mar 1, 2017

Contributor

Indeed, this is a typical case of signature annotation outside of module with nesting, which requires a more local annotation since 4.04.

Contributor

garrigue commented Mar 1, 2017

Indeed, this is a typical case of signature annotation outside of module with nesting, which requires a more local annotation since 4.04.

@thierry-martinez

This comment has been minimized.

Show comment
Hide comment
@thierry-martinez

thierry-martinez Mar 9, 2017

Contributor

Coccinelle has also to be fixed. Here is another bit of code that was correctly typed by 4.04 and is now rejected with 4.05 beta.

module type S = sig
  type 'a t = N

  val r: (unit -> unit t) ref
end

module F (X: S) = struct
end

module M = struct
  type 'a t = N

  let r = ref (fun () -> raise Not_found)
end

module N = F (M)
Contributor

thierry-martinez commented Mar 9, 2017

Coccinelle has also to be fixed. Here is another bit of code that was correctly typed by 4.04 and is now rejected with 4.05 beta.

module type S = sig
  type 'a t = N

  val r: (unit -> unit t) ref
end

module F (X: S) = struct
end

module M = struct
  type 'a t = N

  let r = ref (fun () -> raise Not_found)
end

module N = F (M)

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Fix MPR#7414 (#929)
* Fix PR#7414 by adding Mtype.lower_nongen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment