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

Thu Mar 23 01:52:31 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

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

> 447:      * statically imported explicitly.
> 448:      */
> 449:     static final SimpleProcessor<StringTemplate> RAW = st -> st;

Should we omit the modifiers `static final`, which are implicit for all interface fields?

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

> 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.
> 455:      *

Paragraph break intended here?

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

> 548:      * SimpleProcessor<JSONObject> jsonProcessor = st -> new JSONObject(st.interpolate());
> 549:      * }
> 550:      * @implNote The Java compiler automatically imports {@link StringTemplate#STR}

Does this note belong here? And does this come with a rule in the Java Language Specification, which can be linked?

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

> 590:          */
> 591:         @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
> 592:         public sealed interface Linkage permits FormatProcessor {

I fail to see any reason a user would call `linkage`; is there anything that's not covered by `TemplateRuntime::processStringTemplate`? And if users can't use this, this can just be relocated to one of the `jdk.internal` packages.

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

> 677:     @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
> 678:     @FunctionalInterface
> 679:     public interface StringProcessor extends SimpleProcessor<String> {

Just curious, what's the rationale of a `SimpleProcessor` or `StringProcessor` as opposed to:

static <T> TemplateProcessor<T, RuntimeException> simple(Function<StringTemplate, T> function) {...}
static TemplateProcessor<String, RuntimeException> string(Function<StringTemplate, String> function) {...}

which avoids the requirement of specifying the type of the template processor local variable; users can use `var` instead.

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1051:

> 1049:      * @param ptypes    list of expression types
> 1050:      *
> 1051:      * @return {@link MethodHandle}

Suggestion:

     * @return the {@link MethodHandle} for concatenation

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1169:

> 1167:      * have an extra {@link String} slot for the result from the previous
> 1168:      * {@link MethodHandle}.
> 1169:      * {@link java.lang.invoke.StringConcatFactory#makeConcatWithTemplate}

Suggestion:

     * {@link #makeConcatWithTemplate}

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1246:

> 1244:      * This method creates a {@link MethodHandle} expecting one input, the
> 1245:      * receiver of the supplied getters. This method uses
> 1246:      * {@link java.lang.invoke.StringConcatFactory#makeConcatWithTemplateCluster}

Suggestion:

     * {@link #makeConcatWithTemplateCluster}

src/java.base/share/classes/java/lang/runtime/Carriers.java line 370:

> 368:          */
> 369:         private static Map<MethodType, CarrierElements>
> 370:                 methodTypeCache = ReferencedKeyMap.create(() -> new ConcurrentHashMap<>());

Suggestion:

                methodTypeCache = ReferencedKeyMap.create(ConcurrentHashMap::new);

src/java.base/share/classes/java/lang/runtime/Carriers.java line 421:

> 419:          */
> 420:         protected CarrierObject(int primitiveCount, int objectCount) {
> 421:             this.primitives = createPrimitivesArray(primitiveCount );

Suggestion:

            this.primitives = createPrimitivesArray(primitiveCount);

src/java.base/share/classes/java/lang/runtime/Carriers.java line 776:

> 774:          * @throws IllegalArgumentException if number of component slots exceeds maximum
> 775:          */
> 776:         static CarrierElements of(Class < ? >...ptypes) {

Suggestion:

        static CarrierElements of(Class<?>... ptypes) {

src/java.base/share/classes/java/lang/runtime/Carriers.java line 824:

> 822:         private CarrierElements() {
> 823:             throw new AssertionError("private constructor");
> 824:         }

Suggestion:

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

> 47: interface ReferenceKey<T> {
> 48:     /**
> 49:      * {@return the value of the unwrapped key.}

Suggestion:

     * {@return the value of the unwrapped key}

Inline return tag generates a period already.

src/java.base/share/classes/java/lang/runtime/ReferencedKeyMap.java line 127:

> 125:     @SuppressWarnings("unchecked")
> 126:     static <K, V> ReferencedKeyMap<K, V>
> 127:     create(boolean isSoft, Supplier<Map<?, ?>> supplier) {

What prevents this to take `Supplier<Map<ReferencedKey<K>, V>>`? Same for below.

src/java.base/share/classes/java/lang/runtime/StringTemplateImpl.java line 40:

> 38:  * <p>
> 39:  * Values are stored by subclassing {@link Carriers.CarrierObject}. This allows specializations
> 40:  * and sharing of value shapes without creating a new class for each shape.

Just curious, what specific benefits does subclassing offer over having `CarrierObject` as a field? I don't see how a new class is ever created for each shape in that scenario, as shapes are only used to construct the 2 MHs in the factory.

src/java.base/share/classes/java/lang/runtime/StringTemplateImpl.java line 65:

> 63:      * {@link java.lang.invoke.CallSite CallSite}.
> 64:      */
> 65:     private final MethodHandle valuesMH;

I don't think `java.lang.runtime` is a package where final fields are [trusted](https://github.com/openjdk/jdk/blob/91f407d6fe285c44bcc25c1acdf5dc0c43be0172/src/hotspot/share/ci/ciField.cpp#L226), so these method handles might have a performance overhead. However, records appear to be optimized and eligible for inlining.

src/java.base/share/classes/java/lang/runtime/StringTemplateImplFactory.java line 88:

> 86:      *
> 87:      * @param fragments  string template fragments
> 88:      * @param type       method type

Suggestion:

     * @param type method type accepting values' types and returning a StringTemplate

src/java.base/share/classes/java/lang/runtime/StringTemplateImplFactory.java line 169:

> 167:      * @return StringTemplate composed from fragments and values
> 168:      */
> 169:     static StringTemplate newStringTemplate(List<String> fragments, Object[] values) {

I recommend renaming this one and the above methods (taking Object[] for values) `newTrustedStringTemplate` to indicate these two trust the passed values array.

src/java.base/share/classes/java/lang/runtime/TemplateRuntime.java line 204:

> 202:             Object[] values
> 203:     ) throws Throwable {
> 204:         List<Object> asList = Collections.unmodifiableList(new ArrayList<>(Arrays.asList(values)));

Suggestion:

        List<Object> asList = List.of(values);

For defensive copy.
Don't think processors are harmed by the null-hostile behavior of these list, for many template implementations already use null-hostile lists.

src/java.base/share/classes/java/lang/runtime/TemplateRuntime.java line 233:

> 231:      */
> 232:     private static StringTemplate fromArrays(String[] fragments, Object[] values) {
> 233:         return StringTemplateImplFactory.newStringTemplate(fragments, values);

Should perform defensive copying for the input arrays, specifically `values` (fragments is passed to `List.of`, which is already safe)

In addition, I recommend renaming methods that don't perform defensive copies to like `ofTrusted`, so we can better avoid such problems.

src/java.base/share/classes/java/util/Digits.java line 68:

> 66:      */
> 67:     final class DecimalDigits implements Digits {
> 68:         private static final short[] DIGITS;

Can add `@Stable` to speed up array element access. Same for below.

src/java.base/share/classes/java/util/FormatProcessor.java line 157:

> 155:         Objects.requireNonNull(stringTemplate);
> 156:         String format = stringTemplateFormat(stringTemplate.fragments());
> 157:         Object[] values = stringTemplate.values().toArray(new Object[0]);

Suggestion:

        Object[] values = stringTemplate.values().toArray();

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

PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145500281
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145500568
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145503416
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145582334
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145509343
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145511968
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145520158
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145521984
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145532679
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145528633
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145531061
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145531547
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145533037
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145534263
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145566980
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145555878
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145567708
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145576079
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145577290
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145570455
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145578159
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1145579950