Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce MoreTypes utility class #234

Merged
merged 4 commits into from
Oct 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
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.MoreTypes.generic;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.raw;
import static tech.picnic.errorprone.bugpatterns.util.MoreTypes.subOf;

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;
Expand All @@ -16,9 +18,7 @@
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 com.sun.tools.javac.util.List;
import java.util.Optional;

/** A {@link BugChecker} that flags nesting of {@link Optional Optionals}. */
Expand All @@ -33,24 +33,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))));

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

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
return isOptionalOfOptional(tree, state) ? describeMatch(tree) : Description.NO_MATCH;
}

private static boolean isOptionalOfOptional(Tree tree, VisitorState state) {
Type optionalType = OPTIONAL.get(state);
Type type = ASTHelpers.getType(tree);
if (!ASTHelpers.isSubtype(type, optionalType, state)) {
return false;
}

List<Type> typeArguments = type.getTypeArguments();
return !typeArguments.isEmpty()
&& ASTHelpers.isSubtype(Iterables.getOnlyElement(typeArguments), optionalType, state);
return state.getTypes().isSubtype(ASTHelpers.getType(tree), OPTIONAL_OF_OPTIONAL.get(state))
? describeMatch(tree)
: Description.NO_MATCH;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import javax.lang.model.element.Modifier;

/**
* A {@link BugChecker} which suggests a canonical set of modifiers for Refaster class and method
* A {@link BugChecker} that suggests a canonical set of modifiers for Refaster class and method
* definitions.
*/
@AutoService(BugChecker.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa
"org.springframework.http.HttpMethod",
"org.springframework.http.MediaType",
"org.testng.Assert",
"reactor.function.TupleUtils");
"reactor.function.TupleUtils",
"tech.picnic.errorprone.bugpatterns.util.MoreTypes");

/** Type members that should be statically imported. */
@VisibleForTesting
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/**
* A {@link BugChecker} which flags {@link String#format(String, Object...)} invocations which can
* be replaced with a {@link String#join(CharSequence, CharSequence...)} or even a {@link
* A {@link BugChecker} that flags {@link String#format(String, Object...)} invocations which can be
* replaced with a {@link String#join(CharSequence, CharSequence...)} or even a {@link
* String#valueOf} invocation.
*/
// XXX: What about `v1 + "sep" + v2` and similar expressions? Do we want to rewrite those to
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
package tech.picnic.errorprone.bugpatterns.util;

import static java.util.stream.Collectors.toCollection;

import com.google.errorprone.VisitorState;
import com.google.errorprone.suppliers.Supplier;
import com.google.errorprone.suppliers.Suppliers;
import com.sun.tools.javac.code.BoundKind;
import com.sun.tools.javac.code.Type;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.function.BiFunction;

/**
* A set of helper methods which together define a DSL for defining {@link Type types}.
*
* <p>These methods are meant to be statically imported. Example usage:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add this class to StaticImport.

*
* <pre>{@code
* Supplier<Type> type =
* VisitorState.memoize(
* generic(
* type("reactor.core.publisher.Flux"),
* subOf(generic(type("org.reactivestreams.Publisher"), unbound()))));
* }</pre>
*
* This statement produces a memoized supplier of the type {@code Flux<? extends Publisher<?>>}.
*/
public final class MoreTypes {
private MoreTypes() {}

/**
* Creates a supplier of the type with the given fully qualified name.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for not using {@link Supplier}? :)

*
* <p>This method should only be used when building more complex types in combination with other
* {@link MoreTypes} methods. In other cases prefer directly calling {@link
* Suppliers#typeFromString(String)}.
*
* @param typeName The type of interest.
* @return A supplier which returns the described type if available in the given state, and {@code
* null} otherwise.
*/
public static Supplier<Type> type(String typeName) {
return Suppliers.typeFromString(typeName);
}

/**
* Creates a supplier of the described generic type.
*
* @param type The base type of interest.
* @param typeArgs The desired type arguments.
* @return A supplier which returns the described type if available in the given state, and {@code
* null} otherwise.
*/
// XXX: The given `type` should be a generic type, so perhaps `withParams` would be a better
// method name. But the DSL wouldn't look as nice that way.
@SafeVarargs
@SuppressWarnings("varargs")
public static Supplier<Type> generic(Supplier<Type> type, Supplier<Type>... typeArgs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static Supplier<Type> generic(Supplier<Type> type, Supplier<Type>... typeArgs) {
public static Supplier<Type> genericWithParams(Supplier<Type> type, Supplier<Type>... typeArgs) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, generic could be a bit more descriptive. One needs to really dive into the MoreTypes before understanding how this works. genericWithParams would be a nice option.
Another option would be genericType{,WithParams} with String as first parameter. However, in the current situation we always pass Types as parameter.

I know MoreTypes.genericType(...... has a little duplication, but we statically import every method here of course.

But the DSL wouldn't look as nice that way.

IMHO the DSL is very concise (even if we would make some of the method names a little longer).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The given type should be a generic type

In EP I see checks for this:

boolean isGeneric = typeParams != null && !typeParams.isEmpty();

Maybe it'd be nice to add a check for that in this method? Using checkArgument we can enforce that others use this DSL correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not yet feeling the first suggestion: in practice the code will look like generic(type("fqcn"), ...), which should be clear enough. The class also has only a few methods, and there's an example at the top, which shows how this method can be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

W.r.t. the second suggestion: since the type evaluation is deferred, I'm not sure an IllegalArgumentException is warranted; perhaps an IllegalStateException would fit. Even so: the type might not have a matching set of generic type parameters due to an unexpected type on the classpath (perhaps due to an unsupported dependency version), which IMHO should not cause an error, but simply cause the expression to yield false.

return propagateNull(
type,
(state, baseType) -> {
List<Type> params =
Arrays.stream(typeArgs).map(s -> s.get(state)).collect(toCollection(ArrayList::new));
if (params.stream().anyMatch(Objects::isNull)) {
return null;
}

return state.getType(baseType, /* isArray= */ false, params);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isArray is always false because Suppliers.arrayOf(...) should be used to describe that type, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I wrote this, but indeed, I think Suppliers.arrayOf(...) would then be used to wrap the result of this method. 👍

});
}

/**
* Creates a raw (erased, non-generic) variant of the given type.
*
* @param type The base type of interest.
* @return A supplier which returns the described type if available in the given state, and {@code
* null} otherwise.
*/
public static Supplier<Type> raw(Supplier<Type> type) {
return propagateNull(type, (state, baseType) -> baseType.tsym.erasure(state.getTypes()));
}

/**
* Creates a {@code ? super T} wildcard type, with {@code T} bound to the given type.
*
* @param type The base type of interest.
* @return A supplier which returns the described type if available in the given state, and {@code
* null} otherwise.
*/
public static Supplier<Type> superOf(Supplier<Type> type) {
return propagateNull(
type,
(state, baseType) ->
new Type.WildcardType(baseType, BoundKind.SUPER, state.getSymtab().boundClass));
}

/**
* Creates a {@code ? extends T} wildcard type, with {@code T} bound to the given type.
*
* @param type The base type of interest.
* @return A supplier which returns the described type if available in the given state, and {@code
* null} otherwise.
*/
public static Supplier<Type> subOf(Supplier<Type> type) {
return propagateNull(
type,
(state, baseType) ->
new Type.WildcardType(
type.get(state), BoundKind.EXTENDS, state.getSymtab().boundClass));
}

/**
* Creates an unbound wildcard type ({@code ?}).
*
* @return A supplier which returns the described type.
*/
public static Supplier<Type> unbound() {
return state ->
new Type.WildcardType(
state.getSymtab().objectType, BoundKind.UNBOUND, state.getSymtab().boundClass);
}

private static Supplier<Type> propagateNull(
Supplier<Type> type, BiFunction<VisitorState, Type, Type> transformer) {
return state ->
Optional.ofNullable(type.get(state)).map(t -> transformer.apply(state, t)).orElse(null);
}
}
Loading