Closed panic08 closed 1 month ago
I wanted to make sure that I came back and said something here because this PR prompted a bunch of activity :)
throwIfUnchecked(e.getCause())
, it had throwIfUnchecked(e)
.
InvocationTargetException
case) because our build runs https://errorprone.info/bugpattern/ThrowIfUncheckedKnownChecked. (Sadly, running our build is still slightly more complex than it should be, but ./mvn clean install
should normally do the trick.)UncheckedExecutionException
case) because Error Prone doesn't have a checker for a known unchecked exception. (There would probably have been a test error, at least.) I'm proposing adding such a checker in https://github.com/google/error-prone/pull/4625.SubscriberRegistry
was rethrowing an exception that could (in rare cases) come from another thread. This can be confusing, since the stack trace in that thread doesn't necessarily match the stack trace in the thread that eventually catches the exception. I ended up changing that in https://github.com/google/guava/pull/7449, and I just sent up a followup in https://github.com/google/guava/pull/7453.MoreExecutors
was more reasonable, but I've come to view "throw new RuntimeException(e)
" as a mild code smell: While it's very rarely actually important to throw a specific subclass of RuntimeException
instead, "throw new RuntimeException(e)
" has historically sometimes indicated that the author just didn't want to think about what might have caused the exception or what should actually be done about it. We ended up adding a comment and using UndeclaredThrowableException
, and then I made similar changes in some other files in https://github.com/google/guava/pull/7451, plus https://github.com/google/truth/pull/1353 in lieu of a similar change elsewhere (which then tipped me off to search for some other Java 7 / Android updates in https://github.com/google/guava/pull/7446 / https://github.com/google/truth/pull/1352).
The suggestion here is to change the use of the deprecated
Throwables.propagate()
in theSubscriberRegistry.java
,MoreExecutors.java
classes to the alternative that is recommended in the documentation for this method: