-
Notifications
You must be signed in to change notification settings - Fork 109
8351362: Post-process @Strict annotation for testing #1395
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
Open
liach
wants to merge
7
commits into
openjdk:lworld
Choose a base branch
from
liach:test/strict-processor
base: lworld
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
bb1fd0d
8351362: Post-process @Strict annotation for testing
liach 65eb51b
Merge branch 'lworld' of https://github.com/openjdk/valhalla into tes…
liach 30d78fb
Add very primitive super rewrite functionality
liach a44c269
Fix a bug and enhance tests
liach 5343101
Merge branch 'lworld' of https://github.com/openjdk/valhalla into tes…
liach 06894d3
Revert back to the jdk internal strict
liach 84bdb7a
Rollback to the jdk internal strict annotation
liach File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
131 changes: 131 additions & 0 deletions
131
test/lib-test/jdk/test/lib/StrictCompilerSuperTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
/* | ||
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
* under the terms of the GNU General Public License version 2 only, as | ||
* published by the Free Software Foundation. | ||
* | ||
* This code is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
* version 2 for more details (a copy is included in the LICENSE file that | ||
* accompanied this code). | ||
* | ||
* You should have received a copy of the GNU General Public License version | ||
* 2 along with this work; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
* | ||
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
* or visit www.oracle.com if you need additional information or have any | ||
* questions. | ||
*/ | ||
|
||
/* @test | ||
* @bug 8351362 | ||
* @summary Unit Test for StrictCompiler super rewrite | ||
* @enablePreview | ||
* @library /test/lib | ||
* @run main/othervm jdk.test.lib.value.StrictCompiler --deferSuperCall StrictCompilerSuperTest.java | ||
* @run junit StrictCompilerSuperTest | ||
*/ | ||
|
||
import java.io.IOException; | ||
import java.io.UncheckedIOException; | ||
import java.lang.classfile.Attributes; | ||
import java.lang.classfile.ClassFile; | ||
import java.lang.classfile.ClassModel; | ||
import java.lang.classfile.Instruction; | ||
import java.lang.classfile.Opcode; | ||
import java.lang.reflect.AccessFlag; | ||
import java.lang.reflect.Modifier; | ||
import java.util.ArrayList; | ||
import java.util.stream.Stream; | ||
|
||
import jdk.test.lib.value.Strict; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.MethodSource; | ||
|
||
import static java.lang.classfile.ClassFile.*; | ||
import static java.lang.constant.ConstantDescs.INIT_NAME; | ||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertSame; | ||
|
||
class StrictCompilerSuperTest { | ||
static Stream<Class<?>> testClasses() { | ||
return Stream.of(Rec.class, Exp.class, Inner.class); | ||
} | ||
|
||
static Stream<ClassModel> testClassModels() { | ||
return testClasses().map(cls -> { | ||
try (var in = StrictCompilerSuperTest.class.getResourceAsStream("/" + cls.getName() + ".class")) { | ||
return ClassFile.of().parse(in.readAllBytes()); | ||
} catch (IOException ex) { | ||
throw new UncheckedIOException(ex); | ||
} | ||
}); | ||
} | ||
|
||
@MethodSource("testClasses") | ||
@ParameterizedTest | ||
void testReflectRewrittenRecord(Class<?> cls) throws Throwable { | ||
for (var field : cls.getDeclaredFields()) { | ||
if (Modifier.isStatic(field.getModifiers()) || field.isSynthetic()) | ||
continue; | ||
assertEquals(ACC_PRIVATE | ACC_STRICT | ACC_FINAL, field.getModifiers(), () -> "For field: " + field.getName()); | ||
} | ||
} | ||
|
||
@MethodSource("testClassModels") | ||
@ParameterizedTest | ||
void testRewrittenStrictAccessInClassFile(ClassModel cm) throws Throwable { | ||
for (var f : cm.fields()) { | ||
if (f.flags().has(AccessFlag.STATIC) || f.flags().has(AccessFlag.SYNTHETIC)) | ||
continue; | ||
assertEquals(ACC_PRIVATE | ACC_STRICT | ACC_FINAL, f.flags().flagsMask(), () -> "Field " + f); | ||
} | ||
} | ||
|
||
@MethodSource("testClassModels") | ||
@ParameterizedTest | ||
void testRewrittenCtorBytecode(ClassModel cm) throws Throwable { | ||
var ctor = cm.methods().stream().filter(m -> m.methodName().equalsString(INIT_NAME)).findFirst().orElseThrow(); | ||
var insts = new ArrayList<Instruction>(); | ||
ctor.findAttribute(Attributes.code()).orElseThrow().forEach(ce -> { | ||
if (ce instanceof Instruction inst) { | ||
insts.add(inst); | ||
} | ||
}); | ||
assertSame(Opcode.RETURN, insts.getLast().opcode()); | ||
assertSame(Opcode.INVOKESPECIAL, insts.get(insts.size() - 2).opcode()); | ||
} | ||
|
||
record Rec(@Strict int a, @Strict long b) { | ||
static final String NOISE = "noise"; | ||
} | ||
|
||
static class Exp { | ||
private @Strict final int a; | ||
private @Strict final long b; | ||
|
||
Exp(int a, long b) { | ||
this.a = a; | ||
this.b = b; | ||
} | ||
} | ||
|
||
class Inner { | ||
private @Strict final int a; | ||
private @Strict final long b; | ||
|
||
Inner(int a, long b) { | ||
this.a = a; | ||
this.b = b; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return a + " " + StrictCompilerSuperTest.this + " " + b; | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/* | ||
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
* under the terms of the GNU General Public License version 2 only, as | ||
* published by the Free Software Foundation. | ||
* | ||
* This code is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
* version 2 for more details (a copy is included in the LICENSE file that | ||
* accompanied this code). | ||
* | ||
* You should have received a copy of the GNU General Public License version | ||
* 2 along with this work; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
* | ||
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
* or visit www.oracle.com if you need additional information or have any | ||
* questions. | ||
*/ | ||
|
||
/* @test | ||
* @bug 8351362 | ||
* @summary Unit Test for StrictCompiler | ||
* @enablePreview | ||
* @library /test/lib | ||
* @run main/othervm jdk.test.lib.value.StrictCompiler StrictCompilerTest.java | ||
* @run junit StrictCompilerTest | ||
*/ | ||
|
||
import jdk.test.lib.value.Strict; | ||
import org.junit.jupiter.api.Test; | ||
|
||
import static java.lang.classfile.ClassFile.ACC_FINAL; | ||
import static java.lang.classfile.ClassFile.ACC_STRICT; | ||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
class StrictCompilerTest { | ||
@Test | ||
void testReflectMyself() throws Throwable { | ||
for (var field : StrictTarget.class.getDeclaredFields()) { | ||
assertEquals(ACC_STRICT | ACC_FINAL, field.getModifiers(), () -> field.getName()); | ||
} | ||
} | ||
|
||
static final class StrictTarget { | ||
@Strict | ||
final int a; | ||
@Strict | ||
final Object b; | ||
|
||
StrictTarget() { | ||
this.a = 1; | ||
this.b = 2392352234L; | ||
super(); | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/* | ||
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
* under the terms of the GNU General Public License version 2 only, as | ||
* published by the Free Software Foundation. | ||
* | ||
* This code is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
* version 2 for more details (a copy is included in the LICENSE file that | ||
* accompanied this code). | ||
* | ||
* You should have received a copy of the GNU General Public License version | ||
* 2 along with this work; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
* | ||
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
* or visit www.oracle.com if you need additional information or have any | ||
* questions. | ||
*/ | ||
|
||
package jdk.test.lib.value; | ||
|
||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
|
||
/** | ||
* Annotation to indicate the compiler that the ACC_STRICT flag should be set to | ||
* the annotated field. Used by StrictTransformer. | ||
*/ | ||
@Target(ElementType.FIELD) | ||
@Retention(RetentionPolicy.RUNTIME) | ||
public @interface Strict { | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use import
jdk.internal.vm.annotation.Strict
and not introduce a duplicate annotation?This code is already
using jdk.internal.vm.annotation.NullRestricted
.And/or add a comment here indicating it is an intentional duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dansmithcode aims to migrate all usages of the internal strict to this test-specific strict. This is actually the reason behind this PR - annotations in the Java Language must not change program semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would then apply to NullRestricted also?
That seems like an unnecessary split, since Strict is an internal annotation, it doesn't matter whether it is declared in a test specific library or jdk.internal.value. and it make maintenance harder because they are split.
And it makes prototyping uses of Strict more difficult.
And not defined or specified anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that the
@Strict
annotation should live next to the tool that interprets it.If we decide we need a translation tool in
jdk.internal
, then putting@Strict
there too makes sense. If this is purely a test-writing tool, then@Strict
belongs in tests.@NullRestricted
is interpret by the JVM, so it makes sense that it lives in the JVM's internal API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been said that in tests every NullRestricted annotated element should also have @strict.
Since @strict is there only for test generation; would it be simpler to have the tool look for NullRestricted and do the right thing; avoiding both excessive editing of tests and parallel but dependent annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, yes. The trade-off is then it's harder or confusing to write tests of validation behavior—when you explicitly want a class file that has
@NullRestricted
but not ACC_STRICT. Maybe that's a corner case we can deal with though. It also obscures the dependency on the translation tool (importing the test library is a helpful signal that you intend to use the translation tool). But, sure, that's a possible enhancement.