Did you consider moving the Texp_function constructor to a record (inline or not)? That would not make the change less invasive, and could help make future changes less verbose.

@gasche Only briefly. It wasn't clear to me that it was really necessary in this case.

I understand how this fixes the problem, but it seems a bit weird conceptually to invent an Ident.t at the Typedtree level, even if it not used at that level at all, and only to serve as a lookup key later on.

Shouldn't we aim for a more general approach which could apply to other cases as well? The need seems to be able to refer back to nodes in the Typedtree in later representations (cmm, lambda, etc). There might be other uses of such pointers, e.g. to retrieve some partial type information later in the compilation pipeline. So what about adding a uid (int, or a pair compilation_unit*int) to nodes in the Typedtree (an extra field added to Typedtree.expression, perhaps also Typedtree.pattern and co)?

I don't think it's right to say that it's only used as a lookup key---it's also used for the actual identifier required for the parameter in Lambda (just as it was before).

My feeling is that this alone doesn't call for a more general approach. This seems like a minor modification. However I think it is likely the case that in the future, at least in the context of generating debugging information, we may need to have some better method of either passing type information through to the backend or else retrieving it from earlier as you suggest. (This isn't needed now because we don't describe OCaml types in DWARF information; we instead retrieve them from the .cmt file.) I think it would be a mistake to try to design that scheme now for a couple of reasons: firstly, the need for it may be quite far out and how it would work hasn't yet been thought about; secondly, there is already a very large body of code involved with the first tranche of the debugging patches and I think we would do well to avoid adding any more.

I don't think it's right to say that it's only used as a lookup key---it's also used for the actual identifier required for the parameter in Lambda (just as it was before).

It's a lookup key at the Typedtree level in the sense that only its identity matters; you can create an arbitrary identifier -- it is not used anywhere else in the Typedtree representation.

That said, I agree that the change is rather small and I'm not opposed to it if you don't think something more general should be considered now.

I'd tend to second @gasche suggestion about switching to a record at the same time. It's not only about minimizing future changes, but also avoiding breaking external tools that works on .cmt files too often.

I have changed Texp_function to use an inline record. Any more comments?

I'm fine with the code as it is now (the use of ; _ is inconsistent and I'll let you decide what to do about it), and I would let @alainfrisch handle the merge.

I'm not sure human-eyes could reliably review such changes, so I'll trust Mark and let him merge.

I have fixed the underscores.