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

Make Pattern_env.t private #12361

Merged
merged 2 commits into from
Jul 7, 2023
Merged

Make Pattern_env.t private #12361

merged 2 commits into from
Jul 7, 2023

Conversation

garrigue
Copy link
Contributor

@garrigue garrigue commented Jul 7, 2023

Follow-up to #12331, making Pattern_env.t private, so that we can track better who modifies it.

Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making Pattern_env.t private sounds like a good compromise to me.

@gasche gasche added the merge-me label Jul 7, 2023
typing/typecore.ml Show resolved Hide resolved
@@ -733,7 +733,7 @@ let solve_constructor_annotation
let decl = new_local_type ~loc:name.loc () in
let (id, new_env) =
Env.enter_type ~scope:expansion_scope name.txt decl !!penv in
penv.env <- new_env;
Pattern_env.set_env penv new_env;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can eliminate set_env here:

let solve_constructor_annotation
    tps (penv : Pattern_env.t) name_list sty ty_args ty_ex =
  let expansion_scope = penv.equations_scope in
  let ids, new_env =
    List.fold_left
      (fun (ids, env) name ->
        let decl = new_local_type ~loc:name.loc () in
        let (id, new_env) =
          Env.enter_type ~scope:expansion_scope name.txt decl env in
        {name with txt = id} :: ids, new_env)
      ([], !!penv) name_list
  in
  let cty, ty, force =
    with_local_level ~post:(fun (_,ty,_) -> generalize_structure ty)
      (fun () -> Typetexp.transl_simple_type_delayed new_env sty)
  in
  tps.tps_pattern_force <- force :: tps.tps_pattern_force;
  let ty_args =
    let ty1 = instance ty and ty2 = instance ty in
    match ty_args with
      [] -> assert false
    | [ty_arg] ->
        unify_pat_types cty.ctyp_loc new_env ty1 ty_arg;
        [ty2]
    | _ ->
        unify_pat_types cty.ctyp_loc new_env ty1 (newty (Ttuple ty_args));
        match get_desc (expand_head new_env ty2) with
          Ttuple tyl -> tyl
        | _ -> assert false
  in
  if ids <> [] then ignore begin
    let ids = List.map (fun x -> x.txt) ids in
    let rem =
      List.fold_left
        (fun rem tv ->
          match get_desc tv with
            Tconstr(Path.Pident id, [], _) when List.mem id rem ->
              list_remove id rem
          | _ ->
              raise (Error (cty.ctyp_loc, new_env,
                            Unbound_existential (ids, ty))))
        ids ty_ex
    in
    if rem <> [] then
      raise (Error (cty.ctyp_loc, new_env,
                    Unbound_existential (ids, ty)))
  end;
  ty_args, Some (ids, cty)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added types are required in ty_args and cty, so ultimately we would still need to call Pattern_env.set_env penv new_env before returning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. The above code indeed made two tests fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the code more functional might be a good idea eventually, but this seems orthogonal to the change in Pattern_env.

@garrigue garrigue merged commit fff8f84 into ocaml:trunk Jul 7, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants