Skip to content

Commit 59cfb22

Browse files
cpovirkGoogle Java Core Libraries
cpovirk
authored and
Google Java Core Libraries
committed
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
1 parent d4633f8 commit 59cfb22

8 files changed

Lines changed: 152 additions & 32 deletions

File tree

‎android/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedInputs.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.google.common.annotations.GwtCompatible;
2020
import java.util.concurrent.Future;
21+
import javax.annotation.CheckForNull;
2122

2223
/**
2324
* Classes and futures used in {@link FuturesGetCheckedTest} and {@link FuturesGetUncheckedTest}.
@@ -58,6 +59,47 @@ private ExceptionWithPrivateConstructor(String message, Throwable cause) {
5859
}
5960
}
6061

62+
public static final class ExceptionWithManyConstructorsButOnlyOneThrowable extends Exception {
63+
@CheckForNull private Throwable antecedent;
64+
65+
public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, String a1) {
66+
super(message);
67+
}
68+
69+
public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, String a1, String a2) {
70+
super(message);
71+
}
72+
73+
public ExceptionWithManyConstructorsButOnlyOneThrowable(
74+
String message, String a1, String a2, String a3) {
75+
super(message);
76+
}
77+
78+
public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, Throwable antecedent) {
79+
super(message);
80+
this.antecedent = antecedent;
81+
}
82+
83+
public ExceptionWithManyConstructorsButOnlyOneThrowable(
84+
String message, String a1, String a2, String a3, String a4) {
85+
super(message);
86+
}
87+
88+
public ExceptionWithManyConstructorsButOnlyOneThrowable(
89+
String message, String a1, String a2, String a3, String a4, String a5) {
90+
super(message);
91+
}
92+
93+
public ExceptionWithManyConstructorsButOnlyOneThrowable(
94+
String message, String a1, String a2, String a3, String a4, String a5, String a6) {
95+
super(message);
96+
}
97+
98+
public Throwable getAntecedent() {
99+
return antecedent;
100+
}
101+
}
102+
61103
@SuppressWarnings("unused") // we're testing that they're not used
62104
public static final class ExceptionWithSomePrivateConstructors extends Exception {
63105
private ExceptionWithSomePrivateConstructors(String a) {}

‎android/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,13 @@
3030
import static com.google.common.util.concurrent.FuturesGetCheckedInputs.RUNTIME_EXCEPTION_FUTURE;
3131
import static com.google.common.util.concurrent.FuturesGetCheckedInputs.UNCHECKED_EXCEPTION;
3232
import static java.util.concurrent.TimeUnit.SECONDS;
33+
import static org.junit.Assert.assertThrows;
3334

3435
import com.google.common.testing.GcFinalization;
3536
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithBadConstructor;
3637
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithGoodAndBadConstructor;
3738
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithManyConstructors;
39+
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithManyConstructorsButOnlyOneThrowable;
3840
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithPrivateConstructor;
3941
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithSomePrivateConstructors;
4042
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithWrongTypesConstructor;
@@ -349,6 +351,18 @@ public void testGetCheckedUntimed_exceptionClassUsedInitCause() {
349351
}
350352
}
351353

354+
public void testPrefersConstructorWithThrowableParameter() {
355+
ExceptionWithManyConstructorsButOnlyOneThrowable exception =
356+
assertThrows(
357+
ExceptionWithManyConstructorsButOnlyOneThrowable.class,
358+
() ->
359+
getChecked(
360+
FAILED_FUTURE_CHECKED_EXCEPTION,
361+
ExceptionWithManyConstructorsButOnlyOneThrowable.class));
362+
assertThat(exception).hasMessageThat().contains("mymessage");
363+
assertThat(exception.getAntecedent()).isEqualTo(CHECKED_EXCEPTION);
364+
}
365+
352366
// Class unloading test:
353367

354368
public static final class WillBeUnloadedException extends Exception {}

‎android/guava/src/com/google/common/util/concurrent/Futures.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1167,10 +1167,10 @@ public String toString() {
11671167
*
11681168
* <p>Instances of {@code exceptionClass} are created by choosing an arbitrary public constructor
11691169
* that accepts zero or more arguments, all of type {@code String} or {@code Throwable}
1170-
* (preferring constructors with at least one {@code String}) and calling the constructor via
1171-
* reflection. If the exception did not already have a cause, one is set by calling {@link
1172-
* Throwable#initCause(Throwable)} on it. If no such constructor exists, an {@code
1173-
* IllegalArgumentException} is thrown.
1170+
* (preferring constructors with at least one {@code String}, then preferring constructors with at
1171+
* least one {@code Throwable}) and calling the constructor via reflection. If the exception did
1172+
* not already have a cause, one is set by calling {@link Throwable#initCause(Throwable)} on it.
1173+
* If no such constructor exists, an {@code IllegalArgumentException} is thrown.
11741174
*
11751175
* @throws X if {@code get} throws any checked exception except for an {@code ExecutionException}
11761176
* whose cause is not itself a checked exception
@@ -1219,10 +1219,10 @@ public String toString() {
12191219
*
12201220
* <p>Instances of {@code exceptionClass} are created by choosing an arbitrary public constructor
12211221
* that accepts zero or more arguments, all of type {@code String} or {@code Throwable}
1222-
* (preferring constructors with at least one {@code String}) and calling the constructor via
1223-
* reflection. If the exception did not already have a cause, one is set by calling {@link
1224-
* Throwable#initCause(Throwable)} on it. If no such constructor exists, an {@code
1225-
* IllegalArgumentException} is thrown.
1222+
* (preferring constructors with at least one {@code String}, then preferring constructors with at
1223+
* least one {@code Throwable}) and calling the constructor via reflection. If the exception did
1224+
* not already have a cause, one is set by calling {@link Throwable#initCause(Throwable)} on it.
1225+
* If no such constructor exists, an {@code IllegalArgumentException} is thrown.
12261226
*
12271227
* @throws X if {@code get} throws any checked exception except for an {@code ExecutionException}
12281228
* whose cause is not itself a checked exception

‎android/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import com.google.common.annotations.GwtIncompatible;
2222
import com.google.common.annotations.J2ktIncompatible;
2323
import com.google.common.annotations.VisibleForTesting;
24-
import com.google.common.base.Function;
2524
import com.google.common.collect.Ordering;
2625
import com.google.errorprone.annotations.CanIgnoreReturnValue;
2726
import java.lang.ref.WeakReference;
@@ -193,7 +192,7 @@ private static <X extends Exception> X newWithCause(Class<X> exceptionClass, Thr
193192
// getConstructors() guarantees this as long as we don't modify the array.
194193
@SuppressWarnings({"unchecked", "rawtypes"})
195194
List<Constructor<X>> constructors = (List) Arrays.asList(exceptionClass.getConstructors());
196-
for (Constructor<X> constructor : preferringStrings(constructors)) {
195+
for (Constructor<X> constructor : preferringStringsThenThrowables(constructors)) {
197196
X instance = newFromConstructor(constructor, cause);
198197
if (instance != null) {
199198
if (instance.getCause() == null) {
@@ -209,17 +208,22 @@ private static <X extends Exception> X newWithCause(Class<X> exceptionClass, Thr
209208
cause);
210209
}
211210

212-
private static <X extends Exception> List<Constructor<X>> preferringStrings(
211+
private static <X extends Exception> List<Constructor<X>> preferringStringsThenThrowables(
213212
List<Constructor<X>> constructors) {
214-
return WITH_STRING_PARAM_FIRST.sortedCopy(constructors);
213+
return WITH_STRING_PARAM_THEN_WITH_THROWABLE_PARAM.sortedCopy(constructors);
215214
}
216215

217-
private static final Ordering<Constructor<?>> WITH_STRING_PARAM_FIRST =
216+
// TODO: b/296487962 - Consider defining a total order over constructors.
217+
private static final Ordering<List<Class<?>>> ORDERING_BY_CONSTRUCTOR_PARAMETER_LIST =
218218
Ordering.natural()
219-
.onResultOf(
220-
(Function<Constructor<?>, Boolean>)
221-
input -> asList(input.getParameterTypes()).contains(String.class))
219+
.onResultOf((List<Class<?>> params) -> params.contains(String.class))
220+
.compound(
221+
Ordering.natural()
222+
.onResultOf((List<Class<?>> params) -> params.contains(Throwable.class)))
222223
.reverse();
224+
private static final Ordering<Constructor<?>> WITH_STRING_PARAM_THEN_WITH_THROWABLE_PARAM =
225+
ORDERING_BY_CONSTRUCTOR_PARAMETER_LIST.onResultOf(
226+
constructor -> asList(constructor.getParameterTypes()));
223227

224228
@CheckForNull
225229
private static <X> X newFromConstructor(Constructor<X> constructor, Throwable cause) {

‎guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedInputs.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.google.common.annotations.GwtCompatible;
2020
import java.util.concurrent.Future;
21+
import javax.annotation.CheckForNull;
2122

2223
/**
2324
* Classes and futures used in {@link FuturesGetCheckedTest} and {@link FuturesGetUncheckedTest}.
@@ -58,6 +59,47 @@ private ExceptionWithPrivateConstructor(String message, Throwable cause) {
5859
}
5960
}
6061

62+
public static final class ExceptionWithManyConstructorsButOnlyOneThrowable extends Exception {
63+
@CheckForNull private Throwable antecedent;
64+
65+
public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, String a1) {
66+
super(message);
67+
}
68+
69+
public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, String a1, String a2) {
70+
super(message);
71+
}
72+
73+
public ExceptionWithManyConstructorsButOnlyOneThrowable(
74+
String message, String a1, String a2, String a3) {
75+
super(message);
76+
}
77+
78+
public ExceptionWithManyConstructorsButOnlyOneThrowable(String message, Throwable antecedent) {
79+
super(message);
80+
this.antecedent = antecedent;
81+
}
82+
83+
public ExceptionWithManyConstructorsButOnlyOneThrowable(
84+
String message, String a1, String a2, String a3, String a4) {
85+
super(message);
86+
}
87+
88+
public ExceptionWithManyConstructorsButOnlyOneThrowable(
89+
String message, String a1, String a2, String a3, String a4, String a5) {
90+
super(message);
91+
}
92+
93+
public ExceptionWithManyConstructorsButOnlyOneThrowable(
94+
String message, String a1, String a2, String a3, String a4, String a5, String a6) {
95+
super(message);
96+
}
97+
98+
public Throwable getAntecedent() {
99+
return antecedent;
100+
}
101+
}
102+
61103
@SuppressWarnings("unused") // we're testing that they're not used
62104
public static final class ExceptionWithSomePrivateConstructors extends Exception {
63105
private ExceptionWithSomePrivateConstructors(String a) {}

‎guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,13 @@
3030
import static com.google.common.util.concurrent.FuturesGetCheckedInputs.RUNTIME_EXCEPTION_FUTURE;
3131
import static com.google.common.util.concurrent.FuturesGetCheckedInputs.UNCHECKED_EXCEPTION;
3232
import static java.util.concurrent.TimeUnit.SECONDS;
33+
import static org.junit.Assert.assertThrows;
3334

3435
import com.google.common.testing.GcFinalization;
3536
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithBadConstructor;
3637
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithGoodAndBadConstructor;
3738
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithManyConstructors;
39+
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithManyConstructorsButOnlyOneThrowable;
3840
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithPrivateConstructor;
3941
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithSomePrivateConstructors;
4042
import com.google.common.util.concurrent.FuturesGetCheckedInputs.ExceptionWithWrongTypesConstructor;
@@ -349,6 +351,18 @@ public void testGetCheckedUntimed_exceptionClassUsedInitCause() {
349351
}
350352
}
351353

354+
public void testPrefersConstructorWithThrowableParameter() {
355+
ExceptionWithManyConstructorsButOnlyOneThrowable exception =
356+
assertThrows(
357+
ExceptionWithManyConstructorsButOnlyOneThrowable.class,
358+
() ->
359+
getChecked(
360+
FAILED_FUTURE_CHECKED_EXCEPTION,
361+
ExceptionWithManyConstructorsButOnlyOneThrowable.class));
362+
assertThat(exception).hasMessageThat().contains("mymessage");
363+
assertThat(exception.getAntecedent()).isEqualTo(CHECKED_EXCEPTION);
364+
}
365+
352366
// Class unloading test:
353367

354368
public static final class WillBeUnloadedException extends Exception {}

‎guava/src/com/google/common/util/concurrent/Futures.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1202,10 +1202,10 @@ public String toString() {
12021202
*
12031203
* <p>Instances of {@code exceptionClass} are created by choosing an arbitrary public constructor
12041204
* that accepts zero or more arguments, all of type {@code String} or {@code Throwable}
1205-
* (preferring constructors with at least one {@code String}) and calling the constructor via
1206-
* reflection. If the exception did not already have a cause, one is set by calling {@link
1207-
* Throwable#initCause(Throwable)} on it. If no such constructor exists, an {@code
1208-
* IllegalArgumentException} is thrown.
1205+
* (preferring constructors with at least one {@code String}, then preferring constructors with at
1206+
* least one {@code Throwable}) and calling the constructor via reflection. If the exception did
1207+
* not already have a cause, one is set by calling {@link Throwable#initCause(Throwable)} on it.
1208+
* If no such constructor exists, an {@code IllegalArgumentException} is thrown.
12091209
*
12101210
* @throws X if {@code get} throws any checked exception except for an {@code ExecutionException}
12111211
* whose cause is not itself a checked exception
@@ -1254,10 +1254,10 @@ public String toString() {
12541254
*
12551255
* <p>Instances of {@code exceptionClass} are created by choosing an arbitrary public constructor
12561256
* that accepts zero or more arguments, all of type {@code String} or {@code Throwable}
1257-
* (preferring constructors with at least one {@code String}) and calling the constructor via
1258-
* reflection. If the exception did not already have a cause, one is set by calling {@link
1259-
* Throwable#initCause(Throwable)} on it. If no such constructor exists, an {@code
1260-
* IllegalArgumentException} is thrown.
1257+
* (preferring constructors with at least one {@code String}, then preferring constructors with at
1258+
* least one {@code Throwable}) and calling the constructor via reflection. If the exception did
1259+
* not already have a cause, one is set by calling {@link Throwable#initCause(Throwable)} on it.
1260+
* If no such constructor exists, an {@code IllegalArgumentException} is thrown.
12611261
*
12621262
* @throws X if {@code get} throws any checked exception except for an {@code ExecutionException}
12631263
* whose cause is not itself a checked exception

‎guava/src/com/google/common/util/concurrent/FuturesGetChecked.java‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import com.google.common.annotations.GwtIncompatible;
2222
import com.google.common.annotations.J2ktIncompatible;
2323
import com.google.common.annotations.VisibleForTesting;
24-
import com.google.common.base.Function;
2524
import com.google.common.collect.Ordering;
2625
import com.google.errorprone.annotations.CanIgnoreReturnValue;
2726
import com.google.j2objc.annotations.J2ObjCIncompatible;
@@ -234,7 +233,7 @@ private static <X extends Exception> X newWithCause(Class<X> exceptionClass, Thr
234233
// getConstructors() guarantees this as long as we don't modify the array.
235234
@SuppressWarnings({"unchecked", "rawtypes"})
236235
List<Constructor<X>> constructors = (List) Arrays.asList(exceptionClass.getConstructors());
237-
for (Constructor<X> constructor : preferringStrings(constructors)) {
236+
for (Constructor<X> constructor : preferringStringsThenThrowables(constructors)) {
238237
X instance = newFromConstructor(constructor, cause);
239238
if (instance != null) {
240239
if (instance.getCause() == null) {
@@ -250,17 +249,22 @@ private static <X extends Exception> X newWithCause(Class<X> exceptionClass, Thr
250249
cause);
251250
}
252251

253-
private static <X extends Exception> List<Constructor<X>> preferringStrings(
252+
private static <X extends Exception> List<Constructor<X>> preferringStringsThenThrowables(
254253
List<Constructor<X>> constructors) {
255-
return WITH_STRING_PARAM_FIRST.sortedCopy(constructors);
254+
return WITH_STRING_PARAM_THEN_WITH_THROWABLE_PARAM.sortedCopy(constructors);
256255
}
257256

258-
private static final Ordering<Constructor<?>> WITH_STRING_PARAM_FIRST =
257+
// TODO: b/296487962 - Consider defining a total order over constructors.
258+
private static final Ordering<List<Class<?>>> ORDERING_BY_CONSTRUCTOR_PARAMETER_LIST =
259259
Ordering.natural()
260-
.onResultOf(
261-
(Function<Constructor<?>, Boolean>)
262-
input -> asList(input.getParameterTypes()).contains(String.class))
260+
.onResultOf((List<Class<?>> params) -> params.contains(String.class))
261+
.compound(
262+
Ordering.natural()
263+
.onResultOf((List<Class<?>> params) -> params.contains(Throwable.class)))
263264
.reverse();
265+
private static final Ordering<Constructor<?>> WITH_STRING_PARAM_THEN_WITH_THROWABLE_PARAM =
266+
ORDERING_BY_CONSTRUCTOR_PARAMETER_LIST.onResultOf(
267+
constructor -> asList(constructor.getParameterTypes()));
264268

265269
@CheckForNull
266270
private static <X> X newFromConstructor(Constructor<X> constructor, Throwable cause) {

0 commit comments

Comments
 (0)