Skip to content

Commit

Permalink
Add a check for Guice/JSR330 Qualifiers
Browse files Browse the repository at this point in the history
where the Qualifier is @target'ed to be a TYPE_PARAMETER or TYPE_USE.
Injection frameworks currently don't understand these uses.

RELNOTES: New experiment check: QualifierWithTypeUse, to flag instances
where Qualifier annotations are declared as eligible for use in TYPE_USE
or TYPE_PARAMETER contexts.

MOE_MIGRATED_REVID=133635458
  • Loading branch information
nick-someone authored and cushon committed Sep 21, 2016
1 parent e19649b commit 8316dea
Show file tree
Hide file tree
Showing 7 changed files with 256 additions and 1 deletion.
Expand Up @@ -100,10 +100,15 @@ public final Description matchClass(ClassTree classTree, VisitorState state) {
* Rewrite the annotation with static imports, adding TYPE and METHOD to the @Target annotation
* value (and reordering them to their declaration order in ElementType).
*/
private Fix replaceTargetAnnotation(Target annotation, AnnotationTree targetAnnotationTree) {
private static Fix replaceTargetAnnotation(
Target annotation, AnnotationTree targetAnnotationTree) {
Set<ElementType> types = EnumSet.copyOf(REQUIRED_ELEMENT_TYPES);
types.addAll(Arrays.asList(annotation.value()));

return replaceTargetAnnotation(targetAnnotationTree, types);
}

static Fix replaceTargetAnnotation(AnnotationTree targetAnnotationTree, Set<ElementType> types) {
Builder builder =
SuggestedFix.builder()
.replace(targetAnnotationTree, "@Target({" + Joiner.on(", ").join(types) + "})");
Expand Down
@@ -0,0 +1,104 @@
/*
* Copyright 2016 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.bugpatterns.inject;

import static com.google.errorprone.BugPattern.Category.INJECT;
import static com.google.errorprone.BugPattern.MaturityLevel.EXPERIMENTAL;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.annotations;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.hasAnnotation;
import static com.google.errorprone.matchers.Matchers.isType;
import static com.google.errorprone.matchers.Matchers.kindIs;
import static com.sun.source.tree.Tree.Kind.ANNOTATION_TYPE;

import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.InjectMatchers;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.MultiMatcher;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.AnnotationTree;
import com.sun.source.tree.ClassTree;
import java.lang.annotation.ElementType;
import java.lang.annotation.Target;
import java.util.Arrays;
import java.util.Collections;
import java.util.EnumSet;
import java.util.Set;

/** @author glorioso@google.com (Nick Glorioso) */
@BugPattern(
name = "QualifierWithTypeUse",
summary =
"Injection frameworks currently don't understand Qualifiers in TYPE_PARAMETER or"
+ " TYPE_USE contexts.",
category = INJECT,
severity = WARNING,
maturity = EXPERIMENTAL
)
public class QualifierWithTypeUse extends BugChecker implements ClassTreeMatcher {

private static final MultiMatcher<ClassTree, AnnotationTree> HAS_TARGET_ANNOTATION =
annotations(AT_LEAST_ONE, isType("java.lang.annotation.Target"));

private static final Matcher<ClassTree> IS_QUALIFIER_WITH_TARGET =
allOf(
kindIs(ANNOTATION_TYPE),
anyOf(
hasAnnotation(InjectMatchers.JAVAX_QUALIFIER_ANNOTATION),
hasAnnotation(InjectMatchers.GUICE_BINDING_ANNOTATION)),
HAS_TARGET_ANNOTATION);

private static final ImmutableSet<ElementType> FORBIDDEN_ELEMENT_TYPES =
ImmutableSet.of(ElementType.TYPE_PARAMETER, ElementType.TYPE_USE);

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
if (IS_QUALIFIER_WITH_TARGET.matches(tree, state)) {
AnnotationTree annotationTree = HAS_TARGET_ANNOTATION.getMatchingNodes().get(0);
Target targetAnnotation = ASTHelpers.getAnnotation(tree, Target.class);
if (hasTypeUseOrTypeParameter(targetAnnotation)) {
return describeMatch(annotationTree, removeTypeUse(targetAnnotation, annotationTree));
}
}
return Description.NO_MATCH;
}

private boolean hasTypeUseOrTypeParameter(Target targetAnnotation) {
// Should only be in cases where Target is not in the classpath
return targetAnnotation != null
&& !Collections.disjoint(FORBIDDEN_ELEMENT_TYPES, Arrays.asList(targetAnnotation.value()));
}

private Fix removeTypeUse(Target targetAnnotation, AnnotationTree tree) {
Set<ElementType> elements = EnumSet.copyOf(Arrays.asList(targetAnnotation.value()));
elements.removeAll(FORBIDDEN_ELEMENT_TYPES);
if (elements.isEmpty()) {
return SuggestedFix.delete(tree);
}
return InvalidTargetingOnScopingAnnotation.replaceTargetAnnotation(tree, elements);
}
}
Expand Up @@ -159,6 +159,7 @@
import com.google.errorprone.bugpatterns.inject.MoreThanOneQualifier;
import com.google.errorprone.bugpatterns.inject.MoreThanOneScopeAnnotationOnClass;
import com.google.errorprone.bugpatterns.inject.OverlappingQualifierAndScopeAnnotation;
import com.google.errorprone.bugpatterns.inject.QualifierWithTypeUse;
import com.google.errorprone.bugpatterns.inject.ScopeAnnotationOnInterfaceOrAbstractClass;
import com.google.errorprone.bugpatterns.inject.ScopeOrQualifierAnnotationRetention;
import com.google.errorprone.bugpatterns.inject.dagger.EmptySetMultibindingContributions;
Expand Down Expand Up @@ -375,6 +376,7 @@ public static ScannerSupplier errorChecks() {
PrivateConstructorForUtilityClass.class,
PrivateConstructorForNoninstantiableModule.class,
ProtoStringFieldReferenceEquality.class,
QualifierWithTypeUse.class,
RedundantThrows.class,
RemoveUnusedImports.class,
ScopeAnnotationOnInterfaceOrAbstractClass.class,
Expand Down
@@ -0,0 +1,45 @@
/*
* Copyright 2016 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.bugpatterns.inject;

import com.google.errorprone.CompilationTestHelper;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** @author glorioso@google.com (Nick Glorioso) */
@RunWith(JUnit4.class)
public class QualifierWithTypeUseTest {

private CompilationTestHelper compilationHelper;

@Before
public void setUp() {
compilationHelper = CompilationTestHelper.newInstance(QualifierWithTypeUse.class, getClass());
}

@Test
public void testPositiveCase() throws Exception {
compilationHelper.addSourceFile("QualifierWithTypeUsePositiveCases.java").doTest();
}

@Test
public void testNegativeCase() throws Exception {
compilationHelper.addSourceFile("QualifierWithTypeUseNegativeCases.java").doTest();
}
}
@@ -0,0 +1,32 @@
/*
* Copyright 2016 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.bugpatterns.inject.testdata;

import java.lang.annotation.ElementType;
import java.lang.annotation.Target;
import javax.inject.Qualifier;

/** Tests for {@code QualifierWithTypeUse} */
public class QualifierWithTypeUseNegativeCases {

@Qualifier
@Target({ElementType.CONSTRUCTOR})
@interface Qualifier1 {}

@Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER})
@interface NotAQualifier {}
}
@@ -0,0 +1,51 @@
/*
* Copyright 2016 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.bugpatterns.inject.testdata;

import com.google.inject.BindingAnnotation;
import java.lang.annotation.ElementType;
import java.lang.annotation.Target;
import javax.inject.Qualifier;

/** Tests for {@code QualifierWithTypeUse} */
public class QualifierWithTypeUsePositiveCases {

@Qualifier
// BUG: Diagnostic contains: @Target({CONSTRUCTOR})
@Target({ElementType.TYPE_USE, ElementType.CONSTRUCTOR})
@interface Qualifier1 {}

@Qualifier
// BUG: Diagnostic contains: remove
@Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER})
@interface Qualifier2 {}

@BindingAnnotation
// BUG: Diagnostic contains: @Target({FIELD})
@Target({ElementType.FIELD, ElementType.TYPE_USE})
@interface BindingAnnotation1 {}

@BindingAnnotation
// BUG: Diagnostic contains: remove
@Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER})
@interface BindingAnnotation2 {}

@BindingAnnotation
// BUG: Diagnostic contains: remove
@Target(ElementType.TYPE_USE)
@interface BindingAnnotation3 {}
}
16 changes: 16 additions & 0 deletions docs/bugpattern/QualifierWithTypeUse.md
@@ -0,0 +1,16 @@
Allowing a qualifier annotation in [`TYPE_PARAMETER`] or [`TYPE_USE`] contexts
allows end users to write code like:

```java
@Inject Foo(List<@MyAnnotation String> strings)
```

Guice, Dagger, and other dependency injection frameworks don't currently see
type annotations in this context, so the above code is equivalent to:

```java
@Inject Foo(List<String> strings)
```

[`TYPE_PARAMETER`]: https://docs.oracle.com/javase/8/docs/api/java/lang/annotation/ElementType.html#TYPE_PARAMETER
[`TYPE_USE`]: https://docs.oracle.com/javase/8/docs/api/java/lang/annotation/ElementType.html#TYPE_USE

0 comments on commit 8316dea

Please sign in to comment.