-
Notifications
You must be signed in to change notification settings - Fork 294
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
Collectors.toMap
handling for streams
#938
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #938 +/- ##
============================================
+ Coverage 87.04% 87.13% +0.08%
- Complexity 1995 2011 +16
============================================
Files 77 78 +1
Lines 6447 6513 +66
Branches 1252 1264 +12
============================================
+ Hits 5612 5675 +63
Misses 422 422
- Partials 413 416 +3 ☔ View full report in Codecov by Sentry. |
Collectors.toMap
handling for streams Collectors.toMap
handling for streams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach makes sense to me, mostly a few requests to update the docs :)
"<T,K,U>toMap(java.util.function.Function<? super T,? extends K>,java.util.function.Function<? super T,? extends U>)", | ||
ImmutableSet.of(0, 1), | ||
"apply", | ||
ImmutableSet.of(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Rx have an equivalent? See getRxStreamNullabilityPropagator()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RxJava 3 does have an Observable.collect
method that takes a Collector:
And there was a previous collect
API that acts like reduce
or fold
where this support may be relevant. But I'd rather address our immediate need in this PR, and maybe file a follow-up task on systematically going through the Stream and RxJava APIs to add support for further methods. Sound ok?
return this; | ||
} | ||
|
||
public StreamModelBuilder withCollectMethodFromSignature( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc, please, specially because this has a lot of arguments with complex to understand semantics (e.g. argsToCollectorFactoryMethod
vs argsToCollectorFactoryMethod
). An example in the docs might even be a good idea :)
Edit: After reading a bit more, an alternative is to link to the docs on CollectLikeMethodRecord
, but I also wouldn't mind a bit of redundant documentation of this arguments here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullaway/src/main/java/com/uber/nullaway/handlers/stream/StreamTypeRecord.java
Outdated
Show resolved
Hide resolved
streamType.getCollectlikeMethodRecord(methodSymbol); | ||
if (collectlikeMethodRecord != null && methodSymbol.getParameters().length() == 1) { | ||
handleCollectCall(tree, collectlikeMethodRecord); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but wonder if as a follow up you want to extract the other two cases of this if into their own methods too, with onMatchMethodInvocation(...)
as just a dispatcher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #944 to keep this PR simpler
|
||
/** | ||
* Handles a call to a collect-like method. If the argument to the method is supported, updates | ||
* the {@link #collectCallToInnerMethodsOrLambdas} map appropriately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a description of the arguments here would make this method easier to follow. What method invocation is tree
referring to on entry? (collect(...)
itself, I presume)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MethodInvocationTree tree, CollectLikeMethodRecord collectlikeMethodRecord) { | ||
ExpressionTree argTree = tree.getArguments().get(0); | ||
if (argTree instanceof MethodInvocationTree) { | ||
MethodInvocationTree collectInvokeArg = (MethodInvocationTree) argTree; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is the Collectors.toMap(...)
invocation, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, 8b3f984
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required, but should we update the docs in lines 74-100 and the list of examples in the docs for observableCallToInnerMethodOrLambda
to include info on .collect()
calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few lines in 8b3f984
LinkedHashMultimap.create(); | ||
|
||
// Map from map or collect method (or lambda) to corresponding previous filter method (e.g. | ||
// B.apply => A.filter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need an example here for the collect case, specially if we update the global docs above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lazaroclapp this is ready for another look. I added support for a couple more collector factory methods based on feedback in #934 (comment). This required some generalization, as we couldn't support multiple factory methods for a single collect method before; see d317d0e and c6ac215. But the overall logical structure is the same. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One minor comment about a potential refactoring, but entirely up to you at this point :)
Fixes #934
The key new thing with the support here is we have further nesting. Rather than a
map
method, where the relevant lambda is passed directly:In this case we have a
collect
call, which gets as its argument the result ofCollectors.toMap
, and the relevant lambdas are passed totoMap
:Supporting this requires some new types of logic in our streams handler (particularly because there are multiple relevant lambdas for a single
collect
call). We do also handle anonymous inner classes. I only added support for collecting intotoMap
for now; I'm not sure if there are other important cases.I did a couple drive-by refactorings in this PR as I couldn't help it. I can undo them if they really make review more difficult, but I hope it's ok.