diff options
author | cpovirk <cpovirk@google.com> | 2021-08-19 09:41:17 -0700 |
---|---|---|
committer | Google Java Core Libraries <java-core-libraries-team+copybara@google.com> | 2021-08-19 09:44:03 -0700 |
commit | fb109b051c9ec48d5e77bf73b33a228ab5d57462 (patch) | |
tree | 5bf73cd03fa4f922b68dd832057621807c5bffca | |
parent | c7d9fef73b9314581bf4acd0d97b12178935eb11 (diff) | |
download | guava-fb109b051c9ec48d5e77bf73b33a228ab5d57462.tar.gz |
Remove `ClassValue` implementation of `Futures.getChecked` from the Android flavor.
It doesn't work there, even under new versions of Android, so it always triggers fallback, and the process of falling back burns resources and produces noise.
(JRE users of guava-android could benefit from the `ClassValue` implementation, but now that we're dropping support for Java 7 from guava-android, there's no reason for them to use it. OK, probably some users run _Robolectric_ tests against guava-android. But presumably they aren't _too_ sensitive to the performance of `getChecked` in the failure case.)
Removing `ClassValue` support also lets us remove a bunch of Animal-Sniffer cruft. That includes upgrading it to 1.19, which in turn forces us to remove some usages of new APIs from our _tests_ -- not nearly as important as usages in _prod_ but still a good idea so that any future Android test failures are reported to us correctly instead of hidden by `NoSuchMethodError` or similar. Or actually, Animal-Sniffer 1.20 [turned checking of test sources back off](https://github.com/mojohaus/animal-sniffer/commit/ac40ac46b3e02f0edf986bb43be7a45cc5382937), but let's opt in anyway, at least to see how it goes, at least for the tests of guava-testlib. (guava-tests itself is more of a pain, so I left a TODO.)
RELNOTES=n/a
PiperOrigin-RevId: 391779058
14 files changed, 36 insertions, 135 deletions
diff --git a/android/guava-testlib/test/com/google/common/collect/testing/MapTestSuiteBuilderTests.java b/android/guava-testlib/test/com/google/common/collect/testing/MapTestSuiteBuilderTests.java index c23d0524f..a4d216d15 100644 --- a/android/guava-testlib/test/com/google/common/collect/testing/MapTestSuiteBuilderTests.java +++ b/android/guava-testlib/test/com/google/common/collect/testing/MapTestSuiteBuilderTests.java @@ -302,8 +302,8 @@ public final class MapTestSuiteBuilderTests extends TestCase { return method.invoke(map, args); } catch (InvocationTargetException e) { throw e.getCause(); - } catch (ReflectiveOperationException e) { - throw new LinkageError(e.getMessage(), e); + } catch (IllegalAccessException e) { + throw newLinkageError(e); } } } @@ -338,4 +338,10 @@ public final class MapTestSuiteBuilderTests extends TestCase { .withTearDown(tearDown) .createTestSuite(); } + + private static LinkageError newLinkageError(Throwable cause) { + LinkageError error = new LinkageError(cause.toString()); + error.initCause(cause); + return error; + } } diff --git a/android/guava-tests/benchmark/com/google/common/util/concurrent/FuturesGetCheckedBenchmark.java b/android/guava-tests/benchmark/com/google/common/util/concurrent/FuturesGetCheckedBenchmark.java index dd1883bcb..bb764203f 100644 --- a/android/guava-tests/benchmark/com/google/common/util/concurrent/FuturesGetCheckedBenchmark.java +++ b/android/guava-tests/benchmark/com/google/common/util/concurrent/FuturesGetCheckedBenchmark.java @@ -21,7 +21,6 @@ import static com.google.common.collect.Lists.newArrayList; import static com.google.common.util.concurrent.Futures.immediateFailedFuture; import static com.google.common.util.concurrent.Futures.immediateFuture; import static com.google.common.util.concurrent.FuturesGetChecked.checkExceptionClassValidity; -import static com.google.common.util.concurrent.FuturesGetChecked.classValueValidator; import static com.google.common.util.concurrent.FuturesGetChecked.getChecked; import static com.google.common.util.concurrent.FuturesGetChecked.isCheckedException; import static com.google.common.util.concurrent.FuturesGetChecked.weakSetValidator; @@ -52,7 +51,7 @@ public class FuturesGetCheckedBenchmark { NON_CACHING_WITH_CONSTRUCTOR_CHECK(nonCachingWithConstructorCheckValidator()), NON_CACHING_WITHOUT_CONSTRUCTOR_CHECK(nonCachingWithoutConstructorCheckValidator()), WEAK_SET(weakSetValidator()), - CLASS_VALUE(classValueValidator()); + ; final GetCheckedTypeValidator validator; diff --git a/android/guava-tests/pom.xml b/android/guava-tests/pom.xml index 5f56f4b3a..b5923d73c 100644 --- a/android/guava-tests/pom.xml +++ b/android/guava-tests/pom.xml @@ -102,6 +102,9 @@ <plugin> <groupId>org.codehaus.mojo</groupId> <artifactId>animal-sniffer-maven-plugin</artifactId> + <configuration> + <checkTestClasses>false</checkTestClasses> <!-- TODO(cpovirk): Consider checking them. --> + </configuration> </plugin> <plugin> <groupId>org.codehaus.mojo</groupId> 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 72b5a4656..3bc69bd32 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 @@ -381,5 +381,8 @@ public class FuturesGetCheckedTest extends TestCase { * environment that forces Futures.getChecked to its fallback WeakSetValidator. One awful way of * doing so would be to derive a separate test library by using remove_from_jar to strip out * ClassValueValidator. + * + * Fortunately, we get pretty good coverage "by accident": We run all these tests against the + * *backport*, where ClassValueValidator is not present. */ } 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 d40a1cc72..04564f3a0 100644 --- a/android/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java +++ b/android/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java @@ -23,7 +23,6 @@ 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; import java.lang.ref.WeakReference; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; @@ -101,12 +100,6 @@ final class FuturesGetChecked { return GetCheckedTypeValidatorHolder.WeakSetValidator.INSTANCE; } - @J2ObjCIncompatible // ClassValue - @VisibleForTesting - static GetCheckedTypeValidator classValueValidator() { - return GetCheckedTypeValidatorHolder.ClassValueValidator.INSTANCE; - } - /** * Provides a check of whether an exception type is valid for use with {@link * FuturesGetChecked#getChecked(Future, Class)}, possibly using caching. @@ -115,35 +108,8 @@ final class FuturesGetChecked { */ @VisibleForTesting static class GetCheckedTypeValidatorHolder { - static final String CLASS_VALUE_VALIDATOR_NAME = - GetCheckedTypeValidatorHolder.class.getName() + "$ClassValueValidator"; - static final GetCheckedTypeValidator BEST_VALIDATOR = getBestValidator(); - @IgnoreJRERequirement // getChecked falls back to another implementation if necessary - @J2ObjCIncompatible // ClassValue - enum ClassValueValidator implements GetCheckedTypeValidator { - INSTANCE; - - /* - * Static final fields are presumed to be fastest, based on our experience with - * UnsignedBytesBenchmark. TODO(cpovirk): benchmark this - */ - private static final ClassValue<Boolean> isValidClass = - new ClassValue<Boolean>() { - @Override - protected Boolean computeValue(Class<?> type) { - checkExceptionClassValidity(type.asSubclass(Exception.class)); - return true; - } - }; - - @Override - public void validateClass(Class<? extends Exception> exceptionClass) { - isValidClass.get(exceptionClass); // throws if invalid; returns safely (and caches) if valid - } - } - enum WeakSetValidator implements GetCheckedTypeValidator { INSTANCE; @@ -190,13 +156,7 @@ final class FuturesGetChecked { * unable to do so. */ static GetCheckedTypeValidator getBestValidator() { - try { - Class<? extends Enum> theClass = - Class.forName(CLASS_VALUE_VALIDATOR_NAME).asSubclass(Enum.class); - return (GetCheckedTypeValidator) theClass.getEnumConstants()[0]; - } catch (Throwable t) { // ensure we really catch *everything* - return weakSetValidator(); - } + return weakSetValidator(); } } diff --git a/android/guava/src/com/google/common/util/concurrent/IgnoreJRERequirement.java b/android/guava/src/com/google/common/util/concurrent/IgnoreJRERequirement.java deleted file mode 100644 index 49531b421..000000000 --- a/android/guava/src/com/google/common/util/concurrent/IgnoreJRERequirement.java +++ /dev/null @@ -1,25 +0,0 @@ -/* - * Copyright 2019 The Guava Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except - * in compliance with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the License - * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express - * or implied. See the License for the specific language governing permissions and limitations under - * the License. - */ - -package com.google.common.util.concurrent; - -import static java.lang.annotation.ElementType.CONSTRUCTOR; -import static java.lang.annotation.ElementType.METHOD; -import static java.lang.annotation.ElementType.TYPE; - -import java.lang.annotation.Target; - -@Target({METHOD, CONSTRUCTOR, TYPE}) -@ElementTypesAreNonnullByDefault -@interface IgnoreJRERequirement {} diff --git a/android/pom.xml b/android/pom.xml index b6e712e4c..2bf293988 100644 --- a/android/pom.xml +++ b/android/pom.xml @@ -16,23 +16,7 @@ <test.include>%regex[.*.class]</test.include> <truth.version>1.1.2</truth.version> <checker-framework.version>3.12.0</checker-framework.version> - <!-- - Upgrading to 1.19 breaks things: Animal Sniffer reports a problem with the - use of ClassValue in FuturesGetChecked, even though we've added a - suppression annotation. - - The problem might be - https://github.com/mojohaus/animal-sniffer/issues/131. - - We could probably work around this with a different suppression strategy, - perhaps (untested): - - <ignores><ignore>com.google.common.util.concurrent.FuturesGetChecked$GetCheckedTypeValidatorHolder$ClassValueValidator</ignore></ignores> - - But upgrading doesn't really buy us much, as far as I know. (1.19 does - check test code in addition to prod code, but I don't think we care.) - --> - <animal.sniffer.version>1.18</animal.sniffer.version> + <animal.sniffer.version>1.20</animal.sniffer.version> <maven-javadoc-plugin.version>3.1.0</maven-javadoc-plugin.version> <maven-source-plugin.version>3.2.1</maven-source-plugin.version> <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> @@ -167,7 +151,7 @@ <artifactId>animal-sniffer-maven-plugin</artifactId> <version>${animal.sniffer.version}</version> <configuration> - <annotations>com.google.common.util.concurrent.IgnoreJRERequirement</annotations> + <checkTestClasses>true</checkTestClasses> <signature> <groupId>org.codehaus.mojo.signature</groupId> <artifactId>java16-sun</artifactId> diff --git a/guava-testlib/test/com/google/common/collect/testing/MapTestSuiteBuilderTests.java b/guava-testlib/test/com/google/common/collect/testing/MapTestSuiteBuilderTests.java index bcacec4ac..5fd0d0368 100644 --- a/guava-testlib/test/com/google/common/collect/testing/MapTestSuiteBuilderTests.java +++ b/guava-testlib/test/com/google/common/collect/testing/MapTestSuiteBuilderTests.java @@ -313,8 +313,8 @@ public final class MapTestSuiteBuilderTests extends TestCase { return method.invoke(map, args); } catch (InvocationTargetException e) { throw e.getCause(); - } catch (ReflectiveOperationException e) { - throw new LinkageError(e.getMessage(), e); + } catch (IllegalAccessException e) { + throw newLinkageError(e); } } } @@ -349,4 +349,10 @@ public final class MapTestSuiteBuilderTests extends TestCase { .withTearDown(tearDown) .createTestSuite(); } + + private static LinkageError newLinkageError(Throwable cause) { + LinkageError error = new LinkageError(cause.toString()); + error.initCause(cause); + return error; + } } diff --git a/guava-tests/benchmark/com/google/common/util/concurrent/FuturesGetCheckedBenchmark.java b/guava-tests/benchmark/com/google/common/util/concurrent/FuturesGetCheckedBenchmark.java index dd1883bcb..c25a8ff31 100644 --- a/guava-tests/benchmark/com/google/common/util/concurrent/FuturesGetCheckedBenchmark.java +++ b/guava-tests/benchmark/com/google/common/util/concurrent/FuturesGetCheckedBenchmark.java @@ -52,7 +52,8 @@ public class FuturesGetCheckedBenchmark { NON_CACHING_WITH_CONSTRUCTOR_CHECK(nonCachingWithConstructorCheckValidator()), NON_CACHING_WITHOUT_CONSTRUCTOR_CHECK(nonCachingWithoutConstructorCheckValidator()), WEAK_SET(weakSetValidator()), - CLASS_VALUE(classValueValidator()); + CLASS_VALUE(classValueValidator()), + ; final GetCheckedTypeValidator validator; diff --git a/guava-tests/pom.xml b/guava-tests/pom.xml index add78b401..a03dff23d 100644 --- a/guava-tests/pom.xml +++ b/guava-tests/pom.xml @@ -106,6 +106,9 @@ <plugin> <groupId>org.codehaus.mojo</groupId> <artifactId>animal-sniffer-maven-plugin</artifactId> + <configuration> + <checkTestClasses>false</checkTestClasses> <!-- TODO(cpovirk): Consider checking them. --> + </configuration> </plugin> <plugin> <groupId>org.codehaus.mojo</groupId> 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 72b5a4656..3bc69bd32 100644 --- a/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java +++ b/guava-tests/test/com/google/common/util/concurrent/FuturesGetCheckedTest.java @@ -381,5 +381,8 @@ public class FuturesGetCheckedTest extends TestCase { * environment that forces Futures.getChecked to its fallback WeakSetValidator. One awful way of * doing so would be to derive a separate test library by using remove_from_jar to strip out * ClassValueValidator. + * + * Fortunately, we get pretty good coverage "by accident": We run all these tests against the + * *backport*, where ClassValueValidator is not present. */ } diff --git a/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java b/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java index d40a1cc72..6f09b8066 100644 --- a/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java +++ b/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java @@ -120,7 +120,6 @@ final class FuturesGetChecked { static final GetCheckedTypeValidator BEST_VALIDATOR = getBestValidator(); - @IgnoreJRERequirement // getChecked falls back to another implementation if necessary @J2ObjCIncompatible // ClassValue enum ClassValueValidator implements GetCheckedTypeValidator { INSTANCE; diff --git a/guava/src/com/google/common/util/concurrent/IgnoreJRERequirement.java b/guava/src/com/google/common/util/concurrent/IgnoreJRERequirement.java deleted file mode 100644 index 49531b421..000000000 --- a/guava/src/com/google/common/util/concurrent/IgnoreJRERequirement.java +++ /dev/null @@ -1,25 +0,0 @@ -/* - * Copyright 2019 The Guava Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except - * in compliance with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the License - * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express - * or implied. See the License for the specific language governing permissions and limitations under - * the License. - */ - -package com.google.common.util.concurrent; - -import static java.lang.annotation.ElementType.CONSTRUCTOR; -import static java.lang.annotation.ElementType.METHOD; -import static java.lang.annotation.ElementType.TYPE; - -import java.lang.annotation.Target; - -@Target({METHOD, CONSTRUCTOR, TYPE}) -@ElementTypesAreNonnullByDefault -@interface IgnoreJRERequirement {} @@ -16,23 +16,7 @@ <test.include>%regex[.*.class]</test.include> <truth.version>1.1.2</truth.version> <checker-framework.version>3.12.0</checker-framework.version> - <!-- - Upgrading to 1.19 breaks things: Animal Sniffer reports a problem with the - use of ClassValue in FuturesGetChecked, even though we've added a - suppression annotation. - - The problem might be - https://github.com/mojohaus/animal-sniffer/issues/131. - - We could probably work around this with a different suppression strategy, - perhaps (untested): - - <ignores><ignore>com.google.common.util.concurrent.FuturesGetChecked$GetCheckedTypeValidatorHolder$ClassValueValidator</ignore></ignores> - - But upgrading doesn't really buy us much, as far as I know. (1.19 does - check test code in addition to prod code, but I don't think we care.) - --> - <animal.sniffer.version>1.18</animal.sniffer.version> + <animal.sniffer.version>1.20</animal.sniffer.version> <maven-javadoc-plugin.version>3.1.0</maven-javadoc-plugin.version> <maven-source-plugin.version>3.2.1</maven-source-plugin.version> <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> @@ -168,7 +152,7 @@ <artifactId>animal-sniffer-maven-plugin</artifactId> <version>${animal.sniffer.version}</version> <configuration> - <annotations>com.google.common.util.concurrent.IgnoreJRERequirement</annotations> + <checkTestClasses>true</checkTestClasses> <signature> <groupId>org.codehaus.mojo.signature</groupId> <artifactId>java18</artifactId> |