Support 'let open' in class and class type expressions by alainfrisch · Pull Request #1249 · ocaml/ocaml

alainfrisch · 2017-07-18T13:31:13Z

Add support for:

class c =
  let open M in
  ...

class type ct =
  let open M in
  ...

References:

https://caml.inria.fr/mantis/view.php?id=6271
https://caml.inria.fr/mantis/view.php?id=7529

@garrigue I'm not entirely satisfied with the type-checking of open in classes, where the open is performed on both environments val_env/met_env (with a hack to share the usage marker). Do you see a better way?

[EDIT: now ready for review]

trefis · 2017-07-18T14:45:47Z

typing/typedtree.mli

      class_expr * class_type option * string list * string list * Concr.t
-    (* Visible instance variables, methods and concretes methods *)
+  (* Visible instance variables, methods and concretes methods *)
+  | Tcl_open of override_flag * Path.t * Longident.t loc * Env.t * class_expr


The Env.t parameter never appears to be read anywhere.

I think it's the same as for the Texp_open case. I assumed this could be used by external tools parsing the .cmt files.

Fair point, thanks for explaining :)

trefis · 2017-07-18T14:49:05Z

typing/typeclass.ml

+      rc {cl_desc = Tcl_open (ovf, path, lid, new_val_env, cl);
+          cl_loc = scl.pcl_loc;
+          cl_type = cl.cl_type;
+          cl_env = val_env;


You want to use new_val_env here.

I don't think so. We record here the original env used to type-check the class expression.

the implementation of Pcl_let a few lines above seems to disagree.

But it is also possible that it is the handling of Pcl_let which is "wrong". At least I agree it is inconsistent with the way we handle _env fields in other places (eg. expressions)

Indeed, but I think this is a bug. (Pcl_fun, for instance, stores the initial val_env.) This dates back from commit f209562. @garrigue : can you comment?

My understanding was that the environment here is just there so that we can properly interpret identifiers.
So any of the two environments is ok (the new one has just more information).
At least when it was introduced, the goal was not to be able to use this information to compile source code.
If we want to do that, we should define a more precise semantics.

My understanding was that the environment here is just there so that we can properly interpret identifiers.

Indeed, a quick look at translclass suggest that cl_env is mostly used on Tcl_ident node. There is also build_class_lets, but it deals with Tcl_let directly (and return the inner env explicitly). The remaining use is through Translobj.oo_wrap/Translobj.oo_add_class, which ends up defining the top_env in transl_class (and then passed to Env.diff). And here I'm a bit lost.

So any of the two environments is ok (the new one has just more information).
If we want to do that, we should define a more precise semantics.

I think the least confusing semantics would be to store the val_env "around" the class expression. This is similar to the semantics of exp_env, pat_env, mod_env. Do you agree?

At least when it was introduced, the goal was not to be able to use this information to compile source code.

I'm not sure what you are referring to, but in my understanding, this more is more about supporting external tools (documentation generators, IDE support, etc) so that they can do some lookups. It's not really about allowing to compile source code fragment (although this might be useful as well).

Anyway, do we agree that for Pcl_open, it's ok to keep the initial val_env (as for Pcl_fun)? The change to Pcl_let should be dealt with separately.

I agree that the code for Pcl_open is correct.

garrigue · 2017-07-19T08:32:59Z

The need to update both environments seems natural to me. It would be hard to give a semantics of open where you update only one of them...
As for the hack in Env.open_signature it has the advantage of making things explicit.
I actually find the implicit approach to usage warnings, using lots of side-effects, to be mind-boggling, and would prefer more of this.

trefis

Considering the previous discussion this looks OK to me.

(N.B. I don't have any opinion on the manual changes, as I don't know how the manual is organized)

Octachron · 2017-07-19T14:11:18Z

The manual changes look goods to me. I was thinking that using a local open inside of the examples in the class chapter of the tutorial may be nice but this feels very optional since the syntax is easily discoverable starting from the syntax for local open in expression.

alainfrisch · 2017-07-19T14:14:47Z

@garrigue If you agree with the PR, can you create a formal review? As per the new rule, we need a review from someone with commit rights to merge something that touches the compiler.

garrigue

The change is a simple one, and easily tested. As any syntactic change, there are many files involved, but there shouldn't be any negative impact on existing code anyway.

4.06 incompatibility was caused by changes to Typedtree in ocaml/ocaml#1118 and ocaml/ocaml#1249.

Support 'let open' in class and class type expressions.

a6a13c2

alainfrisch added the work-in-progress label Jul 18, 2017

trefis requested changes Jul 18, 2017

View reviewed changes

alainfrisch added 5 commits July 19, 2017 14:23

Adapt ocamlprof.

823b501

Adapt ocamldoc.

ee0337e

Add tests.

96d9082

Changelog.

59ef440

Manual.

78cbb8e

alainfrisch removed the work-in-progress label Jul 19, 2017

trefis approved these changes Jul 19, 2017

View reviewed changes

garrigue approved these changes Jul 20, 2017

View reviewed changes

alainfrisch merged commit fd47ba9 into trunk Jul 20, 2017

alainfrisch deleted the let_open_in_classes branch July 20, 2017 07:13

aantron mentioned this pull request Jul 22, 2017

4.06/trunk support ocaml-ppx/ocaml-migrate-parsetree#19

Closed

gasche added the caml-weekly-news label Aug 11, 2017

aantron added a commit to ocamllabs/odoc-dev that referenced this pull request Nov 22, 2017

Update opam file to allow installation on 4.06

24355d1

4.06 incompatibility was caused by changes to Typedtree in ocaml/ocaml#1118 and ocaml/ocaml#1249.

anmonteiro mentioned this pull request Jul 1, 2018

Upgrade Parser to 4.06 AST reasonml/reason#2036

Closed

3 tasks

This was referenced Mar 14, 2019

'let open' in class expressions #6271

Closed

Class definition syntax restrictions #7529

Closed

jonludlam pushed a commit to jonludlam/odoc-parser-cleaned that referenced this pull request Jul 1, 2021

Update opam file to allow installation on 4.06

49e0716

4.06 incompatibility was caused by changes to Typedtree in ocaml/ocaml#1118 and ocaml/ocaml#1249.

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

Announce Dune 3.8.{0,1,2} (ocaml#1249)

2c85b71

Conversation

alainfrisch commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garrigue commented Jul 19, 2017

Uh oh!

trefis left a comment

Choose a reason for hiding this comment

Uh oh!

Octachron commented Jul 19, 2017

Uh oh!

alainfrisch commented Jul 19, 2017

Uh oh!

garrigue left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alainfrisch commented Jul 18, 2017 •

edited

Loading