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

[Vicente Romero]

On Thu, 10 Mar 2022 10:48:40 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> I think the pieces are all there. When looking at the code, the impression I have is that we're doing a lot of work just to pass some kind of boolean or enum flag which tells us if some extra loose behavior should be allowed or not (in both the subtyping, containment and sameType visitors).
> 
> I was about to suggest that in the previous review rounds, but then held back because the existing visitors cannot take external parameters. I think your changes to the `isSameType` visitor demonstrate that problem: the visitor is just a function on types, and there's no way to pass an extra boolean/enum to it - so you have to store it in a field, which then leads to issues because either (i) you create one visitor instance per subtyping check (which would be very expensive, I fear) or you resort to tricks using mutable fields.
> 
> Unfortunately fixing this mismatch is hard, as we're already using the visitor parameter to pass the second type of these type relations. So some other kind of workaround is required.
> 
> A possible suggestion for trying to get to a simpler form would be to introduce a new "parameterized" relation, like this:
> 
> ```
> public abstract static class ParameterizedTypeRelation<P> extends TypeRelation {
>         P param;
> 
>         public final Boolean visit(Type t, Type s, P param) {
>             P prevParam = this.param;
>             try {
>                 this.param = param;
>                 return visit(t, s);
>             } finally {
>                 this.param = prevParam;
>             }
>         }
>     }
> ```
> 
> This basically generalizes the trick you did with `isSameType` (but resets the field value correctly after a visit). Then you make isSubtype/contains/isSameType subclasses of this visitor class, and define an enum with two values for the strict/loose behavior.
> 
> I believe this should give you enough flexibility to do what you want, and, at the same time, by minimizing set of changes compared to mainline.

sure I will try this, thanks

> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 1496:
> 
>> 1494: 
>> 1495:     public boolean isSameType(Type t, Type s, ContainsType containmentRelation) {
>> 1496:         isSameTypeVisitor.setContainmentRel(containmentRelation);
> 
> This should be reset after the call?

right

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

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