@garrigue , what do you think of the choice of field names?

(I wondered if it would make sense to have pairs (id, declaration) instead of two fields, but the current proposal seems fine and incremental to me.)

Oh, since this is an internal change, I didn't update the change log. Please let me know if you also want an entry in the log.

Sorry I missed this discussion.
Yes, these field names look ok.
The code itself is fine, but I wonder why use let module T = Typeclass in rather than let open Typeclass in. I see no semantical problem in the latter.

Thanks!

You actually got me. To be frank, I never did local open before, because I was afraid that open might introducing runtime penalty.

I re-read the manual, and it makes sense to me now that open only affects parsing. I really appreciate you ask this question :D

So, is local open preferred in this case? I'd like to change the code if so.

Sometimes we avoid open when there is a risk that it would hide a definition from the outer environment that is used in scope of the open. So open is preferred either at a very global level (when there are no values in the environment to hide) or at a very local level (when the expression under open is short and its references are easy to understand). Here, it would be a very local use, so it is perfectly fine.

(Amusingly, let module X = M in ... is the construction that had a (neglectible) performance impact in older OCaml versions, while let open M was always free.)

Yes, it is fine to use open in that case.

@gasche Thanks for the clarifications! Updated.

I actually think it would be better to use record field disambiguation and a type annotation on the function arguments of your new record type. See Flambda.

I decided to go ahead and merge the PR as-is: let open is consistent with the rest of the type-checker codebase (see use of let open Ast_helper in typecore and let open Variance in typedecl).

@lijunsong thanks for the patch! Next time, when you are making a small change to a proposed PR, do not hesitate to use an interactive rebase to squash the minor commits into the proposed commit, and git push -f to your private branch to refresh the pull-request.

@gasche Thanks! I'll do next time.