Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize -XepPatchChecks command line parsing #787

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Stephan202
Copy link
Contributor

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.)

ScannerSupplier scannerSupplier,
ErrorProneOptions epOptions,
Context context,
boolean forceFilter) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This forceFilter flag makes sure that the default patch mode of applying all built-in checks is disabled when a Refaster rule is supplied. I added this for backwards compatibility, but it does mean there's (still) no way to apply all built-in checks and any additional Refaster rules. That's another way in which Refaster is "special" (contrast this with service-loaded custom plugins: those are applied by default).

This is another thing I'd like to improve; input welcome!

@Stephan202
Copy link
Contributor Author

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

@knutwannheden
Copy link

@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
Copy link
Contributor Author

@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 #552 (comment), but I'll admit that while this works, I'm no closer to polishing and open-sourcing that code. Chronic lack of time.)

@knutwannheden
Copy link

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
Copy link
Contributor Author

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
Copy link

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) 😒

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
Copy link
Contributor Author

Okay, I stand corrected. This changeset is not included in the fork. 🤦‍♂️ 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 😅.)

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
Copy link

Thanks for the heads-up. For my use case (part of jOOQ, which I think you are familiar with 😄) 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
Copy link
Contributor Author

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 #552 (comment).)

@lukaseder
Copy link

Hi @Stephan202

- Support arguments combining built-in and Refaster rules.
- Support arguments listing multiple Refaster rules.

Fixes google#595
@cushon
Copy link
Collaborator

cushon commented Sep 19, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants