Generalize Ctype.free_variables to Ctype.free_variables_list

[rtjoa]

(See also #1519, which attempts to fix the same problem)

The motivation for this change is similar to #1503's: we sometimes collect all free variables in a list of types tyl by passing a fake TTuple to Ctype.free_variables, as in Ctype.free_variables (Btype.newgenty (Ttuple tyl)).

However, this is fragile to changes in tuples, and in particular, becomes messy when adding labeled tuples: the above would become Ctype.free_variables (Btype.newgenty (Ttuple (List.map (fun t -> None, t) tyl))).

[goldfirere]

I think this is a good direction of travel. But there's a small piece of logic that is repeated in this implementation: would it be possible to consolidate the set-up and tear-down steps into a higher order function? I'm thinking of something like run_free_vars_rec : ?env:Env.t -> (unit -> unit) -> type_expr list that fiddles with the free_variables and really_closed variables.

[ccasin]

@goldfirere Ryan subsequently made a more substantial refactoring of this code in #1519. He and I discussed it, and while it's certainly an improvement, my general feeling was that the cost of differing from upstream was higher than the benefit of having nicer code.

I had forgotten this one was still open. It's a less substantial refactor, so maybe benefits outweigh the costs?

As for your suggestion, I agree that's an improvement. I recommended against doing something similar in #1503 because it seemed much clearer to duplicate the code, but here we don't have the complicated control flow.

[goldfirere]

Yeah -- I was worried about the upstream interaction, but decided that the change was small enough that any merge conflicts there would be manageable. I don't know how painful it is not to make this change; I'm fine either way (making the change or not). I also like the idea floated in #1519 to prepare an upstream patch with these changes (and then including them locally as well).

[mshinwell]

@goldfirere what is going to happen with this PR? Nearly a year!

[goldfirere]

I think we should just close. This should just be an upstream patch, but I'm not inclined to push that through. @ccasin: second opinion?