Don't rewrite non-JDK Optional in OptionalNot{Present,Empty} recipes#1147
Closed
timtebeek wants to merge 1 commit into
Closed
Don't rewrite non-JDK Optional in OptionalNot{Present,Empty} recipes#1147timtebeek wants to merge 1 commit into
Optional in OptionalNot{Present,Empty} recipes#1147timtebeek wants to merge 1 commit into
Conversation
When a third-party Optional (e.g. com.bazaarvoice.jolt.common.Optional) is off the recipe classpath, javac can misattribute its isPresent()/isEmpty() calls to java.util.Optional, fooling the MethodMatcher into emitting an uncompilable isEmpty()/isPresent() call. Skip the file when the simple name Optional is bound to some other explicitly imported type. Fixes #1146
Member
Author
|
This should be fixed in the type information upstream. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's changed?
OptionalNotPresentToIsEmptyandOptionalNotEmptyToIsPresentnow skip a source file when the simple nameOptionalis bound to an explicitly imported type other thanjava.util.Optional.Why?
!isPresent()on a non-java.util.Optionaltype, producing uncompilableisEmpty()#1146.When a third-party
Optional(e.g.com.bazaarvoice.jolt.common.Optional, which hasisPresent()but noisEmpty()) is off the recipe's typed classpath, javac can misattributebar.isPresent()tojava.util.Optional— for instance when ajava.util.*import is in scope. TheMethodMatcher(java.util.Optional isPresent()) then matches and the recipe rewrites the call toisEmpty(), which does not exist on the third-party type, so the module no longer compiles.Because the type information is uniformly (mis)attributed to
java.util.Optionalin this scenario, a type-based guard on the receiver can't distinguish the cases. The reliable signal is the source itself: if the file explicitly imports a non-JDK type namedOptional, the bareOptionalname cannot bejava.util.Optional, so the recipe conservatively leaves the file untouched.The sibling
OptionalNotEmptyToIsPresenthas the identical structure and the same failure mode, so it gets the same guard.Tests
Added
doNotChangeNonJdkOptionalMisattributedToJdkto both recipe test classes. Both reproduce the misfire (they fail onmainand pass with this change). A test covering a fully-resolved third-partyOptionalis also added.