RFR: universal type variables: initial prototype

Maurizio Cimadamore mcimadamore at openjdk.java.net
Wed Aug 4 10:49:51 UTC 2021


On Wed, 4 Aug 2021 03:51:50 GMT, Vicente Romero <vromero at openjdk.org> wrote:

> First iteration of the protype for universal type variables

Overall looks like a solid first attempt. I think we should spend some cycles making sure we get the TypeVar model right, and that we put info (e.g. universality) where it belongs (e.g. symbol), because that can affect how clients interact with these variables quite a lot.

src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 64:

> 62:     public enum Feature {
> 63:         SWITCH_PATTERN_MATCHING,
> 64:         UNIVERSAL_TVARS,

As Dan noted yesterday, for other Valhalla features we do not generally use preview mechanisms (this code needs to be pushed in the valhalla repo). We can postpone the preview dance for later. In fact, to ease experiments in JDK code it would be better to just make universal type variable syntax available by default.

There are some other places where this UNIVERSAL_TVARS constant is used which should be cleaned up for now (e.g. Preview.java).

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java line 1946:

> 1944:         public Type lower;
> 1945: 
> 1946:         public boolean universal = false;

Can we use a flag on the tsym for this? After all, being "universal" is a declaration (symbol) not a use (type) property?

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java line 2058:

> 2056: 
> 2057:         @Override
> 2058:         public Type withTypeVar(Type t) {

Uhm - isn't this method used on wildcard types, and it all other types is supposed to just return `this` ?

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java line 2091:

> 2089:             this.setUpperBound(upper);
> 2090:             this.wildcard = wildcard;
> 2091:             this.universal = upper.hasTag(TYPEVAR) && ((TypeVar)upper).universal ||

This probably requires tweaking - I see two issues:
* right now it detects cases where upper/lower are type variables themselves - but doesn't detect cases originating from e.g. `List<? extends Point>` - where you just have a captured variable whose bound is `Point`.
* the logic does not recurse - but maybe that's ok if `universal` is always set on creation

I think another approach could be to look at the wildcard type associated with the captured type. The wildcard type keeps track of the declared type variable it comes from, which means you can then look at the type symbol in there and decide if it's an universal variable or not. E.g.


captured.isUniversal() == captured.wildcard.bound.isUniversal()

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 1089:

> 1087:             } else if (isSubtype(t, s, capture)) {
> 1088:                 return true;
> 1089:             } else if (allowUniversalTVars && t.hasTag(TYPEVAR) && s.hasTag(TYPEVAR) && t.tsym == s.tsym) {

Are you sure the check does what the comment says?

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 3615:

> 3613:                     return to.head.withTypeVar(t);
> 3614:                 }
> 3615:                 if (allowUniversalTVars &&

Not sure about this - the main issue here seems that `t.equalsIgnoreMetadata` is too weak and would return `false` for `ref` vs.`val` mismatches. But again I'm uncertain about the need of overriding `withTypeVar` (see comment above).

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 825:

> 823:     }
> 824: 
> 825:     List<Type> attribTypes(List<JCExpression> trees, Env<AttrContext> env, Predicate<JCExpression> valueOK) {

Do you really need a predicate here? Wouldn't a boolean be ok? Or perhaps a flag in env, or some other means?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 5160:

> 5158: 
> 5159:         // Attribute type parameters
> 5160:         List<Type> actuals = !allowUniversalTVars ?

If you are doing this, you might be better off just attributing types one at a time inside the loop above, and avoid using a set to collect the ones which need special treatment?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 707:

> 705:          } else if (!a.hasTag(WILDCARD)) {
> 706:              a = types.cvarUpperBound(a);
> 707:              return types.isBoundedBy(a, bound, (t, s, w) -> types.isSubtype(t, s));

This is a good change - I'm a bit scared about the other calls in this routine, to `isCastable` and `notSoftSubtype`. These type relations are mostly doing things outside the spec, and we probably need to sprinkle `isBounded` in there as well.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 1056:

> 1054:             InferenceContext inferenceContext = deferredAttrContext.inferenceContext;
> 1055:             return strict ?
> 1056:                     types.isBoundedBy(inferenceContext.asUndetVar(found), inferenceContext.asUndetVar(req), warn,

I'm not 100% sure on this. I think step one should use just plain subtyping, while step 2 is allowed to use the more powerful relationship. My mental model is `int -> Object` which is only allowed on step 2 - so I think we want to do something similar for `Point -> Object` ? Of course `Point.ref -> Object` would be allowed on step 1.

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

PR: https://git.openjdk.java.net/valhalla/pull/521


More information about the valhalla-dev mailing list