From f320353b3627c34e26eaca3811c580ecaa62dedb Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Fri, 18 Feb 2022 14:39:24 +0000 Subject: [PATCH] Lint for @hide tags in comments Errorprone will warn for normal javadoc tags in comments, but doesn't know about @hide and so ignores it. This can lead to accidental exposure of APIs when folks mistakenly use a comment block instead of a javadoc block. Also, adds some static dependencies to enable actually running the tests on the host. Fixes: 181140148 Test: atest --host error_prone_android_framework_test:com.google.errorprone.bugpatterns.android.HideInCommentsCheckerTest Change-Id: I46a59075f93468fc0e57264bca0d86994ebb3d86 --- errorprone/Android.bp | 3 +- .../android/HideInCommentsChecker.java | 158 ++++++++++++ .../android/HideInCommentsCheckerTest.java | 235 ++++++++++++++++++ 3 files changed, 395 insertions(+), 1 deletion(-) create mode 100644 errorprone/java/com/google/errorprone/bugpatterns/android/HideInCommentsChecker.java create mode 100644 errorprone/tests/java/com/google/errorprone/bugpatterns/android/HideInCommentsCheckerTest.java diff --git a/errorprone/Android.bp b/errorprone/Android.bp index a927f53e3b5f5..8f32f0eab37ff 100644 --- a/errorprone/Android.bp +++ b/errorprone/Android.bp @@ -1,4 +1,3 @@ - package { // See: http://go/android-license-faq // A large-scale-change added 'default_applicable_licenses' to import @@ -42,8 +41,10 @@ java_test_host { static_libs: [ "truth-prebuilt", "kxml2-2.3.0", + "compile-testing-prebuilt", "error_prone_android_framework_lib", "error_prone_test_helpers", + "google_java_format", "hamcrest-library", "hamcrest", "platform-test-annotations", diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/HideInCommentsChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/HideInCommentsChecker.java new file mode 100644 index 0000000000000..07f1d4a09006d --- /dev/null +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/HideInCommentsChecker.java @@ -0,0 +1,158 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * 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.android; + +import static com.google.errorprone.BugPattern.LinkType.NONE; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.getStartPosition; +import static com.google.errorprone.util.ASTHelpers.getSymbol; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.ErrorProneToken; +import com.google.errorprone.util.ErrorProneTokens; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.parser.Tokens; + +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; + +import javax.lang.model.element.ElementKind; + +/** + * Bug checker to warn about {@code @hide} directives in comments. + * + * {@code @hide} tags are only meaningful inside of Javadoc comments. Errorprone has checks for + * standard Javadoc tags but doesn't know anything about {@code @hide} since it's an Android + * specific tag. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "AndroidHideInComments", + summary = "Warns when there are @hide declarations in comments rather than javadoc", + linkType = NONE, + severity = WARNING) +public class HideInCommentsChecker extends BugChecker implements + BugChecker.CompilationUnitTreeMatcher { + + @Override + public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { + final Map javadocableTrees = findJavadocableTrees(tree); + final String sourceCode = state.getSourceCode().toString(); + for (ErrorProneToken token : ErrorProneTokens.getTokens(sourceCode, state.context)) { + for (Tokens.Comment comment : token.comments()) { + if (!javadocableTrees.containsKey(token.pos())) { + continue; + } + generateFix(comment).ifPresent(fix -> { + final Tree javadocableTree = javadocableTrees.get(token.pos()); + state.reportMatch(describeMatch(javadocableTree, fix)); + }); + } + } + // We might have multiple matches, so report them via VisitorState rather than the return + // value from the match function. + return NO_MATCH; + } + + private static Optional generateFix(Tokens.Comment comment) { + final String text = comment.getText(); + if (text.startsWith("/**")) { + return Optional.empty(); + } + + if (!text.contains("@hide")) { + return Optional.empty(); + } + + if (text.startsWith("/*")) { + final int pos = comment.getSourcePos(1); + return Optional.of(SuggestedFix.replace(pos, pos, "*")); + } else if (text.startsWith("//")) { + final int endPos = comment.getSourcePos(text.length() - 1); + final char endChar = text.charAt(text.length() - 1); + String javadocClose = " */"; + if (endChar != ' ') { + javadocClose = endChar + javadocClose; + } + final SuggestedFix fix = SuggestedFix.builder() + .replace(comment.getSourcePos(1), comment.getSourcePos(2), "**") + .replace(endPos, endPos + 1, javadocClose) + .build(); + return Optional.of(fix); + } + + return Optional.empty(); + } + + + private Map findJavadocableTrees(CompilationUnitTree tree) { + Map javadoccableTrees = new HashMap<>(); + new SuppressibleTreePathScanner() { + @Override + public Void visitClass(ClassTree classTree, Void unused) { + javadoccableTrees.put(getStartPosition(classTree), classTree); + return super.visitClass(classTree, null); + } + + @Override + public Void visitMethod(MethodTree methodTree, Void unused) { + // Generated constructors never have comments + if (!ASTHelpers.isGeneratedConstructor(methodTree)) { + javadoccableTrees.put(getStartPosition(methodTree), methodTree); + } + return super.visitMethod(methodTree, null); + } + + @Override + public Void visitVariable(VariableTree variableTree, Void unused) { + ElementKind kind = getSymbol(variableTree).getKind(); + if (kind == ElementKind.FIELD) { + javadoccableTrees.put(getStartPosition(variableTree), variableTree); + } + if (kind == ElementKind.ENUM_CONSTANT) { + javadoccableTrees.put(getStartPosition(variableTree), variableTree); + if (variableTree.getInitializer() instanceof NewClassTree) { + // Skip the generated class definition + ClassTree classBody = + ((NewClassTree) variableTree.getInitializer()).getClassBody(); + if (classBody != null) { + scan(classBody.getMembers(), null); + } + return null; + } + } + return super.visitVariable(variableTree, null); + } + + }.scan(tree, null); + return javadoccableTrees; + } + +} diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/HideInCommentsCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/HideInCommentsCheckerTest.java new file mode 100644 index 0000000000000..f3e6c72756e0a --- /dev/null +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/HideInCommentsCheckerTest.java @@ -0,0 +1,235 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * 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.android; + +import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class HideInCommentsCheckerTest { + private static final String REFACTORING_FILE = "Test.java"; + + private BugCheckerRefactoringTestHelper mRefactoringHelper; + private CompilationTestHelper mCompilationHelper; + + @Before + public void setUp() { + mRefactoringHelper = BugCheckerRefactoringTestHelper.newInstance( + HideInCommentsChecker.class, HideInCommentsCheckerTest.class); + mCompilationHelper = CompilationTestHelper.newInstance( + HideInCommentsChecker.class, HideInCommentsCheckerTest.class); + } + + + @Test + public void refactorSingleLineComment() { + mRefactoringHelper + .addInputLines( + REFACTORING_FILE, + "public class Test {", + " // Foo @hide", + " void foo() {}", + "}") + .addOutputLines( + REFACTORING_FILE, + "public class Test {", + " /** Foo @hide */", + " void foo() {}", + "}") + .doTest(TEXT_MATCH); + } + + @Test + public void refactorSingleLineComment_doesntAddUnnecessarySpace() { + mRefactoringHelper + .addInputLines( + REFACTORING_FILE, + "public class Test {", + " // Foo @hide ", + " void foo() {}", + "}") + .addOutputLines( + REFACTORING_FILE, + "public class Test {", + " /** Foo @hide */", + " void foo() {}", + "}") + .doTest(TEXT_MATCH); + } + + @Test + public void refactorSingleLineBlockComment() { + mRefactoringHelper + .addInputLines( + REFACTORING_FILE, + "public class Test {", + " /* Foo @hide */", + " void foo() {}", + "}") + .addOutputLines( + REFACTORING_FILE, + "public class Test {", + " /** Foo @hide */", + " void foo() {}", + "}") + .doTest(TEXT_MATCH); + } + + @Test + public void refactorMultiLineBlockComment() { + mRefactoringHelper + .addInputLines( + REFACTORING_FILE, + "public class Test {", + " /*", + " * Foo.", + " *", + " * @hide", + " */", + " void foo(int foo) {}", + "}") + .addOutputLines( + REFACTORING_FILE, + "public class Test {", + " /**", + " * Foo.", + " *", + " * @hide", + " */", + " void foo(int foo) {}", + "}") + .doTest(TEXT_MATCH); + } + + @Test + public void refactorFieldComment() { + mRefactoringHelper + .addInputLines( + REFACTORING_FILE, + "public class Test {", + " /* Foo @hide */", + " public int foo = 0;", + "}") + .addOutputLines( + REFACTORING_FILE, + "public class Test {", + " /** Foo @hide */", + " public int foo = 0;", + "}") + .doTest(TEXT_MATCH); + } + + @Test + public void refactorClassComment() { + mRefactoringHelper + .addInputLines( + REFACTORING_FILE, + "/* Foo @hide */", + "public class Test {}") + .addOutputLines( + REFACTORING_FILE, + "/** Foo @hide */", + "public class Test {}") + .doTest(TEXT_MATCH); + } + + @Test + public void refactorEnumComment() { + mRefactoringHelper + .addInputLines( + REFACTORING_FILE, + "public enum Test {", + " /* Foo @hide */", + " FOO", + "}") + .addOutputLines( + REFACTORING_FILE, + "public enum Test {", + " /** Foo @hide */", + " FOO", + "}") + .doTest(TEXT_MATCH); + } + + @Test + public void canBeSuppressed() { + mCompilationHelper + .addSourceLines( + REFACTORING_FILE, + "public class Test {", + " /* Foo @hide */", + " @SuppressWarnings(\"AndroidHideInComments\")", + " void foo() {}", + "}") + .doTest(); + } + + @Test + public void isInJavadoc() { + mCompilationHelper + .addSourceLines( + REFACTORING_FILE, + "public class Test {", + " /** Foo @hide */", + " void foo() {}", + "}") + .doTest(); + } + + @Test + public void isInMultilineJavadoc() { + mCompilationHelper + .addSourceLines( + REFACTORING_FILE, + "public class Test {", + " /**", + " * Foo.", + " *", + " * @hide", + " */", + " void foo(int foo) {}", + "}") + .doTest(); + } + + @Test + public void noHidePresent() { + mCompilationHelper + .addSourceLines( + "test/" + REFACTORING_FILE, + "package test;", + "// Foo.", + "public class Test {", + " // Foo.", + " public int a;", + " /*", + " * Foo.", + " *", + " */", + " void foo(int foo) {}", + "}") + .doTest(); + } + +}