Skip to content

Commit

Permalink
Discourage AnnotationMirror#toString and AnnotationValue#toString
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 385258311
  • Loading branch information
cushon authored and Error Prone Team committed Jul 17, 2021
1 parent 5a29a1a commit 2b41cc9
Show file tree
Hide file tree
Showing 7 changed files with 297 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* Copyright 2021 The Error Prone Authors.
*
* 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;

import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;

import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.predicates.TypePredicate;
import com.google.errorprone.predicates.TypePredicates;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.Tree;
import java.util.Optional;

/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
name = "AnnotationMirrorToString",
summary =
"AnnotationMirror#toString doesn't use fully qualified type names, prefer auto-common's"
+ " AnnotationMirrors#toString",
severity = SUGGESTION)
public class AnnotationMirrorToString extends AbstractToString {

private static final TypePredicate TYPE_PREDICATE =
TypePredicates.isExactType("javax.lang.model.element.AnnotationMirror");

@Override
protected TypePredicate typePredicate() {
return TYPE_PREDICATE;
}

@Override
protected Optional<Fix> implicitToStringFix(ExpressionTree tree, VisitorState state) {
return fix(tree, tree, state);
}

@Override
protected Optional<Fix> toStringFix(Tree parent, ExpressionTree tree, VisitorState state) {
return fix(parent, tree, state);
}

private static Optional<Fix> fix(Tree replace, Tree with, VisitorState state) {
SuggestedFix.Builder fix = SuggestedFix.builder();
return Optional.of(
fix.replace(
replace,
String.format(
"%s.toString(%s)",
qualifyType(state, fix, "com.google.auto.common.AnnotationMirrors"),
state.getSourceForNode(with)))
.build());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* Copyright 2021 The Error Prone Authors.
*
* 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;

import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;

import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.predicates.TypePredicate;
import com.google.errorprone.predicates.TypePredicates;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.Tree;
import java.util.Optional;

/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
name = "AnnotationValueToString",
summary =
"AnnotationValue#toString doesn't use fully qualified type names, prefer auto-common's"
+ " AnnotationValues#toString",
severity = SUGGESTION)
public class AnnotationValueToString extends AbstractToString {

private static final TypePredicate TYPE_PREDICATE =
TypePredicates.isExactType("javax.lang.model.element.AnnotationValue");

@Override
protected TypePredicate typePredicate() {
return TYPE_PREDICATE;
}

@Override
protected Optional<Fix> implicitToStringFix(ExpressionTree tree, VisitorState state) {
return fix(tree, tree, state);
}

@Override
protected Optional<Fix> toStringFix(Tree parent, ExpressionTree tree, VisitorState state) {
return fix(parent, tree, state);
}

private static Optional<Fix> fix(Tree replace, Tree with, VisitorState state) {
SuggestedFix.Builder fix = SuggestedFix.builder();
return Optional.of(
fix.replace(
replace,
String.format(
"%s.toString(%s)",
qualifyType(state, fix, "com.google.auto.common.AnnotationValues"),
state.getSourceForNode(with)))
.build());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@
import com.google.errorprone.BugCheckerInfo;
import com.google.errorprone.bugpatterns.AmbiguousMethodReference;
import com.google.errorprone.bugpatterns.AnnotateFormatMethod;
import com.google.errorprone.bugpatterns.AnnotationMirrorToString;
import com.google.errorprone.bugpatterns.AnnotationPosition;
import com.google.errorprone.bugpatterns.AnnotationValueToString;
import com.google.errorprone.bugpatterns.ArrayAsKeyOfSetOrMap;
import com.google.errorprone.bugpatterns.ArrayEquals;
import com.google.errorprone.bugpatterns.ArrayFillIncompatibleType;
Expand Down Expand Up @@ -951,7 +953,9 @@ public static ScannerSupplier errorChecks() {
getSuppliers(
// keep-sorted start
AndroidJdkLibsChecker.class,
AnnotationMirrorToString.class,
AnnotationPosition.class,
AnnotationValueToString.class,
AssertFalse.class,
AssistedInjectAndInjectOnConstructors.class,
AutoFactoryAtInject.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright 2021 The Error Prone Authors.
*
* 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;

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

/** {@link AnnotationMirrorToString}Test */
@RunWith(JUnit4.class)
public class AnnotationMirrorToStringTest {

private final BugCheckerRefactoringTestHelper testHelper =
BugCheckerRefactoringTestHelper.newInstance(AnnotationMirrorToString.class, getClass());

@Test
public void refactoring() {
testHelper
.addInputLines(
"Test.java",
"import javax.lang.model.element.AnnotationMirror;",
"class Test {",
" String f(AnnotationMirror av) {",
" return av.toString();",
" }",
"}")
.addOutputLines(
"Test.java",
"import com.google.auto.common.AnnotationMirrors;",
"import javax.lang.model.element.AnnotationMirror;",
"class Test {",
" String f(AnnotationMirror av) {",
" return AnnotationMirrors.toString(av);",
" }",
"}")
.allowBreakingChanges() // TODO(cushon): remove after the next auto-common release
.doTest();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright 2021 The Error Prone Authors.
*
* 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;

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

/** {@link AnnotationValueToString}Test */
@RunWith(JUnit4.class)
public class AnnotationValueToStringTest {

private final BugCheckerRefactoringTestHelper testHelper =
BugCheckerRefactoringTestHelper.newInstance(AnnotationValueToString.class, getClass());

@Test
public void refactoring() {
testHelper
.addInputLines(
"Test.java",
"import javax.lang.model.element.AnnotationValue;",
"class Test {",
" String f(AnnotationValue av) {",
" return av.toString();",
" }",
"}")
.addOutputLines(
"Test.java",
"import com.google.auto.common.AnnotationValues;",
"import javax.lang.model.element.AnnotationValue;",
"class Test {",
" String f(AnnotationValue av) {",
" return AnnotationValues.toString(av);",
" }",
"}")
.allowBreakingChanges() // TODO(cushon): remove after the next auto-common release
.doTest();
}
}
28 changes: 28 additions & 0 deletions docs/bugpattern/AnnotationMirrorToString.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
In [recent versions of javac](https://bugs.openjdk.java.net/browse/JDK-8268729),
`AnnotationMirror#toString` returns a string representation of the annotation
that uses simple names. If the string is used in generated source code, it may
require additional imports.

For example, instead of this:

```
@com.pkg.Foo(bar = com.pkg.Bar.class, baz = com.pkg.Baz.ONE)
```

javac now generates the following:

```
@Foo(bar = Bar.class, baz = Baz.ONE)
```

which may require imports for `com.pkg.Foo`.

`auto-common`'s `AnnotationMirrors#toString` method produces a string that uses
fully qualified names for annotations, class literals, and enum constants,
ensuring that source code containing that string will compile without additional
imports.

TIP: `AnnotationMirrors#toString` may be beneficial even if the string isn't
being used in generated code, e.g. if it's part of a diagnostic message or
assertion failure message, since the fully qualified names makes it clearer
which types are being referred to.
17 changes: 17 additions & 0 deletions docs/bugpattern/AnnotationValueToString.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
In [recent versions of javac](https://bugs.openjdk.java.net/browse/JDK-8268729),
`AnnotationValue#toString` returns a string representation of the annotation
value that uses simple names. If the string is used in generated source code, it
may require additional imports.

For example, instead of the class literal `com.pkg.Bar.class`, javac now returns
just `Bar.class`, which may require adding an import for for `com.pkg.Bar`.

`auto-common`'s `AnnotationValues#toString` method produces a string that uses
fully qualified names for annotations, class literals, and enum constants,
ensuring that source code containing that string will compile without additional
imports.

TIP: `AnnotationValues#toString` may be beneficial even if the string isn't
being used in generated code, e.g. if it's part of a diagnostic message or
assertion failure message, since the fully qualified names makes it clearer
which types are being referred to.

0 comments on commit 2b41cc9

Please sign in to comment.