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

Extending open #2147

Merged
merged 8 commits into from Nov 26, 2018

Conversation

Projects
None yet
5 participants
@trefis
Copy link
Contributor

commented Nov 13, 2018

This is a refresh of #1506 which includes the restrictions that were discussed during the last developer meeting and since then on github (cf. #1506 (comment)).

More precisely:

  • open in signatures is changed to accept extended module paths
  • open in structures, as well as let open in expressions, are extended to accept any module expression
  • let open in classes and class types is unchanged: before extending it we would need to add support for let module, which is complicated. Cf. the discussion on MPR#6271
  • the short syntaxes for local module opens (e.g. M.(expr), M.(pat), M.[expr], etc.) are still restricted to module paths.

Things to note:

  • I had to update Rec_check, but I didn't spend to much time on it, and it is untested. So it's likely to be wrong. It'd be nice if @gasche or @yallop could have a look.
  • I imported the tests from #1506 as they were, and updated the output in a separate commit, so it's easier to spot the differences between the two PRs
  • the manual still needs to be updated

@trefis trefis force-pushed the trefis:extended-open branch from 243d6bd to 3d4f72a Nov 13, 2018

@yallop

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

Thanks, @trefis. I plan to take a look at this soon (but not before next week).

@lpw25
Copy link
Contributor

left a comment

I've looked over the translation parts so far. Looks good but there are a couple of bugs and some places where we could avoid allocating in the open struct ... end case.

Show resolved Hide resolved bytecomp/translmod.ml
Show resolved Hide resolved bytecomp/translmod.ml Outdated
Show resolved Hide resolved bytecomp/translmod.ml Outdated
Show resolved Hide resolved bytecomp/translmod.ml
Show resolved Hide resolved bytecomp/translcore.ml Outdated

@trefis trefis force-pushed the trefis:extended-open branch 2 times, most recently from 268ae41 to 3d4f72a Nov 16, 2018

Show resolved Hide resolved typing/types.ml
Show resolved Hide resolved typing/types.ml Outdated
@lpw25
Copy link
Contributor

left a comment

There are a couple of bugs, and a couple of bits that should be tidied up. Once they're fixed this should be good to go.

Show resolved Hide resolved typing/typedtree.ml Outdated
Show resolved Hide resolved bytecomp/translmod.ml Outdated
Show resolved Hide resolved bytecomp/translmod.ml Outdated
Show resolved Hide resolved bytecomp/translmod.ml Outdated
Show resolved Hide resolved bytecomp/translmod.ml Outdated
Show resolved Hide resolved bytecomp/translmod.ml Outdated
Show resolved Hide resolved bytecomp/translmod.ml Outdated
Show resolved Hide resolved typing/rec_check.ml Outdated
Show resolved Hide resolved typing/typemod.ml Outdated
@Octachron

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

On the documentation front, I have a subpr here that adds some basic explanations and examples for the new features, if people wish to discuss the documentation separately.

@trefis trefis force-pushed the trefis:extended-open branch from 3440ed0 to 12ad467 Nov 23, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

I have rebased, answered all of @lpw25's comments, and included @Octachron's changes to the manual.

Show resolved Hide resolved manual/manual/refman/exten.etex Outdated

@trefis trefis force-pushed the trefis:extended-open branch from 12ad467 to 9857699 Nov 23, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

I pushed one last commit which updates the manual as discussed.
I think this is now ready to go.

@yallop

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

I had to update Rec_check, but I didn't spend to much time on it, and it is untested. So it's likely to be wrong. It'd be nice if @gasche or @yallop could have a look.

Is the following intentional?

# let rec x = let module M = struct let y = lazy x end in ();;
val x : unit = ()
# let rec x = let module M = struct include struct let y = lazy x end end in ();;
val x : unit = ()
# let rec x = let module M = struct module M = struct let y = lazy x end end in ();;
val x : unit = ()
# let rec x = let module M = struct open struct let y = lazy x end end in ();;
Line 1, characters 12-74:
1 | let rec x = let module M = struct open struct let y = lazy x end end in ();;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This kind of expression is not allowed as right-hand side of `let rec'

@trefis trefis force-pushed the trefis:extended-open branch from e6a8a82 to 1bbac36 Nov 23, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

I guess it is:

# let rec x = let module M = struct include (struct let y = lazy x end : sig end) end in ();;                                                                       
Line 1, characters 12-89:
1 | let rec x = let module M = struct include (struct let y = lazy x end : sig end) end in ();;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This kind of expression is not allowed as right-hand side of `let rec'

I guess you might want to make modexp a bit cleverer when looking at Tmod_constraint.

Show resolved Hide resolved manual/manual/refman/exten.etex Outdated
@lpw25

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2018

From the CI, it looks like ocamlnat needs updating.

else
type_args [] [] ty (instance ty) ty sargs []
let ty = funct.exp_type in
if ignore_labels then

This comment has been minimized.

Copy link
@objmagic

objmagic Nov 26, 2018

Contributor

just curious, do we have an official code style for ocaml compiler source code now? are we using ocamlformat?

This comment has been minimized.

Copy link
@trefis

trefis Nov 26, 2018

Author Contributor

No.

@@ -366,6 +365,8 @@ module MakeIterator(Iter : IteratorArgument) : sig
()
| Texp_extension_constructor _ ->
()
| Texp_open (_od, e) ->
iter_expression e

This comment has been minimized.

Copy link
@objmagic

objmagic Nov 26, 2018

Contributor

should we iter _od here? also we should iter Tstr_open?

This comment has been minimized.

Copy link
@trefis

trefis Nov 26, 2018

Author Contributor

I updated this and Tstr_open to do the same thing as with includes.

@@ -403,6 +403,7 @@ module MakeMap(Map : MapArgument) = struct
Texp_unreachable
| Texp_extension_constructor _ as e ->
e
| Texp_open (od, exp) -> Texp_open (od, map_expression exp)

This comment has been minimized.

Copy link
@objmagic

objmagic Nov 26, 2018

Contributor

should we map over module expression of od here? Also Tstr_open

This comment has been minimized.

Copy link
@trefis

trefis Nov 26, 2018

Author Contributor

Updated.

match m.mod_desc with
Tmod_ident _ -> Alias
| Tmod_constraint (m,_,_,_) -> pure_module m
| _ -> Strict

This comment has been minimized.

Copy link
@objmagic

objmagic Nov 26, 2018

Contributor

can we list out all other cases here explicitly?

This comment has been minimized.

Copy link
@trefis

trefis Nov 26, 2018

Author Contributor

I just moved this code from Translmod to Translcore. I didn't actually write it. So I'd rather not update it.

(** Extracts the list of "value" identifiers bound by a signature.
"Value" identifiers are identifiers for signature components that
correspond to a run-time value: values, extensions, modules, classes.
Note: manifest primitives do not correspond to a run-time value! *)

This comment has been minimized.

Copy link
@objmagic

objmagic Nov 26, 2018

Contributor

I know this is copied over from another file, but I'd like to know what "manifest primitives" means. Thanks.

This comment has been minimized.

Copy link
@trefis

trefis Nov 26, 2018

Author Contributor

I think reading this as "externals" is acceptable.

Show resolved Hide resolved bytecomp/translmod.ml
\begin{caml_example}{verbatim}
module M = struct
open struct type 'a t = 'a option = None | Some of 'a end
let x : int t = Some 1

This comment has been minimized.

Copy link
@objmagic

objmagic Nov 26, 2018

Contributor

add one whitespace before let

This comment has been minimized.

Copy link
@trefis

trefis Nov 26, 2018

Author Contributor

Done.

@trefis trefis force-pushed the trefis:extended-open branch from 1bbac36 to 33ec63e Nov 26, 2018

@lpw25

lpw25 approved these changes Nov 26, 2018

Copy link
Contributor

left a comment

Good to merge once CI passes.

@trefis trefis merged commit 4e4c0a3 into ocaml:trunk Nov 26, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Thanks, @trefis!

@objmagic

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

Thank you for the work @trefis!

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

There's a bug in this PR. is_nonexpansive in typecore.ml needs to be extended to handle Texp_open. We should also add a test for this case to the testsuite, and maybe not use a fragile match for that function.

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.