8237067: Implementation of <anyType>.default - (lworld branch)

Srikanth srikanth.adayapalam at oracle.com
Fri Jan 31 05:31:01 UTC 2020


Hi Jesper,

Here are some additional review comments as promised:

(7) I think the changes to allow Type.default should be keyed off of 
Feature.INLINE_TYPES (see 
com.sun.tools.javac.code.Source.Feature#allowedInSource). Earlier code 
changes done by me may have missed checking for this. Sorry about that! 
We can take the present opportunity to fix that. It may be cleaner to 
always allow this in the parser and report an error in Attr.

As of now

class X {
     X x = X.default;
}

compiles fine with valhalla tip when javac is invoked as: 
valhalla/build/linux-x86_64-server-release/images/jdk/bin/javac 
--release 13 X.java

Otherwise, it looks good!

Could you make these changes and add suitable additional tests and send 
a fresh tested patch ?

TIA,
Srikanth





On 29/01/20 10:46 am, Srikanth wrote:
>
> Hi Jesper,
>
> Thanks for the patch. Here is a first batch of review comments. I will 
> review it once more and share any additional comments on Friday, so 
> please hold off on sending a patch that incorporates these comments, 
> but this batch is shared early so you can mull over it already.
>
> (1) The change in LocaleProvidersRun.java seems unconnected to the 
> fix. I am able to back it out and the test still passes.
>
> (2) CheckBadSelector.java: it is customary to include the bug number 
> with a @bug xxxxxxxx tag
>
> (3) CheckMakeDefault.java: I apologize for having left stale 
> identifiers in this file. But now that we are changing it,
> could you (a) remove the comments "// NO: Sinner is not a value 
> class." in line 13, (b) drop the "// OK" from line 19 (c) Drop the 
> comment "// Allowed in the new 'State of Valhalla'" in line 41 (it was 
> allowed already, the name badFactory was stale)
>
> (4) DefaultNonInlines.java: (a) Copyright mentions 2018. (b) I am 
> curious - why are the statements using while instead if ?? (c) The SOP 
> is to throw new AssertionError rather than just Exception. (d) The 
> second "// Array type - differnt syntactically" is inappropriate. (d) 
> '\0' shows up highlit in red in Vim - (why ?) It accepts '\u0000' 
> without gripes.
>
> (5) Gen.java: Can we factor out the common code result = 
> items.makeStackItem(tree.type); from the 3 paths ??
>
> (6) JavacParser.java: 1345 - Just curious - what necessitates this 
> change ??
>
> Thanks
> Srikanth
>
>
>
> On 21/01/20 10:44 pm, Jesper Steen Møller wrote:
>> Hi again
>>
>> Patch appended in plain text.
>>
>>> On 21 Jan 2020, at 18.01, Srikanth <srikanth.adayapalam at oracle.com> 
>>> wrote:
>>>
>>> Hi Jesper,
>>>
>>> It looks like the patch attachment got stripped. Don't know why. 
>>> Could you inline the fix and send it ?
>>>
>>> TIA
>>> Srikanth (Mere mortal, neither Norse, nor God :) )
>>>
>>> On 21/01/20 10:28 pm, Jesper Steen Møller wrote:
>>>> Hi people (or Norse gods ?)
>>>>
>>>> Here's an attempted patch for JDK-8237067. It works with tier1 and 
>>>> 2 tests against the lworld tip, so I think it’s ready for review.
>>>>
>>>> The purpose of the change is described in the ticket. As per the 
>>>> discussion in the week-end [1], default values are NOT introduced 
>>>> as constant expressions.
>>>>
>>>> -Jesper
>>>>
>>>> [1]: 
>>>> https://mail.openjdk.java.net/pipermail/valhalla-dev/2020-January/006713.html
>>>>
>>>>
>>>>
>>>>
>>
>> diff -r 7fca6c5e0d99 
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>> --- 
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java 
>> Fri Nov 22 15:19:11 2019 +0100
>> +++ 
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java 
>> Sun Jan 19 13:49:09 2020 +0100
>> @@ -3832,10 +3832,15 @@
>>              while (elt.hasTag(ARRAY))
>>                  elt = ((ArrayType)elt).elemtype;
>>              if (elt.hasTag(TYPEVAR)) {
>> -                log.error(tree.pos(), Errors.TypeVarCantBeDeref);
>> -                result = tree.type = 
>> types.createErrorType(tree.name, site.tsym, site);
>> -                tree.sym = tree.type.tsym;
>> -                return ;
>> +                if (tree.name == names._default) {
>> +                    result = check(tree, litType(BOT).constType(null),
>> +                            KindSelector.VAL, resultInfo);
>> +                } else {
>> +                    log.error(tree.pos(), Errors.TypeVarCantBeDeref);
>> +                    result = tree.type = 
>> types.createErrorType(tree.name, site.tsym, site);
>> +                    tree.sym = tree.type.tsym;
>> +                    return;
>> +                }
>>              }
>>          }
>>
>> @@ -3993,12 +3998,7 @@
>>                      // visitSelect that qualifier expression is a type.
>>                      return syms.getClassField(site, types);
>>                  } else if (name == names._default) {
>> -                    if (!types.isValue(site)) {
>> -                        log.error(pos, Errors.MakeDefaultWithNonvalue);
>> -                        return syms.errSymbol;
>> -                    } else {
>> -                        return new VarSymbol(STATIC, names._default, 
>> site, site.tsym);
>> -                    }
>> +                    return new VarSymbol(STATIC, names._default, 
>> site, site.tsym);
>>                  } else {
>>                      // We are seeing a plain identifier as selector.
>>                      Symbol sym = rs.findIdentInType(pos, env, site, 
>> name, resultInfo.pkind);
>> @@ -4014,6 +4014,11 @@
>>                  // done before attributing the type variables. In
>>                  // other words, we are seeing this illegal program:
>>                  // class B<T> extends A<T.foo> {}
>> +
>> +                if (name == names._default) {
>> +                    // Be sure to return the default value before 
>> examining bounds
>> +                    return new VarSymbol(STATIC, names._default, 
>> site, site.tsym);
>> +                }
>>                  Symbol sym = (site.getUpperBound() != null)
>>                      ? selectSym(tree, location, 
>> capture(site.getUpperBound()), env, resultInfo)
>>                      : null;
>> @@ -4032,11 +4037,13 @@
>>                  return types.createErrorType(name, site.tsym, 
>> site).tsym;
>>              default:
>>                  // The qualifier expression is of a primitive type 
>> -- only
>> -                // .class is allowed for these.
>> +                // .class and .default are allowed for these.
>>                  if (name == names._class) {
>>                      // In this case, we have already made sure in 
>> Select that
>>                      // qualifier expression is a type.
>>                      return syms.getClassField(site, types);
>> +                } else if (name == names._default) {
>> +                    return new VarSymbol(STATIC, names._default, 
>> site, site.tsym);
>>                  } else {
>>                      log.error(pos, Errors.CantDeref(site));
>>                      return syms.errSymbol;
>> diff -r 7fca6c5e0d99 
>> src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
>> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java 
>> Fri Nov 22 15:19:11 2019 +0100
>> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java 
>> Sun Jan 19 13:49:09 2020 +0100
>> @@ -2401,8 +2401,16 @@
>>              result = items.makeStackItem(pt);
>>              return;
>>          } else if (tree.name == names._default) {
>> -            code.emitop2(defaultvalue, checkDimension(tree.pos(), 
>> tree.type), PoolWriter::putClass);
>> -            result = items.makeStackItem(tree.type);
>> +            if (tree.type.asElement().isValue()) {
>> +                code.emitop2(defaultvalue, 
>> checkDimension(tree.pos(), tree.type), PoolWriter::putClass);
>> +                result = items.makeStackItem(tree.type);
>> +            } else if (tree.type.isReference()) {
>> +                code.emitop0(aconst_null);
>> +                result = items.makeStackItem(tree.type);
>> +            } else {
>> +                code.emitop0(zero(Code.typecode(tree.type)));
>> +                result = items.makeStackItem(tree.type);
>> +            }
>>              return;
>>          }
>>
>> diff -r 7fca6c5e0d99 
>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java
>> --- 
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java 
>> Fri Nov 22 15:19:11 2019 +0100
>> +++ 
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java 
>> Sun Jan 19 13:49:09 2020 +0100
>> @@ -477,6 +477,22 @@
>>          }
>>      }
>>
>> +    /** If next input token matches one of the two given tokens, 
>> skip it, otherwise report
>> +     *  an error.
>> +     *
>> +     * @return The actual token kind.
>> +     */
>> +    public TokenKind accept2(TokenKind tk1, TokenKind tk2) {
>> +        TokenKind returnValue = token.kind;
>> +        if (token.kind == tk1 || token.kind == tk2) {
>> +            nextToken();
>> +        } else {
>> +            setErrorEndPos(token.pos);
>> +            reportSyntaxError(S.prevToken().endPos, 
>> Errors.Expected2(tk1, tk2));
>> +        }
>> +        return returnValue;
>> +    }
>> +
>>      /** Report an illegal start of expression/type error at given 
>> position.
>>       */
>>      JCExpression illegal(int pos) {
>> @@ -1327,7 +1343,7 @@
>>                              case DEFAULT:
>>                                  if (typeArgs != null) return illegal();
>>                                  selectExprMode();
>> -                                t = F.at(pos).Select(t, 
>> names._default);
>> +                                t = to(F.at(pos).Select(t, 
>> names._default));
>>                                  nextToken();
>>                                  break loop;
>>                              case CLASS:
>> @@ -2221,7 +2237,7 @@
>>          return t;
>>      }
>>
>> -    /** BracketsSuffixExpr = "." CLASS
>> +    /** BracketsSuffixExpr = "." (CLASS | DEFAULT)
>>       *  BracketsSuffixType =
>>       */
>>      JCExpression bracketsSuffix(JCExpression t) {
>> @@ -2229,7 +2245,7 @@
>>              selectExprMode();
>>              int pos = token.pos;
>>              nextToken();
>> -            accept(CLASS);
>> +            TokenKind selector = accept2(CLASS, DEFAULT);
>>              if (token.pos == endPosTable.errorEndPos) {
>>                  // error recovery
>>                  Name name;
>> @@ -2247,7 +2263,7 @@
>>                  // taking care to handle some interior dimension(s) 
>> being annotated.
>>                  if ((tag == TYPEARRAY && 
>> TreeInfo.containsTypeAnnotation(t)) || tag == ANNOTATED_TYPE)
>>                      syntaxError(token.pos, 
>> Errors.NoAnnotationsOnDotClass);
>> -                t = toP(F.at(pos).Select(t, names._class));
>> +                t = toP(F.at(pos).Select(t, selector == CLASS ? 
>> names._class : names._default));
>>              }
>>          } else if ((mode & TYPE) != 0) {
>>              if (token.kind != COLCOL) {
>> diff -r 7fca6c5e0d99 
>> src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
>> --- 
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties 
>> Fri Nov 22 15:19:11 2019 +0100
>> +++ 
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties 
>> Sun Jan 19 13:49:09 2020 +0100
>> @@ -3545,9 +3545,6 @@
>> compiler.err.value.does.not.support=\
>>      Inline types do not support {0}
>>
>> -compiler.err.make.default.with.nonvalue=\
>> -    Default value creation requires an inline type
>> -
>> compiler.err.value.may.not.extend=\
>>      Inline type may not extend another inline type or class
>>
>> diff -r 7fca6c5e0d99 test/jdk/java/util/Locale/LocaleProvidersRun.java
>> --- a/test/jdk/java/util/Locale/LocaleProvidersRun.java    Fri Nov 22 
>> 15:19:11 2019 +0100
>> +++ b/test/jdk/java/util/Locale/LocaleProvidersRun.java    Sun Jan 19 
>> 13:49:09 2020 +0100
>> @@ -172,6 +172,8 @@
>>                  .addToolArg("-cp")
>>                  .addToolArg(Utils.TEST_CLASS_PATH)
>>                  .addToolArg("-Djava.locale.providers=" + prefList)
>> +                .addToolArg("-Duser.language=en")
>> +                .addToolArg("-Duser.country=US")
>> .addToolArg("--add-exports=java.base/sun.util.locale.provider=ALL-UNNAMED")
>>                  .addToolArg("LocaleProviders")
>>                  .addToolArg(methodName)
>> diff -r 7fca6c5e0d99 
>> test/langtools/tools/javac/diags/examples.not-yet.txt
>> --- a/test/langtools/tools/javac/diags/examples.not-yet.txt Fri Nov 
>> 22 15:19:11 2019 +0100
>> +++ b/test/langtools/tools/javac/diags/examples.not-yet.txt Sun Jan 
>> 19 13:49:09 2020 +0100
>> @@ -203,7 +203,6 @@
>> # Value types
>> compiler.err.cyclic.value.type.membership
>> compiler.err.value.does.not.support
>> -compiler.err.make.default.with.nonvalue
>> compiler.err.value.may.not.extend
>> compiler.warn.potential.null.pollution
>> compiler.err.empty.value.not.yet
>> diff -r 7fca6c5e0d99 
>> test/langtools/tools/javac/valhalla/lworld-values/CheckBadSelector.java
>> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> +++ 
>> b/test/langtools/tools/javac/valhalla/lworld-values/CheckBadSelector.java 
>> Sun Jan 19 13:49:09 2020 +0100
>> @@ -0,0 +1,14 @@
>> +/*
>> + * @test /nodynamiccopyright/
>> + * @summary Check that syntax constraints still exist
>> + *
>> + * @compile/fail/ref=CheckBadSelector.out -XDrawDiagnostics 
>> CheckBadSelector.java
>> + */
>> +inline final class Point {
>> +
>> +    void badSelector() {
>> +        Class<?> c = int.class;
>> +        int i = int.default;
>> +        int x = int.whatever;
>> +    }
>> +}
>> diff -r 7fca6c5e0d99 
>> test/langtools/tools/javac/valhalla/lworld-values/CheckBadSelector.out
>> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> +++ 
>> b/test/langtools/tools/javac/valhalla/lworld-values/CheckBadSelector.out 
>> Sun Jan 19 13:49:09 2020 +0100
>> @@ -0,0 +1,2 @@
>> +CheckBadSelector.java:12:21: compiler.err.expected2: class, default
>> +1 error
>> diff -r 7fca6c5e0d99 
>> test/langtools/tools/javac/valhalla/lworld-values/CheckMakeDefault.java
>> --- 
>> a/test/langtools/tools/javac/valhalla/lworld-values/CheckMakeDefault.java 
>> Fri Nov 22 15:19:11 2019 +0100
>> +++ 
>> b/test/langtools/tools/javac/valhalla/lworld-values/CheckMakeDefault.java 
>> Sun Jan 19 13:49:09 2020 +0100
>> @@ -23,11 +23,22 @@
>>      final int x;
>>      final int y;
>>
>> +    final int nonbool = boolean.default;
>> +    final boolean nonbyte = byte.default;
>> +    final boolean nonchar = char.default;
>> +    final boolean nonint = int.default;
>> +    final boolean nonshort = short.default;
>> +    final boolean nonlong = long.default;
>> +    final boolean nonfloat = float.default;
>> +    final boolean nondouble = double.default;
>> +    final int nonString = String.default;
>> +    final int nonbyteArray = byte[].default;
>> +
>>      Point() {}
>>      Point (int x, int y) {}
>>
>> -    Point badFactory(int x, int y) {
>> -        return Point.default;
>> +    Point goodFactory(int x, int y) {
>> +        return Point.default; // Allowed in the new 'State of Valhalla'
>>      }
>>
>>      static Point make(int x, int y) {
>> diff -r 7fca6c5e0d99 
>> test/langtools/tools/javac/valhalla/lworld-values/CheckMakeDefault.out
>> --- 
>> a/test/langtools/tools/javac/valhalla/lworld-values/CheckMakeDefault.out 
>> Fri Nov 22 15:19:11 2019 +0100
>> +++ 
>> b/test/langtools/tools/javac/valhalla/lworld-values/CheckMakeDefault.out 
>> Sun Jan 19 13:49:09 2020 +0100
>> @@ -1,5 +1,13 @@
>> CheckMakeDefault.java:9:12: 
>> compiler.err.illegal.combination.of.modifiers: interface, inline
>> CheckMakeDefault.java:10:21: 
>> compiler.err.illegal.combination.of.modifiers: abstract, inline
>> -CheckMakeDefault.java:13:26: compiler.err.make.default.with.nonvalue
>> -CheckMakeDefault.java:35:25: compiler.err.make.default.with.nonvalue
>> -4 errors
>> +CheckMakeDefault.java:26:32: compiler.err.prob.found.req: 
>> (compiler.misc.inconvertible.types: boolean, int)
>> +CheckMakeDefault.java:27:33: compiler.err.prob.found.req: 
>> (compiler.misc.inconvertible.types: byte, boolean)
>> +CheckMakeDefault.java:28:33: compiler.err.prob.found.req: 
>> (compiler.misc.inconvertible.types: char, boolean)
>> +CheckMakeDefault.java:29:31: compiler.err.prob.found.req: 
>> (compiler.misc.inconvertible.types: int, boolean)
>> +CheckMakeDefault.java:30:35: compiler.err.prob.found.req: 
>> (compiler.misc.inconvertible.types: short, boolean)
>> +CheckMakeDefault.java:31:33: compiler.err.prob.found.req: 
>> (compiler.misc.inconvertible.types: long, boolean)
>> +CheckMakeDefault.java:32:35: compiler.err.prob.found.req: 
>> (compiler.misc.inconvertible.types: float, boolean)
>> +CheckMakeDefault.java:33:37: compiler.err.prob.found.req: 
>> (compiler.misc.inconvertible.types: double, boolean)
>> +CheckMakeDefault.java:34:33: compiler.err.prob.found.req: 
>> (compiler.misc.inconvertible.types: java.lang.String, int)
>> +CheckMakeDefault.java:35:36: compiler.err.prob.found.req: 
>> (compiler.misc.inconvertible.types: byte[], int)
>> +12 errors
>> \ No newline at end of file
>> diff -r 7fca6c5e0d99 
>> test/langtools/tools/javac/valhalla/lworld-values/CheckValueFactoryWithReference.java
>> --- 
>> a/test/langtools/tools/javac/valhalla/lworld-values/CheckValueFactoryWithReference.java 
>> Fri Nov 22 15:19:11 2019 +0100
>> +++ /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> @@ -1,12 +0,0 @@
>> -/*
>> - * @test /nodynamiccopyright/
>> - * @summary Do not allow mismatched instantiation syntax between 
>> value & reference types.
>> - *
>> - * @compile/fail/ref=CheckValueFactoryWithReference.out 
>> -XDrawDiagnostics CheckValueFactoryWithReference.java
>> - */
>> -
>> -final class CheckValueFactoryWithReference {
>> -    final Object o = Object.default;
>> -    inline final class Point { int x = 10; }
>> -    Point p = new Point();
>> -}
>> diff -r 7fca6c5e0d99 
>> test/langtools/tools/javac/valhalla/lworld-values/CheckValueFactoryWithReference.out
>> --- 
>> a/test/langtools/tools/javac/valhalla/lworld-values/CheckValueFactoryWithReference.out 
>> Fri Nov 22 15:19:11 2019 +0100
>> +++ /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> @@ -1,2 +0,0 @@
>> -CheckValueFactoryWithReference.java:9:28: 
>> compiler.err.make.default.with.nonvalue
>> -1 error
>> diff -r 7fca6c5e0d99 
>> test/langtools/tools/javac/valhalla/lworld-values/DefaultNonInlines.java
>> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> +++ 
>> b/test/langtools/tools/javac/valhalla/lworld-values/DefaultNonInlines.java 
>> Sun Jan 19 13:49:09 2020 +0100
>> @@ -0,0 +1,88 @@
>> +/*
>> + * Copyright (c) 2018, 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.  Oracle designates this
>> + * particular file as subject to the "Classpath" exception as provided
>> + * by Oracle in the LICENSE file that accompanied this code.
>> + *
>> + * 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 Check default values for non-inline types
>> + * @bug 8237067
>> + * @summary [lworld] Provide linguistic support to denote default 
>> values.
>> + * @run main/othervm DefaultNonInlines
>> + */
>> +
>> +public class DefaultNonInlines {
>> +
>> +    static inline class Val {
>> +        public int v = 42;
>> +    }
>> +
>> +    static <T> void checkDefaultT(Class<T> clazz) throws Exception {
>> +        while (T.default != null)
>> +            throw new Exception("Generic object should default to 
>> null");
>> +    }
>> +
>> +    public static void main(String[] args) throws Exception {
>> +        // Default value is set by inline class constructor
>> +        while (Val.default.v != int.default)
>> +            throw new Exception("inline object fields should default 
>> to defaults");
>> +
>> +        while ((new Val()).v != 42)
>> +        throw new Exception("inline object fields should default to 
>> whatever constructor says");
>> +
>> +        // Simple reference default is just null
>> +        while (String.default != null)
>> +            throw new Exception("reference object should default to 
>> null");
>> +
>> +        // Reference default checked in method above
>> +        checkDefaultT(String.class);
>> +
>> +        // Array type - differnt syntactically
>> +        while (int[].default != null)
>> +            throw new Exception("arrays should default to null");
>> +
>> +        // Array type - differnt syntactically
>> +        while (boolean.default != false)
>> +            throw new Exception("boolean should default to false");
>> +
>> +        while (char.default != '\0')
>> +            throw new Exception("char should default to '\0'");
>> +
>> +        while (int.default != 0)
>> +            throw new Exception("int should default to 0");
>> +
>> +        while (byte.default != 0)
>> +            throw new Exception("byte should default to 0");
>> +
>> +        while (short.default != 0)
>> +            throw new Exception("short should default to 0");
>> +
>> +        while (long.default != 0L)
>> +            throw new Exception("long should default to 0L");
>> +
>> +        while (float.default != 0.0F)
>> +            throw new Exception("float should default to 0.0F");
>> +
>> +        while (double.default != 0.0D)
>> +            throw new Exception("double should default to 0.0D");
>> +    }
>> +}
>>
>



More information about the valhalla-dev mailing list