Skip to content

Commit

Permalink
Suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
Stephan202 committed Aug 12, 2023
1 parent 4547ce9 commit b4ce298
Show file tree
Hide file tree
Showing 7 changed files with 203 additions and 90 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
import static tech.picnic.errorprone.bugpatterns.util.MoreMatchers.isSubTypeOf;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.generic;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.raw;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.subOf;
Expand All @@ -14,14 +15,19 @@
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.suppliers.Supplier;
import com.google.errorprone.suppliers.Suppliers;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Type;
import java.util.Optional;

/** A {@link BugChecker} that flags nesting of {@link Optional Optionals}. */
// XXX: Extend this checker to also flag method return types and variable/field types.
// XXX: Consider generalizing this checker and `NestedPublishers` to a single `NestedMonad` check,
// which e.g. also flags nested `Stream`s. Alternatively, combine these and other checkers into an
// even more generic `ConfusingType` checker.
@AutoService(BugChecker.class)
@BugPattern(
summary =
Expand All @@ -33,19 +39,16 @@
public final class NestedOptionals extends BugChecker implements MethodInvocationTreeMatcher {
private static final long serialVersionUID = 1L;
private static final Supplier<Type> OPTIONAL = Suppliers.typeFromClass(Optional.class);
private static final Supplier<Type> OPTIONAL_OF_OPTIONAL =
VisitorState.memoize(generic(OPTIONAL, subOf(raw(OPTIONAL))));
private static final Matcher<Tree> IS_OPTIONAL_OF_OPTIONAL =
isSubTypeOf(VisitorState.memoize(generic(OPTIONAL, subOf(raw(OPTIONAL)))));

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

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
Type type = OPTIONAL_OF_OPTIONAL.get(state);
if (type == null || !state.getTypes().isSubtype(ASTHelpers.getType(tree), type)) {
return Description.NO_MATCH;
}

return describeMatch(tree);
return IS_OPTIONAL_OF_OPTIONAL.matches(tree, state)
? describeMatch(tree)
: Description.NO_MATCH;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,32 +3,38 @@
import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE;
import static com.google.errorprone.matchers.Matchers.typePredicateMatcher;
import static com.google.errorprone.predicates.TypePredicates.allOf;
import static com.google.errorprone.predicates.TypePredicates.not;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypePredicates.hasTypeParameter;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypePredicates.isSubTypeOf;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.generic;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.raw;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.subOf;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.type;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.unbound;

import com.google.auto.service.AutoService;
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.MethodInvocationTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.suppliers.Supplier;
import com.google.errorprone.suppliers.Suppliers;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.tools.javac.code.Type;
import org.jspecify.annotations.Nullable;
import org.reactivestreams.Publisher;

/** A {@link BugChecker} that flags nesting of {@link Publisher Publishers}. */
/**
* A {@link BugChecker} that flags {@code Publisher<? extends Publisher>} instances, unless the
* nested {@link org.reactivestreams.Publisher} is a {@link reactor.core.publisher.GroupedFlux}.
*/
// XXX: See the `NestedOptionals` check for some ideas on how to generalize this kind of checker.
@AutoService(BugChecker.class)
@BugPattern(
summary =
"Avoid nesting `Publisher`s inside `Publishers`s; "
"Avoid `Publisher`s that emit other `Publishers`s; "
+ "the resultant code is hard to reason about",
link = BUG_PATTERNS_BASE_URL + "NestedPublishers",
linkType = CUSTOM,
Expand All @@ -37,32 +43,21 @@
public final class NestedPublishers extends BugChecker implements MethodInvocationTreeMatcher {
private static final long serialVersionUID = 1L;
private static final Supplier<Type> PUBLISHER = type("org.reactivestreams.Publisher");
private static final Supplier<Type> PUBLISHER_OF_PUBLISHERS =
VisitorState.memoize(generic(PUBLISHER, subOf(generic(PUBLISHER, unbound()))));
private static final Supplier<Type> GROUPED_FLUX =
VisitorState.memoize(
generic(
Suppliers.typeFromString("reactor.core.publisher.GroupedFlux"),
unbound(),
unbound()));
private static final Matcher<ExpressionTree> IS_NON_GROUPED_PUBLISHER_OF_PUBLISHERS =
typePredicateMatcher(
allOf(
isSubTypeOf(generic(PUBLISHER, subOf(raw(PUBLISHER)))),
not(
hasTypeParameter(
0, isSubTypeOf(raw(type("reactor.core.publisher.GroupedFlux")))))));

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

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
Type type = ASTHelpers.getType(tree);

if (!isSubType(type, PUBLISHER_OF_PUBLISHERS.get(state), state)
|| isSubType(
Iterables.getOnlyElement(type.getTypeArguments()), GROUPED_FLUX.get(state), state)) {
return Description.NO_MATCH;
}

return describeMatch(tree);
}

private static boolean isSubType(Type subType, @Nullable Type type, VisitorState state) {
return type != null && state.getTypes().isSubtype(subType, type);
return IS_NON_GROUPED_PUBLISHER_OF_PUBLISHERS.matches(tree, state)
? describeMatch(tree)
: Description.NO_MATCH;
}
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package tech.picnic.errorprone.bugpatterns.util;

import static com.google.errorprone.matchers.Matchers.typePredicateMatcher;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypePredicates.hasAnnotation;

import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.predicates.TypePredicate;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.suppliers.Supplier;
import com.sun.source.tree.AnnotationTree;
import com.sun.tools.javac.code.Symbol;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Type;

/**
* A collection of general-purpose {@link Matcher}s.
Expand All @@ -25,15 +28,21 @@ private MoreMatchers() {}
* @return A {@link Matcher} that matches trees with the specified meta-annotation.
*/
public static <T extends AnnotationTree> Matcher<T> hasMetaAnnotation(String annotationType) {
TypePredicate typePredicate = hasAnnotation(annotationType);
return (tree, state) -> {
Symbol sym = ASTHelpers.getSymbol(tree);
return sym != null && typePredicate.apply(sym.type, state);
};
return typePredicateMatcher(hasAnnotation(annotationType));
}

// XXX: Consider moving to a `MoreTypePredicates` utility class.
private static TypePredicate hasAnnotation(String annotationClassName) {
return (type, state) -> ASTHelpers.hasAnnotation(type.tsym, annotationClassName, state);
/**
* Returns a {@link Matcher} that determines whether the type of a given {@link Tree} is a subtype
* of the type return by the specified {@link Supplier}.
*
* <p>This method differs from {@link Matchers#isSubtypeOf(Supplier)} in that it does not perform
* type erasure.
*
* @param <T> The type of tree to match against.
* @param type The {@link Supplier} that returns the type to match against.
* @return A {@link Matcher} that matches trees with the specified type.
*/
public static <T extends Tree> Matcher<T> isSubTypeOf(Supplier<Type> type) {
return typePredicateMatcher(MoreTypePredicates.isSubTypeOf(type));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package tech.picnic.errorprone.bugpatterns.util;

import com.google.errorprone.VisitorState;
import com.google.errorprone.predicates.TypePredicate;
import com.google.errorprone.predicates.TypePredicates;
import com.google.errorprone.suppliers.Supplier;
import com.google.errorprone.util.ASTHelpers;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.util.List;

/**
* A collection of general-purpose {@link TypePredicate}s.
*
* <p>These methods are additions to the ones found in {@link TypePredicates}.
*/
// XXX: The methods in this class are tested only indirectly. Consider adding a dedicated test
// class, or make sure that each method is explicitly covered by a tested analog in `MoreMatchers`.
public final class MoreTypePredicates {
private MoreTypePredicates() {}

/**
* Returns a {@link TypePredicate} that matches types that are annotated with the indicated
* annotation type.
*
* @param annotationType The fully-qualified name of the annotation type.
* @return A {@link TypePredicate} that matches appropriate types.
*/
public static TypePredicate hasAnnotation(String annotationType) {
return (type, state) -> ASTHelpers.hasAnnotation(type.tsym, annotationType, state);
}

/**
* Returns a {@link TypePredicate} that matches subtypes of the type returned by the specified
* {@link Supplier}.
*
* @implNote This method does not use {@link ASTHelpers#isSubtype(Type, Type, VisitorState)}, as
* that method performs type erasure.
* @param bound The {@link Supplier} that returns the type to match against.
* @return A {@link TypePredicate} that matches appropriate subtypes.
*/
public static TypePredicate isSubTypeOf(Supplier<Type> bound) {
Supplier<Type> memoizedType = VisitorState.memoize(bound);
return (type, state) -> {
Type boundType = memoizedType.get(state);
return boundType != null && state.getTypes().isSubtype(type, boundType);
};
}

/**
* Returns a {@link TypePredicate} that matches generic types with a type parameter that matches
* the specified {@link TypePredicate} at the indicated index.
*
* @param index The index of the type parameter to match against.
* @param predicate The {@link TypePredicate} to match against the type parameter.
* @return A {@link TypePredicate} that matches appropriate generic types.
*/
public static TypePredicate hasTypeParameter(int index, TypePredicate predicate) {
return (type, state) -> {
List<Type> typeArguments = type.getTypeArguments();
return typeArguments.size() > index && predicate.apply(typeArguments.get(index), state);
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,4 @@ void identification() {
"}")
.doTest();
}

@Test
void identificationOptionalTypeNotLoaded() {
CompilationTestHelper.newInstance(NestedOptionals.class, getClass())
.addSourceLines(
"A.java",
"import java.time.Duration;",
"",
"class A {",
" void m() {",
" Duration.ofSeconds(1);",
" }",
"}")
.doTest();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,42 +9,27 @@ void identification() {
CompilationTestHelper.newInstance(NestedPublishers.class, getClass())
.addSourceLines(
"A.java",
"import org.reactivestreams.Publisher;",
"import reactor.core.publisher.Flux;",
"import reactor.core.publisher.GroupedFlux;",
"import reactor.core.publisher.Mono;",
"",
"class A {",
" void m() {",
" Mono.empty();",
" Mono.just(1);",
" Flux.just(1);",
" Flux.just(1, 2).groupBy(i -> i).map(groupedFlux -> (GroupedFlux) groupedFlux);",
"",
" // BUG: Diagnostic contains:",
" Mono.just(Mono.empty());",
" // BUG: Diagnostic contains:",
" Mono.just(Mono.just(1));",
" // BUG: Diagnostic contains:",
" Mono.just(Flux.empty());",
" // BUG: Diagnostic contains:",
" Mono.just(Flux.just(1));",
" // BUG: Diagnostic contains:",
" Mono.just(1).map(Mono::just);",
" // BUG: Diagnostic contains:",
" Mono.just(1).map(Flux::just);",
"",
" Flux.empty();",
" Flux.just(1);",
" // BUG: Diagnostic contains:",
" Flux.just(Flux.empty());",
" // BUG: Diagnostic contains:",
" Flux.just(Flux.just(1));",
" // BUG: Diagnostic contains:",
" Flux.just(Mono.empty());",
" Mono.just((Flux) Flux.just(1));",
" // BUG: Diagnostic contains:",
" Flux.just(Mono.just(1));",
" Flux.just((Publisher) Mono.just(1));",
" // BUG: Diagnostic contains:",
" Flux.just(1).map(Flux::just);",
" // BUG: Diagnostic contains:",
" Flux.just(1).map(Mono::just);",
"",
" Flux.just(1, 2).groupBy(i -> i);",
" Mono.just(1).map(Mono::just);",
" }",
"}")
.doTest();
Expand Down
Loading

0 comments on commit b4ce298

Please sign in to comment.