google / error-prone

Catch common Java mistakes as compile-time errors
https://errorprone.info
Apache License 2.0
6.98k stars 757 forks source link

`PlaceholderUnificationVisitor` using javac 21 API #4846

Open msridhar opened 2 months ago

msridhar commented 2 months ago

In f093681 the PlaceholderUnificationVisitor class was modified to call CaseTree.getGuard():

https://github.com/google/error-prone/blob/cc12b63b439df353672c89d08acba3baa4c8b43f/core/src/main/java/com/google/errorprone/refaster/PlaceholderUnificationVisitor.java#L665

But, this API was only introduced in JDK 21:

https://github.com/openjdk/jdk/blob/9477c705c0bd5ce2d445abb5ca44d46656fc315f/src/jdk.compiler/share/classes/com/sun/source/tree/CaseTree.java#L78-L84

So I think this code will fail if it is run on JDK 17 (which Error Prone still supports). Maybe this wasn't detected on CI due to missing test coverage?

graememorgan commented 2 months ago

Thanks for the report. Unfortunately the test we have to repro this isn't open-sourced, so wasn't running on 17 anywhere.

I'm trying to wrangle it into OSS.

msridhar commented 2 months ago

Hi @graememorgan thanks! I saw #4873 which I think will fix the issue at runtime, but the new code still won't compile on JDK 17 due to the non-reflective call to getGuard(). I actually noticed this not because I run this code, but because I use IntelliJ. With the default import of the Error Prone Maven project into IntelliJ, the code doesn't compile in the IDE since IntelliJ decides to use JDK 17 to compile.

Is it intended that Error Prone requires JDK 21+ to compile (though it will still run on JDK 17)? One can work around this issue in the IDE if needed, I'm just wondering if this is intentional. It might be good to keep the code compiling on JDK 17 (and maybe to somehow test that in CI?) since that might catch this kind of issue even if the code isn't covered by unit tests.

cushon commented 2 months ago

I think the status quo is that the CI for 17 is compiling on newer versions and then using 17 for maven-surefire-plugin. I haven't paged in all of the history here.

Due to the lack of binary compatibility guarantees for the internal javac APIs, compiling against 17 doesn't guarantee we're compatible with 21, so I'm not sure there's a perfect alternative to having good test coverage for supported versions to catch issues at runtime.

msridhar commented 2 months ago

Ok, at some point I might write some docs on how to import into IntelliJ since it's slightly non-trivial. I can document changing the compiler version required there.