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

Srikanth srikanth.adayapalam at oracle.com
Wed Jan 29 05:16:58 UTC 2020


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