RFR: 8344942: Template-Based Testing Framework [v71]
Christian Hagedorn
chagedorn at openjdk.org
Mon Jun 2 13:58:22 UTC 2025
On Mon, 2 Jun 2025 03:30:24 GMT, Emanuel Peter <epeter at openjdk.org> wrote:
>> **Goal**
>> We want to generate Java source code:
>> - Make it easy to generate variants of tests. E.g. for each offset, for each operator, for each type, etc.
>> - Enable the generation of domain specific fuzzers (e.g. random expressions and statements).
>>
>> Note: with the Template Library draft I was already able to find a [list of bugs](https://bugs.openjdk.org/issues/?jql=labels%20%3D%20template-framework%20ORDER%20BY%20created%20DESC%2C%20summary%20DESC).
>>
>> **How to get started**
>> When reviewing, please start by looking at:
>> https://github.com/openjdk/jdk/blob/d21a8aabaf3b191e851b6997c11bb30fcd0f942f/test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestSimple.java#L60-L76
>>
>> We have a Template with two arguments. They are typed (Integer and String). We then apply the arguments `template.withArgs(42, "7")`, producing a `TemplateWithArgs`. This can then be `render`ed to a String. And then that can be compiled and executed with the CompileFramework.
>>
>> Second, look at this advanced test:
>> https://github.com/openjdk/jdk/blob/77079807042fc5a3af04e0ccccad4ecd89e21cdb/test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestAdvanced.java#L102-L119
>>
>> And then for a "tutorial", look at:
>> `test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestTutorial.java`
>>
>> It shows these features:
>> - The `body` of a Template is essentially a list of `Token`s that are concatenated.
>> - Templates can be nested: a `TemplateWithArgs` is also a `Token`.
>> - We can use `#name` replacements to directly format values into the String. If we had proper String Templates in Java, we would not need this feature.
>> - We can use `$var` to make variable names unique: if we applied the same template twice, we would get variable collisions. `$var` is then replaced with e.g. `var_7` in one template use and `var_42` in the other template use.
>> - The use of `Hook`s to insert code into outer (earlier) code locations. This is useful, for example, to insert fields on demand.
>> - The use of recursive templates, and `fuel` to limit the recursion.
>> - `Name`s: useful to register field and variable names in code scopes.
>>
>> Next, look at the documentation in. This file is the heart of the Template Framework, and describes all the important features.
>> https://github.com/openjdk/jdk/blob/d21a8aabaf3b191e851b6997c11bb30fcd0f942f/test/hotspot/jtreg/compiler/lib/template_framework/Template.java#L31-L76
>>
>> For a better experience, you may want...
>
> Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 91 commits:
>
> - Merge branch 'master' into JDK-8344942-TemplateFramework-v3
> - validation tests
> - dollar and hashtag parsing validatiaon
> - wip refactor parsing dollar and hashtag
> - more fixes from Christian
> - more improvements
> - more suggestions applied
> - good practice
> - rename template arguments
> - more from Christian
> - ... and 81 more: https://git.openjdk.org/jdk/compare/90d6ad01...cb7037e7
I worked my way through the rest of the implementation. Impressive work Emanuel! I left some more mostly minor comments. But otherwise, this looks great!
test/hotspot/jtreg/compiler/lib/template_framework/Code.java line 26:
> 24: package compiler.lib.template_framework;
> 25:
> 26: import java.util.ArrayList;
Unused:
Suggestion:
test/hotspot/jtreg/compiler/lib/template_framework/Code.java line 33:
> 31: * All the {@link String}s are later collected in a {@link StringBuilder}. If we used a {@link StringBuilder}
> 32: * directly to collect the {@link String}s, we could not as easily insert code at an "earlier" position, i.e.
> 33: * reaching out to a {@link Hook#set}.
Suggestion:
* reaching out to a {@link Hook#anchor}.
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 37:
> 35: * When a {@link Hook} is {@link Hook#set}, this separates the Template into an outer and inner
> 36: * {@link CodeFrame}, ensuring that names that are {@link Template#addName}'d inside the inner frame
> 37: * are only available inside that frame.
Still references old method names. Suggestion:
Suggestion:
* The {@link CodeFrame} represents a frame (i.e. scope) of code, appending {@link Code} to the {@code 'codeList'}
* as {@link Token}s are rendered, and adding names to the {@link NameSet}s with {@link Template#addStructuralName}/
* {@link Template#addDataName}. {@link Hook}s can be added to a frame, which allows code to be inserted at that
* location later. When a {@link Hook} is {@link Hook#anchor}ed, it separates the Template into an outer and inner
* {@link CodeFrame}, ensuring that names that are added inside the inner frame are only available inside that frame.
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 52:
> 50: class CodeFrame {
> 51: public final CodeFrame parent;
> 52: private final List<Code> codeList = new ArrayList<Code>();
Suggestion:
private final List<Code> codeList = new ArrayList<>();
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 58:
> 56: * The {@link NameSet} is used for variable and fields etc.
> 57: */
> 58: final NameSet names;
I think this can also be made private:
Suggestion:
private final NameSet names;
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 70:
> 68: } else {
> 69: // New NameSet, to make sure we have a nested scope for the names.
> 70: this.names = new NameSet(parent.names);
Indentation is off:
Suggestion:
this.names = parent.names;
} else {
// New NameSet, to make sure we have a nested scope for the names.
this.names = new NameSet(parent.names);
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 92:
> 90: /**
> 91: * Creates a special frame, which has a {@link #parent} but uses the {@link NameSet}
> 92: * from the parent frame, allowing {@link Template#defineName} to persist in the outer
`defineName` -> `addName`?
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 96:
> 94: * where we would possibly want to make field or variable definitions during the insertion
> 95: * that are not just local to the insertion but affect the {@link CodeFrame} that we
> 96: * {@link Hook#set} earlier and are now {@link Hook#insert}ing into.
Suggestion:
* {@link Hook#anchor} earlier and are now {@link Hook#insert}ing into.
test/hotspot/jtreg/compiler/lib/template_framework/CodeFrame.java line 118:
> 116: }
> 117:
> 118: boolean hasHook(Hook hook) {
Can be made private:
Suggestion:
private boolean hasHook(Hook hook) {
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 33:
> 31: * count, list or even sample random {@link DataName}s. Every {@link DataName} has a {@link DataName.Type},
> 32: * so that sampling can be restricted to these types.
> 33: *
Suggestion:
*
* <p>
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 123:
> 121: if (mutability == Mutability.IMMUTABLE && dn.mutable()) { return false; }
> 122: if (subtype != null && !dn.type().isSubtypeOf(subtype)) { return false; }
> 123: if (supertype != null && !supertype.isSubtypeOf(dn.type())) { return false; }
I suggest to use the full term:
Suggestion:
if (!(name instanceof DataName dataName)) { return false; }
if (mutability == Mutability.MUTABLE && !dataName.mutable()) { return false; }
if (mutability == Mutability.IMMUTABLE && dataName.mutable()) { return false; }
if (subtype != null && !dataName.type().isSubtypeOf(subtype)) { return false; }
if (supertype != null && !supertype.isSubtypeOf(dataName.type())) { return false; }
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 134:
> 132: * @return The filtered {@link View}.
> 133: * @throws UnsupportedOperationException If this {@link View} was already filtered with
> 134: * {@link subtypeOf} or {@link exactOf}.
Also for links at methods below:
Suggestion:
* {@link #subtypeOf} or {@link #exactOf}.
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 144:
> 142:
> 143: /**
> 144: * Create a filtered {@link View}, where all {@link DataName}s must be subtypes of {@code type}.
Suggestion:
* Create a filtered {@link View}, where all {@link DataName}s must be supertypes of {@code type}.
test/hotspot/jtreg/compiler/lib/template_framework/DataName.java line 181:
> 179: */
> 180: public DataName sample() {
> 181: DataName n = (DataName)Renderer.getCurrent().sampleName(predicate());
Do you really need this cast? Can't you just return a `Name`. From the uses it seems that you only call interface methods from `Name` at the use-sites.
test/hotspot/jtreg/compiler/lib/template_framework/Hook.java line 34:
> 32: * "back" or to some outer scope, e.g. while generating code for a method, one can reach out
> 33: * to the class scope to insert fields.
> 34: *
Suggestion:
*
* <p>
test/hotspot/jtreg/compiler/lib/template_framework/Name.java line 35:
> 33: * The name of the name, that can be used in code.
> 34: *
> 35: * @return The {@String} name of the name, that can be used in code.
Suggestion:
* @return The {@link String} name of the name, that can be used in code.
test/hotspot/jtreg/compiler/lib/template_framework/Name.java line 54:
> 52: int weight();
> 53:
> 54: public interface Type {
Implicitly public:
Suggestion:
interface Type {
test/hotspot/jtreg/compiler/lib/template_framework/NameSet.java line 38:
> 36: */
> 37: class NameSet {
> 38: static final Random RANDOM = Utils.getRandomInstance();
Suggestion:
private static final Random RANDOM = Utils.getRandomInstance();
test/hotspot/jtreg/compiler/lib/template_framework/NameSet.java line 58:
> 56:
> 57: private long weight(Predicate predicate) {
> 58: long w = names.stream().filter(n -> predicate.check(n)).mapToInt(Name::weight).sum();
Suggestion:
long w = names.stream().filter(predicate::check).mapToInt(Name::weight).sum();
test/hotspot/jtreg/compiler/lib/template_framework/NameSet.java line 64:
> 62:
> 63: public int count(Predicate predicate) {
> 64: int c = (int)names.stream().filter(n -> predicate.check(n)).count();
Suggestion:
int c = (int)names.stream().filter(predicate::check).count();
test/hotspot/jtreg/compiler/lib/template_framework/NameSet.java line 70:
> 68:
> 69: public boolean hasAny(Predicate predicate) {
> 70: return names.stream().anyMatch(n -> predicate.check(n)) ||
Suggestion:
return names.stream().anyMatch(predicate::check) ||
test/hotspot/jtreg/compiler/lib/template_framework/NameSet.java line 77:
> 75: List<Name> list = (parent != null) ? parent.toList(predicate)
> 76: : new ArrayList<>();
> 77: list.addAll(names.stream().filter(n -> predicate.check(n)).toList());
Suggestion:
list.addAll(names.stream().filter(predicate::check).toList());
test/hotspot/jtreg/compiler/lib/template_framework/NameSet.java line 88:
> 86: if (w <= 0) {
> 87: return null;
> 88: }
Shouldn't the weight always be positive?
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 66:
> 64: // another non-capturing group.
> 65: "(?:\\{" +
> 66: // capturing group for "name" inside of "{name}"
Suggestion:
// capturing group for "name" inside "{name}"
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 199:
> 197: /**
> 198: * Formats values to {@link String} with the goal of using them in Java code.
> 199: * By default we use the overrides of {@link Object#toString}.
Suggestion:
* By default, we use the overrides of {@link Object#toString}.
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 266:
> 264: case StringToken(String s) -> {
> 265: renderStringWithDollarAndHashtagReplacements(s);
> 266: }
Suggestion:
case StringToken(String s) -> renderStringWithDollarAndHashtagReplacements(s);
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 321:
> 319: callerCodeFrame.addCode(currentCodeFrame.getCode());
> 320: currentCodeFrame = callerCodeFrame;
> 321: }
For readability:
Suggestion:
case HookInsertToken(Hook hook, TemplateToken templateToken) -> {
// Switch to hook CodeFrame.
CodeFrame callerCodeFrame = currentCodeFrame;
CodeFrame hookCodeFrame = codeFrameForHook(hook);
// Use a transparent nested CodeFrame. We need a CodeFrame so that the code generated
// by the TemplateToken can be collected, and hook insertions from it can still
// be made to the hookCodeFrame before the code from the TemplateToken is added to
// the hookCodeFrame.
// But the CodeFrame must be transparent, so that its name definitions go out to
// the hookCodeFrame, and are not limited to the CodeFrame for the TemplateToken.
currentCodeFrame = CodeFrame.makeTransparentForNames(hookCodeFrame);
renderTemplateToken(templateToken);
hookCodeFrame.addCode(currentCodeFrame.getCode());
// Switch back from hook CodeFrame to caller CodeFrame.
currentCodeFrame = callerCodeFrame;
}
case TemplateToken templateToken -> {
// Use a nested CodeFrame.
CodeFrame callerCodeFrame = currentCodeFrame;
currentCodeFrame = CodeFrame.make(currentCodeFrame);
renderTemplateToken(templateToken);
callerCodeFrame.addCode(currentCodeFrame.getCode());
currentCodeFrame = callerCodeFrame;
}
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 324:
> 322: case AddNameToken(Name name) -> {
> 323: currentCodeFrame.addName(name);
> 324: }
Suggestion:
case AddNameToken(Name name) -> currentCodeFrame.addName(name);
test/hotspot/jtreg/compiler/lib/template_framework/Renderer.java line 338:
> 336: }
> 337:
> 338: private void renderStringWithDollarAndHashtagReplacements(String s) {
Hard to grasp the logic of that method. But I trust you on that :-) I leave it up to you if you want to improve readability to extract some of the logic to separate methods such that this method becomes easier to understand.
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 33:
> 31: * count, list or even sample random {@link StructuralName}s. Every {@link StructuralName} has a {@link StructuralName.Type},
> 32: * so that sampling can be restricted to these types.
> 33: *
Suggestion:
*
* <p>
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 47:
> 45: */
> 46: public StructuralName {
> 47: }
Is this required? Is it not automatically added? Same for `DataName`.
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 68:
> 66: */
> 67: boolean isSubtypeOf(StructuralName.Type other);
> 68: }
This is identical to `DataName.Type`. What is the benefit of having separate interfaces `DataName.Type` and `StructuralName.Type`? Couldn't we just move `isSubtypeOf()` directly to the `Name.Type` interface and use that one below and for the fields and expose that one instead to the user? This would mean that you can update all `DataName/StructuralName.Type` to `Name.Type`. I have not checked if this is fully possible but it just occurred to me when reviewing this duplicated interface now.
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 96:
> 94: if (!(name instanceof StructuralName dn)) { return false; }
> 95: if (subtype != null && !dn.type().isSubtypeOf(subtype)) { return false; }
> 96: if (supertype != null && !supertype.isSubtypeOf(dn.type())) { return false; }
Suggestion:
if (!(name instanceof StructuralName structuralName)) { return false; }
if (subtype != null && !structuralName.type().isSubtypeOf(subtype)) { return false; }
if (supertype != null && !supertype.isSubtypeOf(structuralName.type())) { return false; }
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 107:
> 105: * @return The filtered {@link View}.
> 106: * @throws UnsupportedOperationException If this {@link View} was already filtered with
> 107: * {@link subtypeOf} or {@link exactOf}.
Same here and in methods below:
Suggestion:
* {@link #subtypeOf} or {@link #exactOf}.
test/hotspot/jtreg/compiler/lib/template_framework/StructuralName.java line 117:
> 115:
> 116: /**
> 117: * Create a filtered {@link View}, where all {@link StructuralName}s must be subtypes of {@code type}.
Suggestion:
* Create a filtered {@link View}, where all {@link StructuralName}s must be supertypes of {@code type}.
test/hotspot/jtreg/compiler/lib/template_framework/TemplateBinding.java line 43:
> 41: * Creates a new {@link TemplateBinding} that has no Template bound to it yet.
> 42: */
> 43: public TemplateBinding() {}
Can also be removed since it's the default constructor that is automatically added for you.
Suggestion:
test/hotspot/jtreg/compiler/lib/template_framework/Token.java line 31:
> 29:
> 30: /**
> 31: * The {@link Template#body} and {@link Hook#set} are given a list of tokens, which are either
Suggestion:
* The {@link Template#body} and {@link Hook#anchor} are given a list of tokens, which are either
test/hotspot/jtreg/compiler/lib/template_framework/Token.java line 74:
> 72: case Float s -> outputList.add(new StringToken(Renderer.format(s)));
> 73: case Boolean s -> outputList.add(new StringToken(Renderer.format(s)));
> 74: case List l -> parseList(l, outputList);
Not sure if we should use a raw `List` here. Would `List<?>` work as well? Would then need to update `parseList(List<Object> inputList ...)` to `List<?>` as well.
test/hotspot/jtreg/compiler/lib/template_framework/library/Hooks.java line 32:
> 30: */
> 31: public abstract class Hooks {
> 32: private Hooks() {} // Avoid instantiation and need for documentation.
With `abstract` you cannot call the constructor. But you could make `Hooks` final instead of abstract and keep the private constructor.
-------------
PR Review: https://git.openjdk.org/jdk/pull/24217#pullrequestreview-2888138689
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121190567
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121189211
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121045420
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121041625
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121043407
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121047490
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121166922
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121167248
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121170840
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121066303
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121094470
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121065215
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121100321
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121117160
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121184120
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121002671
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121005423
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121050914
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121052214
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121054013
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121054604
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121054802
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121074504
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121194420
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121221834
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121206168
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121217757
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121220490
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121228275
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121119033
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121122026
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121143144
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121124204
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121124996
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2121125964
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2120995640
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2120976577
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2120983233
PR Review Comment: https://git.openjdk.org/jdk/pull/24217#discussion_r2120989645
More information about the hotspot-compiler-dev
mailing list