error-prone icon indicating copy to clipboard operation
error-prone copied to clipboard

CheckedExceptionNotThrown doesn't work

Open delanym opened this issue 2 years ago • 9 comments

This rule simply doesn't work. EP v2.16 I tried it by adding an IOException and a custom checked exception.

delanym avatar Feb 09 '23 11:02 delanym

@delanym the latest release is 2.18.0. For me the rule does work, and there are tests. Are you looking for CheckedExceptionNotThrown instead?

If not, please provide a minimal reproducible example.

Stephan202 avatar Feb 10 '23 09:02 Stephan202

@Stephan202 ok I tried to update to 2.17.0 or 2.18.0 but now for some reason the ImmutableEnumChecker rule is being activated (I didn't ask for it) and its failing

delanym avatar Feb 10 '23 09:02 delanym

Has some other rule changed to depend on this rule perhaps?

delanym avatar Feb 10 '23 09:02 delanym

@delanym asking this kind of open-ended question without providing details is not helpful. You'll get better support if you spend time providing a minimal reproducible example.

Stephan202 avatar Feb 10 '23 12:02 Stephan202

@tbroyer Having a bad day?

I see what's going on. I had the "Immutable" rule set using error-prone 2.16, which only became a valid alias for "ImmutableEnumChecker" in 2.17. Since invalid pattern names cause the build to fail, the Immutable pattern must be present but inactive in 2.16. That's my guess anyway.

I've had similar issues in the past https://github.com/google/error-prone/issues/976 The https://errorprone.info/bugpatterns page makes no attempt to differentiate versions so no surprises there. There should at least be a version number on the page. Thanks for your efforts!

@Stephan202 yes it is CheckedExceptionNotThrown rather than InvalidThrows. I have that set as well, but its not working on my large closed source project. My simplistic attempt at a reproducer did not prove anything, as I expected.

Its at times like these when I'm really just giving you a hint that there's a potential problem with the rule, and if you, the maintainer, the next person searching through issues — you get the sense that something might be wrong and you can be extra vigilant around that code. I simply don't have time to create complex reproducers for every issue I find, but ill reply to this ticket if the situation changes.

delanym avatar Feb 10 '23 19:02 delanym

I see there is a separate checker called https://errorprone.info/bugpattern/Immutable

Please sort out the docs

image

delanym avatar Feb 11 '23 08:02 delanym

My simplistic attempt at a reproducer did not prove anything, as I expected. Its at times like these when I'm really just giving you a hint that there's a potential problem with the rule, and if you, the maintainer, the next person searching through issues — you get the sense that something might be wrong and you can be extra vigilant around that code. I simply don't have time to create complex reproducers for every issue I find, but ill reply to this ticket if the situation changes.

Thanks for the heads up. CheckedExceptionNotThrown works for us, but I'll keep an eye out for issues with it. Please do update the bug if you're able to reproduce the issue you're seeing.

cushon avatar Feb 11 '23 21:02 cushon

Here's a reproducer https://github.com/delanym/checkedexceptionnotthrown

delanym avatar Feb 11 '23 22:02 delanym

Thanks for the repro.

The check doesn't currently complain about methods that can be overridden, since overrides of those methods may throw checked exceptions and it isn't correct to remove the throws clause without analyzing the overrides:

https://github.com/google/error-prone/blob/b80fcaf00a7bd0702f78a536b84ac0fa1407d657/core/src/main/java/com/google/errorprone/bugpatterns/CheckedExceptionNotThrown.java#L78

You can see an error in your repro if the method is modified to be final:

diff --git a/src/main/java/com/xml/xsd/XsdTraversalHandler.java b/src/main/java/com/xml/xsd/XsdTraversalHandler.java
index b25e6e9..5d83c7a 100644
--- a/src/main/java/com/xml/xsd/XsdTraversalHandler.java
+++ b/src/main/java/com/xml/xsd/XsdTraversalHandler.java
@@ -6,7 +6,7 @@ public class XsdTraversalHandler {
     throw new XsdTraversalHandlerException();
   }

-  public void out() throws XsdTraversalHandlerException {
+  public final void out() throws XsdTraversalHandlerException {

     // dont throw new XsdTraversalHandlerException();
     System.out.println("But do create bytecode");
[ERROR] /usr/local/google/home/cushon/src/checkedexceptionnotthrown/src/main/java/com/xml/xsd/XsdTraversalHandler.java:[9,34] [CheckedExceptionNotThrown] This method does not throw checked exceptions (XsdTraversalHandlerException) despite claiming to. This may cause consumers of the API to incorrectly attempt to handle, or propagate, this exception.
[ERROR]     (see https://errorprone.info/bugpattern/CheckedExceptionNotThrown)
[ERROR]   Did you mean 'public final void out()  {'?

cushon avatar Feb 11 '23 23:02 cushon