From 390d84d3dd81765b5affe42ac0cc7c25de5af1d9 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 12 Apr 2023 13:58:00 +0200 Subject: [PATCH] WIP --- .../RefasterExpressionTypeInvariance.java | 163 ++++++++++++++++++ .../RefasterParameterTypeGenerics.java | 78 +++++++++ .../refasterrules/OptionalRules.java | 4 +- .../RefasterExpressionTypeInvarianceTest.java | 28 +++ ...sterRefasterParameterTypeGenericsTest.java | 25 +++ .../refaster/annotation/Description.java | 1 + 6 files changed, 297 insertions(+), 2 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterExpressionTypeInvariance.java create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterParameterTypeGenerics.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterExpressionTypeInvarianceTest.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterRefasterParameterTypeGenericsTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterExpressionTypeInvariance.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterExpressionTypeInvariance.java new file mode 100644 index 00000000..8802ed66 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterExpressionTypeInvariance.java @@ -0,0 +1,163 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; +import static com.google.errorprone.matchers.Matchers.hasAnnotation; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +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.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.refaster.Refaster; +import com.google.errorprone.refaster.annotation.AfterTemplate; +import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.ReturnTree; +import com.sun.source.tree.Tree; +import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.code.Type; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import org.jspecify.annotations.Nullable; + +/** + * A {@link BugChecker} that flags Refaster expression templates for which the + * {@code @AfterTemplate} return type is not a subtype of the `@BeforeTemplate` return type. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Refaster templates should have invariant return types", + link = BUG_PATTERNS_BASE_URL + "RefasterExpressionTypeInvariance", + linkType = CUSTOM, + severity = WARNING, + tags = LIKELY_ERROR) +public final class RefasterExpressionTypeInvariance extends BugChecker implements ClassTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher BEFORE_TEMPLATE_METHOD = hasAnnotation(BeforeTemplate.class); + private static final Matcher AFTER_TEMPLATE_METHOD = hasAnnotation(AfterTemplate.class); + private static final Matcher REFASTER_ANY_OF = + staticMethod().onClass(Refaster.class.getName()).named("anyOf"); + + // XXX: False positive in `OptionalMap`. First write a rule to simplify such case. + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + Optional afterTemplateResultType = getAfterTemplateResultType(tree, state); + if (afterTemplateResultType.isEmpty()) { + return Description.NO_MATCH; + } + + ImmutableSet beforeTemplateResultTypes = getBeforeTemplateResultTypes(tree, state); + if (beforeTemplateResultTypes.isEmpty()) { + return Description.NO_MATCH; + } + + Type replacementType = afterTemplateResultType.orElseThrow(); + if (areAllSuperTypeOf(replacementType, beforeTemplateResultTypes, state)) { + return Description.NO_MATCH; + } + + Type alternativeReplacementType = + (replacementType.isPrimitive() + ? state.getTypes().boxedTypeOrType(replacementType) + : state.getTypes().unboxedType(replacementType)); + + boolean requiresBoxingOrUnboxing = + areAllSuperTypeOf(alternativeReplacementType, beforeTemplateResultTypes, state); + + // @Considerations + + // enum ConsiderationType: + // MAY_CAUSE_BOXING + // MAY_CAUSE_UNBOXING + // MAY_CAUSE_COMPILATION_ERROR\ + + if (requiresBoxingOrUnboxing) { + if (replacementType.isPrimitive()) { + // MAY_CAUSE_UNBOXING + // XXX: Flag this using a custom annotation. + return Description.NO_MATCH; + } + + // MAY_CAUSE_ + // XXX: Flag this using a custom annotation. + return Description.NO_MATCH; + } + + // MAY_CAUSE_COMPILATION_ERROR + // XXX: Flag this using a custom annotation. + // XXX: This case contains false positives; maybe leave out for now? + return describeMatch(tree); + } + + private static boolean areAllSuperTypeOf( + Type subType, ImmutableSet superTypes, VisitorState state) { + return superTypes.stream().allMatch(type -> state.getTypes().isSuperType(type, subType)); + } + + private static ImmutableSet getBeforeTemplateResultTypes( + ClassTree tree, VisitorState state) { + return tree.getMembers().stream() + .filter(m -> BEFORE_TEMPLATE_METHOD.matches(m, state)) + .map(m -> getResultType((MethodTree) m, state)) + .collect(toImmutableSet()); + } + + private static Optional getAfterTemplateResultType(ClassTree tree, VisitorState state) { + return tree.getMembers().stream() + .filter(m -> AFTER_TEMPLATE_METHOD.matches(m, state)) + .map(m -> getResultType((MethodTree) m, state)) + .reduce(state.getTypes()::lub); + } + + // XXX: A variant of this pattern also occurs a few times inside Error Prone; contribute upstream. + // XXX: Copied from `JUnitValueSource`. + private static Type getResultType(MethodTree methodTree, VisitorState state) { + List resultTypes = new ArrayList<>(); + new TreeScanner<@Nullable Void, @Nullable Void>() { + @Override + public @Nullable Void visitClass(ClassTree node, @Nullable Void unused) { + /* Ignore `return` statements inside anonymous/local classes. */ + return null; + } + + @Override + public @Nullable Void visitReturn(ReturnTree node, @Nullable Void unused) { + if (REFASTER_ANY_OF.matches(node.getExpression(), state)) { + for (ExpressionTree arg : ((MethodInvocationTree) node.getExpression()).getArguments()) { + resultTypes.add(ASTHelpers.getType(arg)); + } + } else if (ASTHelpers.stripParentheses(node.getExpression()).getKind() + != Tree.Kind.NULL_LITERAL) { + resultTypes.add(ASTHelpers.getType(node.getExpression())); + } + + return super.visitReturn(node, unused); + } + + @Override + public @Nullable Void visitLambdaExpression( + LambdaExpressionTree node, @Nullable Void unused) { + /* Ignore `return` statements inside lambda expressions. */ + return null; + } + }.scan(methodTree, null); + + return resultTypes.stream() + .reduce(state.getTypes()::lub) + .orElseGet(() -> state.getSymtab().botType); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterParameterTypeGenerics.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterParameterTypeGenerics.java new file mode 100644 index 00000000..1b57e313 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterParameterTypeGenerics.java @@ -0,0 +1,78 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; +import static com.google.errorprone.BugPattern.StandardTags.STYLE; +import static com.google.errorprone.matchers.Matchers.hasAnnotation; +import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; + +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.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Type; + +// XXX: The plan towards canonical Refaster templates: +// 1. Replace wildcard types. +// 2. Enforce rule naming conventions. +// 3. Split rules with incompatible parameter types. +// 4. (Un)annotate templates with incompatible result expressions. +// +// Also: +// 0. Canonicalize template method order (@PlaceHolder, @BeforeTemplate, @AfterTemplate). +// ^ There may be logic by which to break ties; TBD. +// 1. Canonicalize template method parameter order. +// ^ See https://github.com/PicnicSupermarket/error-prone-support/pull/775. +// 2. Canonicalize template type parameter order following a similar approach. +// 3. Canonicalize template method parameter names by deriving them from the corresponding parameter +// names of the code invoked in the `@AfterTemplate` method. +// 4. Canonicalize type parameter names following a similar approach. + +/** + * A {@link BugChecker} that flags Refaster expression templates parameters with wildcard types, and + * suggests simplifying these, possibly by declaring a corresponding class type parameter. + */ +// XXX: Given `? extends T`/`? super T`, suggest just dropping the wildcard if `T` is not referenced +// elsewhere in the same `@BeforeTemplate` signature. +// XXX: The type parameters can also be defined at the method level. Such cases should be moved to +// the class level. +// ^ Relatedly, if not already disallowed, constrain all Refaster method modifiers. Should not be +// `static`, etc. +@AutoService(BugChecker.class) +@BugPattern( + summary = "Refaster template parameter types should not contain wildcards", + link = BUG_PATTERNS_BASE_URL + "RefasterParameterTypeGenerics", + linkType = CUSTOM, + severity = SUGGESTION, + tags = {SIMPLIFICATION, STYLE}) +public final class RefasterParameterTypeGenerics extends BugChecker implements ClassTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher BEFORE_TEMPLATE_METHOD = hasAnnotation(BeforeTemplate.class); + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + for (Tree member : tree.getMembers()) { + if (BEFORE_TEMPLATE_METHOD.matches(member, state)) { + MethodTree method = (MethodTree) member; + for (VariableTree parameter : method.getParameters()) { + Type type = ASTHelpers.getType(parameter); + if (type != null && type.isParameterized()) { + return describeMatch(parameter); + } + } + } + } + + return Description.NO_MATCH; + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java index a8eb97b4..d4194770 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java @@ -427,12 +427,12 @@ final class OptionalRules { // dropped in favour of `StreamMapFirst` and `OptionalIdentity`. static final class OptionalMap { @BeforeTemplate - Optional before(Optional optional, Function function) { + Optional before(Optional optional, Function function) { return optional.stream().map(function).findAny(); } @AfterTemplate - Optional after(Optional optional, Function function) { + Optional after(Optional optional, Function function) { return optional.map(function); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterExpressionTypeInvarianceTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterExpressionTypeInvarianceTest.java new file mode 100644 index 00000000..eff3c4c7 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterExpressionTypeInvarianceTest.java @@ -0,0 +1,28 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class RefasterExpressionTypeInvarianceTest { + @Test + void identification() { + CompilationTestHelper.newInstance(RefasterExpressionTypeInvariance.class, getClass()) + .addSourceLines( + "A.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "", + "class A {", + " @BeforeTemplate", + " Object before(String str) {", + " if (str == null) {", + " return null;", + " }", + " if (str != null) {", + " return (CharSequence) null;", + " }", + " return str;", + " }", + "}") + .doTest(); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterRefasterParameterTypeGenericsTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterRefasterParameterTypeGenericsTest.java new file mode 100644 index 00000000..145cbded --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterRefasterParameterTypeGenericsTest.java @@ -0,0 +1,25 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class RefasterRefasterParameterTypeGenericsTest { + // XXX: Rename inner class. + @Test + void identification() { + CompilationTestHelper.newInstance(RefasterParameterTypeGenerics.class, getClass()) + .addSourceLines( + "A.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "import java.util.function.Function;", + "", + "class A {", + " static class B {", + " @BeforeTemplate", + " // BUG: Diagnostic contains:", + " void before(Function function) {}", + " }", + "}") + .doTest(); + } +} diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Description.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Description.java index 84af8a57..025f82ac 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Description.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/Description.java @@ -10,6 +10,7 @@ import java.lang.annotation.Target; * *

Annotations on nested classes override the description associated with any enclosing class. */ +// XXX: This becomes easier to use once we can use text blocks. @Target(ElementType.TYPE) @Retention(RetentionPolicy.SOURCE) public @interface Description {