typetools / checker-framework

Pluggable type-checking for Java
http://checkerframework.org/
Other
1.02k stars 354 forks source link

Package `@DefaultQualifier` overrides class `@DefaultQualifier` #3677

Open vlsi opened 4 years ago

vlsi commented 4 years ago

Sample:

package-info.java

@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.FIELD)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.PARAMETER)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.RETURN)
package org.apache.calcite.util;

import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.framework.qual.DefaultQualifier;
import org.checkerframework.framework.qual.TypeUseLocation;

Glossary.java

package org.apache.calcite.util;

import org.checkerframework.checker.nullness.qual.Nullable;
import org.checkerframework.framework.qual.DefaultQualifier;
import org.checkerframework.framework.qual.TypeUseLocation;

@DefaultQualifier(
    value = Nullable.class,
    locations = TypeUseLocation.ALL
)
public interface Glossary {
  Glossary PATTERN = null;
  Glossary SQL2003 = null;
}

Error:

core/src/main/java/org/apache/calcite/util/Glossary.java:331: error: [assignment.type.incompatible] incompatible types in assignment.
  Glossary PATTERN = null;
                     ^
  found   : null
  required: @Initialized @NonNull Glossary
smillst commented 4 years ago

Here's a single file test case.

import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.checkerframework.framework.qual.DefaultQualifier;
import org.checkerframework.framework.qual.TypeUseLocation;

@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.FIELD)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.PARAMETER)
@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.RETURN)
public class Glossary {

    // :: error: (assignment.type.incompatible)
    Glossary PATTERN = null;

    @DefaultQualifier(value = Nullable.class, locations = TypeUseLocation.ALL)
    class Inner {
        Glossary SQL2003 = null;
    }
}
cpovirk commented 3 years ago

Coincidentally, I was just looking at this code. I am suspicious of this line: https://github.com/typetools/checker-framework/blob/874073fb9bf41a6d7f37116e63293653d8e8dd09/framework/src/main/java/org/checkerframework/framework/util/defaults/QualifierDefaults.java#L631

It looks to me like the higher priority (based on TypeUseLocation order) annotations from the enclosing element get added to the set of the child element, where they then take priority over ALL. Perhaps setting ALL on an element should prevent the earlier values from being added. Arguably even setting OTHERWISE should do that.

In fact, arguably defaulting should be more sophisticated still: Maybe I should be able to set EXPLICIT_UPPER_BOUND on a child element and have that override an existing UPPER_BOUND on the enclosing element, even though UPPER_BOUND has a higher priority. That is, maybe the child DefaultSet should never take on elements from the enclosing element's. Instead, maybe we should apply all the child-specific rules first and then apply all the rules on the enclosing types, working from the in out. That may be slower, though, and there are probably subtleties to this that I haven't understood yet.

A lot of this hinges on the precise meaning of:

If multiple DefaultQualifier annotations are in scope, the innermost one takes precedence.

cpovirk commented 3 years ago

(Huh, I wonder if even a default of FOO on a child necessarily wins over a default of FOO on an enclosing element. It seems as if both may get added to the DefaultSet, and the conflict may be resolved in favor of whichever default wins the AnnotationUtils.compareAnnotationMirrors comparison?)