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.

Sign up for GitHub

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 IdentityConversionCheck #27

Merged
merged 13 commits into from
Apr 8, 2022
Merged

Introduce IdentityConversionCheck #27

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b592066
Introduce `IdentityConversionCheck`
rickie Jan 4, 2022
57180ca
Remove obsolete Refaster templates
rickie Jan 6, 2022
07ebfd3
Add extra testcases and apply suggestion
rickie Jan 8, 2022
e060553
Add more test cases
rickie Jan 11, 2022
8bf0f8f
IdentityConversion: drop `*Sorted*` candidates
rickie Feb 11, 2022
9c9825e
`IdentityConversion`: validate types have a defined equality check
rickie Feb 18, 2022
bd40a22
Add check for `java.lang.Object` in `IdentityConversion`
rickie Mar 2, 2022
6886c85
Remove `checkState` as targetType can be null in the case of a `yield`
rickie Mar 18, 2022
47205e6
Suggestions
Stephan202 Apr 5, 2022
943eebc
More suggestions
Stephan202 Apr 5, 2022
d9bc85a
Trim some more templates
Stephan202 Apr 5, 2022
1f0ccf9
Suggestions
rickie Apr 6, 2022
0506118
Suggestion
Stephan202 Apr 8, 2022
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
9 changes: 3 additions & 6 deletions ...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package tech.picnic.errorprone.bugpatterns;

import static com.google.common.base.Preconditions.checkState;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.staticMethod;
import static com.google.errorprone.suppliers.Suppliers.typeFromString;
Expand Down Expand Up @@ -79,11 +78,9 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
ExpressionTree sourceTree = arguments.get(0);
Type sourceType = ASTHelpers.getType(sourceTree);
TargetType targetType = ASTHelpers.targetType(state);
Copy link
Member

Choose a reason for hiding this comment

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

Genuine question: What's the target type here exactly? Why not ASTHelpers.getType(state)? 🙇

Copy link
Member Author

Choose a reason for hiding this comment

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

Javadoc getType:

Returns the Type of the given tree, or null if the type could not be determined.

Javadoc getTargetType:

Returns the target type of the tree at the given VisitorState's path, or else null.
For example, the target type of an assignment expression is the variable's type, and the target type of a return statement is the enclosing method's type.

Nice question; the type would only give us information about the current expression itself (state), while we want to know what the "expected" type of the current expression is (i.o.w. what the parent expects/wants to receive) and thus what the target type is of this expression.

So in the Flux#flatMap case, let's say we matched a RxJava2Adapter.fluxToFlowable(flux) and we ask for the target type, we know that we want to have Publisher in the end. A Flux is already satiesfies that, so we can remove the conversion.

checkState(
sourceType != null && targetType != null,
"sourceType `%s` or targetType `%s` is null.",
sourceType,
targetType);
if (sourceType == null || targetType == null) {
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 changed the checkState to an if statement, while we could also add these checks to the one below. However, I feel that this is a nice separation and (arguably) improves readability. Also fine with merging the two 😅 .

return Description.NO_MATCH;
}

if (state.getTypes().isSameType(sourceType, ASTHelpers.getType(tree))
Copy link
Member

Choose a reason for hiding this comment

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

I think that in practice ASTHelpers.getType(tree) won't ever return null here, but the method does allow it. Since isSameType would then throw an NPE, perhaps we can make this a bit more robust.

|| isSubtypeWithDefinedEquality(sourceType, targetType.type(), state)) {
Expand Down