I'm confused. @mshinwell said "the comment may be wrong when flat_float_array is not set", and the comment says "we don't need to wrap with Popaque", so I thought the fix would be to wrap with Popaque as in the "Identifier Forward_value" case, but in fact your proposed fix is to not create a forward tag as in the Constant_or_function case.

Also, do we need to keep a distinction between Constant(_or_function) and Float when flat_float_array is not set? Couldn't we instead change Float into Flattenable_float, and lump the floating-point case into Constant(_or_function) when the flag is not set?

Your second question answers the first: when flat_float_array is false, you just don't need to wrap the Float into a forward block. If we did use a forward block, we most probably would also need to wrap it with Popaque, but we don't.

As for the answer to your second question, we could probably do it that way because classify_lazy_argument is only used in two places, but I was going for a minimal patch. Since we cannot change the type of the function depending on the value of flat_float_array, just keep the type as is and treat Float as a normal value when it is false.

I'm suggesting to change the type in all cases by renaming the Float constructor to Flattenable_float. When flat_float_array is not set, no value is classified as "flattenable float", but it is used for floating-point values when the flag is set.

I've done the change you suggest, but I'm not completely convinced that it's better this way.

How about renaming Flattenable_float to Gc_will_never_shortcut? I think that may be clearer.

I believe that without this change, the test file testsuite/tests/typing-misc/pr6939.ml-noflat is broken. When -no-flat-float-array is set, the following definitions should be accepted as valid recursive definitions:

let rec x = [| x |]; 1.;;
let rec x = let u = [|y|] in 10. and y = 1.;;

and currently they are rejected.

Hm, actually the patchset itself does not seem to change the let-rec-checking of flat arrays. I may be misunderstanding something (currently I cannot manage to get this test working with -no-flat-float-array), or maybe the breaking change was Jeremy's complete PR that removed the special no-flat-array provisions that Damien had in his original PR.

maybe the breaking change was Jeremy's complete PR that removed the special no-flat-array provisions that Damien had in his original PR.

#1516 fixes the issue, taking Config.flat_float_array into consideration in the let rec check.

@damiendoligez Are you able to update this so we can merge to 4.07?

Rebased. Needs a formal review. @gasche @mshinwell

I'd like to restate my suggestion of renaming Flattenable_float to Gc_will_never_shortcut. I don't think the meaning of Flattenable_float is very clear... (I will review the rest of this too.)

@mshinwell I agree it's clearer but it's not quite correct: it's not just the GC that cannot shortcut this value, but also transl_exp0 itself. How about `Float_that_cannot_be_shortcut ?

merged and cherry-picked to 4.07 (commit df741e4)