Make ThirdPartyLibrary compatible with -source 8
#627
Conversation
When targeting Java 8, `unnamedModule` is not properly initialized, causing an NPE when trying to load a class from it. In that context `noModule` should be used instead. Resolves #626.
|
Kudos, SonarCloud Quality Gate passed! |
| // XXX: Drop support for targeting Java 8 once the oldest supported JDK drops such support. | ||
| ModuleSymbol module = | ||
| Source.instance(state.context).compareTo(Source.JDK9) < 0 | ||
| ? symtab.noModule | ||
| : symtab.unnamedModule; |
There was a problem hiding this comment.
Unconditionally using noModule seems to work as well, but:
- The documentation claims it should only be used with
-source 8. - I suspect (but did not test) that if using
noModulewhen targeting Java 9+, the requested classes may be loaded a second time. That's not very nice/efficient.
There was a problem hiding this comment.
Naturally Pitest complains that one of the branches isn't covered. We can't prevent that, but it does raise the question whether we should have a build targeting JDK 8. With Jabel introduced in #603 this would in theory be possible, but (a) it'd require a number of workarounds and (b) that setup doesn't seem capable of reproducing the NPE reported in #626 (not sure why; didn't deep-dive). So all-in-all not worth the hassle.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Well, it can be tested using the reproduction case of #626. When using a SNAPSHOT built off this branch the NPE goes away :)
🤷 |
There was a problem hiding this comment.
Well, it can be tested using the reproduction case of #626. When using a SNAPSHOT built off this branch the NPE goes away :)
😅 , it's time to call it a day haha.
Verified that it works, nice fix @Stephan202 🚀 !
Suggested commit message: