Support modules of non-legacy modes #3759

riaqn · 2025-03-27T17:24:09Z

This PR adds the support for modules of non-legacy modes. Functor parameter and body are still fixed to be legacy.

TODO:

Improve inclusion error messages
Add tests reflecting subtle code changes in type checking
Add tests showing functor application mode error message

The room for improvement shouldn't block preliminary review and discussion. In particular, changes to the existing tests seem correct, and can serve as a good starting point.

typing/typedtree.mli

typing/includecore.mli

lukemaurer · 2025-03-31T13:13:30Z

typing/includecore.mli

  *)

+(** Gives the modes suitable for the inclusion check of a child item. *)
+val child_modes: string ->
+  ?modalities:(Mode.Modality.Value.t * Mode.Modality.Value.t) ->


Comment on what modalities means?

It could still be clearer what modalities is saying: why are there two of them and what do they mean? (Too bad we don't have named tuples here)

testsuite/tests/typing-local/local.ml

stdlib/ephemeron.mli

goldfirere · 2025-04-03T17:01:48Z

testsuite/tests/typing-modes/val_modalities.ml

+    module M' = (M @ nonportable)
+    (* CR zqian: The following should fail but doesn't, due to A limitation
+       described in "inferred modalities" in [mode.ml] *)
+    let _ = portable_use M'.x (* [M] is nonportable and [x] doesn't have modality *)


This seems pretty bad. Is this a soundness bug?

It's not a soundness bug in the sense that we are just forgetting the explicit weakening.

goldfirere · 2025-04-29T16:15:03Z

testsuite/tests/parsetree/source_jane_street.ml

This PR should also include documentation for the new syntax. (Just making this comment here so that GitHub sees it as a Discussion that won't get lost.)

Probably not in this PR as it's not still stable, with various issues.

As discussed, we will add documentation in another PR.

stdlib/moreLabels.mli

testsuite/tests/typing-local/local.ml

lukemaurer · 2025-04-10T14:40:41Z

testsuite/tests/typing-local/local.ml

+3 |   (module M : S)
+              ^
+Error: Signature mismatch:
+       Got "regional" but expected "global".


I doubt users will understand what regional means, and local seems fine here even though it's a bit imprecise?

lukemaurer · 2025-04-10T14:52:26Z

testsuite/tests/typing-modes/md_modalities.ml

-       is not included in
-         val foo : 'a -> 'a @@ portable
-       The second is portable and the first is nonportable.
+       Got "nonportable" but expected "portable".


This is a bit sad. We don't need to drill down all the way to foo anymore, but the user should still find out that the module M' is expected to be portable but is nonportable.

lukemaurer · 2025-04-22T11:19:41Z

otherlibs/stdlib_alpha/effect.mli

@@ -123,7 +123,6 @@ module type S = sig
      with [t] to tie the knot on recursive operations. *)

  module Result : sig
-    @@ portable


This seems strange for this PR?

This is not needed, since there is @@ portable at the begining of this file.

Sure. I don't care enough to put my foot down here, but I think we should avoid this kind of unrelated cleanup on otherwise unaffected files.

lukemaurer · 2025-04-22T11:23:49Z

typing/includecore.mli

  *)

+(** Gives the modes suitable for the inclusion check of a child item. *)
+val child_modes: string ->
+  ?modalities:(Mode.Modality.Value.t * Mode.Modality.Value.t) ->


It could still be clearer what modalities is saying: why are there two of them and what do they mean? (Too bad we don't have named tuples here)

lukemaurer · 2025-04-22T16:59:41Z

typing/typemod.ml

+  | Sig_module (id, pres, md, rec_, vis) ->
+      let md_modalities =
+        md.md_modalities
+        |> Mode.Modality.Value.to_const_exn


Why do we expect to_const_exn to succeed here? I suppose a modality variable in a signature wouldn't make much sense? (There should be a comment here in any case)

include S @@ modalities, here S must be a constant modality (as opposed to inferred modalities). See maybe_infer_modalities in this file.

Can we rename the function apply_modalities_included_signature then to express this constraint, in addition to adding the comment here?

maybe apply_constant_modalities_sg

lukemaurer · 2025-04-22T17:17:12Z

typing/typemod.ml

-  match Mode.Modality.Value.Const.is_id modalities with
-  | true -> mty
-  | false -> apply_modalities_module_type env modalities mty
+let transl_modalities ~sig_modalities modalities =


This surprises me as a thing we want to do? This suggests we're treating sig_modalities as defaults rather than something onto which we want to compose other modalities. Oh, I guess they're the default modalities from the start of the signature? I guess then the overriding makes sense, but I think we should find a better name than sig_modalities

You are right that this is very bad. Ideally we want per-axis overriding: I.e., if default modality is portable, then later if we write nonportable, that will override. If later we write global, that will compose into global portable.

This should be fixed ASAP.

Oh heh good point. Still, calling the argument ~default_modalities seems like an improvement

lukemaurer · 2025-04-23T13:23:09Z

typing/typemod.ml

+              (* the module is added only for its types, so we give it the
+              strongest mode to avoid false mode errors. *)


As discussed, this could be a bit clearer. I would emphasize that we're in a signature here, so this newenv will only be used to typecheck signature items

riaqn · 2025-04-30T13:00:50Z

testsuite/tests/typing-modes/val_modalities.ml

+(* CR zqian: currently modules are checked bottom-up, which gives worse error
+  messages. The following example should point to [foo] instead. *)
+module M @ many = struct
+    let (foo @ once) () = ()
+end
+[%%expect{|
+Lines 1-3, characters 18-3:
+1 | ..................struct
+2 |     let (foo @ once) () = ()
+3 | end
+Error: This is "once", but expected to be "many".


just to highlight this issue.

goldfirere

Took a quick look at the tests, just to understand the design better. Looks good to me, just a few comments as I was scrolling through.

goldfirere · 2025-04-30T11:42:57Z

testsuite/tests/parsetree/source_jane_street.ml

-1 | module M' = (M @ portable)
-                     ^^^^^^^^
-Error: Mode annotations on modules are not supported yet.
+module M' = M


I would expect a portable modality to be printed here, I think

Even though the module is portable, its "exposed mode" could be nonportable, and since printtyp.ml zaps to legacy, it would be nonportable.

(Yes, we should let printtyp.ml zap to floor, in align with module type of and mli-less files).

goldfirere · 2025-04-30T11:43:34Z

testsuite/tests/parsetree/source_jane_street.ml

-1 | module M @ portable = struct end
-               ^^^^^^^^
-Error: Mode annotations on modules are not supported yet.
+module M : sig end


I think I would expect to see a portable modality printed here. Maybe mode-crossing eliminates it?

goldfirere · 2025-04-30T11:44:55Z

testsuite/tests/parsetree/source_jane_street.ml

-                    ^^^^^^^^
-Error: Mode annotations on modules are not supported yet.
+File "_none_", line 1:
+Error: Recursive modules require an explicit module type.


I think this test would be more informative if we put on the type signatures to fix this error, so that the rest of the syntax gets checked.

Wait is the syntax check interrupted by type error? Oh wow.

In the long run this file should just be about syntax... the type errors are already checked by other files...

goldfirere · 2025-04-30T11:56:08Z

testsuite/tests/typing-local/local.ml

+(* packing is allocation, and we can't construct a global module using local values *)
+let bar (local_ m) =
+  let module M = (val m : S) in
+  (module M : S)


I think this restriction (that an expression for packing cannot contain any local values) should get a brief mention in the reference page for stack allocation. (Why document this small point? Because I think the time cost of writing the doc -- 5-10 minutes -- is less than my estimate of the expected value of the amount of time lost when someone tries to do this and fails, even taking into account my estimate of the percentage of people who actually read the docs.)

testsuite/tests/typing-modes/md_modalities.ml

goldfirere · 2025-04-30T12:09:33Z

testsuite/tests/typing-modes/module.ml

@@ -160,7 +148,7 @@ module type S = sig
 end

 module M : S = struct
-    let foo = fun x -> x
+    let foo @ nonportable = fun x -> x


Why is this change necessary?

Urgh. I've figured it out. This is necessary to make M nonportable so that a test 70 lines later still tests the right thing. Ok, I suppose. But it would be much better if we included the portability of modules in printing, and then this would be more obvious. Actually I'm a little surprised that printing doesn't zap the modes. Maybe at least leave a comment on this line saying what the motivation for the annotation is? As written, it looks like perhaps the nonportable is necessary in order for M to type-check against S.

Yes, the printing still zaps to id. Now with #3922 , I think the default should be zapping to floor.

testsuite/tests/typing-modes/module.ml

riaqn · 2025-05-01T15:22:08Z

otherlibs/stdlib_alpha/effect.mli

@@ -123,7 +123,6 @@ module type S = sig
      with [t] to tie the knot on recursive operations. *)

  module Result : sig
-    @@ portable


This is not needed, since there is @@ portable at the begining of this file.

stdlib/sys.mli

riaqn · 2025-05-01T15:23:48Z

testsuite/tests/parsetree/source_jane_street.ml

As discussed, we will add documentation in another PR.

riaqn · 2025-05-01T15:25:10Z

typing/typemod.ml

+  | Sig_module (id, pres, md, rec_, vis) ->
+      let md_modalities =
+        md.md_modalities
+        |> Mode.Modality.Value.to_const_exn
+        |> (fun then_ -> Mode.Modality.Value.Const.concat ~then_ modalities)
+        |> Mode.Modality.Value.of_const
+      in
+      let md = {md with md_modalities} in


As mentioned in #3943 , now include S @@ modalities is properly implemented, we can mention it in syntax.md.

riaqn · 2025-05-01T15:26:28Z

typing/typemod.ml

+  | Sig_module (id, pres, md, rec_, vis) ->
+      let md_modalities =
+        md.md_modalities
+        |> Mode.Modality.Value.to_const_exn


include S @@ modalities, here S must be a constant modality (as opposed to inferred modalities). See maybe_infer_modalities in this file.

testsuite/tests/typing-modes/md_modalities.ml

riaqn · 2025-05-01T15:32:53Z

testsuite/tests/typing-modes/module.ml

@@ -160,7 +148,7 @@ module type S = sig
 end

 module M : S = struct
-    let foo = fun x -> x
+    let foo @ nonportable = fun x -> x


Yes, the printing still zaps to id. Now with #3922 , I think the default should be zapping to floor.

testsuite/tests/typing-modes/module.ml

testsuite/tests/typing-modes/val_modalities.ml

riaqn · 2025-05-01T15:44:03Z

testsuite/tests/typing-modes/val_modalities.ml

+module M' = (M @ nonportable)
+let () = portable_use M'.length


Well, M has an explicit signature that doesn't have @@ portable.

lukemaurer

Still not entirely reviewed but just have env.ml and env.mli left.

typing/typemod.ml

lukemaurer · 2025-05-02T11:22:23Z

typing/typemod.ml

@@ -2977,6 +3038,8 @@ and type_open_decl_aux ?used_slot ?toplevel funct_body names env od =
    in
    let md = { mod_desc = Tmod_ident (path, lid);
               mod_type = Mty_alias path;
+               mod_mode = Value.(disallow_right max), None;
+               (* CR zqian: maybe put in the correct mode and locks. *)


What's up here?

I'm being lazy and not putting in the correct mode. This is fine because the type checking only cares about newenv, and this open_descr is only used in typedtree.

However, it's probably also not much work to do it properly.

I'd rather the typedtree have the correct info if it's not much work—at some point, odoc's going to want to know these things from the typedtree as well. (But that won't be for a bit so if this turns out to be hard I won't lose any sleep over it)

typing/includecore.ml

typing/includemod.ml

lukemaurer · 2025-05-02T16:41:22Z

typing/includemod.ml

@@ -799,7 +857,6 @@ and signature_components :
      let id, item, shape_map, present_at_runtime =
        match sigi1, sigi2 with
        | Sig_value(id1, valdecl1, _) ,Sig_value(_id2, valdecl2, _) ->
-            let mmodes = append_ldot (Ident.name id1) mmodes in


Not clear to me why we don't need this anymore?

This is now handled in child_modes.

I don't see a call to child_modes though?

This is handled in Includecore.value_descriptions.

typing/includemod.ml

typing/ctype.ml

typing/env.ml

riaqn requested a review from lukemaurer March 27, 2025 17:24

riaqn added typing modes Work on modes. There's some overlap with the `multicore` label, but not strictly so. labels Mar 27, 2025

riaqn force-pushed the modal-module branch from 99a623d to 077fea5 Compare March 31, 2025 12:22

lukemaurer reviewed Mar 31, 2025

View reviewed changes

riaqn force-pushed the modal-module branch 4 times, most recently from eefebc5 to 2f89dc6 Compare April 4, 2025 16:59

riaqn commented Apr 8, 2025

View reviewed changes

testsuite/tests/typing-local/local.ml Show resolved Hide resolved

testsuite/tests/typing-local/local.ml Show resolved Hide resolved

stdlib/ephemeron.mli Show resolved Hide resolved

modal modules

208cd85

riaqn force-pushed the modal-module branch from 2f89dc6 to 208cd85 Compare April 8, 2025 13:37

riaqn changed the base branch from main to minor-fix-modes April 8, 2025 13:43

riaqn mentioned this pull request Apr 18, 2025

Refactor and clean up mode syntax janestreet/ocamlformat#108

Draft

goldfirere reviewed Apr 29, 2025

View reviewed changes

lukemaurer reviewed Apr 30, 2025

View reviewed changes

riaqn mentioned this pull request Apr 30, 2025

Clarify and expand modes syntax docs #3943

Open

riaqn commented Apr 30, 2025

View reviewed changes

goldfirere reviewed Apr 30, 2025

View reviewed changes

riaqn commented May 1, 2025

View reviewed changes

lukemaurer reviewed May 2, 2025

View reviewed changes

lukemaurer reviewed May 6, 2025

View reviewed changes

typing/env.ml Show resolved Hide resolved

Code review

8346860

		(* the module is added only for its types, so we give it the
		strongest mode to avoid false mode errors. *)

		module M' = (M @ nonportable)
		let () = portable_use M'.length

Support modules of non-legacy modes #3759

Are you sure you want to change the base?

Support modules of non-legacy modes #3759

Conversation

riaqn commented Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldfirere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riaqn May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukemaurer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riaqn May 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riaqn commented Mar 27, 2025 •

edited

Loading

riaqn May 9, 2025 •

edited

Loading

riaqn May 6, 2025 •

edited

Loading