<i18n dev> RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v46]

Tue Mar 21 14:55:32 UTC 2023

On Mon, 20 Mar 2023 20:31:46 GMT, Jim Laskey <jlaskey at openjdk.org> wrote:

>> Enhance the Java programming language with string templates, which are similar to string literals but contain embedded expressions. A string template is interpreted at run time by replacing each expression with the result of evaluating that expression, possibly after further validation and transformation. This is a [preview language feature and API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Tidy javadoc
>  - Rename StringTemplate classes

I like the new naming scheme!

src/java.base/share/classes/java/lang/StringTemplate.java line 28:

> 26: package java.lang;
> 27: 
> 28: import java.lang.Object;

You might want to do another pass to check for unused imports

src/java.base/share/classes/java/lang/StringTemplate.java line 44:

> 42:  * {@link StringTemplate} is the run-time representation of a string template or
> 43:  * text block template in a template expression.
> 44:  *

Extra newline?

src/java.base/share/classes/java/lang/StringTemplate.java line 56:

> 54:  * {@link StringTemplate} is primarily used in conjunction with a template processor
> 55:  * to produce a string or other meaningful value. Evaluation of a template expression
> 56:  * first produces an instance of {@link StringTemplate}, representing the template

The "template of the template expression" - is this nomenclature official (e.g. backed by any text in the JLS?). I must admit I find it a tad jarring.

src/java.base/share/classes/java/lang/StringTemplate.java line 69:

> 67:  * List<Object> values = st.values();
> 68:  * }
> 69:  * The value of {@code fragments} will be equivalent to {@code List.of("", " + ", " = ", "")},

This is a bit hard to parse, due to the use of `the value of` (which then becomes problematic in the next para, as we are talking about `values`). Either change the name of the variable `values` in the snippet, or use some other locution, like "the list {@code fragments} is equivalent to ..." etc.

src/java.base/share/classes/java/lang/StringTemplate.java line 324:

> 322:      * assert Objects.equals(sc, "abcxyz");
> 323:      * }
> 324:      * the last character {@code "c"} from the first string is juxtaposed with the first

I was playing with this example in jshell:

jshell> var st1 = RAW."{1}"
st1 ==> StringTemplate{ fragments = [ "", "" ], values = [1] }

jshell> var st2 = RAW."{2}"
st2 ==> StringTemplate{ fragments = [ "", "" ], values = [2] }

jshell> StringTemplate.combine(st1, st2);
$7 ==> StringTemplate{ fragments = [ "", "", "" ], values = [1, 2] }

The result is obviously well-formed - but I'm not sure I can derive what the method does just by reading the javadoc. The javadoc says:

Fragment lists from the StringTemplates are combined end to end with the last fragment from each StringTemplate concatenated with the first fragment of the next

I think I see what you want to say - (e.g. the end fragment of string template `N` is concatenated with the starting fragment of string template `N + 1`).

src/java.base/share/classes/java/lang/StringTemplate.java line 431:

> 429:      * Java compilation unit.<p>The result of interpolation is not interned.
> 430:      */
> 431:     static final StringProcessor STR = StringTemplate::interpolate;

`static final` is redundant here (we are in an interface)

src/java.base/share/classes/java/lang/StringTemplate.java line 454:

> 452:      * This interface describes the methods provided by a generalized string template processor. The
> 453:      * primary method {@link Processor#process(StringTemplate)} is used to validate
> 454:      * and compose a result using a {@link StringTemplate StringTemplate's} fragments and values lists.

nit: `{@link StringTemplate StringTemplate's}` will probably not render as you'd expect (as it will all be preformat, including the `'s`).

src/java.base/share/classes/java/lang/runtime/ReferenceKey.java line 47:

> 45:  */
> 46: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
> 47: interface ReferenceKey<T> {

This (and other) class are package-private. Do we still need @PreviewFeature for stuff like this, since it's not meant to be used publicly?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 226:

> 224:                         List.nil(), expressions);
> 225:                 valuesArray.type = new ArrayType(syms.objectType, syms.arrayClass);
> 226:                 return bsmCall(names.process, names.newLargeStringTemplate, syms.stringTemplateType,

nit: the indy name is irrelevant here - that said, `process` is a tad confusing, since it's also the name of a StringTemplate method.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 252:

> 250:                 staticArgsTypes = staticArgsTypes.append(syms.stringType);
> 251:             }
> 252:             return bsmCall(names.process, names.processStringTemplate, tree.type,

What happens if we have too many fragments here? (e.g. > 200). That case seems handled in the RAW case.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line 281:

> 279:             make.at(tree.pos);
> 280: 
> 281:             if (processor == null || isNamedProcessor(names.RAW)) {

is `processor == null` still a thing?

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

PR Review: https://git.openjdk.org/jdk/pull/10889#pullrequestreview-1350457321
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143370713
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143371242
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143373826
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143379210
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143441733
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143444292
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143448931
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143484050
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143501675
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143507003
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143487363