Skip to content

Commit

Permalink
ThrowSpecificExceptions code finding: Revise finding message and docu…
Browse files Browse the repository at this point in the history
…mentation.

Although I agree with the recommendation, the current wording always gives me pause, because it focuses on a situation that often does not apply when the finding comes up: an expectation for the exception to be caught.  We should not assume that all code is library code, and catching a base exception type with intent to handle a specific exception is a problematic practice that should be detected anyway, so having this finding as a purely defensive practice is questionable.

However, in light of the readability benefit, this finding is justified.  So I've tried to update the finding text to avoid emphasis on the defensive justification, and to provide simple guidance in either case.

My hope is that the finding text is sufficient to remind the reader of the check's justifications (defensive and clarity) and leave thorough explanation to the documentation.

PiperOrigin-RevId: 321970137
  • Loading branch information
java-team-github-bot authored and Error Prone Team committed Jul 18, 2020
1 parent 14d4f3e commit 3dad92a
Showing 1 changed file with 12 additions and 4 deletions.
Expand Up @@ -33,13 +33,21 @@
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.ThrowTree;

/** Bugpattern to discourage throwing base exception classes.. */
/** Bugpattern to discourage throwing base exception classes. */
@BugPattern(
name = "ThrowSpecificExceptions",
summary =
"Consider throwing more specific exceptions rather than (e.g.) RuntimeException. Throwing"
+ " generic exceptions forces any users of the API that wish to handle the failure"
+ " mode to catch very non-specific exceptions that convey little information.",
"Base exception classes should be treated as abstract. If the exception is intended to be"
+ " caught, throw a domain-specific exception. Otherwise, prefer a more specific"
+ " exception for clarity. Common alternatives include: AssertionError,"
+ " IllegalArgumentException, IllegalStateException, and (Guava's) VerifyException.",
explanation =
"1. Defensive coding: Using a generic exception would force a caller that wishes to catch"
+ " it to potentially catch unrelated exceptions as well."
+ "\n\n"
+ "2. Clarity: Base exception classes offer no information on the nature of the"
+ " failure.",

severity = WARNING)
public final class ThrowSpecificExceptions extends BugChecker implements NewClassTreeMatcher {
private static final ImmutableList<AbstractLikeException> ABSTRACT_LIKE_EXCEPTIONS =
Expand Down

0 comments on commit 3dad92a

Please sign in to comment.