Skip to content

Commit

Permalink
Introduce BugPatternLink check (#1026)
Browse files Browse the repository at this point in the history 
This check validates that checks defined by Error Prone Support
reference the public website, unless they explicitly specify that their
diagnostics output must not include a link.
  • Loading branch information
@Stephan202
Stephan202 authored Feb 12, 2024
1 parent 1fe6767 commit 28bb4f6
Show file tree
Hide file tree
Showing 3 changed files with 335 additions and 1 deletion.
140 changes: 140 additions & 0 deletions ...uidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/BugPatternLink.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
package tech.picnic.errorprone.guidelines.bugpatterns;

import static com.google.common.base.Verify.verify;
import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR;
import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE;
import static com.google.errorprone.matchers.FieldMatchers.staticField;
import static com.google.errorprone.matchers.Matchers.annotations;
import static com.google.errorprone.matchers.Matchers.isType;
import static com.google.errorprone.matchers.Matchers.packageStartsWith;
import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;

import com.google.auto.service.AutoService;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
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.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.AnnotationMatcherUtils;
import com.google.errorprone.matchers.Description;
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.BinaryTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.Tree.Kind;
import com.sun.tools.javac.util.Constants;
import javax.lang.model.element.Name;

/**
* A {@link BugChecker} that flags {@link BugChecker} declarations inside {@code
* tech.picnic.errorprone.*} packages that do not reference the Error Prone Support website.
*/
// XXX: Introduce a similar check to enforce the Refaster `@OnlineDocumentation` annotation. (Or
// update the website generation to document Refaster collections by default, and provide an
// exclusion annotation instead. This may make more sense.)
@AutoService(BugChecker.class)
@BugPattern(
summary = "Error Prone Support checks must reference their online documentation",
link = BUG_PATTERNS_BASE_URL + "BugPatternLink",
linkType = CUSTOM,
severity = SUGGESTION,
tags = LIKELY_ERROR)
public final class BugPatternLink extends BugChecker implements ClassTreeMatcher {
private static final long serialVersionUID = 1L;
private static final Matcher<ClassTree> IS_ERROR_PRONE_SUPPORT_CLASS =
packageStartsWith("tech.picnic.errorprone");
private static final Matcher<ExpressionTree> IS_LINK_TYPE_NONE =
staticField(BugPattern.LinkType.class.getCanonicalName(), "NONE");
private static final Matcher<ExpressionTree> IS_BUG_PATTERNS_BASE_URL =
staticField("tech.picnic.errorprone.utils.Documentation", "BUG_PATTERNS_BASE_URL");
private static final MultiMatcher<ClassTree, AnnotationTree> HAS_BUG_PATTERN_ANNOTATION =
annotations(AT_LEAST_ONE, isType(BugPattern.class.getCanonicalName()));

/** Instantiates a new {@link BugPatternLink} instance. */
public BugPatternLink() {}

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
if (ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class) != null) {
/*
* This is a nested class; even if it's bug checker, then it's likely declared within a test
* class.
*/
return Description.NO_MATCH;
}

if (!IS_ERROR_PRONE_SUPPORT_CLASS.matches(tree, state)) {
/*
* Bug checkers defined elsewhere are unlikely to be documented on the Error Prone Support
* website.
*/
return Description.NO_MATCH;
}

ImmutableList<AnnotationTree> bugPatternAnnotations =
HAS_BUG_PATTERN_ANNOTATION.multiMatchResult(tree, state).matchingNodes();
if (bugPatternAnnotations.isEmpty()) {
/* This isn't a bug checker. */
return Description.NO_MATCH;
}

AnnotationTree annotation = Iterables.getOnlyElement(bugPatternAnnotations);
if (isCompliant(annotation, tree.getSimpleName(), state)) {
/* The bug checker is correctly configured. */
return Description.NO_MATCH;
}

return describeMatch(annotation, suggestFix(tree, state, annotation));
}

private static boolean isCompliant(
AnnotationTree annotation, Name className, VisitorState state) {
ExpressionTree linkType = AnnotationMatcherUtils.getArgument(annotation, "linkType");
if (IS_LINK_TYPE_NONE.matches(linkType, state)) {
/* This bug checker explicitly declares that there is no link. */
return true;
}

ExpressionTree link = AnnotationMatcherUtils.getArgument(annotation, "link");
if (!(link instanceof BinaryTree binary)) {
return false;
}

verify(binary.getKind() == Kind.PLUS, "Unexpected binary operator");
return IS_BUG_PATTERNS_BASE_URL.matches(binary.getLeftOperand(), state)
&& className.contentEquals(ASTHelpers.constValue(binary.getRightOperand(), String.class));
}

private static SuggestedFix suggestFix(
ClassTree tree, VisitorState state, AnnotationTree annotation) {
SuggestedFix.Builder fix = SuggestedFix.builder();

String linkPrefix =
SuggestedFixes.qualifyStaticImport(
"tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL", fix, state);
fix.merge(
SuggestedFixes.updateAnnotationArgumentValues(
annotation,
state,
"link",
ImmutableList.of(
linkPrefix + " + " + Constants.format(tree.getSimpleName().toString()))));

String linkType =
SuggestedFixes.qualifyStaticImport(
BugPattern.LinkType.class.getCanonicalName() + ".CUSTOM", fix, state);
fix.merge(
SuggestedFixes.updateAnnotationArgumentValues(
annotation, state, "linkType", ImmutableList.of(linkType)));

return fix.build();
}
}
193 changes: 193 additions & 0 deletions ...lines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/BugPatternLinkTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
package tech.picnic.errorprone.guidelines.bugpatterns;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.Test;

final class BugPatternLinkTest {
@Test
void identification() {
CompilationTestHelper.newInstance(BugPatternLink.class, getClass())
.addSourceLines(
"A.java",
"import com.google.errorprone.BugPattern;",
"",
"@BugPattern(summary = \"Class in default package\", severity = BugPattern.SeverityLevel.ERROR)",
"class A {}")
.addSourceLines(
"com/example/B.java",
"package com.example;",
"",
"import com.google.errorprone.BugPattern;",
"",
"@BugPattern(summary = \"Class in custom package\", severity = BugPattern.SeverityLevel.ERROR)",
"class B {}")
.addSourceLines(
"tech/picnic/errorprone/C.java",
"package tech.picnic.errorprone;",
"",
"import com.google.errorprone.BugPattern;",
"",
"@BugPattern(",
" summary = \"Class explicitly without link\",",
" linkType = BugPattern.LinkType.NONE,",
" severity = BugPattern.SeverityLevel.ERROR)",
"class C {}")
.addSourceLines(
"tech/picnic/errorprone/subpackage/D.java",
"package tech.picnic.errorprone.subpackage;",
"",
"import com.google.errorprone.BugPattern;",
"import tech.picnic.errorprone.utils.Documentation;",
"",
"@BugPattern(",
" summary = \"Error Prone Support class in subpackage with proper link\",",
" link = Documentation.BUG_PATTERNS_BASE_URL + \"D\",",
" linkType = BugPattern.LinkType.CUSTOM,",
" severity = BugPattern.SeverityLevel.ERROR)",
"class D {}")
.addSourceLines(
"tech/picnic/errorprone/E.java",
"package tech.picnic.errorprone;",
"",
"import static com.google.errorprone.BugPattern.LinkType.CUSTOM;",
"import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;",
"import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;",
"",
"import com.google.errorprone.BugPattern;",
"",
"@BugPattern(",
" summary = \"Error Prone Support class with proper link and static imports\",",
" link = BUG_PATTERNS_BASE_URL + \"E\",",
" linkType = CUSTOM,",
" severity = ERROR)",
"class E {}")
.addSourceLines(
"tech/picnic/errorprone/F.java",
"package tech.picnic.errorprone;",
"",
"import com.google.errorprone.BugPattern;",
"",
"class F {",
" @BugPattern(",
" summary = \"Nested Error Prone Support class\",",
" severity = BugPattern.SeverityLevel.ERROR)",
" class Inner {}",
"}")
.addSourceLines(
"tech/picnic/errorprone/G.java",
"package tech.picnic.errorprone;",
"",
"import com.google.errorprone.BugPattern;",
"",
"// BUG: Diagnostic contains:",
"@BugPattern(",
" summary = \"Error Prone Support class lacking link\",",
" severity = BugPattern.SeverityLevel.ERROR)",
"class G {}")
.addSourceLines(
"tech/picnic/errorprone/H.java",
"package tech.picnic.errorprone;",
"",
"import com.google.errorprone.BugPattern;",
"import tech.picnic.errorprone.utils.Documentation;",
"",
"// BUG: Diagnostic contains:",
"@BugPattern(",
" summary = \"Error Prone Support class with incorrect link\",",
" link = Documentation.BUG_PATTERNS_BASE_URL + \"NotH\",",
" linkType = BugPattern.LinkType.CUSTOM,",
" severity = BugPattern.SeverityLevel.ERROR)",
"class H {}")
.addSourceLines(
"tech/picnic/errorprone/I.java",
"package tech.picnic.errorprone;",
"",
"import com.google.errorprone.BugPattern;",
"",
"// BUG: Diagnostic contains:",
"@BugPattern(",
" summary = \"Error Prone Support class with non-canonical link\",",
" link = \"https://error-prone.picnic.tech/bugpatterns/I\",",
" linkType = BugPattern.LinkType.CUSTOM,",
" severity = BugPattern.SeverityLevel.ERROR)",
"class I {}")
.addSourceLines(
"tech/picnic/errorprone/J.java",
"package tech.picnic.errorprone;",
"",
"import com.google.errorprone.BugPattern;",
"",
"// BUG: Diagnostic contains:",
"@BugPattern(",
" summary = \"Error Prone Support class in with non-canonical link\",",
" link = \"https://error-prone.picnic.tech/bugpatterns/\" + \"J\",",
" linkType = BugPattern.LinkType.CUSTOM,",
" severity = BugPattern.SeverityLevel.ERROR)",
"class J {}")
.doTest();
}

@Test
void replacement() {
BugCheckerRefactoringTestHelper.newInstance(BugPatternLink.class, getClass())
.addInputLines(
"tech/picnic/errorprone/A.java",
"package tech.picnic.errorprone;",
"",
"import com.google.errorprone.BugPattern;",
"",
"@BugPattern(",
" summary = \"Error Prone Support class lacking link\",",
" severity = BugPattern.SeverityLevel.ERROR)",
"class A {}")
.addOutputLines(
"tech/picnic/errorprone/A.java",
"package tech.picnic.errorprone;",
"",
"import static com.google.errorprone.BugPattern.LinkType.CUSTOM;",
"import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;",
"",
"import com.google.errorprone.BugPattern;",
"",
"@BugPattern(",
" link = BUG_PATTERNS_BASE_URL + \"A\",",
" linkType = CUSTOM,",
" summary = \"Error Prone Support class lacking link\",",
" severity = BugPattern.SeverityLevel.ERROR)",
"class A {}")
.addInputLines(
"tech/picnic/errorprone/B.java",
"package tech.picnic.errorprone;",
"",
"import static com.google.errorprone.BugPattern.LinkType.CUSTOM;",
"import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;",
"",
"import com.google.errorprone.BugPattern;",
"",
"@BugPattern(",
" summary = \"Error Prone Support class with incorrect link\",",
" link = \"Not the right link\",",
" linkType = CUSTOM,",
" severity = ERROR)",
"class B {}")
.addOutputLines(
"tech/picnic/errorprone/B.java",
"package tech.picnic.errorprone;",
"",
"import static com.google.errorprone.BugPattern.LinkType.CUSTOM;",
"import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;",
"import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;",
"",
"import com.google.errorprone.BugPattern;",
"",
"@BugPattern(",
" summary = \"Error Prone Support class with incorrect link\",",
" link = BUG_PATTERNS_BASE_URL + \"B\",",
" linkType = CUSTOM,",
" severity = ERROR)",
"class B {}")
.doTest(TestMode.TEXT_MATCH);
}
}
3 changes: 2 additions & 1 deletion ...st-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
import static com.google.errorprone.BugPattern.LinkType.NONE;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static java.util.Comparator.naturalOrder;
import static tech.picnic.errorprone.refaster.runner.Refaster.INCLUDED_RULES_PATTERN_FLAG;
Expand Down Expand Up @@ -60,7 +61,7 @@
// XXX: This check currently only validates that one `Refaster.anyOf` branch in one
// `@BeforeTemplate` method is covered by a test. Review how we can make sure that _all_
// `@BeforeTemplate` methods and `Refaster.anyOf` branches are covered.
@BugPattern(summary = "Exercises a Refaster rule collection", severity = ERROR)
@BugPattern(summary = "Exercises a Refaster rule collection", linkType = NONE, severity = ERROR)
@SuppressWarnings("java:S2160" /* Super class equality definition suffices. */)
public final class RefasterRuleCollection extends BugChecker implements CompilationUnitTreeMatcher {
private static final long serialVersionUID = 1L;
Expand Down

0 comments on commit 28bb4f6

Please sign in to comment.