Fix variant coercion with as attribute #7058
Conversation
I thought about that too, but I was not quite sure why it is always converted to |
|
Constructors used to always compile to Great fix. Like @cristianoc I also wonder whether there are more places to fix here, since int is just one of the things you can set the payload as with |
I was thinking the same: constructors used to always compile to int. |
shulhi
force-pushed
the
fix-variant-coercion-with-as-attribute
branch
2 times, most recently
from
79ded60 to
0ba6d8a
Compare
|
@zth I took a different approach in e238013 (compared to c8a8e08). Maybe this is a better approach because it doesn't change how the constructor is represented with/without Also I added more tests. |
jscomp/core/lam.ml
Outdated
| match args with | ||
| | [ Lconst a ] -> ( | ||
| match (prim, a) with | ||
| | Pnegint, Const_int { i } -> Lift.int (Int32.neg i) | ||
| | Pnegint, Const_int { i; comment = pointer_info} -> | ||
| let ii = extract_const_as pointer_info i in |
jscomp/core/lam.ml
Outdated
| @@ -532,7 +543,8 @@ let prim ~primitive:(prim : Lam_primitive.t) ~args loc : t = | |||
| Lift.string (a ^ b) | |||
| | ( (Pstringrefs | Pstringrefu), | |||
| Const_string { s = a; unicode = false }, | |||
| Const_int { i = b } ) -> ( | |||
| Const_int { i = b; comment = pointer_info } ) -> ( | |||
| let b = extract_const_as pointer_info b in | |||
There was a problem hiding this comment.
At this point, after seeing all these case, why not having this directly inside Const_int?
Or rather, having this done, in whatever form, at Const_int creation (or earlier)?
There was a problem hiding this comment.
Another way of asking the question: are there cases where the "original" int is what's needed, and cases where the override one is what's needed.
If both are needed, 2 values need to be accessed/stored.
Otherwise, it's cleaner to ever only have 1.
There was a problem hiding this comment.
After further checking, there are only several cases that match Lconst (Const_int ...) like in the following code,
I've fallback to the original implementation since it is simpler but it does change the representation of variant in runtime (just for int case). I don't think it affect anything else and I've added several more coercion tests just in case to cover cases like in the above linked code.
Another way of asking the question: are there cases where the "original" int is what's needed, and cases where the override one is what's needed.
As far as I understand, the override one is still needed for case where the as payload is not an int. The "original" int is needed for runtime representation for the variants, and when the case is int both override and original values are the same.
There was a problem hiding this comment.
@shulhi I don't follow entirely, how is the runtime representation changed?
shulhi
force-pushed
the
fix-variant-coercion-with-as-attribute
branch
2 times, most recently
from
6a19249 to
e6fb6e5
Compare
|
@shulhi Thanks a lot! 👍 Could you add a CHANGELOG entry? |
And could you please rebase? |
shulhi
force-pushed
the
fix-variant-coercion-with-as-attribute
branch
from
e6fb6e5 to
b922e7b
Compare
shulhi
force-pushed
the
fix-variant-coercion-with-as-attribute
branch
from
843970b to
cadb22c
Compare
|
Doesn't build because of the formatting, please run |
How about adding a very visible message at the end to suggest doing |
|
Currently it says We can change that to |
In flashing red. |
|
This is what it looks like currently. Not quite "flashing red", but visible enough I think? 
|
|
Thanks again @shulhi! Merging. |
Fix for #7036