Resolve build warnings

[Stephan202]

Suggested commit message:

Resolve build warnings (#122)

Two things:

• I think that this shows that using SeverityLevel.SUGGESTION doesn't really work in the current setup, unless we rely on a CI build applying the patch profile. But since such an extra patch build takes extra time, I'm not sure that's a desirable end state.
• I noticed that -Perror-prone-fork -Pself-check -Ppatch -Derror-prone.patch-checks='' doesn't work; I had to explicitly specify LexicographicalAnnotationListing. This indicates that the changes of Support running all available patch checks google/error-prone#947 no longer fully work. That requires separate investigation.

[Stephan202]

I noticed that -Perror-prone-fork -Pself-check -Ppatch -Derror-prone.patch-checks='' doesn't work

After a lot of debugging I found the culprit, and it's related to #76 (comment): even though -Perror-prone-fork is specified, the annotation processor classpath uses the non-forked version of error_prone_check_api, because it is pulled in by error-prone-contrib. One cannot fix this simply by explicitly specifying ${groupId.error-prone}:error_prone_check_api anywhere on the annotation processor classpath, because it seems that (unlike regular Maven dependency resolution, which is ~breadth-first), the annotation processor classpath is populated depth-first. So the only "fix" I found so far is to add this dependency to the default/always-configured annotationProcessorPaths section:

diff --git a/pom.xml b/pom.xml
index 7a0636fc..70bf9dbd 100644
--- a/pom.xml
+++ b/pom.xml
@@ -732,6 +732,11 @@
                     <version>3.10.1</version>
                     <configuration>
                         <annotationProcessorPaths>
+                            <path>
+                                <groupId>${groupId.error-prone}</groupId>
+                                <artifactId>error_prone_check_api</artifactId>
+                                <version>${version.error-prone}</version>
+                            </path>
                             <path>
                                 <groupId>com.google.auto.service</groupId>
                                 <artifactId>auto-service</artifactId>

But that feels wrong. (I also tried moving up the error-prone profile and adding the dependency there, but that didn't work. Since adding error_prone_check_api right after auto-service does work, that's surprising, because based on mvn help:effective-pom I'd expect those approaches to be equivalent when the error-prone profile is active. Right now I don't have an explanation for this.)

All this is quite ugly, and if we go this route it warrants a comment. But TBH: the more I think about it, the more it feels like we should generate two sets of artifacts: those compatible with vanilla Error Prone, and those that use the fork. Details TBD.

[rickie]

We could decide to not allow SUGGESTION currently, as it is not obvious enough.

W.r.t. the second point, thanks for explaining. Looks like a pretty difficult issue. Perhaps we can discuss solutions together.

Code LGTM!

[Badbond]

Extensive analysis @Stephan202. Must admit that I don't have the time/brain capacity to dive deep into it to get a full understanding of it. Would be happy to join any offline discussions around this to get a better understanding for when we wish to improve this setup.

Code changes LGTM.

[rickie]

We decided to create a new flag, AllSuggestionsAsWarnings to make sure we can still use SUGGESTION but fail the build if there is a violation.

[rickie]

Just opened PicnicSupermarket/error-prone#33.

[Stephan202]

Closing this PR: these changes were merged as part of #76. That PR also includes -XepAllSuggestionsAsWarnings, which resolves the first issue identified above. The second issue is covered by #149.