RFR: bug: no warning shown for parameterized conversions [v4]

[Maurizio Cimadamore]

On Wed, 9 Mar 2022 04:21:13 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> no warning is shown for parameterized conversions
>
> Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
> 
>   renaming argument

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

> 2032: 
> 2033:         public Type withTypeVar(Type t) {
> 2034:             return t.hasTag(TYPEVAR) && t.isReferenceProjection() && t == projection ?

Uhm - this method is typically used to set type-variables on wildcard types. It is a bit of a corner case in the compiler code, and I think it would be better to leave this alone. What is this code trying to do - and can the same be achieved in another way?

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

> 1179:     }
> 1180: 
> 1181:     record TypeRelations(boolean uncheckedAllowed, TypeRelation subtypingRelation, TypeRelation containmentRelation) {}

The `containmentRelation` parameter is unused - because you dispatch to the correct containment check using a visitor subclass. Either you keep this field, and use it in the main subtype visitor, or you drop it. If you drop it, I think it would be better to rename the record to something that is more connected to subtyping.

Also, I think an enum here would be better than a record. You only have two options.


enum SubtypingMode {
    STRICT(new IsSubtypeRelation())
    ALLOW_REF_VAL(new IsSubtypeUncheckedRelation())
    ...
}

Note that I did not use the term "unchecked". That might be misleading, as that doesn't mean "isSubtypeUnchecked" is called. I suggest we come up with a different term for the concept we want and stick to that. The difference between the two routines is that one allows (recursively) loose comparisons between ref/val, while the other does not.

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

> 1251:             private Set<TypePair> cache = new HashSet<>();
> 1252: 
> 1253:             public boolean containsTypeRecursive(Type t, Type s) {

Can this code be shared with subclass?

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

> 4947:     private boolean containsTypeEquivalent(Type t, Type s) {
> 4948:         return isSameType(t, s) || // shortcut
> 4949:             containsType(t, s, containsTypeUnchecked) && containsType(s, t, containsTypeUnchecked);

Is this the right replacement? It seems to me that the answer is "it depends". This is called both by "hasSameArgs" which is used to determine overriding (in which case a loose semantics could be ok), but also by the isSameType visitor - in which case the loose semantics should only be enabled conditionally.

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

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