I believe that the fix is correct, but I find it unsatisfying.

1. The logic of the primitive is now duplicated in the Texp_ident and Texp_apply cases of transl_exp0. You should factor it out in its own function and call it from the two places -- in particular it would make it obvious that one is just an eta-expansion of the other.

2. This is more of an open question, but can we change the code structure so that a similar error would have been caught at compile-time in the future? For example we could have a variant type of "primitives that must be handled specially" that we match exhaustively over in the two cases.

3. transl_primitive, which gets called in the fallback/default case of Texp_indent, already has a final default case that does the eta-expansion. Why is this case not reached, why is the fix not to make sure that raise_with_backtrace reaches it, and can we change the code so that it is reached by default for new primitives? (I guess that could be a way to answer question (2)).

(Looking more at the code of transl_primitive, I now suppose that the fallback case is reached and eta-expansion happens, but what is does in the body of the generated function is simplistic. Just calling transl_primitive_application may be better, but having it use the logic currently located in Texp_apply case would require more refactoring -- if that even is the right thing to do.)

(cc @let-def and @bobot who may have an opinion about this code)

Thanks for the review @gasche. Indeed, I did not overlook the code duplication. But I held off doing it in the first try because 1) I wanted to make sure the fix was correct before refactoring, and 2) there are a couple other "special" primitives that would also profit from this kind of refactoring.

I will give some thought to refactoring along the lines of your 2. and 3.

@gasche Your solution 1. is the simplest. We could reuse Simplif.beta_reduce params body args in Texp_apply case and at the same time check more correctly the arities (instead of List.hd or assert false)

@nojb Thank you for fixing that.

I think (1) is a necessary first step (I don't really agree with @nojb's view that explicit duplication helps ensure that the patch is correct, because I have to check that the two places do the same thing to review it, whereas a function abstraction would make that immediate), but that it does not solve the longer-term problem that the addition of any additional primitive is likely to be broken in the same way if we don't make try to rule this error out.

but that it does not solve the longer-term problem that the addition of any additional primitive is likely to be broken in the same way if we don't make try to rule this error out.

I don't see why, send* functions could not also be added to solution 1., so new primitive will also be added once to solution 1.

@gasche FWIW, I do not think it helps check the patch is correct, but the refactoring itself is not completely trivial so I thought it would be better to separate the two.

Closing this to concentrate efforts on #1557 which is a much more general solution to fixing the issues addressed by this PR.