fix primitive constants #264

Ladicek · 2025-03-27T10:27:51Z

We need constants of types byte, short and char because without them, we cannot statically make a difference between them and int, which may lead to all kinds of issues. For example, we could emit an invocation of a different overload of a method than expected, if the target class has an overload of the method for both byte (or short or char) and int.

In addition to that, this commit also adds Constant.of(Boolean), because java.lang.Boolean constants were not of the primitive type, unlike all others. This might be a bad idea, but all other constants of wrapper types are in fact constants of the corresponding primitive type, and consistency is warranted. We might later want to rethink that.

dmlloyd · 2025-03-27T15:00:39Z

TL;DR I agree with the basic idea. There are some details to work out though.

Longer explanation & background:

As you guessed, I left off these types initially because they are all represented as int when loaded into bytecode, and I hoped to avoid the complexity which comes with these values. But you're right, we need a way to represent constants with different static types for easy method resolution at least (the user could always give an explicit signature and use int constants, but that's pretty unwieldy).

What complexity you may ask? Well...

Relying on Constable means that any time a constant of this type shows up in bytecode, we're doing a ldc with a dynamic constant whose initializer is basically taking an int constant and casting it to byte or short or whatever. However, ideally when we generate code for a constant byte (for example) we'd want to just load the int constant and use it directly (since we generate the constant, we will know it's in range). This will prevent our constant table from getting too bloated in the common case.

So, to do this right, we need to do a couple of things:

Create ByteConstant, ShortConstant, and CharConstant classes whose writeCode method is the same as IntConstant (maybe extract a common superclass called IntLoadableConstant or something)
- BooleanConstant is already basically doing this correctly
- CharConstant wouldn't have negative values but this might not matter, depending on how you do it
Modify io.quarkus.gizmo2.impl.constant.ConstantImpl#of(java.lang.constant.Constable) to detect Byte, Short, and Character (and Boolean, oops I forgot that one!), and call a type-specific of method much like io.quarkus.gizmo2.impl.constant.ConstantImpl#of(java.lang.constant.ConstantDesc) does when it gets an Integer or Long etc.
Now the ugly one: modify io.quarkus.gizmo2.impl.constant.ConstantImpl#of(java.lang.constant.DynamicConstantDesc<?>) to detect a DynamicConstantDesc which matches the following described constants from the wrapper classes, and redirects them to the correct type-specific of method:
- For these three, check the component constantName(), constantType(), and bootstrapMethod():
  - DynamicConstantDesc.ofNamed(BSM_EXPLICIT_CAST, DEFAULT_NAME, CD_byte, (int) value)
  - DynamicConstantDesc.ofNamed(BSM_EXPLICIT_CAST, DEFAULT_NAME, CD_char, (int) value)
  - DynamicConstantDesc.ofNamed(BSM_EXPLICIT_CAST, DEFAULT_NAME, CD_short, (int) value)
- For these two, just use equals:
  - java.lang.constant.ConstantDescs#TRUE
  - java.lang.constant.ConstantDescs#FALSE

This way, no matter what weird way the user sends in a Byte, Short, etc., we always get the same correctly-typed constant out of it.

I hope this makes sense!

Ladicek · 2025-03-27T15:10:11Z

Yeah, I originally started doing something like that, but @mkouba's comment that Byte, Short and Character are Constable made me lazy :-) I'll fix this properly, thanks!

Ladicek · 2025-03-27T17:01:29Z

Done!

dmlloyd

Looks good other than two minor questions. Thanks!

src/main/java/io/quarkus/gizmo2/impl/constant/ConstantImpl.java

dmlloyd · 2025-03-27T17:53:27Z

src/main/java/io/quarkus/gizmo2/Constant.java

+     * @param value the value to create a constant from
+     */
+    static Constant of(byte value) {
+        return ConstantImpl.of((Byte) value);


These methods could just be ConstantImpl.of(value) right?

Good point, will fix.

mkouba · 2025-03-28T07:28:49Z

src/main/java/io/quarkus/gizmo2/impl/GizmoImpl.java

@@ -98,6 +101,18 @@ public StringConstant stringConstant(String value) {
        return (StringConstant) constants.computeIfAbsent(value, StringConstant::new);
    }

+    public ByteConstant byteConstant(Byte value) {
+        return (ByteConstant) funnyHashCodeConstants.computeIfAbsent(new ByteConstant(value), Function.identity());


Shouldn't we try to find a better name for funnyHashCodeConstants? 🤔

I would personally merge constants and funnyHashCodeConstants together (using the funnyHashCodeConstants approach), not sure why they're separate (except perhaps a little less allocations in the constants case?).

mkouba

We could also extend the BoxUnboxTest now...

Ladicek · 2025-03-28T07:43:23Z

We could also extend the BoxUnboxTest now...

Let me take a look.

Ladicek · 2025-03-28T08:46:09Z

Done. I also adjusted BoxUnboxTest, which prompted a small fix in Rel.

src/main/java/io/quarkus/gizmo2/impl/constant/ConstantImpl.java

We need constants of types `byte`, `short` and `char` because without them, we cannot statically make a difference between them and `int`, which may lead to all kinds of issues. For example, we could emit an invocation of a different overload of a method than expected, if the target class has an overload of the method for both `byte` (or `short` or `char`) and `int`. In addition to that, this commit also adds `Constant.of(Boolean)`, because `java.lang.Boolean` constants were not of the primitive type, unlike all others. This might be a bad idea, but all other constants of wrapper types are in fact constants of the corresponding primitive type, and consistency is warranted. We might later want to rethink that.

dmlloyd · 2025-03-28T12:31:30Z

Awesome, thanks!

Ladicek requested review from dmlloyd and mkouba March 27, 2025 10:27

Ladicek force-pushed the fix-primitive-constants-2.x branch from cd4fbe4 to 673db7f Compare March 27, 2025 14:51

Ladicek force-pushed the fix-primitive-constants-2.x branch from 673db7f to a8913c7 Compare March 27, 2025 17:01

dmlloyd reviewed Mar 27, 2025

View reviewed changes

mkouba reviewed Mar 28, 2025

View reviewed changes

Ladicek force-pushed the fix-primitive-constants-2.x branch from a8913c7 to 8aae32a Compare March 28, 2025 08:45

mkouba approved these changes Mar 28, 2025

View reviewed changes

dmlloyd reviewed Mar 28, 2025

View reviewed changes

src/main/java/io/quarkus/gizmo2/impl/constant/ConstantImpl.java Outdated Show resolved Hide resolved

Ladicek force-pushed the fix-primitive-constants-2.x branch from 8aae32a to 879cafa Compare March 28, 2025 12:30

dmlloyd approved these changes Mar 28, 2025

View reviewed changes

dmlloyd merged commit 80e7448 into quarkusio:2.x Mar 28, 2025
2 checks passed

Ladicek deleted the fix-primitive-constants-2.x branch March 28, 2025 12:32

Ladicek added the 2.x Issue applies to Gizmo 2.x label Mar 28, 2025

Ladicek added this to WG - Gizmo 2 Apr 7, 2025

Ladicek moved this to Done in WG - Gizmo 2 Apr 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix primitive constants #264

fix primitive constants #264

Uh oh!

Ladicek commented Mar 27, 2025

Uh oh!

dmlloyd commented Mar 27, 2025 •

edited

Loading

Uh oh!

Ladicek commented Mar 27, 2025 •

edited

Loading

Uh oh!

Ladicek commented Mar 27, 2025

Uh oh!

dmlloyd left a comment

Uh oh!

Uh oh!

dmlloyd Mar 27, 2025

Uh oh!

Ladicek Mar 28, 2025

Uh oh!

Ladicek Mar 28, 2025

Uh oh!

mkouba Mar 28, 2025

Uh oh!

Ladicek Mar 28, 2025

Uh oh!

mkouba left a comment

Uh oh!

Ladicek commented Mar 28, 2025

Uh oh!

Ladicek commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

dmlloyd commented Mar 28, 2025

Uh oh!

Uh oh!

fix primitive constants #264

fix primitive constants #264

Uh oh!

Conversation

Ladicek commented Mar 27, 2025

Uh oh!

dmlloyd commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ladicek commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ladicek commented Mar 27, 2025

Uh oh!

dmlloyd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmlloyd Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

Ladicek Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

Ladicek Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

mkouba Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

Ladicek Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

mkouba left a comment

Choose a reason for hiding this comment

Uh oh!

Ladicek commented Mar 28, 2025

Uh oh!

Ladicek commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

dmlloyd commented Mar 28, 2025

Uh oh!

Uh oh!

dmlloyd commented Mar 27, 2025 •

edited

Loading

Ladicek commented Mar 27, 2025 •

edited

Loading