Implement `let mutable` #3964

jra4 · 2025-05-02T20:27:06Z

Most likely not done yet, but should be good for some code review

github-actions · 2025-05-02T20:27:15Z

Parser Change Checklist

This PR modifies the parser. Please check that the following tests are updated:

parsetree/source_jane_street.ml

This test should have examples of every new bit of syntax you are adding. Feel free to just check the box if your PR does not actually change the syntax (because it is refactoring the parser, say).

jra4 · 2025-05-02T20:54:51Z

I've been leaving myself comments in the diff (all start with jra:) for things I was unsure about. Looks like there are 7 comments I still haven't resolved.

riaqn · 2025-05-05T01:00:04Z

I think there are some tricky questions to be asked and answered wrt modes - let's discuss in other channels.

ccasin

Just a quick pass over some of the places where you had questions - we'll chat more tomorrow.

ccasin · 2025-05-07T21:17:32Z

typing/env.ml

@@ -3298,6 +3299,9 @@ let walk_locks ~errors ~loc ~env ~item ~lid mode ty locks =

 let lookup_ident_value ~errors ~use ~loc name env =
  match IdTbl.find_name_and_locks wrap_value ~mark:use name env.values with
+  | Ok (_, locks, Val_bound {vda_description={val_kind=Val_mut}})
+    when List.exists (function Closure_lock _ | Escape_lock _ -> true | _ -> false) locks ->


As a general rule, it's good to avoid "catch-all" patterns and instead just list out all the cases. If we use a catch-all pattern, when someone adds a new lock in the future the compiler won't force them to consider how that lock should be treated here - it'll just silently get handled by the catch-all case. On the other hand, if we list out all the locks now, when someone adds a new lock in the future they'll get an incomplete pattern match warning and be forced to think through what to do.

(Once you list all the locks, this when guard will probably be big enough that it makes sense to pull out into its own function)

ccasin · 2025-05-07T21:41:59Z

typing/env.ml

@@ -3298,6 +3299,9 @@ let walk_locks ~errors ~loc ~env ~item ~lid mode ty locks =

 let lookup_ident_value ~errors ~use ~loc name env =
  match IdTbl.find_name_and_locks wrap_value ~mark:use name env.values with
+  | Ok (_, locks, Val_bound {vda_description={val_kind=Val_mut}})
+    when List.exists (function Closure_lock _ | Escape_lock _ -> true | _ -> false) locks ->
+      may_lookup_error errors loc env (Mutable_value_used_in_closure name)


We're probably going to want an error that reflects the context argument to the relevant lock, and then it would be good to write test cases that hit each one.

lambda/translcore.ml

parsing/parsetree.mli

typing/typemod.ml

ccasin · 2025-05-07T22:32:57Z

typing/env.mli

@@ -257,6 +257,7 @@ type lookup_error =
  | Non_value_used_in_object of Longident.t * type_expr * Jkind.Violation.t
  | No_unboxed_version of Longident.t
  | Error_from_persistent_env of Persistent_env.error
+  | Mutable_value_used_in_closure of string (* jra: Maybe rename this error/add other errors? *)


Agreed - I think we'll want to generalize this now that we have more complicated locks - can discuss tomorrow.

ccasin · 2025-05-07T22:41:12Z

typing/env.ml

-          lookup_error loc env (Not_an_instance_variable name)
+          Instance_variable (path, mut, cl_num, Subst.Lazy.force_type_expr desc.val_type)
+      | Val_mut, Pident id ->
+          let rec mode_of_locks mode = function (* jra: surely this is incorrect *)


There's an ongoing discussion with @riaqn about this, but my guess is we want to do something like the inverse of the walk_locks function above. Note how that function is used by type_ident in Typecore: basically, it tells you how you need to change the mode of something to push it down through locks from its declaration site to a use site. Here, we're doing this in the other direction - we want to know what mode we should require something to have so that it is safe to push it up through the locks from the place where the value is to the place where the mutable variable we're sticking it in is declared.

typing/typeclass.ml

ccasin · 2025-05-07T22:46:20Z

typing/typecore.ml

@@ -1151,6 +1157,8 @@ type pattern_variable =
    pv_id: Ident.t;
    pv_uid: Uid.t;
    pv_mode: Value.l;
+    (* jra: I don't fully understand the difference between mutable_flag and mutability *)
+    pv_mutable: mutable_flag;


Not quite certain what you're referring to by "mutability" here, but I suspect there isn't a difference - we can discuss.

typing/typecore.ml

* Allow mutable variables to be of any layout * Add tests related to modes (not fully worked out) * Avoid lines of more than 80 chars

ccasin

OK - I've now read everything except the modes bits we're going to discuss with @riaqn.

testsuite/tests/parse-errors/let_mutable_at_toplevel.ml

ccasin · 2025-05-15T17:35:10Z

testsuite/tests/typing-local/let_mutable.ml

+let foo4_2 y = (* Can't sneak local out of non-local for loop body region *)
+  let mutable x = [] in
+  let build_loop () =
+    for i = 1 to y do local_


In the original implementation of the local mode and stack allocation, local_ was used in several different ways with its meaning determined by context. At the beginning or end of a region it meant to eliminate that region and use its parent. We now do that by writing exclave_ at the beginning of the region instead. So I think this one (and probably more in this file, which I will try to point out as I go) should be exclave_ instead. (It's actually still the case that local_ works for this purpose, but we're going to change that soon, and should make the tests forwards compatible).

Uses of local_ to check force the expression to have the local mode still work that way, like the one on the line below this, so it's fine to leave that one.

testsuite/tests/typing-local/let_mutable.ml

ccasin · 2025-05-15T18:17:03Z

testsuite/tests/typing-local/let_mutable.ml

+val y_12 : t_12 = Foo_12 42
+|}]
+
+(* Test 13: modes? *)


Just leaving a note to remind us to revisit this after we chat with Zesen.

ccasin · 2025-05-15T18:19:18Z

testsuite/tests/typing-local/let_mutable.ml

+;;
+[%%expect{|
+val r_15 : float = 0.0009765625
+|}]


Is this the correct answer? Probably a comment documenting the expected result or a test whose result is more easy to calculate intuitively would be helpful.

typing/typecore.ml

ccasin · 2025-05-18T11:13:21Z

typing/typecore.ml

+              | Ok sort -> sort
+              | Error _ ->
+                (* unreachable since [x <- e] was already type-checked,
+                   so [e] is representable *)


I think this isn't quite right. We're in the middle of checking x <- e, here. It's unreachable because we checked ty is representable when we created the binding for the mutable variable.

Which, actually, gives me an idea: could we just remember the sort of this variable in the environment rather than recomputing it here?

typing/typecore.ml

jamesrayman added 4 commits April 24, 2025 17:05

Add a test and update the parser for let mutable

fac96b2

It works

bfd44d9

Make let mutable a syntax error in some cases

821925e

Change [local_] to [exclave_] in test and an fix infinite loop

fc761c2

jra4 requested review from ccasin and riaqn May 2, 2025 20:27

jamesrayman added 2 commits May 2, 2025 16:32

Add let mutable to parsetree/source_jane_street.ml

d306842

Run make check-fmt

5fbcff1

Add some currently failing let mutable tests

449509f

ccasin reviewed May 7, 2025

View reviewed changes

Misc

27d2062

* Allow mutable variables to be of any layout * Add tests related to modes (not fully worked out) * Avoid lines of more than 80 chars

ccasin added new language feature typing lambda Lambda language changes labels May 18, 2025

ccasin reviewed May 18, 2025

View reviewed changes

jamesrayman added 3 commits May 19, 2025 06:06

Unfinished

cdd9e7b

Address some comments

d94e261

Fix more concerns

1465fed

Implement let mutable #3964

Are you sure you want to change the base?

Implement let mutable #3964

Uh oh!

Conversation

jra4 commented May 2, 2025

Uh oh!

github-actions bot commented May 2, 2025 • edited by jra4 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Parser Change Checklist

Uh oh!

jra4 commented May 2, 2025

Uh oh!

riaqn commented May 5, 2025

Uh oh!

ccasin left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ccasin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Implement `let mutable` #3964

Implement `let mutable` #3964

github-actions bot commented May 2, 2025 •

edited by jra4

Loading