Implement let mutable
#3964
Implement let mutable
#3964
Conversation
Parser Change ChecklistThis PR modifies the parser. Please check that the following tests are updated:
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). |
|
I've been leaving myself comments in the diff (all start with |
|
I think there are some tricky questions to be asked and answered wrt modes - let's discuss in other channels. |
typing/env.ml
Outdated
| @@ -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 -> | |||
There was a problem hiding this comment.
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)
| @@ -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) | |||
There was a problem hiding this comment.
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.
typing/env.mli
Outdated
| @@ -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? *) | |||
There was a problem hiding this comment.
Agreed - I think we'll want to generalize this now that we have more complicated locks - can discuss tomorrow.
typing/env.ml
Outdated
| 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 was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
Not quite certain what you're referring to by "mutability" here, but I suspect there isn't a difference - we can discuss.
Most likely not done yet, but should be good for some code review