Since there's an open PR doing typetexp.ml refactors, I wanted to address a less-readable function

Community
1

The PR in question did appreciated improvements to the readability of this part of the compiler.
However, one function from before the PR remained as-is (maybe slightly less readable): globalize_used_variables. It really stood out to me.
As to not trespass on the PR with GitHub’s invasive review interface, I opted to post a suggestion patch here… I hope it’s seen valuable enough to use or at least draw ideas from:

let todo = ref [] in
let go name (ty, loc) =
 if not globals_only || is_in_scope name then
    let snap = Btype.snapshot () in
    let v1 = new_global_var () in
    let v1_unification_error = try unify env v1 ty; false with _ -> true in
    if v1_unification_error then Btype.backtrack snap else
       let v2 = try lookup_global_type_variable name with
         Not_found ->
           if fixed && Btype.is_Tvar ty then
              let n, ns = "'"^name, get_in_scope_names () in
              raise (Error (loc, env, Unbound_type_variable (n, ns)))
           else
              let v = new_global_var () in add name v; v
       in
       todo := (v1, v2, loc) :: !todo
in
let unify' (v1, v2, loc) = try unify env v1 v2 with
  Unify err -> raise (Error (loc, env, Type_mismatch err))
in
TyVarMap.iter go !used_variables;
used_variables := TyVarMap.empty;
fun () -> List.iter unify' !todo

Of course I don’t attempt to do any smarter refactors here, just syntactic ones, I don’t claim to have enough familiarity with such a hard spot in the compiler to do more.

2

Why don’t you send a PR yourself? Small refactoring work is fine (even more so while I still have the code that I have just reviewed in my mind). It would make the comparison faster and it would probably attract more viewers and comments. (It is fine to keep on discussion in this thread of course).

Few remarks:

  • why not replace iter and local reference by a fold ?
  • todo, go, v1 and v2 are a slight improvement, but why not go through more explicit names: delayed_unifications, collect_global_unifications, fresh_global, bound_global for instance?
3 Likes