RFR: addressing several issues in the implementation of universal tvars [v6]

[Maurizio Cimadamore]

On Sat, 4 Jun 2022 19:07:49 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> Last review iteration found several issues in the current implementation, addressing them here
>
> Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
> 
>   adding structural type comparator to detect unchecked conversions

I think the cleanup is very good - there are probably some other bits that can be cleaned up as well (see comments). I've added some comments on the new visitor, which seems more complex than it should be.

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

> 1169:             @Override
> 1170:             public Boolean visitType(Type t, Type s) {
> 1171:                 switch (t.getTag()) {

In general - it seems like this routine has been brought in from isSubtype, which I think is wrong. This should borrow the most from isSameType. I believe in the case of this visitor, it would have been ok to simply return true if the two types have same tags? E.g. doesn't seem like adding the `switch` really adds much.

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

> 1177:                     case BOT: case NONE:
> 1178:                         return t.hasTag(s.getTag());
> 1179:                     case WILDCARD: //we shouldn't be here - avoids crash (see 7034495)

comment seems copy and paste error?

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

> 1177:                     case BOT: case NONE:
> 1178:                         return t.hasTag(s.getTag());
> 1179:                     case WILDCARD: //we shouldn't be here - avoids crash (see 7034495)

I do not see a visitor method for wildcards, but I think you need one, as you might get to wildcards recursively, because of `visitClassType`

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

> 1190: 
> 1191:                 if (s.isPartial())
> 1192:                     return visit(s, t);

Note sure these lines are relevant?

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

> 1199:             public Boolean visitClassType(ClassType t, Type s) {
> 1200:                 // If t is an intersection, sup might not be a class type
> 1201:                 if (!t.hasTag(CLASS)) return visit(t, s);

if tags are different, the two types are not structurally comparable - this seems a leftover from subtyping

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

> 1215:                     }
> 1216:                 }
> 1217:                 return isSameType(t, s);

Here you delegate to isSameType... which seems odd - as this is the only place where we do this.

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

> 1219: 
> 1220:             public boolean compareTypeArgsRecursive(Type t, Type s) {
> 1221:                 TypePair pair = new TypePair(t, s);

This is typically needed in subtyping routines, to prevent against cases where we could expand terms forever. But in the case of a structural comparison, this can never happen?

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

> 1499:          */
> 1500:         SameTypeVisitor isSameTypeVisitor = new SameTypeVisitor();
> 1501:         class SameTypeVisitor extends ParameterizedTypeRelation<SubtypingRelationKind> {

The `ParameterizedTypeRelation` class seems to still be there - but I don't think it's still useful?

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

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