question about JDK-8063054 (incorrect raw type warnings for method references)
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Thu Mar 23 11:58:02 UTC 2017
Hi Bernard,
your patch looks good, I think it handles all the problematic cases I
can think of.
My only reservation is in making isRaw a member of Types, which would
hint that that's the definition of a raw type as per JLS 4.8, which it
is not.
Ultimately, this routine is only used by the logic for checking method
references, as an heuristics to try and see if the signature of the
functional descriptor contains enough information to allow inference of
the parameters of the 'raw' method reference receiver type.
Reading closer from the spec (15.13.1), I see the following statement:
"In the second search, if P_1 , ..., P_n is not empty and P_1 is a
subtype of /ReferenceType/, then the method reference expression is
treated as if it were a method invocation expression with argument
expressions of types P_2 , ..., P_n . If /ReferenceType/ is a raw type,
and there exists a parameterization of this type, G|<|...|>|, that is a
supertype of P_1 , the type to search is the result of capture
conversion (§5.1.10
<https://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.1.10>)
applied to G|<|...|>|; *otherwise, the type to search is the same as the
type of the first search*. Again, the type arguments, if any, are given
by the method reference expression."
I think the lines in bold are the ones we need to pay attention to; in
other words, if the receiver type is raw and the type of the first
search is identical to the type of the second search, then it means no
inference occurred. I think something like this could easily be achieved
w/o any visitors, with something like this:
if (that.getMode() == ReferenceMode.INVOKE &&
TreeInfo.isStaticSelector(that.expr, names) &&
that.kind.isUnbound() &&
types.isSameTypes(lookupHelper.site, that.expr.type) {
chk.checkRaw(that.expr, localEnv);
}
I did some quick tests and this simple patch seems to remove the
undesired warnings in the examples we have seen so far, but retains them
where they are truly deserved. What do you think?
Maurizio
On 22/03/17 22:04, B. Blaser wrote:
> On 18 March 2017 at 19:32, B. Blaser <bsrbnd at gmail.com> wrote:
>> Hi,
>>
>> On 16 February 2017 at 11:35, Maurizio Cimadamore
>> <maurizio.cimadamore at oracle.com> wrote:
>>>
>>> On 16/02/17 02:33, Liam Miller-Cushon wrote:
>>>> I think the fix might be as simple as replacing
>>>> `!desc.getParameterTypes().head.isParameterized()` with something that
>>>> handles capture variables.
>>> Hi Liam,
>>> I think your analysis is correct - isParameterized would not work for
>>> non-class types, so something else is required here. I believe intersection
>>> types like Serializable & Comparable<Foo> might also end up with the same
>>> problem - and, presumably, synthetic inner class types.
>>>
>>> Maurizio
> Here are two more examples producing a warning that should probably be
> avoided (am I right?). The first one involves an intersection type and
> the second one an anonymous class:
>
> class Test5 {
> <B extends A & Box<Number>> void test(Box<B> b) {
> Number n = b.<Number>map(Box::get).get();
> }
> interface Func<S,T> { T apply(S arg); }
> interface Box<T> {
> T get();
> <R> Box<R> map(Func<T,R> f);
> }
> interface A {}
> }
>
> class Test6 {
> interface Consumer<T> { void accept(T arg); }
> interface Parent<P> { void foo(); }
> interface Child extends Parent<String> {}
> static <T> void m(T arg, Consumer<T> f) {}
> public void test(Child c) { m(new Child() { public void foo() {}
> }, Parent::foo); }
> }
>
> I had to write (here under) a method Types.isRaw() using a type
> visitor to handle captured type variables, intersection types and
> anonymous classes.
>
> How does this look, did I miss anything?
>
> Thanks,
> Bernard
>
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
> @@ -227,6 +227,52 @@
> };
> // </editor-fold>
>
> + public boolean isRaw(Type t) {
> + return t != null && isRaw.visit(t);
> + }
> + // where
> + private final UnaryVisitor<Boolean> isRaw = new
> UnaryVisitor<Boolean>() {
> + @Override
> + public Boolean visitType(Type t, Void ignored) {
> + return t.isRaw();
> + }
> +
> + @Override
> + public Boolean visitClassType(ClassType t, Void ignored) {
> + if (t.isIntersection()) {
> + for (Type u: ((IntersectionClassType) t).getComponents())
> + if (isRaw(u))
> + return true;
> +
> + return false;
> + }
> + else if (t.tsym.name.isEmpty()) {
> + // Anonymous
> + if (t.interfaces_field != null) {
> + for (Type i: t.interfaces_field)
> + if (isRaw(i))
> + return true;
> + }
> + else if (t.supertype_field != null &&
> isRaw(t.supertype_field))
> + return true;
> +
> + return false;
> + }
> +
> + return t.isRaw();
> + }
> +
> + @Override
> + public Boolean visitTypeVar(TypeVar t, Void ignored) {
> + return t.bound != null && isRaw(t.bound);
> + }
> +
> + @Override
> + public Boolean visitCapturedType(CapturedType t, Void ignored) {
> + return t.bound != null && isRaw(t.bound);
> + }
> + };
> +
> // <editor-fold defaultstate="collapsed" desc="asSub">
> /**
> * Return the least specific subtype of t that starts with symbol
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
> b/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
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
> @@ -2963,7 +2963,7 @@
> if (that.getMode() == ReferenceMode.INVOKE &&
> TreeInfo.isStaticSelector(that.expr, names) &&
> that.kind.isUnbound() &&
> - !desc.getParameterTypes().head.isParameterized()) {
> + types.isRaw(desc.getParameterTypes().head)) {
> chk.checkRaw(that.expr, localEnv);
> }
>
>
>> Regarding Daniel's JBS comment, I think the condition should be made
>> on isRaw() instead of isParameterized().
>> The following example wouldn't produce any warning:
>>
>> class Test2 {
>> interface Consumer<T> { void accept(T arg); }
>> interface Parent<P> { void foo(); }
>> interface Child extends Parent<String> {}
>> static <T> void m(T arg, Consumer<T> f) {}
>> public void test(Child c) { m(c, Parent::foo); }
>> }
>>
>> Then, we could add a check on the bound of the captured type variable,
>> as below. Liam's example wouldn't produce any warning but the
>> following one would:
>>
>> class Test3 {
>> void test(Box<Box> b) {
>> Object o = b.<Object>map(Box::get).get();
>> }
>> interface Func<S,T> { T apply(S arg); }
>> interface Box<T> {
>> T get();
>> <R> Box<R> map(Func<T,R> f);
>> }
>> }
>>
>> Any comment is welcome.
>> Thanks,
>> Bernard
>>
>> diff -r 43a83431f19d
>> 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
>> Wed Mar 15 15:46:43 2017 +0100
>> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>> Sat Mar 18 19:01:02 2017 +0100
>> @@ -2962,9 +2962,15 @@
>>
>> if (that.getMode() == ReferenceMode.INVOKE &&
>> TreeInfo.isStaticSelector(that.expr, names) &&
>> - that.kind.isUnbound() &&
>> - !desc.getParameterTypes().head.isParameterized()) {
>> - chk.checkRaw(that.expr, localEnv);
>> + that.kind.isUnbound()) {
>> +
>> + Type t = desc.getParameterTypes().head;
>> + if (t.isRaw() ||
>> + t.getTag() == TYPEVAR &&
>> + ((TypeVar)t).isCaptured() &&
>> + ((TypeVar)t).bound.isRaw()) {
>> + chk.checkRaw(that.expr, localEnv);
>> + }
>> }
>>
>> if (that.sym.isStatic() &&
>> TreeInfo.isStaticSelector(that.expr, names) &&
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20170323/7173be30/attachment-0001.html>
More information about the compiler-dev
mailing list