False positive for THROWS_METHOD_THROWS_CLAUSE_THROWABLE

[alexpdp7]

Hi,

Updating to 4.7.0 ( alexpdp7/zqxjkcrud#315 ) is causing the following warning for me:

[ERROR] Medium: Method lists Throwable in its throws clause. [net.pdp7.zqxjkcrud.dao.CatalogRepository] At CatalogRepository.java:[line 23] THROWS_METHOD_THROWS_CLAUSE_THROWABLE

But the line it's pointing to doesn't make sense?

https://github.com/alexpdp7/zqxjkcrud/blob/c247e3be1daf02dce1340eee436889b7c2fd1786/src/main/java/net/pdp7/zqxjkcrud/dao/CatalogRepository.java#L23

Cheers,

Álex

[welcome]

Thanks for opening your first issue here! 😃
Please check our contributing guideline. Especially when you report a problem, make sure you share a Minimal, Complete, and Verifiable example to reproduce it in this issue.

[ThrawnCA]

I suspect that this is because Spotbugs doesn't handle lambdas well.

[KengoTODA]

The ConnectionCallable#run() method throws Throwable so the lambda in your code also declares that it throws Throwable.

I believe it is a problem of the interface provided by jOOQ. As a workground, you can filter or suppress the warning reported by SpotBugs, or replace the lamdba with an anonymous class like below:

new ConnectionCallable<YourClass>() {
  @Override
  public YourClass run(Connection connection) /* no `throws Throwable` here */ {
    // your code
  }
}

[Bananeweizen]

Labeling this as a third party bug seems wrong to me. Spotbugs should rather not raise that issue for lambdas used in third party methods. I get that warning hundreds of times for trivial AssertJ assertions like this:

assertThatExceptionOfType(SomeException.class).isThrownBy(() -> foo());

just because the "isThrownBy(...)" method of course must handle every kind of throwable. I would expect the same to be reported for JUnit, Hamcrest and similar assertion libraries, if people start upgrading to spotbugs 4.7.0. Any change on my side to avoid the spotbugs issue just makes the code worse to read, and doesn't improve the security/fault tolerance or any other criteria.

[KengoTODA]

I understand, and in the case of AssertJ, it is understandable to throw Throwable.

However, in case the case of jOOQ, it should not throw Throwable that may be an instance of Error. In my understanding, it has less motivation and no reasonable reason to do so.

And generally, static analysis prefers false-positive over false-negative, then I think the current SpotBugs implementation itself is OK. It is technically possible to treat AssertJ, you may send a PR to realize. :)

[Bananeweizen]

The problem is not about whether some method is reasonable to throw exception. The problem is about the signature being defined outside of the project being analyzed, and Spotbugs raising an error for any derived usage of that unchangable signature. I would fully understand the error being raised for any class declaration in my analyzed files that is not a subclass of such an external class.

[rovarga]

The ConnectionCallable#run() method throws Throwable so the lambda in your code also declares that it throws Throwable.

I believe it is a problem of the interface provided by jOOQ. As a workground, you can filter or suppress the warning reported by SpotBugs, or replace the lamdba with an anonymous class like below:

new ConnectionCallable<YourClass>() {
  @Override
  public YourClass run(Connection connection) /* no `throws Throwable` here */ {
    // your code
  }
}

Actually I do not believe this will work as a workaround. At least with JDK17 there will be a synthetic bridge method defined with the Throwable and SpotBugs will report the same violation against it. See #2050 .

[patrickuhlmann]

I confirm that this rule is messing with my projects unit-tests for exceptions.

It reports every line with assertThrows. For example:

Assertions.assertThrows(IllegalArgumentException.class,
            () -> Application.main(null));

The signature of the Application class is like this:

public static void main(String[] args) throws IOException

So I don't even understand why it is reporting this.

[big-andy-coates]

Almost every one of my repos is failing on this check when upgrading com.github.spotbugs gradle plugin from 5.0.6 to 5.0.7. Mainly due to Junit5's assertThrows.

[leviem1]

So I don't even understand why it is reporting this.

Almost every one of my repos is failing on this check when upgrading com.github.spotbugs gradle plugin from 5.0.6 to 5.0.7. Mainly due to Junit5's assertThrows.

Same here, JUnit5's Executable type throws Throwable, which is failing this check. Technically, all assertThrows() or assertDoesNotThrow() lambdas will throw Throwable, since the lambdas implement the Executable interface.

@KengoTODA Re-writing all of my tests to use an anonymous class instead of lambdas is not a reasonable ask. Neither is adding a suppression to all test classes. Plus, I don't think this will actually work in practice without re-wrapping everything in a runtime exception.

@oroszbd I'm not sure how technically feasible this is, but a few suggestions for how to make this work with JUnit:

1. Ignore this for classes inheriting from external sources. You also can't expect a developer to be able to change the external code that they're implementing.
2. Ignore these checks for JUnit or test classes since this entails calling untrusted code through an abstraction similar to Runnable in a system that logs the exceptions for later debugging, and subsequently shuts down the failing subsystem (i.e. JUnit may be covered by ERR08 exceptions).
3. Remove this check for functional interfaces for reasons listed in my previous point, and because these interfaces were not considered in the specification.
4. Remove this check entirely since the standard enforcing this has not been updated since Java 8, which added many new functional features and resulted in side effects such as valid functional use cases for throwing a general exception.

I have not really thought about the ramifications of each of these suggestions, so I understand if any or all of them are not doable or not preferred.

I definitely have to skip this release in all of my projects that use JUnit (i.e. all of my projects) since this update makes this tool very unfeasible for use with JUnit.

(Happy Monday, everyone! 😝)

[iloveeclipse]

@trancexpress : FYI. I think that's what we see too, right?

The detector is coming from #1956

Reading #1956 description we see link to
https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable

which says it is a direct conclusion from

https://wiki.sei.cmu.edu/confluence/display/java/ERR08-J.+Do+not+catch+NullPointerException+or+any+of+its+ancestors

which, in my personal opinion, it a fully academic rule, writen by someone who never maintained complex software with dozens of 3rd party dependencies and lives in an ideal world where every API used in a product can be fixed by the responsible engineers.

Even assumimg I have full source code and ability to recompile every bit of software I use in a product, there are still valid cases where catching or throwing runtime exceptions is necessary (like in lambdas or some generic error handlers). But in reality we use libraries with bugs known for years and never fixed, and not catching NPE's or not re-throwing some generic exceptions is not even possible.

Ideally such detectors shouldn't be enabled by default, or produce low prio/severity warnings, as they rarely find real issues.

[trancexpress]

@trancexpress : FYI. I think that's what we see too, right?

Yes, we have a few occurrences of this when switching to SpotBugs 4.7.0 (up from 4.1.5).

There are a lot more THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION though. Where the reports for Throwable are tens of, the reports for Exception are hundreds of. Likely due to methods in Eclipse such as:

org.osgi.framework.BundleActivator.start(BundleContext)

	/**
	 * Called when this bundle is started so the Framework can perform the
	 * bundle-specific activities necessary to start this bundle. This method
	 * can be used to register services or to allocate any resources that this
	 * bundle needs.
	 * 
	 * <p>
	 * This method must complete and return to its caller in a timely manner.
	 * 
	 * @param context The execution context of the bundle being started.
	 * @throws Exception If this method throws an exception, this bundle is
	 *         marked as stopped and the Framework will remove this bundle's
	 *         listeners, unregister all services registered by this bundle, and
	 *         release all services used by this bundle.
	 */
	public void start(BundleContext context) throws Exception;

Essentially we will add the following filters without even looking into the multiple problems reported by the newer SpotBugs (as there are too many such reported problems to look into individually):

     <Match>
       <Bug pattern="THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION,
         THROWS_METHOD_THROWS_CLAUSE_THROWABLE,
         THROWS_METHOD_THROWS_RUNTIMEEXCEPTION" />
     </Match>

[big-andy-coates]

Personally, when writing unit tests in Java where the code can throw an exception, I always mark the test as throws Exception, rather than anything more specific. This avoids unnecessary noise in the commit history and PRs in the future if any of the methods being called change the exception they are throwing.

IMHO this is preferable in test code and another valid use case that the new THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION check flags up as an issue.

[rovarga]

I think @Bananeweizen 's (and @leviem1 's option one) is the best solution for production code by far:

• if the Throwable/Exception is part of an inherited method contract, callers are expected to handle it correctly
• the original contract declaration site will be flagged without cascading to all implementations of the method
• if it is appropriate to suppress the warning, it can be done at declaration site
• it works for both anonymous classes and lambdas

[moserp]

I have tried to apply this version to our projects (production, not test code) and come up against many of these and THROWS_METHOD_THROWS_RUNTIMEEXCEPTION and THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION.

In some cases they have been valid and allowed me to improve our code however most have been in cases where a 3rd party class/interface has been extended/implemented and adding a suppression annotation would just add a lot of noise to our code.

For me this and the other 2 rules do have utility, but only if they apply to the class/interface adding the exception/throwable to the method signature, not derived instances, otherwise I'm going to have to exclude these rules.

[jrivard]

This also happens on basic code just calling JDK methods, even without lambdas:

void mavenTest4_anonymous() throws Exception
    {
        // this line triggers THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION
        // because JDK Callable declaration has call() method 'throws Exception'
        final Callable<String> callable = new Callable<String>()
        {
            public String call()
            {
                return "test";
            }
        };
        System.out.println( callable.call() );
    }

(the lambda version also triggers the warning)
See https://github.com/jrivard/spotbugs_issue_4 for a reproducible project

While this may or may not be poor design of API of Callable, it's unlikely to change ever. This results in hundreds of false-positives in my project which means I have to exclude this otherwise useful check, so it might as well not even be there. This issue appeared when upgrading to 4.7.0 spotbugs from 4.6.0.

I'd also agree that throwing Exception on test code is desirable, maybe even best practice.

[iloveeclipse]

If the original contributor of the feature #1956 oroszbd will not propose a fix soon, I propose to disable edu.umd.cs.findbugs.detect.ThrowingExceptions detector by default, see #2063.

It is not acceptable for spotbugs to have a detector that produces such high rate of false positives by default.

[baloghadamsoftware]

If the original contributor of the feature #1956 oroszbd will not propose a fix soon, I propose to disable edu.umd.cs.findbugs.detect.ThrowingExceptions detector by default, see #2063.

It is not acceptable for spotbugs to have a detector that produces such high rate of false positives by default.

We tried to reach the author but without success so far. I will look into the code now and try to fix it.

[iloveeclipse]

We tried to reach the author but without success so far. I will look into the code now and try to fix it.

Thanks. In doubt (too complicated / too many false positives / too less time), I would prefer to disable first and work on fixes without any time pressure. But next SpotBugs release should have that thing fixed or disabled.

[big-andy-coates]

If it helps, I've submitted a PR to exclude methods on synthetics classes (#2067). This should fix most of the noise. This means you won't get bug reports on lines that use lambdas to implement methods that are throws Exception or throws Throwable.

However, I'm still in favour of these new checks being off by default. I think there are perfectly valid cases where code should be marked as throws Exception and throws Throwable, like all of the library methods mentioned above. The check for code throwing RuntimeException is also going to be too noisy to be on by default IMHO. (Though I appreciate this may have already been discussed in detail for the original PR and a decision made).

In short, my vote is to merge both #2067 and #2063, so that the check is off by default, and doesn't generate loads of false positives when turned on.

[baloghadamsoftware]

I already made a partial fix but I am still working on the last step which are generics. I hope to submit a PR this week.

[xzel23]

And generally, static analysis prefers false-positive over false-negative, then I think the current SpotBugs implementation itself is OK. It is technically possible to treat AssertJ, you may send a PR to realize. :)

I have the same issue with JUnit, and I bet TestNG and other test frameworks won't be any different.

[garydgregory]

Hi all, would you set expectations as to when this change will be released in 4.7.x?