Skip to content

[prone] Add error-prone.picnic.tech featuring RedundantStringConversion#2664

Merged
nedtwigg merged 1 commit intodiffplug:mainfrom
Pankraz76:pr-error-prone-SanityCheck
Oct 15, 2025
Merged

[prone] Add error-prone.picnic.tech featuring RedundantStringConversion#2664
nedtwigg merged 1 commit intodiffplug:mainfrom
Pankraz76:pr-error-prone-SanityCheck

Conversation

@Pankraz76
Copy link

@Pankraz76 Pankraz76 commented Oct 6, 2025

@Pankraz76
Copy link
Author

java.lang.RuntimeException: java.net.SocketTimeoutException: Connect timed out

@Pankraz76
Copy link
Author

This seems like a good addition to Rewrite, as in some aspects it is superior and has its unique benefits—like every tool has. Of course, they cover the same topic, like MissingOverrideAnnotation, so I'm wondering why we see this changed again. Maybe it's some bug or config issue; it seems to have a great potential when the right tools make the equation round up. It's faster than Rewrite, even though the configuration is not straightforward opt-in like Rewrite. We will work this out and learn on the job while extending the tool.

It's behaving just like Rewrite—failing the build and suggesting changes. The opt-in is quite unique and will not be communicated like in Rewrite, but the changes should be small, like the changesets.

Do you like to see this going on? @nedtwigg Thanks.

Copy link

@rickie rickie left a comment

Choose a reason for hiding this comment

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

I could be missing some context or another issue / PR in this repository, so please let me know if I miss something.

Not sure about the preference of the maintainers, but we might benefit from a slightly different approach. Now we are enabling a lot of checks in one PR. That makes the PR slightly harder to review. We could benefit from an approach outlined in this comment in JUnit repository.

If we use that approach, reviewing a PR is a lot easier, as all changes relate to one check. In this case there are quite a few that make changes, which makes it harder.

WDYT about that approach?

// 'StaticImport',
)
errorproneArgs.add('-XepOpt:Refaster:NamePattern=^(?!.*Rules\\$).*')
if (!getenv().containsKey('CI') && getenv('IN_PLACE')?.toBoolean()) {
Copy link
Author

Choose a reason for hiding this comment

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

activating IN_PLACE allowing removal of allErrorsAsWarnings resulting in local passing, on CI, without patching of course, positive failing build.

How to automate prone without having this glitch? @rickie Patching only cares for the XepPatchChecks like the documentation tells. XepPatchLocation Only works in paid, despite having it seen in gradle not being the case its a very random plugin behaviour sometimes.

Copy link

Choose a reason for hiding this comment

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

Only works in paid, despite having it seen in gradle not being the case its a very random plugin behaviour sometimes.

I don't fully understand this comment, can you elaborate?

I agree the usage of the XepPatchChecks and location is a bit finicky and requires some effort to get right. You work on a lot of PRs which is cool to see. But right now I don't have time to dive into all of them and find the exact cause. Will try to also look tomorrow, but can't make promises right now.

I once created a demo project (it's in Maven though) but perhaps you can look into that, and based on that try to configure it for one of the projects you're working on: https://github.com/rickie/error-prone-demo.


tasks.withType(JavaCompile).configureEach {
options.errorprone {
allErrorsAsWarnings = true // ModuleHelper.java:37: warning: Unsafe is internal proprietary API and may be removed in a future release
Copy link
Author

Choose a reason for hiding this comment

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

somehow this does not suppress/avoid:

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':lib:compileJava'.
> Compilation failed; see the compiler output below.
  /Users/vincent.potucek/IdeaProjects/spotless/lib/src/main/java/com/diffplug/spotless/java/ModuleHelper.java:37: warning: Unsafe is internal proprietary API and may be removed in a future release
  import sun.misc.Unsafe;
                 ^
  4 errors
  100 warnings

BUILD FAILED in 10s

Copy link

Choose a reason for hiding this comment

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

allErrorsAsWarnings this flag is an Error Prone flag (docs). It doesn't mean that all the errors from the build are being lowered in severity, it's about the checks from Error Prone that are configured with severity ERROR are lowered to WARNING.

In this case there is something else - unrelated to this flag - that is causing the error about the sun.misc.Unsafe. I'm not sure from the top of my head what this error is or why we get it here, but make sure to not misinterpret this flag.

Copy link
Author

Choose a reason for hiding this comment

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

junit mentions something similar but the fix does not help. Can not handle this issue.

@Pankraz76
Copy link
Author

Pankraz76 commented Oct 15, 2025

finally green, the plugin is working really well :-D.

kindly request your feedback on this @nedtwigg, thanks.

@nedtwigg nedtwigg merged commit 68c1254 into diffplug:main Oct 15, 2025
34 of 35 checks passed
@Pankraz76
Copy link
Author

nice, thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants