Incoherent invocation type inference?
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Mon Aug 28 12:18:15 UTC 2017
On 25/08/17 08:23, B. Blaser wrote:
> 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?
Yes, please, go ahead. I've seen this popping out frequently enough that
I think it deserves some closure.
Maurizio
>
> 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
More information about the compiler-dev
mailing list