Stream.mapMulti(Optional::ifPresent) is more efficient than Stream.flatMap(Optional::stream)

[schlosna]

Before this PR

#2946 added a check to convert Stream.flatMap(Optional::stream) to more efficient Stream.filter(Optional::isPresent).map(Optional::get). There was discussion in #2950 whether the autofix should prefer Optional::orElseThrow over Optional::get and using Stream.mapMulti(Optional::ifPresent) as a more concise and efficient alternative.

After this PR

==COMMIT_MSG==

Converts Stream.flatMap(Optional::stream), Stream.filter(Optional::isPresent).map(Optional::get), and filter(Optional::isPresent).map(Optional::orElseThrow) to Stream.mapMulti(Optional::ifPresent).

==COMMIT_MSG==

Possible downsides?

[changelog-app]

Generate changelog in changelog/@unreleased

What do the change types mean?

• feature: A new feature of the service.
• improvement: An incremental improvement in the functionality or operation of the service.
• fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
• break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
  and external consumers of the service's API (e.g. customer-written software or integrations).
• deprecation: Advertises the intention to remove service functionality without any change to the
  operation of the service itself.
• manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
  performing database surgery, ...) at the time of upgrade for it to succeed.
• migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

• ❗The break and manual task changelog types will result in a major release!
• 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
• ✨ All others will result in a minor version release.

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Stream.mapMulti(Optional::ifPresent)is more efficient thanStream.flatMap(Optional::stream)

Check the box to generate changelog(s)

• Generate changelog entry

[carterkozak]

Unclear if this is the best fit for this check, vs a separate error-prone check (if it's uncommon outside of the previous automatic fix, we could even run it via excavator without enforcing every compilation).

Certainly easiest to roll out here, and given the current state of the world it probably makes the most sense

[schlosna]

yeah, this should be much less common after automated roll out, though folks may still write .filter(Optional::isPresent).map(Optional::get) so this would keep that cleaned up.

[carterkozak]

Worth running this on a couple larger internal projects to verify the results compile successfully. If there are edge cases we can't easily isolate, I think it should be safe to over-qualify.

[schlosna]

Yeah, this logic isn't quite right yet. I've tested out on a few local repos and there are some places were I'm not generating qualified types that need them.

I'm trying to avoid adding unnecessary qualifying type args, but there are at least a few cases I haven't quite nailed down that need qualification (e.g. some stream chains extracting layers of optionals).

[carterkozak]

I'm trying to avoid adding unnecessary qualifying type args, but there are at least a few cases I haven't quite nailed down that need qualification (e.g. some stream chains extracting layers of optionals).

Thinking about this a bit more, I think it's actually a good fit for the incredibly expensive SuggestedFixes.compilesWithFix utility, because in the failing case, we can qualify the method reference, and needn't pay the cost again (unlike LambdaMethodReference which used compilesWithFix in the matching component, meaning it must run the test in every subsequent compilation).

[carterkozak]

I think we should guarantee that we propose something after checking compilesWithFix, this could make future compilations expensive if compilation doesn't succeed on any of the options

[carterkozak]

I think these produce the same result in the tests, as long as one of the lines is present, everything seems to pass. Are there any known cases where we need both?

[schlosna]

I think we can remove the latter

[carterkozak]

ASTHelpers.getReceiver follows MethodInvocationTree.getMethodSelect(), we can pass tree directly for the same result

Suggested change

	ExpressionTree methodSelect = tree.getMethodSelect();
	ExpressionTree receiver = ASTHelpers.getReceiver(methodSelect);
	ExpressionTree receiver = ASTHelpers.getReceiver(tree);

[carterkozak]

Suggested change

	ExpressionTree mapTree = ASTHelpers.getReceiver(tree.getMethodSelect());
	ExpressionTree mapTree = ASTHelpers.getReceiver(tree);

[carterkozak]

If it's safe to replace receiverType.getTypeArguments() with List.of(elementType), we can remove the receiver argument to this function entirely. I'd imagine that should be safe, since we're pulling the resulting stream type of the original function (e.g. what's returned by .map(Optional::get)) as opposed to the intermediate/input state of the stream such a function is invoked upon.

[schlosna]

thanks, that updated and simplified things quite a bit

[carterkozak]

Looks great, thanks!

I've added do-not-merge in order to roll out the refactor via an excavator ahead of merging this into gradle-baseline, that way we decouple most of the code churn from the baseline upgrade. I've tagged you on the excavator which has largely identical error-prone code -- let me know if that rollout strategy seems reasonable

[carterkozak]

@schlosna Feel free to remove the do-not-merge label whenever you like, the lions share of these have successfully merged already

[autorelease3]

Released 6.12.0