Skip to content

Commit

Permalink
Make more classes catch Exception instead of RuntimeException eve…
Browse files Browse the repository at this point in the history
…n when only `RuntimeException` is theoretically possible.

This is the followup to cl/587701612. It covers cases in which I thought non-`RuntimeException` exceptions were less likely to happen. I'm not sure that my judgment of that was actually great: Notably, I definitely worry about bad implementations of `Future`, or at least I used to worry about such things years ago.

I hope that this covers the remaining cases. I don't remember offhand how I identified all the sites, though, nor am I certain that I recovered the right snapshot of my earlier work. But this should at least be close to everything. And if my testing from cl/587701612 was correct, we will see no impact on Google's tests from this CL.

If there's anything left to do here, it's probably to add static analysis to _require_ us to always catch the more general `Exception`. But my guess is that that isn't likely to end up being worth the effort.

(I skipped a `catch (RuntimeException e)` block in `MapInterfaceTest` on the grounds that that's the kind of exception that it should be throwing. Of course, we'd like to use something more like `assertThrows`: b/299927833. And probably it should be catching `NullPointerException` specifically....)

RELNOTES=n/a
PiperOrigin-RevId: 591020660
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Dec 14, 2023
1 parent fdd6ead commit 747924e
Show file tree
Hide file tree
Showing 29 changed files with 112 additions and 70 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -58,29 +58,29 @@ private abstract static class PermittedMetaException extends RuntimeException {
static final PermittedMetaException UOE_OR_ISE =
new PermittedMetaException("UnsupportedOperationException or IllegalStateException") {
@Override
boolean isPermitted(RuntimeException exception) {
boolean isPermitted(Exception exception) {
return exception instanceof UnsupportedOperationException
|| exception instanceof IllegalStateException;
}
};
static final PermittedMetaException UOE =
new PermittedMetaException("UnsupportedOperationException") {
@Override
boolean isPermitted(RuntimeException exception) {
boolean isPermitted(Exception exception) {
return exception instanceof UnsupportedOperationException;
}
};
static final PermittedMetaException ISE =
new PermittedMetaException("IllegalStateException") {
@Override
boolean isPermitted(RuntimeException exception) {
boolean isPermitted(Exception exception) {
return exception instanceof IllegalStateException;
}
};
static final PermittedMetaException NSEE =
new PermittedMetaException("NoSuchElementException") {
@Override
boolean isPermitted(RuntimeException exception) {
boolean isPermitted(Exception exception) {
return exception instanceof NoSuchElementException;
}
};
Expand All @@ -89,9 +89,9 @@ private PermittedMetaException(String message) {
super(message);
}

abstract boolean isPermitted(RuntimeException exception);
abstract boolean isPermitted(Exception exception);

void assertPermitted(RuntimeException exception) {
void assertPermitted(Exception exception) {
if (!isPermitted(exception)) {
String message =
"Exception "
Expand Down Expand Up @@ -313,10 +313,11 @@ public enum KnownOrder {
protected void verify(List<E> elements) {}

/** Executes the test. */
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
public final void test() {
try {
recurse(0);
} catch (RuntimeException e) {
} catch (Exception e) { // sneaky checked exception
throw new RuntimeException(Arrays.toString(stimuli), e);
}
}
Expand Down Expand Up @@ -372,16 +373,17 @@ private interface IteratorOperation {
*
* @see Stimulus#executeAndCompare(ListIterator, Iterator)
*/
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
private <T extends Iterator<E>> void internalExecuteAndCompare(
T reference, T target, IteratorOperation method) {
Object referenceReturnValue = null;
PermittedMetaException referenceException = null;
Object targetReturnValue = null;
RuntimeException targetException = null;
Exception targetException = null;

try {
targetReturnValue = method.execute(target);
} catch (RuntimeException e) {
} catch (Exception e) { // sneaky checked exception
targetException = e;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -496,13 +496,14 @@ public FactoryMethodReturnValueTester testEquals() throws Exception {
* @return this tester
*/
@CanIgnoreReturnValue
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
public FactoryMethodReturnValueTester testSerializable() throws Exception {
for (Invokable<?, ?> factory : getFactoriesToTest()) {
Object instance = instantiate(factory);
if (instance != null) {
try {
SerializableTester.reserialize(instance);
} catch (RuntimeException e) {
} catch (Exception e) { // sneaky checked exception
AssertionError error =
new AssertionFailedError("Serialization failed on return value of " + factory);
error.initCause(e.getCause());
Expand All @@ -522,6 +523,7 @@ public FactoryMethodReturnValueTester testSerializable() throws Exception {
* @return this tester
*/
@CanIgnoreReturnValue
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
public FactoryMethodReturnValueTester testEqualsAndSerializable() throws Exception {
for (Invokable<?, ?> factory : getFactoriesToTest()) {
try {
Expand All @@ -533,7 +535,7 @@ public FactoryMethodReturnValueTester testEqualsAndSerializable() throws Excepti
if (instance != null) {
try {
SerializableTester.reserializeAndAssert(instance);
} catch (RuntimeException e) {
} catch (Exception e) { // sneaky checked exception
AssertionError error =
new AssertionFailedError("Serialization failed on return value of " + factory);
error.initCause(e.getCause());
Expand Down
5 changes: 4 additions & 1 deletion android/guava/src/com/google/common/hash/BloomFilter.java
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,7 @@ public void writeTo(OutputStream out) throws IOException {
* @throws IOException if the InputStream throws an {@code IOException}, or if its data does not
* appear to be a BloomFilter serialized using the {@linkplain #writeTo(OutputStream)} method.
*/
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
public static <T extends @Nullable Object> BloomFilter<T> readFrom(
InputStream in, Funnel<? super T> funnel) throws IOException {
checkNotNull(in, "InputStream");
Expand All @@ -554,7 +555,9 @@ public void writeTo(OutputStream out) throws IOException {
}

return new BloomFilter<T>(dataArray, numHashFunctions, funnel, strategy);
} catch (RuntimeException e) {
} catch (IOException e) {
throw e;
} catch (Exception e) { // sneaky checked exception
String message =
"Unable to deserialize BloomFilter from InputStream."
+ " strategyOrdinal: "
Expand Down
3 changes: 2 additions & 1 deletion android/guava/src/com/google/common/reflect/Invokable.java
Original file line number Diff line number Diff line change
Expand Up @@ -116,13 +116,14 @@ public final void setAccessible(boolean flag) {
}

/** See {@link java.lang.reflect.AccessibleObject#trySetAccessible()}. */
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
public final boolean trySetAccessible() {
// We can't call accessibleObject.trySetAccessible since that was added in Java 9 and this code
// should work on Java 8. So we emulate it this way.
try {
accessibleObject.setAccessible(true);
return true;
} catch (RuntimeException e) {
} catch (Exception e) { // sneaky checked exception
return false;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ public final void run() {
+ e.getClass()
+ " without a cause");
}
} catch (RuntimeException | Error e) { // this includes cancellation exception
throwable = e;
} catch (Throwable t) { // this includes CancellationException and sneaky checked exception
throwable = t;
}

if (throwable == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) {

try {
helper = new UnsafeAtomicHelper();
} catch (RuntimeException | Error unsafeFailure) {
} catch (Exception | Error unsafeFailure) { // sneaky checked exception
thrownUnsafeFailure = unsafeFailure;
// catch absolutely everything and fall through to our 'SafeAtomicHelper'
// The access control checks that ARFU does means the caller class has to be AbstractFuture
Expand All @@ -168,7 +168,8 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
newUpdater(AbstractFuture.class, Waiter.class, "waiters"),
newUpdater(AbstractFuture.class, Listener.class, "listeners"),
newUpdater(AbstractFuture.class, Object.class, "value"));
} catch (RuntimeException | Error atomicReferenceFieldUpdaterFailure) {
} catch (Exception // sneaky checked exception
| Error atomicReferenceFieldUpdaterFailure) {
// Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
// getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
// For these users fallback to a suboptimal implementation, based on synchronized. This will
Expand Down Expand Up @@ -874,7 +875,7 @@ protected boolean setFuture(ListenableFuture<? extends V> future) {
Failure failure;
try {
failure = new Failure(t);
} catch (RuntimeException | Error oomMostLikely) {
} catch (Exception | Error oomMostLikely) { // sneaky checked exception
failure = Failure.FALLBACK_INSTANCE;
}
// Note: The only way this CAS could fail is if cancel() has raced with us. That is ok.
Expand Down Expand Up @@ -969,7 +970,7 @@ private static Object getFutureValue(ListenableFuture<?> future) {
cancellation));
}
return new Cancellation(false, cancellation);
} catch (RuntimeException | Error t) {
} catch (Exception | Error t) { // sneaky checked exception
return new Failure(t);
}
}
Expand Down Expand Up @@ -1386,8 +1387,6 @@ public sun.misc.Unsafe run() throws Exception {
UNSAFE = unsafe;
} catch (NoSuchFieldException e) {
throw new RuntimeException(e);
} catch (RuntimeException e) {
throw e;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ abstract class AbstractTransformFuture<
}

@Override
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
public final void run() {
ListenableFuture<? extends I> localInputFuture = inputFuture;
F localFunction = function;
Expand Down Expand Up @@ -104,7 +105,7 @@ public final void run() {
// Set the cause of the exception as this future's exception.
setException(e.getCause());
return;
} catch (RuntimeException e) {
} catch (Exception e) { // sneaky checked exception
// Bug in inputFuture.get(). Propagate to the output Future so that its consumers don't hang.
setException(e);
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ private void collectValueFromNonCancelledFuture(int index, Future<? extends Inpu
collectOneValue(index, getDone(future));
} catch (ExecutionException e) {
handleException(e.getCause());
} catch (RuntimeException | Error t) {
} catch (Throwable t) { // sneaky checked exception
handleException(t);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ abstract class AggregateFutureState<OutputT extends @Nullable Object>
new SafeAtomicHelper(
newUpdater(AggregateFutureState.class, Set.class, "seenExceptions"),
newUpdater(AggregateFutureState.class, "remaining"));
} catch (RuntimeException | Error reflectionFailure) {
} catch (Throwable reflectionFailure) { // sneaky checked exception
// Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
// getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
// For these users fallback to a suboptimal implementation, based on synchronized. This will
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,13 @@ public <T> T newProxy(
}

@Override
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
public void runWithTimeout(Runnable runnable, long timeoutDuration, TimeUnit timeoutUnit) {
checkNotNull(runnable);
checkNotNull(timeoutUnit);
try {
runnable.run();
} catch (RuntimeException e) {
} catch (Exception e) { // sneaky checked exception
throw new UncheckedExecutionException(e);
} catch (Error e) {
throw new ExecutionError(e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ private static boolean hasConstructorUsableByGetChecked(
try {
Exception unused = newWithCause(exceptionClass, new Exception());
return true;
} catch (RuntimeException | Error e) {
} catch (Throwable t) { // sneaky checked exception
return false;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.J2ktIncompatible;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
Expand Down Expand Up @@ -162,10 +161,11 @@ public void addListener(Runnable listener, Executor exec) {
* to return a proper ListenableFuture instead of using listenInPoolThread.
*/
getUninterruptibly(delegate);
} catch (ExecutionException | RuntimeException | Error e) {
// (including CancellationException)
} catch (Throwable t) {
// (including CancellationException and sneaky checked exception)
// The task is presumably done, run the listeners.
// TODO(cpovirk): Do *something* in case of Error (and maybe RuntimeException)?
// TODO(cpovirk): Do *something* in case of Error (and maybe
// non-CancellationException, non-ExecutionException exceptions)?
}
executionList.execute();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -707,7 +707,10 @@ protected String pendingToString() {
* An implementation of {@link ExecutorService#invokeAny} for {@link ListeningExecutorService}
* implementations.
*/
@SuppressWarnings("GoodTime") // should accept a java.time.Duration
@SuppressWarnings({
"GoodTime", // should accept a java.time.Duration
"CatchingUnchecked", // sneaky checked exception
})
@J2ktIncompatible
@GwtIncompatible
@ParametricNullness
Expand Down Expand Up @@ -770,7 +773,9 @@ protected String pendingToString() {
return f.get();
} catch (ExecutionException eex) {
ee = eex;
} catch (RuntimeException rex) {
} catch (InterruptedException iex) {
throw iex;
} catch (Exception rex) { // sneaky checked exception
ee = new ExecutionException(rex);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public void uncaughtException(Thread t, Throwable e) {
SEVERE,
String.format(Locale.ROOT, "Caught an exception in %s. Shutting down.", t),
e);
} catch (RuntimeException | Error errorInLogging) {
} catch (Throwable errorInLogging) { // sneaky checked exception
// If logging fails, e.g. due to missing memory, at least try to log the
// message and the cause for the failed logging.
System.err.println(e.getMessage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,29 +58,29 @@ private abstract static class PermittedMetaException extends RuntimeException {
static final PermittedMetaException UOE_OR_ISE =
new PermittedMetaException("UnsupportedOperationException or IllegalStateException") {
@Override
boolean isPermitted(RuntimeException exception) {
boolean isPermitted(Exception exception) {
return exception instanceof UnsupportedOperationException
|| exception instanceof IllegalStateException;
}
};
static final PermittedMetaException UOE =
new PermittedMetaException("UnsupportedOperationException") {
@Override
boolean isPermitted(RuntimeException exception) {
boolean isPermitted(Exception exception) {
return exception instanceof UnsupportedOperationException;
}
};
static final PermittedMetaException ISE =
new PermittedMetaException("IllegalStateException") {
@Override
boolean isPermitted(RuntimeException exception) {
boolean isPermitted(Exception exception) {
return exception instanceof IllegalStateException;
}
};
static final PermittedMetaException NSEE =
new PermittedMetaException("NoSuchElementException") {
@Override
boolean isPermitted(RuntimeException exception) {
boolean isPermitted(Exception exception) {
return exception instanceof NoSuchElementException;
}
};
Expand All @@ -89,9 +89,9 @@ private PermittedMetaException(String message) {
super(message);
}

abstract boolean isPermitted(RuntimeException exception);
abstract boolean isPermitted(Exception exception);

void assertPermitted(RuntimeException exception) {
void assertPermitted(Exception exception) {
if (!isPermitted(exception)) {
String message =
"Exception "
Expand Down Expand Up @@ -313,10 +313,11 @@ public enum KnownOrder {
protected void verify(List<E> elements) {}

/** Executes the test. */
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
public final void test() {
try {
recurse(0);
} catch (RuntimeException e) {
} catch (Exception e) { // sneaky checked exception
throw new RuntimeException(Arrays.toString(stimuli), e);
}
}
Expand Down Expand Up @@ -388,16 +389,17 @@ private interface IteratorOperation {
*
* @see Stimulus#executeAndCompare(ListIterator, Iterator)
*/
@SuppressWarnings("CatchingUnchecked") // sneaky checked exception
private <T extends Iterator<E>> void internalExecuteAndCompare(
T reference, T target, IteratorOperation method) {
Object referenceReturnValue = null;
PermittedMetaException referenceException = null;
Object targetReturnValue = null;
RuntimeException targetException = null;
Exception targetException = null;

try {
targetReturnValue = method.execute(target);
} catch (RuntimeException e) {
} catch (Exception e) { // sneaky checked exception
targetException = e;
}

Expand Down
Loading

0 comments on commit 747924e

Please sign in to comment.