Extend suppression configuration to be more fine grained

[romani]

When new Static Analysis Tool (SAT) is introduced it always find a lot of violations in code. But it is not always possible to easily fix them (number of reasons). To let use successfully apply tool validation quickly, suppression mechanism should allow suppress cases in fine grained way, and enforce build failure appearance of problem in new code.

Base on problem of forbidden-apis enforcement over Checkstyle's code: checkstyle/checkstyle#1217 (comment)

It would be very useful to suppress certain validation on certain class(es), rather then all violations.

Discussion why annotations is not a complete solution for this - #56 (comment) (and all comments above).
Introduction of annotation is introduction of dependency(through imports) to custom class. forbidden-apis is just a tool and should not go inside of my project, but tool should to do his job on a side.
Good example of tool without intrusion to business code is Maven. It do everything with code (compile,test, verify, .......) and I have no dependency to in my code, it is just a tool that do a job, and project is easily compilable in any IDE and could be switched to Gradle one day. No changes to business code.

[uschindler]

Introduction of annotation is introduction of dependency(through imports) to custom class. forbidden-apis is just a tool and should not go inside of my project, but tool should to do his job on a side.

FYI: You don't need to use the supplied annotation, so there is no dependency problem. Elasticsearch or Lucene/Solr have defined a simple annotation (CLASS retention) in their own package. It is not visible at runtime, just part of bytecode (like @NonNull or similar).

For System.out a possible solution is to define @SysoutAllowed in your own package and annotate the methods with it. In the forbiddenapis config you can place a reference to this annotation class an you are done.

I agree the opinions go in different directions so I am sure we can allow to place a separate list of classes/methods/fields like the signatures files for exclusion. The code is not hard to do. Just in our opinion (Lucene/Elasticsearch) it proved to be much easier to maintain to put those as annotations in the code. A separate list is a desaster (like the chain of excludes statements).

Would you pass a list of methods to javac that should suppress javac warnings?

I will look into adding possibility for supplying a list of signatures to excude. But excluding only specific signatures for specific code parts looks like too fine grained (like unsafe signatures for this method, but not sysout). I would not do this!

It is much easier to add the plugin several times as separate maven executions (one time for system out, one time for unsafe/depreacted). This is the approach we had for Lucene and Elasticsearch in the past: They had separate signatures files for tests, for command line tools, or code using commons-io. We just executed forbidden apis several times with different excludes (see history in our svn).

Maybe the different opinions are also caused by the fact that we see a failure in forbidden apis like a hard compile failure. It is not like checkstyle or PMD something that could be fixed to improve code quality. Its a hard requirement that every code checked in passes this check - we don't allow any code anywhere in Lucene to work around that. Because of this we also require a explanatory string in the added annotation, if it really needs to be suppressed. Because of this it is (like SuppressWarnings) very related to our compilation phase, so we like to have the annotation in code. So user also sees explanatory note while editing the code that there is something not right with the code, but still needed. A separate exclusion file does not offer that information easily. E.g. because of Java 9 we forbid AccessibleObject#setAccessible anywhere in our code. If you really really need to use it for reflection you have to write a explicit description about that :-)

[uschindler]

So just to confirm: I will add a possibility to list exclusions in a separate file (same format like the signatures files). Like with signatures, the maven plugin will also load those files from Maven Central, if configured like that.

[romani]

It is not visible at runtime

Main point is to be not visible in development time (in IDE, when engineer need to focus on business logic )

For System.out a possible solution is to define @SysoutAllowed in your own package

It is not a solution, some workarond - yes, but I do not want to pollute my code with classes that are not related to business meaning of my project.

Would you pass a list of methods to javac that should suppress javac warnings?

no, but there is no way to avoid usage of javac, so usage of standard SuppressWarning annotation become a standard.

I will look into adding possibility for supplying a list of signatures to excude

Thanks, we will never come to agreement what is best way to suppress, that why I am asking for additional options.

But excluding only specific signatures for specific code parts looks like too fine grained

yes, I think the good step forward would be stay on Class level. If application designed in good way , certain problems will be collected in one place/class.

It is much easier to add the plugin several times as separate maven executions

that is workaround that other project could use right now, they have no other choice, I hope they would like to minimize configuraton of forbidden-api in their configurations.

Maybe the different opinions are also caused by the fact that we see a failure in forbidden apis like a hard compile failure

Think about new big project that want to start using you tool. He need suppressions to make a "fence" around old-legacy code for some time, and enforce your tool on new code. Suppression are indispensable to make transition easy.

E.g. because of Java 9 we forbid AccessibleObject#setAccessible anywhere in our code. If you really really need to use it for reflection you have to write a explicit description about that :-)

we use reflection a lot in our UTests to reach 100% coverage. So it would be just another suppression for your tool , because in general reflection is bad but we have very god reason to use it, and we benefit a lot from it. I do not want to suppress all rules in UTs, I want only certain rule to be suppressed on certain files.

[uschindler]

OK, so I think we agree. I will look into adding support for exclusions in the same way like the general signatures file format. The message could also be reused ("why is it forbidden").

One solution for big new projects is also (since 2.0):
http://jenkins.thetaphi.de/job/Forbidden-APIs/javadoc/check-mojo.html#failOnViolation

It would report violations, but not fail build. So you have enough time to clean up your code.

[rdiachenko]

One solution for big new projects is also (since 2.0):
http://jenkins.thetaphi.de/job/Forbidden-APIs/javadoc/check-mojo.html#failOnViolation
It would report violations, but not fail build. So you have enough time to clean up your code.

As for me this solution will not work. We're all human beings and the laziness is our nature and you will definitely never have enough time to clean up your code. If I have a big project and 'forbidden-apis' gives 100 violations without failing the build there is a high probability that I'll skip several more violations that occurred after my changes. In case of the build failure I will have to fix my code right away.

Let's take "Checkstyle" project as another example. There are a lot of pull requests each day. We would definitely not use failOnViolation=false but prefer to exclude all the current errors and make failOnViolation=true that will allow us to filter new violations on a pull request stage and concentrate on fixing just existing violations. As a result quick integration of 'forbidden-apis' into the project.

[nik9000]

I think it could be useful to have a sort of ratcheting mechanism. You can declare the number of failures that are OK somewhere and if that number is exactly the number of failures then the build passes. It'd provide a way to incrementally ban something: declare the number of allowed violations to the ban to be exactly as many as there are now. When you remove violations you ratchet the value down. When someone else tries to add a violation the build fails and they do what the failure message tells them to do.

I've seen it in tools like https://github.com/bbatsov/rubocop and it provides a simple "on ramp" so you can add new rules, stop yourself from violating them in the future, and incrementally fix old violations.

[vlsi]

@uschindler , what do you think of @SuppressWarnings(...)?
I agree with @romani that the annotation is standard, and it is universally understood by developers.

See a sample from ErrorProne: https://errorprone.info/bugpattern/UnsafeLocaleUsage
The Checker Framework follows uses @SuppressWarnings as well: https://checkerframework.org/manual/#suppresswarnings-best-practices

For instance:

> Task :postgresql:forbiddenApisMain FAILED
Forbidden class/interface use: sun.security.krb5.Credentials [non-public internal runtime class in Java 1.8]
  in org.postgresql.core.v3.ConnectionFactoryImpl (ConnectionFactoryImpl.java:415)

The way to suppress it could be:
a) @SuppressWarnings("forbidden.class") (e.g. to allow all forbidden classes in the method)
b) @SuppressWarnings("forbidden.class:sun.security.krb5.Credentials") (allow one class by name)

[uschindler]

Hi,
@SuppressWarnings is Retention.SOURCE, so it is no longer visible in resulting class files.
When we introduced forbidden apis for the first time, of course we investigated to use the Standard annotation, otherwise we would not have invented a new one.
In short: using SuppressWarnings is impossible, as forbidden apis is not a source code analysis tool. It is used to analyze class files and also works with any JVM based language, including Kotlin and Groovy. That's the cool thing, but on the other hand it's not a classical source code static analysis tool and won't see annotations only visible at that stage.

[vlsi]

Is there a way to declare @SuppressForbidden("...") annotation so the suppressions can be added without adding a new annotation every time?

That is want to confine the forbidden signature, and I do not want to suppress too much with a wildcard @SuppressForbidden.

[uschindler]

Hi,
I agree that's theoretically possible, but not yet implemented. I have to go through this issue to figure out what needs to be done.
In Apache Lucene and Elasticsearch the approach used was to put the call that needs suppression into a separate method, so if was never high priority. Adding all those suppression strings to class files may also not always been wanted. But nevertheless, the tool could apply suppressions like glob patterns of class names, if it finds some String value inside the annotation.

The code may get a bit more complicated, as suppressions are applied after class was parsed (e.g. to understand lambdas!), So I would need to keep track of all annotations.

[vlsi]

De-lamdification might be non-trivial indeed.

I guess suppression might be quite usable even if forbidden-apis did not see through lambda.

But nevertheless, the tool could apply suppressions like glob patterns of class names, if it finds some String value inside the annotation.

I thought behind the lines of creating a targeted set of keys for suppression purposes.
However, another option might be to use the signature itself in the annotation: @SuppressForbidden("java.lang.Object#wait(long)"). It would be more-or-less consistent with the existing configuration, and it would be great if suppression could be printed right in the error message :)

[vlsi]

Sample suppressions: https://github.com/apache/calcite/blob/155276591288615c4d02d55fb7d77eceb2e24b2d/core/src/main/java/org/apache/calcite/util/Unsafe.java , https://github.com/pgjdbc/pgjdbc/pull/2015/files#diff-fad1e587e61f470d246ea85f43f1151a0d6adfcecb26c9ada0ae5bdf0fe9534b

It looks like one of the best patterns, for now, is an Unsafe class that is excluded from forbidden-apis check.

[uschindler]

Lambdas are working fine since approx 2013! It just needs adaption to keep track of annotations to support the glob pattern parameter on the annotation. Currently it only keeps track yes/no in a very simple way...

I have to look into it again, since this issue was opened, a lot happened.

[romani]

@uschindler , did you come with some ideas howto make it?

we run into this problem again, and suppress all violations in files rather then specific violation.
Example of our desire to gradually cleanup deprecated internal method call - checkstyle/checkstyle#10417 .
So we want to enforce most strict rules to new code and existing code cleanup will take a bit of time.

[thatsIch]

We would like to have a suppression list.

I can not recompile for example Guava (or any other library) to add some annotations to them. Due to transitive dependencies, I can not exclude them from compile-time, but I would like to prevent developers from using those transitive dependencies (like Commons Lang, Guava).

We used the same approach with Checkstyle and it has been working great!

[uschindler]

Hi @thatsIch,
I do not understand from your comment what you intend to do. This issue here is about adding @SuppressForbidden with some more fine granular exclusion. I plan to implement this based on something like "tags". So each signatures file can have tags (by default e.g. "jdk-unsafe") or you can also tag several forbidden signatures using another @tag XYZ which applies to all following signatures. It would then be possible to add @SuppressForbidden("jdk-unsafe") in code.

From what I understand in your question, it looks like you have transitive dependencies and you want to disallow your own developers from using them. This is a different thing and works out of box with a custom signatures filein your project directory: In that case you need to define a signatures file and add a package-wise forbid. E.g., to disallow usage of commons-lang3 and Guava add the following signatures.txt file to your project and include it in the config:

org.apache.commons.lang3.** @ Don't use Apache Commons Lang 3
com.google.common.** @ Don't use Guava

The signatures would apply to your own code, but any dependency is not scanned, so it can still use those APIs (unless they expose them in their own API.

BTW, Elasticsearch and Solr are doing exactly this with some dependencies, e.g. Solr disallows log4j usage because all logging should be slf4j: https://github.com/apache/solr/blob/main/gradle/validation/forbidden-apis/org.apache.logging.log4j.log4j-api.solr.txt

[uschindler]

Hi, my intro to previous comment was more about #157. This issue is about config file instead of Suppressor bidden annotation.

But from your question I think my answer should be helpful.

[thatsIch]

Thanks, seems I was not aware that the resolution was a config-based suppression system and I misunderstood the given examples in this thread how the annotation-based system works.