Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f320353b authored by Michael Wright's avatar Michael Wright
Browse files

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
parent 69c9f3be
Loading
Loading
Loading
Loading
+2 −1
Original line number Diff line number Diff line

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",
+158 −0
Original line number Diff line number Diff line
/*
 * 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<Integer, Tree> 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<SuggestedFix> 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<Integer, Tree> findJavadocableTrees(CompilationUnitTree tree) {
        Map<Integer, Tree> javadoccableTrees = new HashMap<>();
        new SuppressibleTreePathScanner<Void, Void>() {
            @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;
    }

}
+235 −0
Original line number Diff line number Diff line
/*
 * 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();
    }

}