Remove `METHOD` from `@Target` meta-annotation of `LengthOf`

[mernst]

It is surprising that the type annotation LengthOf is meta-annotated as

@Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER, ElementType.METHOD})

Here is an excerpt from commit 71379bb7de:

-@Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER})
+// Has target of METHOD so that it is stored as a declaration annotation and SameLen Checker can read it.
+@Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER, ElementType.METHOD})
 public @interface LengthOf {

We have a FAQ that says not to make an annotation applicable to both type uses and declarations.

The METHOD target has surprising results. For example, it means that when a source has only one occurrence of @LengthOf, its corresponding .class file has two occurrences. A recent bug report says that this behavior is confusing.

I propose that we clean up the code (and help Shubham, who reported the issue I mentioned above) by doing 2 things:

1. Remove METHOD from LengthOf's @Target meta-annotation.
2. Remove the special-case handling of String.length in isLengthOfMethodInvocation.

Both items require changes to isLengthOfMethodInvocation.

Regarding item 1:

In isLengthOfMethodInvocation this line should look for a type, not a declaration, annotation:

        AnnotationMirror len = factory.getDeclAnnotation(ele, LengthOf.class);

Regarding item 2:

Remove the first clause from the implementation of isLengthOfMethodInvocation:

     public boolean isLengthOfMethodInvocation(ExecutableElement ele) {
        if (stringLength.equals(ele)) {
            // TODO: Why not just annotate String.length with @LengthOf and thus eliminate the
            // special case in this method's implementation?
            return true;
        }

[kelloggm]

This behavior isn't great, I agree.

The reason for it is that @LengthOf isn't actually claimed by any checker, so there is no AnnotatedTypeFactory to query the return type of the method. @LengthOf is an alias for @LTEqLengthOf, but there's no way to differentiate the two from an annotated type factory's perspective.

One alternative would be looking at the annotation the user actually wrote (a la the Value Checker with @Positive annotations), but that's restricted to where source code is available (meaning not the JDK, so we'd still need to special-case String#length()).

An alternative to your proposal is to make @LengthOf only a declaration annotation. We could then infer the @NonNegative @LtEqLengthOf annotations in their appropriate type factories. The resulting bytecode would then have both LenghtOf (as a declaration annotation) and standard lower/upper bound annotations.

Would that address your concern?

[mernst]

@LengthOf expresses a fact about an expression's value. Therefore, it should be a type annotation. Also, for maximal expressiveness and least surprise, it should be permitted to be written on any type, and not just on method return types.

I think a better solution is to make @LengthOf only a type annotation, rather than making it only a method annotation.

If @LengthOf is a type annotation, it needs to be available to checkers. Presumably, this means it needs to exist in an existing hierarchy or a new one. What implementation difficulties does that present?

[kelloggm]

@LengthOf doesn't fit into any of the Index Checker's existing type systems, because it is a "two-sided" annotation - it contains information about both the lowerbound and the upperbound. It's therefore most natural to represent it as an alias for @NonNegative @LtEqLengthOf, which is what is done now.

As we both mentioned above, however, this prevents the SameLen Checker from finding this annotation in particular, because:

• the SameLen Checker runs before both the Lowerbound Checker and the Upperbound Checker, which are responsible for the two parts of @LengthOf
• it might be possible to find the annotation when source code is available, but @LengthOf won't appear in pre-compiled bytecode because the LBC/UBC will replace it with their own annotations

It sounds like you might be suggesting that @LengthOf should be handled by its own type system. I do not think this is wise, because:

• it is unclear where such a type system would run in the hierarchy: it should run before the SameLen checker, but is closer to the LBC/UBC in content
• it's unclear how complete it should be. Is this just a "taint-tracking" system with the annotation being trusted, or is it checked? If it's checked it needs LBC/UBC facts, and therefore has nowhere in the type system order that makes sense.

I also disagree with your characterization of @LengthOf as definitely a fact about an expression's value. We've talked before about a @SizeMethod or @LengthMethod annotation to represent the method that returns the size of a collection when the collection is user defined, and I've always thought of @LengthOf as a prototype implementation of that. Such an annotation is more naturally a declaration annotation, since its expressing a fact about the method (that this is the canonical method to call to get the length of the sequence).

I agree that the name @LengthOf for a declaration annotation is confusing. What if we changed the name to @LengthMethod, and then otherwise implemented what I suggested? (that is, make it a declaration annotation that creates a requirement that the return type is @IndexOrHigh? That check could even be syntactic, and we could require users to write the annotations - I think I would prefer that design)

[mernst]

As currently specified, @LengthOf is a type annotation and not a method annotation.

Changing its name, semantics, and target locations (as you propose) would be better than the current situation, and would address the known use cases. So, I could support that change. Thanks for the suggestion.