Example of error level rule https://error-prone.picnic.tech/bugpatterns/FluxFlatMapUsage/

Please share where -Xplugin:ErrorProne -Xep:CollectorMutability:ERROR will be placed to change default value of severity for this rule/pattern.

Please use in commit message prefix Issue #14137: ....... we keep wide explanations linked to commit to easily restore later on.

As already noticed we use hacky groovy script to put on leash reporting of errorprone. As you very knowledgeable in this tool, do you know better way to get report from tool? We just need print to contributor not all messages but messages that requires to fix.

Is there ability to exclude pattern/rule from execution at all? Or change severity to IGNORE/OFF.
As far as I remember we focused on fixing ERROR only as we did not able to find way to on/off rules or certain violation. To enable all and iteratively and slowly clean up backlog.

Our suppression model: https://github.com/checkstyle/checkstyle/blob/master/config/error-prone-suppressions/compile-phase-suppressions.xml

@Vyom-Yadav , please read this as you were involved in error prone activation in our project

https://github.com/checkstyle/checkstyle/actions/runs/7187166866/job/19574155042?pr=14136
Log file jumped from 100 kb to 2.5 megs in this PR.

Thanks for all the questions, let me know if something is unclear or I'm missing something.

Please share where -Xplugin:ErrorProne -Xep:CollectorMutability:ERROR will be placed to change default value of severity for this rule/pattern.

After the -Xplugin:ErrorProne we have to add exta configuration (here) if we want to. Now that I'm writing down, I we need to have a place in the pom.xml where we define this property as we will use it in two places, namely in the error-prone-compile and error-prone-test-compile profiles. Do we define a property and use it in both profiles?

W.r.t. how we can best configure the checks, I'd like to propose to use a similar approach to what we have here, which allows us to specifically disable and increase the severity of checks.

As already noticed we use hacky groovy script to put on leash reporting of errorprone. As you very knowledgeable in this tool, do you know better way to get report from tool? We just need print to contributor not all messages but messages that requires to fix.

Currently there is nothing yet that gives us the reports easily. There is a GitHub issue related to this: google/error-prone#3766, that's all...

Is there ability to exclude pattern/rule from execution at all? Or change severity to IGNORE/OFF.

We can use the command-line flags to enable and disable checks, see the docs here.
So we simply disable rules by adding -Xep:NameOfTheCheck:OFF, or use one of their convenience flags like -XepDisableAllWarnings.

As far as I remember we focused on fixing ERROR only as we did not able to find way to on/off rules or certain violation. To enable all and iteratively and slowly clean up backlog.

I tried searching fo a discussion related to this in the old Error Prone activation PR but I couldn't find any. We can iteratively enable all of the rules, I don't see blockers (yet?). If you know what was blocking you from doing this, let me know, I'm curious!

Our suppression model: master/config/error-prone-suppressions/compile-phase-suppressions.xml

Thanks for sharing. Is there a specific reason for not using @SuppressWarnings("ArrayHashCode" /* Explanation why we suppress this check. */) for the suppression?

Oh: I think I found the reason in the old Error Prone PR: #12063 (comment), am I correct?

checkstyle/checkstyle/actions/runs/7187166866/job/19574155042?pr=14136
Log file jumped from 100 kb to 2.5 megs in this PR.

Good point, I noticed a few of our checks are creating quite some output. What we can do is already disable some of the rules like StaticImport, LexicographicalAnnotationListing, and JUnitMethodDeclaration. I suggest we disable them in follow up PRs.

Rebased.

FYI: The build is failing because the linked issue is not approved.

Do we define a property and use it in both profiles?

Yes. We can define properties in pom xml, we already have bunch.

propose to use a similar approach to what we have here,

Works for us too.

So we simply disable rules by adding -Xep:NameOfTheCheck:OFF

Yes, this is good for us, as we have rule that if we enforce some rule we do it as build failure, no leaks are allowed.

I don't see blockers (yet?). If you know what was blocking you from doing this, let me know, I'm curious!

No blockers, but we would need your help to make it, we are sinking in PRs, maintainers are overloaded.

Is there a specific reason for not using @SuppressWarnings("ArrayHashCode"

We keep culture of "no pollution in main code for tools". So we keep main code clean and all hacks and unfortunate cases outside, let let readers do not be bothered by extra details. If we put all to code it will have a lot of annotations :). But sometimes we need to suppression on specific line, not whole method.

I suggest we disable them in follow up PRs.

Yes, please. I updated issue with opinion on what should apply and what we should disable.

@Vyom-Yadav , fyi

Yes. We can define properties in pom xml, we already have bunch.

Nice sounds good, will do that as part of the follow up PRs :).

we would need your help to make it

Sounds good, I'll try to keep it doable and not overload with PRs. After this one is merged I'll open the first one(s).

We keep culture of "no pollution in main code for tools".

Interesting, cool approach! Thanks for explaining.

Yes, please. I updated issue with opinion on what should apply and what we should disable.

Yes perfect. Will start creating them after we merge this PR. Oh and if it goes too fast or too slow after we merge this one, just let me know and we will work it out :).

Rebased and resolved conflict.

Sorry, Late to the party, but this is really cool, thanks a lot, @rickie for taking charge here.

Checkstyle is configured to only consider checks that have SeverityLevel.ERROR

Just curious, do you, in your org, consider the warnings as well? Are there any good checks that give out warnings but are good to have as errors? That way we can further improve the quality.

We usually apply/enforce all rules, if we do not see some rule doing any good, of it is against our style or .., we just disable it at all.
There should be no violations of any severity.

Sorry, Late to the party, but this is really cool, thanks a lot, @rickie for taking charge here.

No problem, I really like doing this @Vyom-Yadav.

Just curious, do you, in your org, consider the warnings as well? Are there any good checks that give out warnings but are good to have as errors? That way we can further improve the quality.

Yes we do! I think we have quite the same approach & goals as Checkstyle. Namely, try to enable as much static analysis as possible. For our Error Prone usage, it means that we use the following command-line flags: -XepAllDisabledChecksAsWarnings and -XepAllSuggestionsAsWarnings (fun fact: the last flag we contributed ourselves to Error Prone). These flags combined enable all checks that Error Prone provides, see the full list of checks here: https://errorprone.info/bugpatterns. After enabling all of them with these flags, we selectively disable a few of the checks.

If you want to see an example, here is our configuration from Error Prone Support: https://github.com/PicnicSupermarket/error-prone-support/blob/master/pom.xml#L1701-L1757.

This is roughly similar to what we have in our internal setup at Picnic.

I'd say after we went through the checks of Error Prone Support as defined in #14137, we can take a look at the checks from Error Prone that are listed under "WARNING" and "INFO" https://errorprone.info/bugpatterns. WDYT?

Yes, please, let's try to activate all. It helps a lot to maintenance of project by small team.

Good suggestion. As we have default ERROR in our script, we can enable all, look at the logs to find some value checks from the warning which we can later change to ERROR severity level.