Stuck with 2.4.0 and JDK 11 due to Lombok warnings #3094
Comments
|
I would say that once again Lombok generates an invalid (and unexpected) AST that ErrorProne fails to process. Judging by the reported name ( |
|
@tbroyer Thank you for your looking into it. So far what I see is that the same Lombok version was working fine with ErrorProne 2.4.0, and without any changes in Lombok, it is not working with ErrorProne 2.5.x and above. In case this is a Lombok problem, it seems that it was there all the time, but somehow ErrorProne 2.4.0 was able to handle it, or somehow ignored or hid this issue from us. Even if there is a problem with Lombok, shouldn't it be ignored because of the Would you recommend opening an issue also on the Lombok side to tackle this from both directions? |
Looking at this case in a debugger seems to confirm that. I think this was the corresponding change: Rawi01/lombok@5ec517d. Previously it was adding synthetic AST nodes without source position information, and now it uses the source position information as the lombok annotation associated with the generated code. |
|
My workaround, that seems to work for me currently is to use the Lombok Maven Plugin and delombok the sources into a new folder and I'll use those generated sources as input for the compiler. When ErrorProne is running against the delombok'd sources, it seems to work as expected, no extra warnings. I have the following in my delombok profile <profiles>
<profile>
<id>delombok</id>
<!-- ErrorProne >= 2.7 is needed for JDK 17, but ErrorProne >= 2.5.0 has a problem with Lombok warnings
see https://github.com/google/error-prone/issues/3094
The workaround here is to first delombok the code before compiling. Since we don't want to use a separate
folder for Lombok source files, which would be quite inconvenient, we tell the Lombok plugin to use the
regular source files from 'src/main/java' and then tell Maven to consider the folder with the delobmok
output as the main sourceDirectory.
see https://github.com/awhitford/lombok.maven/issues/7#issuecomment-136607499
This configuration is applied in a profile that we can invoke from the command line, this way IntelliJ
is able to work with the sources normally without knowing that that we use delombok and having to make
overrides in the project.
-->
<properties>
<!-- Do not use 'target/generated-sources/delombok', we don't want IntelliJ to recognize it as a source folder,
it would lead to errors that need manual workaround on each project reload. -->
<lombok.outputDirectory>${project.build.outputDirectory}/target/delombok/generated-sources</lombok.outputDirectory>
<overrideSourceDirectory>${lombok.outputDirectory}</overrideSourceDirectory>
</properties>
<build>
<plugins>
<!-- Delombok sources before build -->
<plugin>
<groupId>org.projectlombok</groupId>
<artifactId>lombok-maven-plugin</artifactId>
<executions>
<execution>
<phase>generate-sources</phase>
<goals>
<goal>delombok</goal>
</goals>
<configuration>
<addOutputDirectory>false</addOutputDirectory>
<sourceDirectory>${project.basedir}/src/main/java</sourceDirectory>
</configuration>
</execution>
</executions>
</plugin>
<!-- Maven Compiler configuration with ErrorProne, without Lombok -->
<!-- Keep this in sync with the default compiler plugin, except for the delombok annotation processor -->
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<forceJavacCompilerUse>true</forceJavacCompilerUse>
<showDeprecation>true</showDeprecation>
<showWarnings>true</showWarnings>
<compilerArgs>
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne -XepDisableWarningsInGeneratedCode
-XepExcludedPaths:.*/target/generated-sources/.*|.*/src/test/java/.*
-Xep:NullAway:${nullaway.level} -XepOpt:NullAway:AnnotatedPackages=mypackages.*
</arg>
</compilerArgs>
<annotationProcessorPaths>
<path>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_core</artifactId>
<version>${errorprone.version}</version>
</path>
<path>
<groupId>com.uber.nullaway</groupId>
<artifactId>nullaway</artifactId>
<version>${nullaway.version}</version>
</path>
<!-- Other annotation processors go here.
If 'annotationProcessorPaths' is set, processors will no longer be
discovered on the regular -classpath; see also 'Using Error Prone
together with other annotation processors' below. -->
</annotationProcessorPaths>
</configuration>
</plugin>
</plugins>
</build>
</profile>
</profiles>In my parent parent pom config <properties>
<!-- Allows to override the default source directory, e.g. for delombok -->
<defaultSourceDirectory>${project.basedir}/src/main/java</defaultSourceDirectory>
<overrideSourceDirectory>${defaultSourceDirectory}</overrideSourceDirectory>
</properties>
<build>
<sourceDirectory>${overrideSourceDirectory}</sourceDirectory>I still have the original maven compiler + errorprone + lombok configuration as the default, this allows me to load the project into IntelliJ with the original source folders and don't have to deal with delombok in IDEA (it will generate the warnings there though, but it seems a bit more manageable). And I had to point checkstyle and PMD to the original sources I've also added At the moment this configuration seems to work for me and I can use JDK 17, ErrorProne 2.12.1 and Lombok 1.18.22, all the latest. However, haven't tested it yet extensively, just enabled on a new project while I figured things out. |
|
I think that the delombok approach is more principled--instead of modifying the AST in place (which breaks source position information), it just makes it a separate preprocessor that doesn't affect other tools running during the compilation. It is maybe a little harder to integrate into builds, though, I'm sure there are reasons not all lombok users are using delombok. We could also consider trying to tolerate ASTs with broken source position information more gracefully. The generated methods here look something like: In theory Error Prone could recognize |
@cushon this is not theory, ErrorProne does have the a flag to recognize and ignore any
Now that I look into it, it seems that you added that feature to check only the simple name of the annotation 🙂 4d6a0cc Here is more when I look into various Lombok versions (I am working with Java 11 here, so only tested Lombok >= 1.18):
I am not sure how did I test before but Lombok >= 1.18.18 shows warnings also with ErrorProne 2.4.0 now just as with ErrorProne 1.18.22 on Java 11... So there is a sweet spot with latest ErrorProne 2.12.1 with Lombok 1.18.16 on JDK 11 that works for me. It seems that something changed with Lombok 1.18.18 that led to these new warnings shown by ErrorProne. What should be the next step here?
|
|
I've checked locally and can confirm that this issue was introduced with the commit @cushon mentioned above: projectlombok/lombok@a234a8e |
We do have some logic that looks for the simple name But I think Additionally, there are a couple of different code paths for testing suppressions, and the one I'll look at cleaning some of this up on our side. In general there's a limit to what I'm prepared to do to try to handle invalid ASTs with broken source position information gracefully, but fixing the |
…orProneScanner` This makes the full, dizzying complexity of `ErrorProneScanner`'s suppression handling available in `BugChecker#isSuppressed`, including handling of custom suppression annotations and `-XepDisableWarningsInGeneratedCode`. #3094 PiperOrigin-RevId: 441534877
…orProneScanner` This makes the full, dizzying complexity of `ErrorProneScanner`'s suppression handling available in `BugChecker#isSuppressed`, including handling of custom suppression annotations and `-XepDisableWarningsInGeneratedCode`. #3094 PiperOrigin-RevId: 441580794
|
Compiling with ErrorProne before your changes (7b6ae68) gives me the warnings: Maven outputCompiling with ErrorProne after your changes (5d647de) gives no warnings anymore! Maven outputAlso tested that not adding I've tested this with both ErrorProne and the test project built with JDK17, with latest Lombok 1.18.22 and I can confirm that the behavior reported in the ticket has been fixed. @cushon you're amazing! Your help is much appreciated! Thank you! |
|
@cushon just a last question, I'm not familiar with ErrorProne release cycles, is it possible to know when will be this patch released? |
|
@mihalyr I have performed a release that includes those fixes. |
|
Reading the comments here, I too tried Lombok 1.18.16 to see if that would get us out of this mess, but it ended up failing at So I tried Lombok 1.18.24, and that then seems to get us through compiling the first module in our codebase - so that's progress. The new failure we get is because But we turn off warnings on generated code, so why it's even checking these is a mystery. It could be that #3108 will fix it and we're just waiting for that. |
Hi everyone,
I'm facing an issue with ErrorProne currently that is making it harder to upgrade from JDK 11 to 17.
In short, ErrorProne 2.4.0 with JDK 11 works well for me, but I cannot use 2.4.0 with JDK 17, and when I upgrade to anything newer than 2.4.0 I get crazy amounts of warnings for Lombok annotations.
Here is a minimal reproducer - just a single class with a single "@lombok.Value" annotation (use JDK 11 with this):
pom.xml
src/main/java/errorprone/App.java
lombok.config
Case 1: ErrorProne 2.4.0 and JDK 11
Everything is working as expected.
Maven output
Case 2: ErrorProne 2.5.0 and JDK 11
This fails with the below error:
Case 3: ErrorProne 2.5.1 and JDK 11
The Lombok warnings start here.
Maven output
Case 4: ErrorProne 2.4.0 and JDK 17
This does not work, fails with the following:
Case 5: ErrorProne 2.12.1 (or any >= 2.5.1) and JDK 17
This throws a bunch of warnings again.
Maven output
I've also tried the following Lombok configuration (added new dependency for
@SuppressFBWarningscom.github.spotbugs:spotbugs-annotations:4.6.0):lombok.config
Dumping 15 warnings for every single Lombok annotation doesn't seem to be right. In our production code the amount of warnings is just too much, so far I kept staying with JDK 11 and 2.4.0, but I would really like to upgrade to JDK 17.
I was considering disabling all warnings temporarily, but adding
-XepDisableAllWarningsdidn't change anything, the warnings remained. It works when I disable or suppressSameNameBugDifferentexplicitly. I could only do that globally, because suppressing every single case on production adds a lot of unnecessary noise to the code. However, this is not the only Lombok annotation that throws warnings.I can switch from
@Valueto@AllArgsConstructorfor example and I get the same warnings but less of them, only 4. But If I add@Datain addition, I get 33 warnings already from this simple piece of code.I am using the exact same Lombok version that worked with 2.4.0, it uses the
@lombok.Generatedannotation and I use ErrorProne with the-XepDisableWarningsInGeneratedCodeas before, the only change is ErrorProne version from 2.4.0 to 2.5.1.Did anything change in ErrorProne in 2.5.0 or 2.5.1 that could be the culprit of this?
Is there any workaround I could use other than disabling any check that is being thrown or suppressing with annotations? Something like telling ErrorProne a list of annotations to ignore (a way to pass a list of lombok annotations that it should skip)?
Or did I miss or forgot something obvious that needs to be done when upgrading from 2.4.0 to 2.5.x? Please let me know if there is anything else I can try.
Thank you in advance.
The text was updated successfully, but these errors were encountered: