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

Extending `open` to accept arbitrary module expression #1506

Open
wants to merge 46 commits into
base: trunk
from

Conversation

Projects
None yet
@objmagic
Contributor

objmagic commented Dec 2, 2017

This pull request allows open to accept an arbitrary module expression.

Full paper accepted at OCaml Workshop 2017 (online playground is not up-to-date with this pull request)

Summary

OCaml provides two operations for introducing names exported from one module into another module: open and include. Both operations introduce M's bindings into the current scope.
include also re-exports the bindings from the current scope.

A second difference between open and include concerns the form of the argument. The argument to open can only be a module path: open A.B.C. The argument to include, however, can be any module expression:

 include F(X)  include (M:S)  include struct ... end

This PR proposes extending open to eliminate that second difference, so that both open and include accept an arbitrary module expression as argument:

type include open
path include A.B.C open A.B.C
with module type constraint include (M: S) open (M:S)
structure include struct ... end open struct ... end
functor application include F(X) open F(X)

This extension is not added into local opens.

Examples

The extended open has many applications. Please see examples in paper above.

Implementation

Most of the work was done in typing/typemod.ml. For an extended open statement like open struct ... end, we go over the following steps:

  1. Type-check the module expression
  2. Generate a module identifier, say, M#1, then insert module M#1 = struct ... end before open statement
  3. Call Env.open_signature on M#1 to export bindings into environment
  4. Check if the generated identifier can be eliminated from rest of the structure (see section 4 "Restrictions and design considerations" in the paper above for details).

Different from paper, this patch also introduced open extension for let-expression. Scope checking of generated module identifier comes for free in let-expression.

I would appreciate reviews and corner cases that I fail to cover.testsuite/tests/typing-modules/open-struct.ml contains some test cases that one may find useful when reviewing this patch. If you want to try out this patch, follow the tips here.

$ opam repo add ocaml-pr https://github.com/ocaml/ocaml-pr-repository.git
$ opam switch 4.06.0+pr1506
$ eval `opam config env`

I am grateful to @yallop and @lpw25 for their help and review.

objmagic added some commits Apr 9, 2017

Extend `open`.
Signed-off-by: Runhang Li <marklrh@gmail.com>
Merge branch 'trunk' into open-struct-final
Signed-off-by: Runhang Li <marklrh@gmail.com>
Correct `open` for module sig.
Check if we can eliminate identifier in module sig
no matter if we are in nested open.

@objmagic objmagic referenced this pull request Dec 2, 2017

Open

ppx_export mark 2 #1251

7 of 7 tasks complete
@objmagic

This comment has been minimized.

Show comment
Hide comment
@objmagic

objmagic Dec 28, 2017

Contributor

cc @alainfrisch

May I have a round of review?

Contributor

objmagic commented Dec 28, 2017

cc @alainfrisch

May I have a round of review?

@Octachron

After a quick look, I had a few questions in term of user-facing messages and API, and code readability:

Show outdated Hide outdated driver/compmisc.ml Outdated
@@ -322,7 +322,7 @@ module Mb:
module Opn:
sig
val mk: ?loc: loc -> ?attrs:attrs -> ?docs:docs ->
?override:override_flag -> lid -> open_description
?override:override_flag -> module_expr -> open_description

This comment has been minimized.

@Octachron

Octachron Dec 28, 2017

Contributor

Similarly, I think it would be useful to have specialized function for the lid case. Moreover, since mk is exposed in compiler-libs, I am wondering if it would not be more pratical for users to add a mk_expr for the generic case and let mk as the specialized version.

@Octachron

Octachron Dec 28, 2017

Contributor

Similarly, I think it would be useful to have specialized function for the lid case. Moreover, since mk is exposed in compiler-libs, I am wondering if it would not be more pratical for users to add a mk_expr for the generic case and let mk as the specialized version.

Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
@ubsan

This comment has been minimized.

Show comment
Hide comment
@ubsan

ubsan Jan 5, 2018

This would be awesome for me! I'm implementing a sort of typeclass thing, and it'd be great to not have to call the typeclass implementation functors, except in open.

(right now, I have something like

(* in Result *)
module Monad(E: Type) = struct
  type error = E.t;
  ...
end

(* in cafec/parse/error.re *)
module Monad_result = Result.Monad(sig type nonrec t = t end);

(* in cafec/parse/parse.re *)
open Error.Monad_result;

I'd rather write

(* Result is the same *)

(* in cafec/parse/parse.re *)
open Result.Monad(sig type t = Error.t end)

)

ubsan commented Jan 5, 2018

This would be awesome for me! I'm implementing a sort of typeclass thing, and it'd be great to not have to call the typeclass implementation functors, except in open.

(right now, I have something like

(* in Result *)
module Monad(E: Type) = struct
  type error = E.t;
  ...
end

(* in cafec/parse/error.re *)
module Monad_result = Result.Monad(sig type nonrec t = t end);

(* in cafec/parse/parse.re *)
open Error.Monad_result;

I'd rather write

(* Result is the same *)

(* in cafec/parse/parse.re *)
open Result.Monad(sig type t = Error.t end)

)

@ubsan

This comment has been minimized.

Show comment
Hide comment
@ubsan

ubsan Jan 6, 2018

There's a bug in this patch. I've installed this and switched to it, and attempted to install jbuilder:

$ opam install jbuilder
The following actions will be performed:
  ∗  install conf-m4   1                      [required by ocamlfind]
  ∗  install ocamlfind 1.7.3                  [required by jbuilder]
  ∗  install jbuilder  1.0+beta16
===== ∗  3 =====
Do you want to continue ? [Y/n] y

=-=- Gathering sources =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
[default] https://opam.ocaml.org/archives/ocamlfind.1.7.3+opam.tar.gz downloaded
[default] https://opam.ocaml.org/archives/jbuilder.1.0+beta16+opam.tar.gz downloaded

=-=- Processing actions -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
∗  installed conf-m4.1
∗  installed ocamlfind.1.7.3
[ERROR] The compilation of jbuilder failed at "ocaml bootstrap.ml".

#=== ERROR while installing jbuilder.1.0+beta16 ===============================#
# opam-version 1.2.2
# os           linux
# command      ocaml bootstrap.ml
# path         /home/nicole/.opam/4.06.0+pr1506/build/jbuilder.1.0+beta16
# compiler     4.06.0+pr1506
# exit-code    2
# env-file     /home/nicole/.opam/4.06.0+pr1506/build/jbuilder.1.0+beta16/jbuilder-6972-8b8a2d.env
# stdout-file  /home/nicole/.opam/4.06.0+pr1506/build/jbuilder.1.0+beta16/jbuilder-6972-8b8a2d.out
# stderr-file  /home/nicole/.opam/4.06.0+pr1506/build/jbuilder.1.0+beta16/jbuilder-6972-8b8a2d.err
### stdout ###
# /home/nicole/.opam/4.06.0+pr1506/bin/ocamllex.opt -q src/sexp_lexer.mll
# /home/nicole/.opam/4.06.0+pr1506/bin/ocamllex.opt -q src/meta_lexer.mll
# /home/nicole/.opam/4.06.0+pr1506/bin/ocamldep.opt -modules src/action.ml src/action_intf.ml src/alias.ml src/ansi_color.ml src/arg_spec.ml src/artifacts.ml src/bin.ml src/build.ml src/build_interpret.ml src/build_system.ml src/clflags.ml src/cm_kind.ml src/config.ml src/context.ml src/file_tree.ml src/findlib.ml src/future.ml src/gen_meta.ml src/gen_rules.ml src/glob_lexer.boot.ml src/import.ml src/install.ml src/io.ml src/jbuild.ml src/jbuild_load.ml vendor/boot/jbuilder_opam_file_format.ml vendor/boot/jbuilder_re.ml src/js_of_ocaml_rules.ml src/lib.ml src/lib_db.ml src/loc.ml src/log.ml src/main.ml src/merlin.ml src/meta.ml src/meta_lexer.ml src/ml_kind.ml src/mode.ml src/module.ml src/module_compilation.ml src/ocaml_flags.ml src/ocamldep.ml src/odoc.ml src/opam_file.ml src/ordered_set_lang.ml src/package.ml src/path.ml src/sexp.ml src/sexp_lexer.ml src/string_with_vars.ml src/super_context.ml src/top_closure.ml src/utils.ml src/utop.ml src/vfile_kind.ml src/watermarks.ml src/workspace.ml > boot-depends.txt
# /home/nicole/.opam/4.06.0+pr1506/bin/ocamlopt.opt -w -40 -o boot.exe unix.cmxa boot.ml
### stderr ###
# Fatal error: exception Invalid_argument("index out of bounds")



=-=- Error report -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
The following actions failed
  ∗  install jbuilder 1.0+beta16
The following changes have been performed
  ∗  install conf-m4   1
  ∗  install ocamlfind 1.7.3

ubsan commented Jan 6, 2018

There's a bug in this patch. I've installed this and switched to it, and attempted to install jbuilder:

$ opam install jbuilder
The following actions will be performed:
  ∗  install conf-m4   1                      [required by ocamlfind]
  ∗  install ocamlfind 1.7.3                  [required by jbuilder]
  ∗  install jbuilder  1.0+beta16
===== ∗  3 =====
Do you want to continue ? [Y/n] y

=-=- Gathering sources =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
[default] https://opam.ocaml.org/archives/ocamlfind.1.7.3+opam.tar.gz downloaded
[default] https://opam.ocaml.org/archives/jbuilder.1.0+beta16+opam.tar.gz downloaded

=-=- Processing actions -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
∗  installed conf-m4.1
∗  installed ocamlfind.1.7.3
[ERROR] The compilation of jbuilder failed at "ocaml bootstrap.ml".

#=== ERROR while installing jbuilder.1.0+beta16 ===============================#
# opam-version 1.2.2
# os           linux
# command      ocaml bootstrap.ml
# path         /home/nicole/.opam/4.06.0+pr1506/build/jbuilder.1.0+beta16
# compiler     4.06.0+pr1506
# exit-code    2
# env-file     /home/nicole/.opam/4.06.0+pr1506/build/jbuilder.1.0+beta16/jbuilder-6972-8b8a2d.env
# stdout-file  /home/nicole/.opam/4.06.0+pr1506/build/jbuilder.1.0+beta16/jbuilder-6972-8b8a2d.out
# stderr-file  /home/nicole/.opam/4.06.0+pr1506/build/jbuilder.1.0+beta16/jbuilder-6972-8b8a2d.err
### stdout ###
# /home/nicole/.opam/4.06.0+pr1506/bin/ocamllex.opt -q src/sexp_lexer.mll
# /home/nicole/.opam/4.06.0+pr1506/bin/ocamllex.opt -q src/meta_lexer.mll
# /home/nicole/.opam/4.06.0+pr1506/bin/ocamldep.opt -modules src/action.ml src/action_intf.ml src/alias.ml src/ansi_color.ml src/arg_spec.ml src/artifacts.ml src/bin.ml src/build.ml src/build_interpret.ml src/build_system.ml src/clflags.ml src/cm_kind.ml src/config.ml src/context.ml src/file_tree.ml src/findlib.ml src/future.ml src/gen_meta.ml src/gen_rules.ml src/glob_lexer.boot.ml src/import.ml src/install.ml src/io.ml src/jbuild.ml src/jbuild_load.ml vendor/boot/jbuilder_opam_file_format.ml vendor/boot/jbuilder_re.ml src/js_of_ocaml_rules.ml src/lib.ml src/lib_db.ml src/loc.ml src/log.ml src/main.ml src/merlin.ml src/meta.ml src/meta_lexer.ml src/ml_kind.ml src/mode.ml src/module.ml src/module_compilation.ml src/ocaml_flags.ml src/ocamldep.ml src/odoc.ml src/opam_file.ml src/ordered_set_lang.ml src/package.ml src/path.ml src/sexp.ml src/sexp_lexer.ml src/string_with_vars.ml src/super_context.ml src/top_closure.ml src/utils.ml src/utop.ml src/vfile_kind.ml src/watermarks.ml src/workspace.ml > boot-depends.txt
# /home/nicole/.opam/4.06.0+pr1506/bin/ocamlopt.opt -w -40 -o boot.exe unix.cmxa boot.ml
### stderr ###
# Fatal error: exception Invalid_argument("index out of bounds")



=-=- Error report -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
The following actions failed
  ∗  install jbuilder 1.0+beta16
The following changes have been performed
  ∗  install conf-m4   1
  ∗  install ocamlfind 1.7.3
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 6, 2018

Member

@ubsan: I believe this is not caused by the patch itself. During a few weeks, trunk broke jbuilder (see #1493). This is now fixed, but I believe the present PR was submitted from a broken-jbuilder trunk version, and it needs to be rebased on the current trunk (which correctly builds jbuilder).

Member

gasche commented Jan 6, 2018

@ubsan: I believe this is not caused by the patch itself. During a few weeks, trunk broke jbuilder (see #1493). This is now fixed, but I believe the present PR was submitted from a broken-jbuilder trunk version, and it needs to be rebased on the current trunk (which correctly builds jbuilder).

@ubsan

This comment has been minimized.

Show comment
Hide comment
@ubsan

ubsan Jan 6, 2018

@gasche ah, okay :) thanks!

on another thing - the paper doesn't support local opens, apparently, but I'd really love local opens to support this as well, for purposes similar to typeclasses.

module type Print = sig
  type t
  val print: t -> unit
end
module Print_int: Print with type t = int = struct
  ...
end
module Print_list(P: Print): Print with type t = P.t list = struct
  ...
end

let print_list_of_int lst = Print_list(Print_int).(print lst)

ubsan commented Jan 6, 2018

@gasche ah, okay :) thanks!

on another thing - the paper doesn't support local opens, apparently, but I'd really love local opens to support this as well, for purposes similar to typeclasses.

module type Print = sig
  type t
  val print: t -> unit
end
module Print_int: Print with type t = int = struct
  ...
end
module Print_list(P: Print): Print with type t = P.t list = struct
  ...
end

let print_list_of_int lst = Print_list(Print_int).(print lst)
@objmagic

This comment has been minimized.

Show comment
Hide comment
@objmagic

objmagic Jan 8, 2018

Contributor

@ubsan Thanks for your comment!

Extension on local open introduces grammar ambiguity. I didn't look closely or try to remove ambiguities because at first I thought such use case is rare. However, one reviewer and you mentioned it would be good to add support on local open as well. I can definitely look into this again.

Contributor

objmagic commented Jan 8, 2018

@ubsan Thanks for your comment!

Extension on local open introduces grammar ambiguity. I didn't look closely or try to remove ambiguities because at first I thought such use case is rare. However, one reviewer and you mentioned it would be good to add support on local open as well. I can definitely look into this again.

@ubsan

This comment has been minimized.

Show comment
Hide comment
@ubsan

ubsan commented Jan 8, 2018

@objmagic thanks <3

Show outdated Hide outdated parsing/ast_invariants.ml Outdated
Show outdated Hide outdated typing/typedtree.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
Show outdated Hide outdated typing/typemod.ml Outdated
@objmagic

This comment has been minimized.

Show comment
Hide comment
@objmagic
Contributor

objmagic commented Apr 8, 2018

@objmagic

This comment has been minimized.

Show comment
Hide comment
@objmagic

objmagic Apr 8, 2018

Contributor

Another problem that we might fix:

The following program is rejected with

Error: The module identifier M#39 cannot be eliminated from val x : M#39.t

but identifier does not really escape

module M : sig end = struct open struct type t = A end let x = A end
Contributor

objmagic commented Apr 8, 2018

Another problem that we might fix:

The following program is rejected with

Error: The module identifier M#39 cannot be eliminated from val x : M#39.t

but identifier does not really escape

module M : sig end = struct open struct type t = A end let x = A end
@objmagic

This comment has been minimized.

Show comment
Hide comment
@objmagic

objmagic Apr 8, 2018

Contributor

@damiendoligez

  1. I am happy to see high-priority label is put on this PR. I'd like to know how this decision was made. Was there any discussion or decision being made during developer meeting?
  2. Is it possible to consider this patch to be included in 4.07 (adding into "consider-for-4.07" milestone)? What is the time frame for 4.07?
Contributor

objmagic commented Apr 8, 2018

@damiendoligez

  1. I am happy to see high-priority label is put on this PR. I'd like to know how this decision was made. Was there any discussion or decision being made during developer meeting?
  2. Is it possible to consider this patch to be included in 4.07 (adding into "consider-for-4.07" milestone)? What is the time frame for 4.07?
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Apr 9, 2018

Member

I'm not Damien, but let me give an attempt at answering these questions.

  1. The "high-priority" label is something we've been trying to adopt internally to direct reviewing resources when we don't have enough reviewers to review every PR. It's fairly new, and the placement has been just Damien's judgment so far. Besides, this is unrelated to the label, but yes the PR was discussed at a developer meeting last November, and there was a wide consensus that the feature is nice (and nicely explained) and that we want it in, once the implementation is satisfactory.

  2. The time frame for 4.07 is "very soon", given that we use time-based release schedules: in theory we are supposed to release in May, which means that the feature freeze is imminent -- but a date has not been fixed, Damien will decide. In particular, I don't think we can give any guarantee that the PR will end in 4.07: it is in theory possible, if Alain (who has done the bulk of the reviewing work, with Octachron) decides that it is mergeable, but to me it seems fairly unlikely given the release calendar. On the other hand, it does seem likely that the PR will be merged in trunk at some point -- possibly only on track for 4.08.

Apologies for not having helped much with this PR myself; I've been very busy with other things, and the type-checker is really not my expertise area.

Member

gasche commented Apr 9, 2018

I'm not Damien, but let me give an attempt at answering these questions.

  1. The "high-priority" label is something we've been trying to adopt internally to direct reviewing resources when we don't have enough reviewers to review every PR. It's fairly new, and the placement has been just Damien's judgment so far. Besides, this is unrelated to the label, but yes the PR was discussed at a developer meeting last November, and there was a wide consensus that the feature is nice (and nicely explained) and that we want it in, once the implementation is satisfactory.

  2. The time frame for 4.07 is "very soon", given that we use time-based release schedules: in theory we are supposed to release in May, which means that the feature freeze is imminent -- but a date has not been fixed, Damien will decide. In particular, I don't think we can give any guarantee that the PR will end in 4.07: it is in theory possible, if Alain (who has done the bulk of the reviewing work, with Octachron) decides that it is mergeable, but to me it seems fairly unlikely given the release calendar. On the other hand, it does seem likely that the PR will be merged in trunk at some point -- possibly only on track for 4.08.

Apologies for not having helped much with this PR myself; I've been very busy with other things, and the type-checker is really not my expertise area.

Pmod_functor _ | Pmod_extension _ ->
enter_struct ();
let ident = gen_mod_ident () in
let tme = !type_module_fwd env me in

This comment has been minimized.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

type_module normally rejects Pmod_unpack within a functor body (unless the packaged module declares no type). This is necessary for safety reason, and I'm concerned that this call to type_module_fwd does not pass the funct_body flag.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

type_module normally rejects Pmod_unpack within a functor body (unless the packaged module declares no type). This is necessary for safety reason, and I'm concerned that this call to type_module_fwd does not pass the funct_body flag.

This comment has been minimized.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

Perhaps this is safe, because the types from the opened module cannot escape to the functor's body type itself.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

Perhaps this is safe, because the types from the opened module cannot escape to the functor's body type itself.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Apr 9, 2018

Contributor

module M : sig end = struct open struct type t = A end let x = A end

I don't think this should be accepted (there is no correctly scoped module type for the structure sub-term).

Contributor

alainfrisch commented Apr 9, 2018

module M : sig end = struct open struct type t = A end let x = A end

I don't think this should be accepted (there is no correctly scoped module type for the structure sub-term).

let popen = Pexp_open (ovf, {me with pmod_desc = pmod}, e) in
let pexp_desc = Pexp_letmodule ({txt=name;loc}, me,
{sexp with pexp_desc=popen}) in
let sexp = {pexp_desc; pexp_loc=loc;

This comment has been minimized.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

Couldn't we deal more directly with local open on non-identifier expressions? The type-checking rule for Pexp_letmodule is not very complex.

Note: in the current version, sexp.pexp_attributes are "duplicated" on the Pexp_open and Pexp_letmodule nodes.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

Couldn't we deal more directly with local open on non-identifier expressions? The type-checking rule for Pexp_letmodule is not very complex.

Note: in the current version, sexp.pexp_attributes are "duplicated" on the Pexp_open and Pexp_letmodule nodes.

Longident.t loc -> Path.t * Env.t)
ref
Parsetree.module_expr ->
(Ident.t * Types.module_declaration * Env.t) option *

This comment has been minimized.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

I'd drop the first component of the 3-tuple returned value. It is always ignored (except when type-checking Ppat_open, where is is asserted that it must be None).

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

I'd drop the first component of the 3-tuple returned value. It is always ignored (except when type-checking Ppat_open, where is is asserted that it must be None).

@@ -346,7 +346,7 @@ and expression_desc =
(module ME : S) is represented as
Pexp_constraint(Pexp_pack, Ptyp_package S) *)
| Pexp_open of override_flag * Longident.t loc * expression
| Pexp_open of override_flag * module_expr * expression

This comment has been minimized.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

Would it make sense to use open_description here (and also for Pcty_open, Pcl_open)?

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

Would it make sense to use open_description here (and also for Pcty_open, Pcl_open)?

Show outdated Hide outdated typing/typemod.mli Outdated
Show outdated Hide outdated typing/typecore.ml Outdated
let open_struct_level = ref 0
let in_nested_struct () = !open_struct_level <> 0
let enter_struct () = incr open_struct_level

This comment has been minimized.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

I'd switch that to a Boolean, and use Misc.protect_refs to set it to true for the call to !type_module_fwd env me in type_open_ (instead of enter_struct/leave_struct).

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

I'd switch that to a Boolean, and use Misc.protect_refs to set it to true for the call to !type_module_fwd env me in type_open_ (instead of enter_struct/leave_struct).

Includemod.compunit initial_env ~mark:Includemod.Mark_positive
sourcefile sg intf_file dclsig
in
Misc.protect_refs [R(open_struct_level, 0); R(mod_ident_counter, 0)]

This comment has been minimized.

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

The same should probably be done in Compile.interface and Optcompile.interface, no?

@alainfrisch

alainfrisch Apr 9, 2018

Contributor

The same should probably be done in Compile.interface and Optcompile.interface, no?

@objmagic

This comment has been minimized.

Show comment
Hide comment
@objmagic

objmagic Apr 10, 2018

Contributor

@gasche

Thanks for your clarification.

there was a wide consensus that the feature is nice (and nicely explained) and that we want it in

I am glad to hear that!

Also, just my 2 cents here: I can understand maintainer are busy and reviewing resources are scarce, but I think maintainers should communicate a bit more with individual contributors (people outside caml-devel).

It certainly would be helpful to me to know internal meeting has reached some conclusion. I can personally reach out to yallop who I guess may attend the meeting, but this might not be the case for every one. Ping you or Damien directly on GitHub looks awkward ("hey have internal meeting at INRIA reached some conclusion on my patch?"). Just put up a short update on PR at your convenience should be fine. Similarly, I know we have time-based release schedules now but it would be great if someone can give a head-up once time frame is decided (like when release branch will be cut).

Contributor

objmagic commented Apr 10, 2018

@gasche

Thanks for your clarification.

there was a wide consensus that the feature is nice (and nicely explained) and that we want it in

I am glad to hear that!

Also, just my 2 cents here: I can understand maintainer are busy and reviewing resources are scarce, but I think maintainers should communicate a bit more with individual contributors (people outside caml-devel).

It certainly would be helpful to me to know internal meeting has reached some conclusion. I can personally reach out to yallop who I guess may attend the meeting, but this might not be the case for every one. Ping you or Damien directly on GitHub looks awkward ("hey have internal meeting at INRIA reached some conclusion on my patch?"). Just put up a short update on PR at your convenience should be fine. Similarly, I know we have time-based release schedules now but it would be great if someone can give a head-up once time frame is decided (like when release branch will be cut).

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Apr 10, 2018

Member

I agree; usually I try to give feedback on affected PRs after meetings take place, but for the last one I forgot. Regarding the release calendar, a beta version of 4.07 should be announced shortly -- we are trying to move out of announcing a set freeze date in advance because that generates stress for everyone.

Member

gasche commented Apr 10, 2018

I agree; usually I try to give feedback on affected PRs after meetings take place, but for the last one I forgot. Regarding the release calendar, a beta version of 4.07 should be announced shortly -- we are trying to move out of announcing a set freeze date in advance because that generates stress for everyone.

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Apr 20, 2018

Contributor

I've made these points elsewhere, but I would just like to give my 2 cents on this proposal. Personally, I'm not in favour of extending open to arbitrary expressions. My preference is instead to allow any structure or signature item to be annotated as private and thus not included in the resulting structure or signature. I intend to implement such a feature and propose it for 4.08.

The reasons I prefer the private approach are:

  1. For creating a local definition, annotating that definition as private is a much more direct representation of what the user is trying to accomplish.
  2. private annotations work in signatures, whereas open struct ... end has serious issues there.
  3. The problem with signatures comes from a fundamental difference between include and open -- include works on modules in structures and module types in signautres, whilst open works on modules in both structures and signatures. This illustrates that open was conceived and designed to be an static operation on environments, not a private version of include -- and I think changing it to partially behave as such creates an inconsistency that can confuse people.

Having private annotated definitions covers all the use cases mentioned in the paper except for the "Restricted open" case. I think that open should be extended to support restricted open, however doing so requires some notion of "transparent ascription" otherwise it does not behave as people expect.

All that said, I'm not so opposed as to campaign against it. If were entirely up to me I wouldn't include it, but if I'm overruled I don't think it will be a huge problem. OCaml has plenty of small inconsistencies already and one more won't do much harm. Either way I think that the support for private annotated items will be necessary in order to support aliases in signatures, so I'll try to get a PR ready for that some time in the next few months.

Contributor

lpw25 commented Apr 20, 2018

I've made these points elsewhere, but I would just like to give my 2 cents on this proposal. Personally, I'm not in favour of extending open to arbitrary expressions. My preference is instead to allow any structure or signature item to be annotated as private and thus not included in the resulting structure or signature. I intend to implement such a feature and propose it for 4.08.

The reasons I prefer the private approach are:

  1. For creating a local definition, annotating that definition as private is a much more direct representation of what the user is trying to accomplish.
  2. private annotations work in signatures, whereas open struct ... end has serious issues there.
  3. The problem with signatures comes from a fundamental difference between include and open -- include works on modules in structures and module types in signautres, whilst open works on modules in both structures and signatures. This illustrates that open was conceived and designed to be an static operation on environments, not a private version of include -- and I think changing it to partially behave as such creates an inconsistency that can confuse people.

Having private annotated definitions covers all the use cases mentioned in the paper except for the "Restricted open" case. I think that open should be extended to support restricted open, however doing so requires some notion of "transparent ascription" otherwise it does not behave as people expect.

All that said, I'm not so opposed as to campaign against it. If were entirely up to me I wouldn't include it, but if I'm overruled I don't think it will be a huge problem. OCaml has plenty of small inconsistencies already and one more won't do much harm. Either way I think that the support for private annotated items will be necessary in order to support aliases in signatures, so I'll try to get a PR ready for that some time in the next few months.

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Apr 20, 2018

Contributor

so I'll try to get a PR ready for that some time in the next few months.

Unless anyone else wants to have a go -- don't take my saying I will do it as a prohibition on other people proposing their own implementations.

Contributor

lpw25 commented Apr 20, 2018

so I'll try to get a PR ready for that some time in the next few months.

Unless anyone else wants to have a go -- don't take my saying I will do it as a prohibition on other people proposing their own implementations.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Apr 20, 2018

Member

I think it's not really an ideal time to restart this whole discussion, after @objmagic was told that the feature was discussed and approved at the developers' meeting, and subsequently spent significant effort on implementation. While it's always fine to raise new technical issues, the objections seem to be old ones, and rather "soft" than technical, which makes it difficult to reach a definite conclusion.

Currently, since open only operates on values, it is compatible with more than one mental model (e.g. as a non-exporting version of include, or as a static operation on environments). The fact that extending it means committing to one of those models (and not the one you prefer) isn't a strong argument in itself. And while people could conceivably be confused by pretty much any change, I haven't encountered anyone who's at all confused by what's proposed here.

Member

yallop commented Apr 20, 2018

I think it's not really an ideal time to restart this whole discussion, after @objmagic was told that the feature was discussed and approved at the developers' meeting, and subsequently spent significant effort on implementation. While it's always fine to raise new technical issues, the objections seem to be old ones, and rather "soft" than technical, which makes it difficult to reach a definite conclusion.

Currently, since open only operates on values, it is compatible with more than one mental model (e.g. as a non-exporting version of include, or as a static operation on environments). The fact that extending it means committing to one of those models (and not the one you prefer) isn't a strong argument in itself. And while people could conceivably be confused by pretty much any change, I haven't encountered anyone who's at all confused by what's proposed here.

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Apr 20, 2018

Contributor

after @objmagic was told that the feature was discussed and approved at the developers' meeting

That only happened a couple of weeks ago, and I don't think Gabriel accurately reflected the discussion at the meeting, where I believe I already raised these points. I have also raised them with @objmagic in person in the past.

If everyone else is in favour, then as I said, I'm not going to try and stop it, but I thought that the public discussion on this feature should include my objections. (This was at least partly motivated by a conscious attempt to try to move more of these discussions into the public domain).

Currently, since open only operates on values, it is compatible with more than one mental model

The problem with signatures indicates that it is not compatible with both mental models. If open were a non-exporting version of include then it would take a signature when used in signatures. This is the inconsistency that I think will be confusing, it also means that this feature is only a partial solution to the problem of local definitions.

Contributor

lpw25 commented Apr 20, 2018

after @objmagic was told that the feature was discussed and approved at the developers' meeting

That only happened a couple of weeks ago, and I don't think Gabriel accurately reflected the discussion at the meeting, where I believe I already raised these points. I have also raised them with @objmagic in person in the past.

If everyone else is in favour, then as I said, I'm not going to try and stop it, but I thought that the public discussion on this feature should include my objections. (This was at least partly motivated by a conscious attempt to try to move more of these discussions into the public domain).

Currently, since open only operates on values, it is compatible with more than one mental model

The problem with signatures indicates that it is not compatible with both mental models. If open were a non-exporting version of include then it would take a signature when used in signatures. This is the inconsistency that I think will be confusing, it also means that this feature is only a partial solution to the problem of local definitions.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Apr 20, 2018

Member

after @objmagic was told that the feature was discussed and approved at the developers' meeting

That only happened a couple of weeks ago

No, I told him the day of the meeting (November last year).

I thought that the public discussion on this feature should include my objections. (This was at least partly motivated by a conscious attempt to try to move more of these discussions into the public domain).

I agree that having the objections & discussion publicly available is a good idea. My only real objection is to the timing.

Member

yallop commented Apr 20, 2018

after @objmagic was told that the feature was discussed and approved at the developers' meeting

That only happened a couple of weeks ago

No, I told him the day of the meeting (November last year).

I thought that the public discussion on this feature should include my objections. (This was at least partly motivated by a conscious attempt to try to move more of these discussions into the public domain).

I agree that having the objections & discussion publicly available is a good idea. My only real objection is to the timing.

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Apr 20, 2018

Contributor

My only real objection is to the timing.

Understandable, but to be fair it's not like I'm making new design objections at the last minute, I'm just reiterating publicly design objections that I've already made privately and consistently to all parties involved.

Contributor

lpw25 commented Apr 20, 2018

My only real objection is to the timing.

Understandable, but to be fair it's not like I'm making new design objections at the last minute, I'm just reiterating publicly design objections that I've already made privately and consistently to all parties involved.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Apr 20, 2018

Member

I'm just reiterating publicly design objections that I've already made privately and consistently to all parties involved.

Nobody objects to that, of course. But you're also planning to implement and propose a competing design, which is a reasonable thing to do before consensus is reached, but not so much afterwards, unless new issues come to light.

Member

yallop commented Apr 20, 2018

I'm just reiterating publicly design objections that I've already made privately and consistently to all parties involved.

Nobody objects to that, of course. But you're also planning to implement and propose a competing design, which is a reasonable thing to do before consensus is reached, but not so much afterwards, unless new issues come to light.

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Apr 20, 2018

Contributor

But you're also planning to implement and propose a competing design, which is a reasonable thing to do before consensus is reached, but not so much afterwards, unless new issues come to light.

Well, as I said, I'm going to do that even if this feature is already released because it addresses important use cases not handled by this feature. I don't think the overlap is likely to cause users any confusion because, whilst they can be used to solve the same problems, the mechanisms they use are very different from a user's perspective.

Contributor

lpw25 commented Apr 20, 2018

But you're also planning to implement and propose a competing design, which is a reasonable thing to do before consensus is reached, but not so much afterwards, unless new issues come to light.

Well, as I said, I'm going to do that even if this feature is already released because it addresses important use cases not handled by this feature. I don't think the overlap is likely to cause users any confusion because, whilst they can be used to solve the same problems, the mechanisms they use are very different from a user's perspective.

@bluddy

This comment has been minimized.

Show comment
Hide comment
@bluddy

bluddy Apr 20, 2018

Thank you for posting your view, @lpw25. I believe at least some of the support this feature has received (including my own) stems from the fact that we had no idea you had reservations about it, which only stresses how important it is to have a transparent process.

@lpw25 do you think this can get us into similar issues as the recent backwards-incompatible module type change?

And in terms of a meta-discussion:
a. I do appreciate all the effort @objmagic put into this specific PR.
b. I really think we should have either a recording or the notes of developer meetings posted online.
c. A PR such as this should probably be posted as an RFC before implementation begins, so that this discussion could take place publicly without the bias of sunk cost. Only some features seem to be suggested this way, probably because there's no explicit procedure specifying it.

Should we start a discussion on Discourse about procedures for these things? Other languages seem to have very specific procedures in place for RFCs and such.

bluddy commented Apr 20, 2018

Thank you for posting your view, @lpw25. I believe at least some of the support this feature has received (including my own) stems from the fact that we had no idea you had reservations about it, which only stresses how important it is to have a transparent process.

@lpw25 do you think this can get us into similar issues as the recent backwards-incompatible module type change?

And in terms of a meta-discussion:
a. I do appreciate all the effort @objmagic put into this specific PR.
b. I really think we should have either a recording or the notes of developer meetings posted online.
c. A PR such as this should probably be posted as an RFC before implementation begins, so that this discussion could take place publicly without the bias of sunk cost. Only some features seem to be suggested this way, probably because there's no explicit procedure specifying it.

Should we start a discussion on Discourse about procedures for these things? Other languages seem to have very specific procedures in place for RFCs and such.

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Apr 20, 2018

Contributor

do you think this can get us into similar issues as the recent backwards-incompatible module type change?

My objections are purely on the design side. There are no technical issues with this change, nor any backwards compatibility concerns.

And in terms of a meta-discussion:

PRs are not the place for meta-discussion. Please use one of the other forums (e.g. discuss, caml-list) for such discussions.

Contributor

lpw25 commented Apr 20, 2018

do you think this can get us into similar issues as the recent backwards-incompatible module type change?

My objections are purely on the design side. There are no technical issues with this change, nor any backwards compatibility concerns.

And in terms of a meta-discussion:

PRs are not the place for meta-discussion. Please use one of the other forums (e.g. discuss, caml-list) for such discussions.

@@ -107,11 +107,15 @@ let type_module =
ref ((fun _env _md -> assert false) :
Env.t -> Parsetree.module_expr -> Typedtree.module_expr)
let gen_mod_ident : (Parsetree.module_expr -> Ident.t option) ref = ref (fun _ -> assert false)

This comment has been minimized.

@gasche

gasche Apr 21, 2018

Member

Could we have a comment on top of this forward declaration, with the declaration site, as for the others above and below?

@gasche

gasche Apr 21, 2018

Member

Could we have a comment on top of this forward declaration, with the declaration site, as for the others above and below?

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Apr 23, 2018

Contributor

I don't think the eventually of another way to do something similar should stop us from merging this PR. What it does is clean from a semantical point of view.

A note on private annotations:

  • This was part of the object system a long time ago, but disappeared in 2.00.
  • At the time, @xavierleroy was strongly opposed to this kind of "ad hoc" scoping, but he may have changed since.
  • Since 2.00, private in classes has a different meaning.

For these reasons, I'm not stopping you from making a proposal in that direction (actually at least then I thought it was handy), but there is no guarantee that it would be accepted.

Contributor

garrigue commented Apr 23, 2018

I don't think the eventually of another way to do something similar should stop us from merging this PR. What it does is clean from a semantical point of view.

A note on private annotations:

  • This was part of the object system a long time ago, but disappeared in 2.00.
  • At the time, @xavierleroy was strongly opposed to this kind of "ad hoc" scoping, but he may have changed since.
  • Since 2.00, private in classes has a different meaning.

For these reasons, I'm not stopping you from making a proposal in that direction (actually at least then I thought it was handy), but there is no guarantee that it would be accepted.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Apr 23, 2018

Contributor

Just in case somebody wants to work on the "private" approach: I did some quick experiment with them on #682 and #683.

Contributor

alainfrisch commented Apr 23, 2018

Just in case somebody wants to work on the "private" approach: I did some quick experiment with them on #682 and #683.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment