Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional no match cases for EmptyCatch #1654

Closed
ben-manes opened this issue May 31, 2020 · 3 comments
Closed

Additional no match cases for EmptyCatch #1654

ben-manes opened this issue May 31, 2020 · 3 comments

Comments

@ben-manes
Copy link

ben-manes commented May 31, 2020

Description of the problem / feature request:

EmptyCatch will skip matching when either (1) a comment or (2) a junit test is detected. It would be nice to extend this to a few more common patterns.

Feature requests: what underlying problem are you trying to solve with this feature?

  1. If the exception is named ignored or expected. I believe this used to be part of the old Google Java style, which can be seen in Guava's JUnit tests. This would also match the behavior of PMD's EmptyCatchBlock rule.
  2. Include detection of TestNG as skippable.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

// Static initializer fallback from Unsafe => MethodHandles => ThreadLocal
StripedBuffer.java:290: warning: [EmptyCatch] Caught exceptions should not be ignored
      } catch (Throwable ignored) {}
// TestNG unit test, e.g. assert NPE is thrown
CaffeineTest.java:133: warning: [EmptyCatch] Caught exceptions should not be ignored
    } catch (NullPointerException expected) {}

What version of Error Prone are you using?

2.4.0

copybara-service bot pushed a commit that referenced this issue Jul 19, 2020
…ing up expectation.

Exempting expected, ignored and ok parameter names.

https://google.github.io/styleguide/javaguide.html#s6.2-caught-exceptions

Inverse flume results (matches that won't be flagged now) : unknown commit

Fixes #1654

PiperOrigin-RevId: 322052035
@sumitbhagwani
Copy link
Contributor

sumitbhagwani commented Jul 20, 2020

Looking at the style guide : https://google.github.io/styleguide/javaguide.html#s6.2-caught-exceptions

either the empty catch block should've comments with "exception" being test code where the exception name ok/expected/ignored etc are allowed.

EmptyCatch check (at HEAD/master as of July 2020) doesn't not flag JUnit test code.

So for feature requests :

  1. such code should've comments - irrespective of exception param name
  2. I think check should be extended for TestNG

@sumitbhagwani
Copy link
Contributor

sumitbhagwani commented Jul 22, 2020

#1730 should fix this issue

@ben-manes
Copy link
Author

ben-manes commented Jul 22, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants