Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ncik-roberts opened this issue Apr 25, 2024 · 2 comments
Open

Comments

@ncik-roberts
Copy link
Contributor

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

ocaml/lambda/translcore.ml

Lines 725 to 734 in 564b2db

and transl_function_without_attributes ~scopes loc repr params body =
let return =
match body with
| Tfunction_body body ->
value_kind body.exp_env body.exp_type
| Tfunction_cases { cases = { c_rhs } :: _ } ->
value_kind c_rhs.exp_env c_rhs.exp_type
| Tfunction_cases { cases = [] } ->
(* With Camlp4/ppx, a pattern matching might be empty *)
Pgenval

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: ocaml-flambda/flambda-backend#2471

@lthls
Copy link
Contributor

lthls commented Apr 25, 2024

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants