[code-reflection] RFR: More systematic binary op tests

[Paul Sandoz]

On Sat, 16 Mar 2024 09:09:19 GMT, Hannes Greule <hgreule at openjdk.org> wrote:

> This is more of an experiment to gather some opinions. Please let me know what you think.
> 
> With this change, I'm trying to cover interpreter and bytecode generation/execution of binary ops in a more systematic approach.
> 
> I didn't find a good way to automatically *generate* test cases, especially as there is insufficient information about allowed types. Therefore, this approach consists of two main steps:
> 
> 1. A test that ensures that all bin ops have at least one test case
> 2. Dynamic tests based on these test cases
> 
> Note that these tests currently all fail. It seems like neither the interpreter nor the bytecode generator can deal with invokevirtual methods properly, so unboxing always fails.
> 
> ## Alternatives
> 
> One other idea I had was to, instead of going through the dynamic test cases, go through all methods in that class annotated with `@CodeReflection` and check this way if all bin ops are covered. This might allow for more extension to e.g. unary ops. The rest of the tests would then work as in other test classes. The problem of different data types and values would still exist though. Maybe a property-testing approach like `jqwik` would help with different values.

`CoreBinaryOpsTest` looks good. Recommend you place the reflected template methods and the test method as close to the top of the class as possible.

Oops, unboxing is not creating the correct method reference. I will fix that separately. The following patch should unblock you.


diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java
index 999ed3b73bb..8cbd9d21b64 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ReflectMethods.java
@@ -646,6 +646,7 @@ Value convert(Value exprVal, Type target) {
         }
 
         Value box(Value valueExpr, Type box) {
+            // Boxing is a static method e.g., java.lang.Integer::valueOf(int)java.lang.Integer
             MethodRef boxMethod = MethodRef.method(typeToTypeElement(box), names.valueOf.toString(),
                     FunctionType.functionType(typeToTypeElement(box), typeToTypeElement(types.unboxedType(box))));
             return append(CoreOps.invoke(boxMethod, valueExpr));
@@ -657,9 +658,10 @@ Value unbox(Value valueExpr, Type box, Type primitive, Type unboxedType) {
                 unboxedType = primitive;
                 valueExpr = append(CoreOps.cast(typeToTypeElement(types.boxedClass(unboxedType).type), valueExpr));
             }
+            // Unboxing is a virtual method e.g., java.lang.Integer::intValue()int
             MethodRef unboxMethod = MethodRef.method(typeToTypeElement(box),
                     unboxedType.tsym.name.append(names.Value).toString(),
-                    FunctionType.functionType(typeToTypeElement(unboxedType), typeToTypeElement(box)));
+                    FunctionType.functionType(typeToTypeElement(unboxedType)));
             return append(CoreOps.invoke(unboxMethod, valueExpr));
         }



I will look more closely at what you have done. It's very unlikely we would be able on additional third party dependencies for testing. We probably don't need to be exhaustive in the data and i suspect we can use junit's API to extend the test functionality.

Thanks for confirming (i integrated a PR to fix it). I still need to look more deeply at the testing approach. In the interim here are a few more general comments.

That's an interesting use of `OpDefinition` i never really anticipated, but it does lend itself to more general production of operations. Here is a more concise way (when the lambda does not capture):


        CoreOps.LambdaOp lop = ...;
        CoreOps.FuncOp fop = CoreOps.func("test", lop.body().copy(CopyContext.create()));


or slightly less concise:


        CoreOps.LambdaOp lop = ...;
        CoreOps.FuncOp fop = CoreOps.func("test", lop.invokableType()).body(b -> {
           b.transformBody(lop.body(), b.parameters(), OpTransformer.COPYING_TRANSFORMER);
        });



However, we would not have to do this if the bytecode generator more generally accepted an invokable op. We can fix that separately.

Very good. We can separate out later if need be (should be simple if source resides in the same package) and when we see an opportunity to adapt and reuse it (we might when a PR is published for string concatenation).

test/jdk/java/lang/reflect/code/CoreBinaryOpsTest.java line 246:

> 244:                 first.throwable.printStackTrace();
> 245:                 second.throwable.printStackTrace();
> 246:                 fail("Different exceptions where thrown");

Suggestion:

                fail("Different exceptions were thrown");

test/jdk/java/lang/reflect/code/CoreBinaryOpsTest.java line 248:

> 246:                 fail("Different exceptions where thrown");
> 247:             }
> 248:             assertEquals(first.throwable.getClass(), second.throwable.getClass());

Never fails? Due to the prior equality check.

-------------

PR Review: https://git.openjdk.org/babylon/pull/40#pullrequestreview-1984194016
PR Comment: https://git.openjdk.org/babylon/pull/40#issuecomment-2004512721
PR Comment: https://git.openjdk.org/babylon/pull/40#issuecomment-2005179260
PR Comment: https://git.openjdk.org/babylon/pull/40#issuecomment-2043512223
PR Review Comment: https://git.openjdk.org/babylon/pull/40#discussion_r1554307628
PR Review Comment: https://git.openjdk.org/babylon/pull/40#discussion_r1554308981