This looks like a use-case for Misc.try_finally ~exceptionally:(...) @@ fun () ->

Honestly, that seems much less clear than match-with-exception!

I disagree, the benefit of my proposal is that the cleanup code is close to the setup code, instead of being at the very end of the code, making the connection between the two harder to see.
But: it's your code, so let's go with your pet preferences instead of mine.

The invariant we want is that every stub created is either resolved or removed. match-with-exception puts stub resolution right beside stub removal, and it's easy to see that exactly one of them happens. Your proposal puts stub removal right beside stub creation (which is nice!), but makes it harder to see that either resolution or removal occurs, because resolution remains far away. (IOW: it's nice to have setup and cleanup together, but I don't like having setup and half of cleanup together with the other half far away).

/me goes writing a RFC about with <handler> match <expr>

Orthogonal to this PR, but: I don't like the fact that those TypeSet, TypeHash module expose half of their functions with the wrong type, and we have to fix them on demand. I wish we had a module ascription on Map.S, and we were forced to expose all operations correctly.

cc @garrigue

No strong opinion on this. Those extra functions are not unsound, they are just not the ones we expect here.
Note however that your ascription would also have to duplicate all the functions that do not involve elt, which already had the right type.

Note however that your ascription would also have to duplicate all the functions that do not involve elt, which already had the right type.

I don't think so.

module type S = sig
  val x : int
  val y : int
  type t
  val z : t
end

module M1 : S with type t = bool = struct
  let x, y = 0, 1
  type t = bool
  let z = true
end

module M2 : S with type t = char = struct
  include M1
  (* no repetition of x, y *)
  type t = char
  let z = 'a'
end

OK, I had misunderstood.
You suggest using Map.S itself.
This probably work (didn't try, there might be strange things in the signature),
but then we have to wrap all the functions of Map.S, which is a lot more than currently.
Is it really worth it.

We have to wrap all functions right now, and then we have to update the code every time Map.S changes with new functions. The latter is more of a problem I guess -- it means that stdlib PRs may break the build of the compilers and require authors to modify the type-checker codebase.

Maybe a compromise would be to have an explicit signature on these modules, a subset of Map.S of course, that explicitly lists what is supported. This way we would not have the weird phenomenon of bindings existing in this module, but at the wrong type, which is surprising for people unfamiliar with the codebase. The less surprises, the better.

We could also get the entirety of Map.S without duplicating code by building these modules in a context where type_expr = transient_expr is visible, either by moving them into types.ml or exporting that fact via a GADT.

I'm missing something, we still need to call the repr normalization function on all transient arguments to turn a TransientTypeHash into a TypeHash, right?

Yes, we need. And this is precisely the reason removing the distinction between type_expr and transient_expr doesn't sound like a good idea here.

Concerning the surprises, this doesn't sound like high priority to me. There are currently many more surprises for outsiders in this codebase. Having a type error rather than an undefined identifier doesn't sound so bad to me. At least you know exactly what you have to wrap then.

At least we could have a comment in the module definition saying that we know that some functions are exported with the wrong type and are in fact doing the wrong thing, that we recommend not using them, and that anyone should feel free to "extend" the module by rewrapping more functions.

Adding this in the mli would indeed be a good idea.