Annotations.isAllDefaultMethods returns false for method less interfaces.

[GoogleCodeExporter]

Description of the issue:

Steps to reproduce:
1. Annotations.isAllDefaultMethods() probably should return true for 
   simple marker annotations.
2. The code:

public static boolean isAllDefaultMethods(Class<? extends Annotation> 
annotationType) {
  boolean hasMethods = false;
  for (Method m : annotationType.getDeclaredMethods()) {
    hasMethods = true;
    if (m.getDefaultValue() == null) {
      return false;
    }
  }
  return hasMethods;
}

This class returns true if the annotation class has at least one method that 
has a default value set. 
It returns false if the annotation class has no methods at all 
It also returns false if there are methods that have no default values.

This code should probably read

public static boolean isAllDefaultMethods(Class<? extends Annotation> 
annotationType) {
  for (Method m : annotationType.getDeclaredMethods()) {
    if (m.getDefaultValue() == null) {
      return false;
    }
  }
  return true;
}

Original issue reported on code.google.com by henn...@schmiedehausen.org on 24 Sep 2013 at 6:59

[GoogleCodeExporter]

Is this causing a bug?  The method is in inject/internal, and AFAIK it's 
working as it's expected to work.  It returns true if there's any methods in 
the class & every method has a default value.  

Your statement, "This class returns true if the annotation class has at least 
one method that has a default value set." does not seem to be true based on 
reading the code.  It returns true if the annotation class has at least one 
method and *every* method has a default value set.

Original comment by sberlin on 24 Sep 2013 at 7:06

[GoogleCodeExporter]

This annotation returns false:

public @interface Foo
{
}

This annotation returns true:

public @interface Bar
{
    String value() default "";
}

Clearly, the first one should return true, too... :-) 

Original comment by henn...@schmiedehausen.org on 24 Sep 2013 at 7:10

[GoogleCodeExporter]

And yes, it only returns true if all declared methods have a default value set. 
The question is, what would happen if no methods are declared. IMHO that should 
return true, not false. 

Original comment by henn...@schmiedehausen.org on 24 Sep 2013 at 7:12

[GoogleCodeExporter]

What bug is this causing?  The code is in internal and is intended for internal 
use in Guice.  External code should not use it because it can change at any 
time.

If this isn't causing a bug, I'm inclined to close it, because it's doing what 
it's designed to do.

Original comment by sberlin on 24 Sep 2013 at 7:33

[GoogleCodeExporter]

Original comment by sberlin on 24 Sep 2013 at 8:03

• Changed state: WorkingAsIntended

[GoogleCodeExporter]

So if it is designed to do what it should do, maybe it is used incorrectly here:

static AnnotationStrategy strategyFor(Class<? extends Annotation> 
annotationType) {
    annotationType = Annotations.canonicalizeIfNamed(annotationType);
    if (isAllDefaultMethods(annotationType)) {
      return strategyFor(generateAnnotation(annotationType));
    }

    checkNotNull(annotationType, "annotation type");
    ensureRetainedAtRuntime(annotationType);
    ensureIsBindingAnnotation(annotationType);
    return new AnnotationTypeStrategy(annotationType, null);
  }

This guy returns an AnnotationTypeStrategy with an annotation type and null for 
an annotation if the class has no methods at all or it has methods that have no 
default value but an AnnotationInstanceStrategy (which in turn has both an 
annotation type and an annotation instance) if it has at least one method and 
all methods have default values.

I still think that this is confusing behavior. Also, the 
strategyFor(Annotation) method might have a similar bug here (it returns an 
AnnotationTypeStrategy for a marker interface, but one that has an annotation 
type and an annotation) and an AnnotationInstanceStrategy otherwise.

Why am I beating around on this? I want go get rid (or at least simplify) this 
ugly piece of code:

private static Key<?> findDependencyKey(TypeLiteral<?> type, Key<?> clazzKey) {
  if (clazzKey.getAnnotation() != null) {
    return Key.get(type, clazzKey.getAnnotation());
  }
  else if (clazzKey.getAnnotationType() != null) {
    return Key.get(type, clazzKey.getAnnotationType());
  }
  else {
    throw new ProviderException("Neither annotation nor annotation type found!");
  }

it would be great if there were a halfway predictable way to know whether 
Key.getAnnotationType() or Key.getAnnotation() return something, no matter 
whether they have been constructed from a Named or a marker annotation. Right 
now, no matter what style is used to generate the key, one of the methods 
returns null. And the reason for that is that piece of code in strategyFor() 
above which in turn relies on isAllDefaultMethods.

        }

Original comment by henn...@schmiedehausen.org on 24 Sep 2013 at 8:38

[GoogleCodeExporter]

I guess I have to repeat my original question: is this causing a bug, and if 
so, what bug?

It's used as expected because if there are no methods then we don't need to 
bother creating a fake implementation of the class -- we explicitly want to use 
the class-based annotation strategy.  There's no point in doing the 
instance-based one, since there's no parameters it has to worry about.  Same 
thing with the strategyFor(Annotation) method -- if the annotation has no 
methods, we don't need to bother with the instance-based strategy.

Granted, this may be confusing internally -- but it's all strictly internal 
code, and shouldn't effect the API in any way.

Can you use the ofType(..) method to do clazzKey.ofType(type)?  If that doesn't 
work that seems like an issue itself, but unrelated to this.

I don't understand why knowing what will return 'null' will make any 
difference.  You want to reconstruct a Key, and the safest way using the 
provided API to do it is to check the return values & call the appropriate 
constructor.  Doing it any other way is re-implementing the internals and is 
subject to break/change at any time. 

Original comment by sberlin on 24 Sep 2013 at 8:56

[GoogleCodeExporter]

So, the problem is that any piece of code has to drag around this dichtonomy 
between "Annotation" (IAW Names.named() and friends) and Class<? extends 
Annotation> (if using a marker annotation). There is no good way to pull them 
together so that subsequent code can use an "XXX" to represent both. Key is 
really close to it but when handing a key around, there are still two methods 
to check, whether it was created from an Annotation or a Class<? extends 
Annotation>.

The code example above does something very different (and you will see why your 
ofType comment does not apply):

- I have a key that represents a type and either an Annotation or a Class<? 
extends Annotation>
- I want to create a new key for a different class that has the same Annotation 
or Class<? extends Annotation> (the use case is that I bind an object with 
those and want to inject dependencies that were bound the same way into the 
objects). To create this key, I now need to inspect the Key passed in and 
figure out whether it was created from an Annotation (then getAnnotation() 
returns non-null) or an AnnotationType (then getAnnotationType() returns non 
null). This would be so much simpler if for any AnnotationType (Class<? extends 
Annotation>) used as a marker annotation, the getAnnotation() would actually 
return something (a fake implementation of the Marker annotation passed in). 

Is it a bug? Well, as you mention that this is intentional, maybe not. Is it a 
bad API? Probably because it forces me to do this crab walk of checking left / 
checking right all the time. And the irony of it all is that I can actually fix 
that trivially in my code by using

public @interface Bar {
    String workaroundGuiceImplementationLimitation() default "";
}

as a marker interface and then only look at clazzKey.getAnnotation() above.

(In my internal code, the method has a different name).

And given that the proposed change would actually align with the current 
behavior (instead of creating an instance only if 1..n methods with default 
present, this now creates an instance when 0..n methods with default are 
present). 

I simply think that right now, the code is just not doing the right thing. So 
it is a bug. IMHO.

Original comment by henn...@schmiedehausen.org on 24 Sep 2013 at 10:52

[GoogleCodeExporter]

> I have a key that represents a type and either an Annotation or a Class<? 
extends Annotation>
> I want to create a new key for a different class that has the same Annotation 
or Class<? extends Annotation>

This sounds exactly like Key.ofType:

  "Returns a new key of the specified type with the same annotation as this key"

Using your previous example you could rewrite it as:

  private static Key<?> findDependencyKey(TypeLiteral<?> type, Key<?> clazzKey) {
    return classKey.ofType(type);
  }

and if you want to guarantee there is at least an annotation instance or marker 
then use:

  private static Key<?> findDependencyKey(TypeLiteral<?> type, Key<?> clazzKey) {
    if (classKey.getAnnotationType() != null) {
      return classKey.ofType(type);
    } else {
      throw new ProviderException("Neither annotation nor annotation type found!");
    }
  }

because Key.getAnnotationType() will only return null when there's no 
annotation instance or marker.

Original comment by mccu...@gmail.com on 24 Sep 2013 at 11:06

[GoogleCodeExporter]

I don't buy that it's a bug.  The code is doing exactly what it was written to 
do.  You can argue that the current API Guice exposes makes what you want to do 
cumbersome -- but there's no bug in the code.

Your workarounds are exploiting implementation details that can change (and 
actually wouldn't have worked in 3.0 -- the "creating fake implementations" is 
only introduced in 4.0).

I still don't understand why ofType doesn't work -- it seems to do exactly what 
you want: create a key with a new Type/TypeLiteral with the exact same 
annotation or annotation class.  Can you explain why that method doesn't work 
for you?  It seems that the method exists solely for your use-case.

Creating fake implementations for marker interfaces would be a waste of memory. 
 The approach to fixing this should be, "What is the right thing to do?"  It 
should not be, "what's the smallest change I can make to solve my current 
problem."

Original comment by sberlin on 24 Sep 2013 at 11:07

[GoogleCodeExporter]

Guess, I read the explanation for ofType wrong. Thanks for pointing this out, I 
will try this.

Original comment by henn...@schmiedehausen.org on 24 Sep 2013 at 11:09