Incoherent invocation type inference?

[B. Blaser]

Maurizio,

On 22 January 2017 at 17:26, B. Blaser <bsrbnd at gmail.com> wrote:
> Hi,
>
> 2017-01-19 18:22 GMT+01:00 Maurizio Cimadamore <maurizio.cimadamore at oracle.com>:
>> Not sure about your proposed patch.
>>
>> To me the warning should be a property of the method declaration, not of the
>> specific inference.
>>
>> If a method returns a naked type-variable which is not mentioned anywhere in
>> the method parameter types -> Lint warning (not an unchecked warning - just
>> an optional Lint one - category TBD).
>>
>> It's true that, by looking at the callsite, you can also warn for clients
>> passing 'null' arguments, but the extra benefit is not worth the extra
>> complexity IMHO. And, I think this is a problem of bad API, not one of bad
>> clients. A well-designed API should not have any methods that match the
>> criteria stated above - as their behavior would ultimately be at the
>> client's mercy.
>>
>> Maurizio
>
> Here under is a suggestion for the implementation of the rule you
> stated above (Attr.visitMethodDef(), upon rev. b6960e2da008).
>
> I kept a check on the callsite for further evaluation (in
> Attr.checkMethod() to be more general). I think both are
> complementary.
>
> "<T> T[] List.toArray(T[])" is a good example of a well designed API.
> But an invocation of the form "a = l.toArray(null)" is dangerous due
> to the obvious API's lack of control over T and might be warned.
>
> For the Lint category, I suggest to name it "generic" and to use it
> for warnings about type variables. Other checks like the "final" one
> could fall into the same category.
>
> Does this look better?

Should I create a JBS issue for that?

Bernard

> Thanks,
> Bernard
>
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java
> @@ -268,6 +268,11 @@
>          UNCHECKED("unchecked"),
>
>          /**
> +         * Warn about issues relating to type variables.
> +         */
> +        GENERIC("generic"),
> +
> +        /**
>           * Warn about potentially unsafe vararg methods
>           */
>          VARARGS("varargs");
> 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
> @@ -947,6 +947,7 @@
>              deferredLintHandler.flush(tree.pos());
>              chk.checkDeprecatedAnnotation(tree.pos(), m);
>
> +            chk.checkGenMethDeclaration(tree.pos(), m);
>
>              // Create a new environment with local scope
>              // for attributing the method.
> @@ -3918,6 +3919,8 @@
>                              final List<JCExpression> argtrees,
>                              List<Type> argtypes,
>                              List<Type> typeargtypes) {
> +        chk.checkGenMethInvocation(env.tree.pos(), sym, resultInfo, argtypes);
> +
>          // Test (5): if symbol is an instance method of a raw type, issue
>          // an unchecked warning if its argument types change under erasure.
>          if ((sym.flags() & STATIC) == 0 &&
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
> @@ -3876,4 +3876,76 @@
>          }
>      }
>
> +    // Check the declaration's precision for type variables.
> +    void checkGenMethDeclaration(DiagnosticPosition pos, Symbol sym) {
> +        if (sym.type.hasTag(FORALL)) {
> +            ForAll gmt = (ForAll)sym.type;
> +            MethodType mt = (MethodType)gmt.qtype;
> +            List<Type> tvars = gmt.tvars;
> +
> +            Type elemResType = mt.restype;
> +            while (elemResType.hasTag(ARRAY)) {
> +                elemResType = ((ArrayType)elemResType).elemtype;
> +            }
> +            if (elemResType.hasTag(TYPEVAR) && tvars.contains(elemResType)) {
> +                boolean ok = false;
> +                for (Type t: mt.argtypes) {
> +                    if (t.contains(elemResType))
> +                        ok = true;
> +                }
> +                if (!ok)
> +                    warnGeneric(
> +                            pos,
> +                            "generic.imprecise.method.declaration",
> +                            Kinds.kindName(sym),
> +                            sym.name,
> +                            Kinds.kindName(sym.location()),
> +                            sym.location(),
> +                            elemResType);
> +            }
> +        }
> +    }
> +
> +    // Check the actual arguments' precision for type variables.
> +    void checkGenMethInvocation(
> +            DiagnosticPosition pos,
> +            Symbol sym,
> +            Attr.ResultInfo resultInfo,
> +            List<Type> argtypes) {
> +
> +        if (sym.type.hasTag(FORALL)) {
> +            ForAll gmt = (ForAll)sym.type;
> +            MethodType mt = (MethodType)gmt.qtype;
> +
> +            Type elemResType = mt.restype;
> +            while (elemResType.hasTag(ARRAY)) {
> +                elemResType = ((ArrayType)elemResType).elemtype;
> +            }
> +            if (resultInfo != null && resultInfo.pt != Type.noType &&
> elemResType.hasTag(TYPEVAR)) {
> +                boolean ok = false;
> +                if (mt.argtypes.length() <= argtypes.length()) {
> +                    for (int i=0; i<mt.argtypes.length(); i++) {
> +                        Type pType = mt.argtypes.get(i);
> +                        Type aType = argtypes.get(i);
> +                        if (pType.contains(elemResType) && aType !=
> syms.botType)
> +                            ok = true;
> +                    }
> +                }
> +                if (!ok)
> +                    warnGeneric(
> +                            pos,
> +                            "generic.imprecise.method.invocation",
> +                            Kinds.kindName(sym),
> +                            sym.name,
> +                            Kinds.kindName(sym.location()),
> +                            sym.location(),
> +                            elemResType);
> +            }
> +        }
> +    }
> +
> +    void warnGeneric(DiagnosticPosition pos, String key, Object... args) {
> +        if (lint.isEnabled(LintCategory.GENERIC))
> +            log.warning(LintCategory.GENERIC, pos, key, args);
> +    }
>  }
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
> @@ -1756,6 +1756,14 @@
>  compiler.warn.unchecked.varargs.non.reifiable.type=\
>      Possible heap pollution from parameterized vararg type {0}
>
> +# 0: symbol kind, 1: name, 2: symbol kind, 3: symbol, 4: type
> +compiler.warn.generic.imprecise.method.declaration=\
> +    Imprecise method declaration: parameters of {0} {1} in {2} {3}
> should refer to {4}
> +
> +# 0: symbol kind, 1: name, 2: symbol kind, 3: symbol, 4: type
> +compiler.warn.generic.imprecise.method.invocation=\
> +    Imprecise method invocation: arguments of {0} {1} in {2} {3}
> should refer to {4}
> +
>  # 0: symbol
>  compiler.warn.varargs.unsafe.use.varargs.param=\
>      Varargs method could cause heap pollution from non-reifiable
> varargs parameter {0}
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties
> @@ -238,6 +238,9 @@
>  javac.opt.Xlint.desc.unchecked=\
>      Warn about unchecked operations.
>
> +javac.opt.Xlint.desc.generic=\
> +    Warn about issues relating to type variables.
> +
>  javac.opt.Xlint.desc.varargs=\
>      Warn about potentially unsafe vararg methods