Expose module signature when typing implementation

[leostera]

While working on a new backend for OCaml to generate Erlang sources, I
found the need to read the .cmi file back into a Types.signature
value.

@Drup spotted that I was reading the file and pointed out this value was
already being read during the type checking process.

A quick check at the Typedtree.module_coercion showed us that it would
be difficul to extract the same information that is readily available in
the signature value.

This change will expose the signature value directly, so other backends
relying on this signature information do not need to do the extra work
of reading it again.

[gasche]

Naive questions:

• I can see why we would want to expose the value as a result of typecheck_implementation,
  but why would we want to add it as unused inputs of the other functions?
  (If this is just to preserve direct piping of one into the other, I'm not convinced.)
• If we start having n-uples with n>2, maybe we should consider using records for readability?

[leostera]

In this case I just went for the smallest change, but I'd be happy to introduce a record like:

type typed_impl = {
  structure: Typedtree.structure;
  coercion: Typedtree.module_coercion;
  signature: Types.signature
}

What do you think?

[gasche]

I think that this would be better than the current PR, yes. It doesn't really answer my question about why we want to pass the signature to the later passes (maybe because it makes sense that they could use it?), but at least it makes it completely transparent/implicit in a pleasing way.

While we are at it: I would prefer it the signature was defined and documented before the coercion, as the explanation of the coercion mentions the signature.

[leostera]

@gasche Regarding your question about passing the signature to the later passes: it doesn't make sense right now for the existing later passes in the backends here. So I've deferred to pattern-matching on the record and reconstructing the tuple of values they actually use.

[leostera]

After trying to use a record defined in Typemod it felt a little strange that we'd be depending on that module to get access to data that would otherwise come from Typedtree. So I moved the type there.

Name is still up for discussion, just let me know what you think :)

[gasche]

I think that the field names are natural and they work well. I initially expected the record to be defined in compile_common, and be used only in functions at this level (not in the "core" modules in typing/). In fact your proposal works fairly well. Just a nitpick, I would prefer Typedtree.impl or Typedtree.implementation rather than Typedtree.t; the latter suggests that this is "the typedtree", when it is really the typedtree of a module checked against a signature.

[leostera]

@gasche rename is in place, and CI says we're ✔️ -- let me know if you'd like anything changed.

Also @Drup let me know what you think 🙌🏼

[Drup]

I like the new Typedtree.implementation type. I see no other information to propagate right now, but it'll be useful if we ever need to, which is nice. Just need a bit of documenting and this will be good to go.

[gasche]

Here is a stamp of approval: this is good to go once we have converged on the documentation. Thanks for your contribution.

[leostera]

Any chance this will make the cut to 4.12?

It seems I missed the deadline 😅 -- nevermind this!

[Octachron]

This is not a bugfix, thus it can wait for the next release.

[gasche]

Merged, thanks! (I forgot to squash, so the history is a bit messier, but oh well.)