ocaml / ocaml

The core OCaml system: compilers, runtime system, base libraries
https://ocaml.org
Other
5.19k stars 1.06k forks source link

`value_kind` classification for `function` cases only looks at first case #13118

Open ncik-roberts opened 2 weeks ago

ncik-roberts commented 2 weeks ago

This code added in #12236 is buggy in the first Tfunction_cases case:

https://github.com/ocaml/ocaml/blob/564b2db8df9837b0390bcf1ca3249eddc7b69fae/lambda/translcore.ml#L725-L734

For a concrete example, take this example, where the first case's return kind is Pintval and the second is Pfloatval:

type _ t = Int : int t | Float : float t

let get (type a) : a t -> a = function
  | Int -> 3
  | Float -> 4.0

trunk finds that the return value_kind is Pintval when it should really be Pgenval.

I'm not sure what the impact is. I intend on posting a fix next week after soliciting a design opinion from a colleague, but if someone deems it urgent to fix this faster than that, you can refer to this PR which fixes the bug (perhaps not with the most elegant design) on one of Jane Street's branches: https://github.com/ocaml-flambda/flambda-backend/pull/2471

lthls commented 2 weeks ago

The return kind on functions was introduced in #2156, but never used (@chambart had a patch that used it but it was never merged). I have checked that removing the field has no impact on the generated code, so having a wrong value shouldn't matter.

Flambda 2 uses it for at least one feature (cross-function unboxing), so I think it makes sense to fix the bug and keep the information.

ncik-roberts commented 2 weeks ago

Thanks for the context. In that case, this does not seem urgent w.r.t. the current release cycle and I'll sync with @goldfirere on his design comments before posting a PR to fix this.