Remove redundant targets from Nullability-annotations

[JarvisCraft]

At current both @NotNull and @Nullable have quite broad scopes which commonly overlap:

java-annotations/common/src/main/java/org/jetbrains/annotations/Nullable.java

Line 39 in d7c469b

@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})

java-annotations/common/src/main/java/org/jetbrains/annotations/NotNull.java

Line 29 in d7c469b

@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})

While this is required for java5 (which obviously does not have ElementType.TYPE_USE) it is more of an issue rather than a feature on java 8+. I.e. overlapping scopes lead to tools such as Javadoc recognizing both targets as matched thus leading to issues such as duplication of the annotation in documentation in case of the latter.

Thus I suggest keeping only ElementType.TYPE_USE on these annotations in java8 version of the project, and keeping only {ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE} on java5 version.

If this gets approved I am ready to create the corresponding PR.

[amaembo]

This may cause source incompatibility, in particular with qualified class names. E.g.:

@NotNull Outer.Inner getInner();

Now, this is compilable (because NotNull has METHOD target) and interpreted as Outer.@NotNull Inner. After the proposed change, it won't be compilable anymore (assuming that Inner is a static inner class). So after an update, users may have to update the sources as well.

Another problem is Groovy. AFAIK it doesn't support type annotations at all. Now, it's possible to use java-annotations package in mixed Groovy-Java projects and annotate Groovy method parameters, return types, and fields. It would be problematic if we remove redundant targets.

I see the JavaDoc problem but I believe it should be fixed in JavaDoc tool. So consider reporting it there.

Probably at some day this change will be implemented, so I'm not closing this. Still, I think it's too early now to do this.

[JarvisCraft]

I see the JavaDoc problem but I believe it should be fixed in JavaDoc tool. So consider reporting it there.

I've had created a test class with both multi-target and single-target annotations and now it looks like Javadoc's behaviour is correct from point of classfile info:

Test class

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Method;
import java.util.Arrays;

class Scratch {
    public static void main(String[] args) throws NoSuchMethodException {
        final Method method = Scratch.class.getDeclaredMethod("foo", Object.class);

        System.out.println("          PARAMETER: " + Arrays.deepToString(method.getParameterAnnotations()));
        System.out.println("             METHOD: " + Arrays.toString(method.getAnnotations()));
        System.out.println("   TYPE_USE(Method): " + method.getAnnotatedReturnType());
        System.out.println("TYPE_USE(Parameter): " + Arrays.toString(method.getAnnotatedParameterTypes()));
    }

    private static @Wide @Specific Object foo(final @Wide @Specific Object param) {
        return param;
    }

    @Retention(RetentionPolicy.RUNTIME)
    @Target({
                    ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER,
                    ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE
    })
    public @interface Wide {}

    @Retention(RetentionPolicy.RUNTIME)
    @Target(ElementType.TYPE_USE)
    public @interface Specific {}
}

Output

          PARAMETER: [[@Scratch$Wide()]]
             METHOD: [@Scratch$Wide()]
   TYPE_USE(Method): @Scratch$Wide() @Scratch$Specific() java.lang.Object
TYPE_USE(Parameter): [@Scratch$Wide() @Scratch$Specific() java.lang.Object]

Process finished with exit code 0

As can be seen, at it is correct that from classfile point both TYPE_USE- and METHOD/PARAMETER get matched whenever both targets are present on the annotations, so Javadoc's behaviour seems to be correct.

This may cause source incompatibility, in particular with qualified class names. E.g.:

@NotNull Outer.Inner getInner();

Now, this is compilable (because NotNull has METHOD target) and interpreted as Outer.@NotNull Inner. After the proposed change, it won't be compilable anymore (assuming that Inner is a static inner class). So after an update, users may have to update the sources as well.

As of this, it actually is a problem, I agree, but I fell like it is worth implementing this change (probably, under a major version bump) as at current state its backwards compatibility causing issues for latest stable users.

Another problem is Groovy. AFAIK it doesn't support type annotations at all. Now, it's possible to use java-annotations package in mixed Groovy-Java projects and annotate Groovy method parameters, return types, and fields. It would be problematic if we remove redundant targets.

As an option, those clients who currently have forward-incompatible changes would simply have an ability to stick to current version (e.g. 20.1.0) while letting up-to-date users move to something like 21.0.0 with these target limitations.
As of Groovy and similar cases, these may stick to java5 artifact (again, no random issues will appear of current dependency on default artifact as it does not contain this changeset) which currently has to be done with Java 8 clients which cannot allow the issues like the Javadoc one. In other words I fell like its a better strategy to force these changes to support newer clients yet giving an option to those who use forward-incompatible changes yet giving them reasons to migrate to newer version.

As for me it is also a moral question of supporting the TYPE_USE concept in general because at current state many developers use such annotations incorrectly (due to them being able to do so) which leads to various forms of ambiguity and inconsistency. I.e. some developers might even be unaware of the ability to annotate types rather than parameters as they simply don't know of this option although this ability is provided and is recommended. At the same time those who use this feature correctly face related issues which are cause by the need to support the previous ones. Thus applying this change would (in my view) be a good push for the developers to use newer better features when possible and be an act of support towards those who use it already.