Prevent NestedOptionals from throwing an NPE
#412
Conversation
The type returned by OPTIONAL_OF_OPTIONAL.get(state) is null for code sample: Duration.of(1, ChronoUnit.YEARS)
|
Looks good. All 5 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
There was a problem hiding this comment.
Thanks for trying the new release and filing a bug fix @gtoison! Let us know if you find other issues (or have requests) :)
I added a commit with some suggestions.
Suggested commit message:
Prevent `NestedOptionals` from throwing an NPE (#412)
Previously, a `NullPointerException` was thrown if during compilation the
`java.util.Optional` class was not loaded at all.
(I suspect there may be similar such booby traps in other checks 🤔.
@gtoison let us know if you'd like to have a 0.6.1 patch release with this change. I might be able to make time for that in the next 1-2 days.
| Type stateType = OPTIONAL_OF_OPTIONAL.get(state); | ||
| if (stateType != null && state.getTypes().isSubtype(ASTHelpers.getType(tree), stateType)) { | ||
| return describeMatch(tree); | ||
| } else { | ||
| return Description.NO_MATCH; | ||
| } |
There was a problem hiding this comment.
Alright, so I guess that the issue happens if the Optional class isn't loaded during compilation at all. That realization also lead to this code.
There was a problem hiding this comment.
We do have a preference for an early return with Description.NO_MATCH:
| Type stateType = OPTIONAL_OF_OPTIONAL.get(state); | |
| if (stateType != null && state.getTypes().isSubtype(ASTHelpers.getType(tree), stateType)) { | |
| return describeMatch(tree); | |
| } else { | |
| return Description.NO_MATCH; | |
| } | |
| Type type = OPTIONAL_OF_OPTIONAL.get(state); | |
| if (type == null || !state.getTypes().isSubtype(ASTHelpers.getType(tree), type)) { | |
| return Description.NO_MATCH; | |
| } | |
| return describeMatch(tree); |
| "", | ||
| "class B {", | ||
| " void m() {", | ||
| " java.time.Duration.of(1, ChronoUnit.YEARS);", |
There was a problem hiding this comment.
I suggest using a slightly more realistic case ;)
jshell> java.time.Duration.of(1, java.time.temporal.ChronoUnit.YEARS)
| Exception java.time.temporal.UnsupportedTemporalTypeException: Unit must not have an estimated duration
| at Duration.plus (Duration.java:717)
| at Duration.of (Duration.java:312)
| at (#1:1)
| @@ -39,4 +39,19 @@ void identification() { | |||
| "}") | |||
| .doTest(); | |||
| } | |||
|
|
|||
| @Test | |||
| void failingTypeCheck() { | |||
There was a problem hiding this comment.
Slightly more in line which what we do in a few other places
| void failingTypeCheck() { | |
| void identificationOptionalTypeNotLoaded() { |
(Funny: I tried to replace the Duration expression below with Stream.empty(), but then the NPE didn't happen. I think because Stream does reference Optional, while Duration (and ChronoUnit) don't.
|
Looks good. All 5 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Thanks for the review and the fixes! A patch release would be nice but there's no need to rush things. I was planning to upgrade error-prone when there's a new release |
|
Ah got one tweak for the suggested commit message @Stephan202 I think it should be "a NPE" instead of "an NPE" 👀? |
|
@rickie for acronyms it depends on pronunciation, and I personally literally say "en-pee-ee", so then "an" is correct. I think this is a gray area. |
|
Aha, I don't do it like that. Anyway, let's roll with what is suggested then :). |
rickie
changed the title
NestedOptionals from throwing an NPE
While testing version 0.6.0 I encountered a
NullPointerExceptionthrown byNestedOptinalwhen analysing this code (please don't judge, it's a bug sample!):This PR adds a unit test reproducing the problem and a proposed fix.
Thanks a lot for providing this project!