From 59cfb2267aa0d8538b00874328943082186857d0 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Wed, 6 Sep 2023 06:44:30 -0700 Subject: [PATCH] Make `Futures.getChecked` prefer constructors with a `Throwable` parameter. It still prefers constructors with a `String` parameter over anything else, but to break ties among the constructors that meet that criterion, it now looks for a `Throwable` parameter, too. This helps with exception classes that accept a `Throwable` and do something with it beyond setting it as the `cause`. RELNOTES=`util.concurrent`: `Futures.getChecked`, which continues to prefer to call constructors with a `String` parameter, now breaks ties based on whether the constructor has a `Throwable` parameter. Beyond that, the choice of constructor remains undefined. (For this and other reasons, we discourage the use of `getChecked`.) PiperOrigin-RevId: 563089337 --- .../concurrent/FuturesGetCheckedInputs.java | 42 +++++++++++++++++++ .../concurrent/FuturesGetCheckedTest.java | 14 +++++++ .../common/util/concurrent/Futures.java | 16 +++---- .../util/concurrent/FuturesGetChecked.java | 20 +++++---- .../concurrent/FuturesGetCheckedInputs.java | 42 +++++++++++++++++++ .../concurrent/FuturesGetCheckedTest.java | 14 +++++++ .../common/util/concurrent/Futures.java | 16 +++---- .../util/concurrent/FuturesGetChecked.java | 20 +++++---- 8 files changed, 152 insertions(+), 32 deletions(-) diff --git a/android/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedInputs.java b/android/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedInputs.java index 27916d8a1005..0ae248837af5 100644 --- a/android/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedInputs.java +++ b/android/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedInputs.java @@ -18,6 +18,7 @@ import com.google.common.annotations.GwtCompatible; import java.util.concurrent.Future; +import javax.annotation.CheckForNull; /** * Classes and futures used in {@link FuturesGetCheckedTest} and {@link FuturesGetUncheckedTest}. @@ -58,6 +59,47 @@ private ExceptionWithPrivateConstructor(String message, Throwable cause) { } } + public static final class ExceptionWithManyConstructorsButOnlyOneThrowable extends Exception { + @CheckForNull private Throwable antecedent; + + public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, String a1) { + super(message); + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, String a1, String a2) { + super(message); + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable( + String message, String a1, String a2, String a3) { + super(message); + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, Throwable antecedent) { + super(message); + this.antecedent = antecedent; + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable( + String message, String a1, String a2, String a3, String a4) { + super(message); + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable( + String message, String a1, String a2, String a3, String a4, String a5) { + super(message); + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable( + String message, String a1, String a2, String a3, String a4, String a5, String a6) { + super(message); + } + + public Throwable getAntecedent() { + return antecedent; + } + } + @SuppressWarnings("unused") // we're testing that they're not used public static final class ExceptionWithSomePrivateConstructors extends Exception { private ExceptionWithSomePrivateConstructors(String a) {} diff --git a/android/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java b/android/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java index 666a1892982f..5479b19a304b 100644 --- a/android/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java +++ b/android/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java @@ -30,11 +30,13 @@ import static com.google.common.util.concurrent.FuturesGetCheckedInputs.RUNTIME_EXCEPTION_FUTURE; import static com.google.common.util.concurrent.FuturesGetCheckedInputs.UNCHECKED_EXCEPTION; import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.Assert.assertThrows; import com.google.common.testing.GcFinalization; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithBadConstructor; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithGoodAndBadConstructor; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithManyConstructors; +import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithManyConstructorsButOnlyOneThrowable; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithPrivateConstructor; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithSomePrivateConstructors; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithWrongTypesConstructor; @@ -349,6 +351,18 @@ public void testGetCheckedUntimed_exceptionClassUsedInitCause() { } } + public void testPrefersConstructorWithThrowableParameter() { + ExceptionWithManyConstructorsButOnlyOneThrowable exception = + assertThrows( + ExceptionWithManyConstructorsButOnlyOneThrowable.class, + () -> + getChecked( + FAILED_FUTURE_CHECKED_EXCEPTION, + ExceptionWithManyConstructorsButOnlyOneThrowable.class)); + assertThat(exception).hasMessageThat().contains("mymessage"); + assertThat(exception.getAntecedent()).isEqualTo(CHECKED_EXCEPTION); + } + // Class unloading test: public static final class WillBeUnloadedException extends Exception {} diff --git a/android/guava/src/com/google/common/util/concurrent/Futures.java b/android/guava/src/com/google/common/util/concurrent/Futures.java index 39b6dfeb7677..e36ff391809c 100644 --- a/android/guava/src/com/google/common/util/concurrent/Futures.java +++ b/android/guava/src/com/google/common/util/concurrent/Futures.java @@ -1167,10 +1167,10 @@ public String toString() { * *

Instances of {@code exceptionClass} are created by choosing an arbitrary public constructor * that accepts zero or more arguments, all of type {@code String} or {@code Throwable} - * (preferring constructors with at least one {@code String}) and calling the constructor via - * reflection. If the exception did not already have a cause, one is set by calling {@link - * Throwable#initCause(Throwable)} on it. If no such constructor exists, an {@code - * IllegalArgumentException} is thrown. + * (preferring constructors with at least one {@code String}, then preferring constructors with at + * least one {@code Throwable}) and calling the constructor via reflection. If the exception did + * not already have a cause, one is set by calling {@link Throwable#initCause(Throwable)} on it. + * If no such constructor exists, an {@code IllegalArgumentException} is thrown. * * @throws X if {@code get} throws any checked exception except for an {@code ExecutionException} * whose cause is not itself a checked exception @@ -1219,10 +1219,10 @@ public String toString() { * *

Instances of {@code exceptionClass} are created by choosing an arbitrary public constructor * that accepts zero or more arguments, all of type {@code String} or {@code Throwable} - * (preferring constructors with at least one {@code String}) and calling the constructor via - * reflection. If the exception did not already have a cause, one is set by calling {@link - * Throwable#initCause(Throwable)} on it. If no such constructor exists, an {@code - * IllegalArgumentException} is thrown. + * (preferring constructors with at least one {@code String}, then preferring constructors with at + * least one {@code Throwable}) and calling the constructor via reflection. If the exception did + * not already have a cause, one is set by calling {@link Throwable#initCause(Throwable)} on it. + * If no such constructor exists, an {@code IllegalArgumentException} is thrown. * * @throws X if {@code get} throws any checked exception except for an {@code ExecutionException} * whose cause is not itself a checked exception diff --git a/android/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java b/android/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java index 9be9e21e8b33..cc9c3cb48512 100644 --- a/android/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java +++ b/android/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java @@ -21,7 +21,6 @@ import com.google.common.annotations.GwtIncompatible; import com.google.common.annotations.J2ktIncompatible; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Function; import com.google.common.collect.Ordering; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.lang.ref.WeakReference; @@ -193,7 +192,7 @@ private static X newWithCause(Class exceptionClass, Thr // getConstructors() guarantees this as long as we don't modify the array. @SuppressWarnings({"unchecked", "rawtypes"}) List> constructors = (List) Arrays.asList(exceptionClass.getConstructors()); - for (Constructor constructor : preferringStrings(constructors)) { + for (Constructor constructor : preferringStringsThenThrowables(constructors)) { X instance = newFromConstructor(constructor, cause); if (instance != null) { if (instance.getCause() == null) { @@ -209,17 +208,22 @@ private static X newWithCause(Class exceptionClass, Thr cause); } - private static List> preferringStrings( + private static List> preferringStringsThenThrowables( List> constructors) { - return WITH_STRING_PARAM_FIRST.sortedCopy(constructors); + return WITH_STRING_PARAM_THEN_WITH_THROWABLE_PARAM.sortedCopy(constructors); } - private static final Ordering> WITH_STRING_PARAM_FIRST = + // TODO: b/296487962 - Consider defining a total order over constructors. + private static final Ordering>> ORDERING_BY_CONSTRUCTOR_PARAMETER_LIST = Ordering.natural() - .onResultOf( - (Function, Boolean>) - input -> asList(input.getParameterTypes()).contains(String.class)) + .onResultOf((List> params) -> params.contains(String.class)) + .compound( + Ordering.natural() + .onResultOf((List> params) -> params.contains(Throwable.class))) .reverse(); + private static final Ordering> WITH_STRING_PARAM_THEN_WITH_THROWABLE_PARAM = + ORDERING_BY_CONSTRUCTOR_PARAMETER_LIST.onResultOf( + constructor -> asList(constructor.getParameterTypes())); @CheckForNull private static X newFromConstructor(Constructor constructor, Throwable cause) { diff --git a/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedInputs.java b/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedInputs.java index 27916d8a1005..0ae248837af5 100644 --- a/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedInputs.java +++ b/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedInputs.java @@ -18,6 +18,7 @@ import com.google.common.annotations.GwtCompatible; import java.util.concurrent.Future; +import javax.annotation.CheckForNull; /** * Classes and futures used in {@link FuturesGetCheckedTest} and {@link FuturesGetUncheckedTest}. @@ -58,6 +59,47 @@ private ExceptionWithPrivateConstructor(String message, Throwable cause) { } } + public static final class ExceptionWithManyConstructorsButOnlyOneThrowable extends Exception { + @CheckForNull private Throwable antecedent; + + public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, String a1) { + super(message); + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, String a1, String a2) { + super(message); + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable( + String message, String a1, String a2, String a3) { + super(message); + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, Throwable antecedent) { + super(message); + this.antecedent = antecedent; + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable( + String message, String a1, String a2, String a3, String a4) { + super(message); + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable( + String message, String a1, String a2, String a3, String a4, String a5) { + super(message); + } + + public ExceptionWithManyConstructorsButOnlyOneThrowable( + String message, String a1, String a2, String a3, String a4, String a5, String a6) { + super(message); + } + + public Throwable getAntecedent() { + return antecedent; + } + } + @SuppressWarnings("unused") // we're testing that they're not used public static final class ExceptionWithSomePrivateConstructors extends Exception { private ExceptionWithSomePrivateConstructors(String a) {} diff --git a/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java b/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java index 666a1892982f..5479b19a304b 100644 --- a/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java +++ b/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java @@ -30,11 +30,13 @@ import static com.google.common.util.concurrent.FuturesGetCheckedInputs.RUNTIME_EXCEPTION_FUTURE; import static com.google.common.util.concurrent.FuturesGetCheckedInputs.UNCHECKED_EXCEPTION; import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.Assert.assertThrows; import com.google.common.testing.GcFinalization; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithBadConstructor; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithGoodAndBadConstructor; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithManyConstructors; +import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithManyConstructorsButOnlyOneThrowable; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithPrivateConstructor; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithSomePrivateConstructors; import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithWrongTypesConstructor; @@ -349,6 +351,18 @@ public void testGetCheckedUntimed_exceptionClassUsedInitCause() { } } + public void testPrefersConstructorWithThrowableParameter() { + ExceptionWithManyConstructorsButOnlyOneThrowable exception = + assertThrows( + ExceptionWithManyConstructorsButOnlyOneThrowable.class, + () -> + getChecked( + FAILED_FUTURE_CHECKED_EXCEPTION, + ExceptionWithManyConstructorsButOnlyOneThrowable.class)); + assertThat(exception).hasMessageThat().contains("mymessage"); + assertThat(exception.getAntecedent()).isEqualTo(CHECKED_EXCEPTION); + } + // Class unloading test: public static final class WillBeUnloadedException extends Exception {} diff --git a/guava/src/com/google/common/util/concurrent/Futures.java b/guava/src/com/google/common/util/concurrent/Futures.java index 31889609a773..c073d1d20b09 100644 --- a/guava/src/com/google/common/util/concurrent/Futures.java +++ b/guava/src/com/google/common/util/concurrent/Futures.java @@ -1202,10 +1202,10 @@ public String toString() { * *

Instances of {@code exceptionClass} are created by choosing an arbitrary public constructor * that accepts zero or more arguments, all of type {@code String} or {@code Throwable} - * (preferring constructors with at least one {@code String}) and calling the constructor via - * reflection. If the exception did not already have a cause, one is set by calling {@link - * Throwable#initCause(Throwable)} on it. If no such constructor exists, an {@code - * IllegalArgumentException} is thrown. + * (preferring constructors with at least one {@code String}, then preferring constructors with at + * least one {@code Throwable}) and calling the constructor via reflection. If the exception did + * not already have a cause, one is set by calling {@link Throwable#initCause(Throwable)} on it. + * If no such constructor exists, an {@code IllegalArgumentException} is thrown. * * @throws X if {@code get} throws any checked exception except for an {@code ExecutionException} * whose cause is not itself a checked exception @@ -1254,10 +1254,10 @@ public String toString() { * *

Instances of {@code exceptionClass} are created by choosing an arbitrary public constructor * that accepts zero or more arguments, all of type {@code String} or {@code Throwable} - * (preferring constructors with at least one {@code String}) and calling the constructor via - * reflection. If the exception did not already have a cause, one is set by calling {@link - * Throwable#initCause(Throwable)} on it. If no such constructor exists, an {@code - * IllegalArgumentException} is thrown. + * (preferring constructors with at least one {@code String}, then preferring constructors with at + * least one {@code Throwable}) and calling the constructor via reflection. If the exception did + * not already have a cause, one is set by calling {@link Throwable#initCause(Throwable)} on it. + * If no such constructor exists, an {@code IllegalArgumentException} is thrown. * * @throws X if {@code get} throws any checked exception except for an {@code ExecutionException} * whose cause is not itself a checked exception diff --git a/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java b/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java index c512d82ee37f..425e7d5559cf 100644 --- a/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java +++ b/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java @@ -21,7 +21,6 @@ import com.google.common.annotations.GwtIncompatible; import com.google.common.annotations.J2ktIncompatible; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Function; import com.google.common.collect.Ordering; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.j2objc.annotations.J2ObjCIncompatible; @@ -234,7 +233,7 @@ private static X newWithCause(Class exceptionClass, Thr // getConstructors() guarantees this as long as we don't modify the array. @SuppressWarnings({"unchecked", "rawtypes"}) List> constructors = (List) Arrays.asList(exceptionClass.getConstructors()); - for (Constructor constructor : preferringStrings(constructors)) { + for (Constructor constructor : preferringStringsThenThrowables(constructors)) { X instance = newFromConstructor(constructor, cause); if (instance != null) { if (instance.getCause() == null) { @@ -250,17 +249,22 @@ private static X newWithCause(Class exceptionClass, Thr cause); } - private static List> preferringStrings( + private static List> preferringStringsThenThrowables( List> constructors) { - return WITH_STRING_PARAM_FIRST.sortedCopy(constructors); + return WITH_STRING_PARAM_THEN_WITH_THROWABLE_PARAM.sortedCopy(constructors); } - private static final Ordering> WITH_STRING_PARAM_FIRST = + // TODO: b/296487962 - Consider defining a total order over constructors. + private static final Ordering>> ORDERING_BY_CONSTRUCTOR_PARAMETER_LIST = Ordering.natural() - .onResultOf( - (Function, Boolean>) - input -> asList(input.getParameterTypes()).contains(String.class)) + .onResultOf((List> params) -> params.contains(String.class)) + .compound( + Ordering.natural() + .onResultOf((List> params) -> params.contains(Throwable.class))) .reverse(); + private static final Ordering> WITH_STRING_PARAM_THEN_WITH_THROWABLE_PARAM = + ORDERING_BY_CONSTRUCTOR_PARAMETER_LIST.onResultOf( + constructor -> asList(constructor.getParameterTypes())); @CheckForNull private static X newFromConstructor(Constructor constructor, Throwable cause) {