Generalize `-XepPatchChecks` command line parsing

[Stephan202]

This is a minimal (?) fix for #595. It adds support for:

• -XepPatchChecks arguments combining built-in and Refaster rules.
• -XepPatchChecks arguments listing multiple Refaster rules.

Open points and questions:

• Is this approach acceptable at all?
• If so, I'll add some tests.
• Right now Refaster is treated specially in both ErrorProneOptions and BaseErrorProneJavaCompiler; a nicer solution would entail making Refaster a first-class citizen. That'd be more work, but if you provide some pointers on the desired "end goal" or boundaries of the design space, I'm willing to have a crack at that, too. (In the context of a separate PR, ideally.)

[Stephan202]

Rebased the branch and resolved a conflict. What does the Error Prone team think of this approach?

[knutwannheden]

@Stephan202 Is this patch also available in the forked v2.3.4-picnic-2 release? I tried specifying the path to multiple .refaster check files, but the compiler then failed.

[Stephan202]

@knutwannheden based on the code I'd say that should work, yes. What error do you see?

NB: there's a possibility that this indeed doesn't work, because I don't think combining multiple Refaster templates like this has been tested recently. While v2.3.4-picnic-2 does indeed contain this patch, it also contains a fix for #552, so the current integration within Picnic actually compiles many Refaster templates into one, and then only passes that one .refaster file to Error Prone.

(I believe a longer term fix entails a more user-friendly integration as described under the second bullet point in https://github.com/google/error-prone/issues/552#issuecomment-516054278, but I'll admit that while this works, I'm no closer to polishing and open-sourcing that code. Chronic lack of time.)

[knutwannheden]

With my code being compiled through Maven I didn't really see any helpful error message at all, just some very generic message. I can report that back here, if you think it helps. Or maybe you know how I can get Maven to print the root cause here.

[Stephan202]

Shot in the dark: add <verbose>true</verbose> to the maven-compiler-plugin configuration and see whether that helps. If not, run with mvn -e -X and see whether anything stands out. If still none the wiser, dig through the -X output to find the exact javac command executed and manually run it on the command line. This can be pretty fiddly, but I recall that at least once in the past this allowed me to see an error message that was somehow not bubbled up by the maven-compiler-plugin.

[knutwannheden]

The error message wasn't displayed because of the <fork>true</fork> I had in my configuration (isn't the first time this happens to me) :unamused:

So the error I get is Can't load Refaster rule from /path/to/first.refaster,refaster:/path/to/second.refaster. In the pom.xml file I have:

    <arg>-Xplugin:ErrorProne -XepPatchChecks:refaster:/path/to/first.refaster,refaster:/path/to/second.refaster -XepPatchLocation:${basedir}</arg>

[Stephan202]

Okay, I stand corrected. This changeset is not included in the fork. :man_facepalming: And then the error message makes perfect sense. Apologies! Can you try to compile your Refaster rules in one go, using the changes from #552? (Those are included in the fork, I double-checked now :sweat_smile:.)

Otherwise I can try to create a new release that does include this change, but it might take a bit before I have time for that.

[knutwannheden]

Thanks for the heads-up. For my use case (part of jOOQ, which I think you are familiar with :smile:) I actually want to have separate .refaster files, which I am building in Maven with separate executions of the Java compiler.

But since we are only in an exploratory phase, it isn't yet important to have the ability to use multiple .refaster files, so no need to make a new release for that.

[Stephan202]

Cool! jOOQ Refaster templates... we're definitely interested in those! (Say hi to @lukaseder :) )

(Going off-topic big time here, but the idea that library authors provide migration or best-practice Refaster rules for their libraries is one of the motivations behind the stuff written under the second bullet point in https://github.com/google/error-prone/issues/552#issuecomment-516054278.)

[lukaseder]

Hi @Stephan202

[cushon]

Hi @Stephan202,

Thanks for the PR! I was taking a fresh look at this, it seems like a nice thing to support.

I was trying to think a bit about how this interacts with #947 and #4028, I wonder if we should decide what the semantics should be for #4028 first and then figure out how to make the refaster patch options consistent with that.