cpovirk

Remove caching of cheap collection views across `common.collect`. (like https://github.com/google/guava/commit/e87d019e47a29bf263a82fe81caede34e9728e15 and https://github.com/google/guava/commit/0a8e1ec55afe1d9361ebbd0c4149adfbb32ccf9a but for a wider variety of collections) This includes removing `ViewCachingAbstractMap` entirely in favor of using `AbstractMap` directly. For J2ObjC safety, I removed `@Weak` and `@WeakOuter` annotations now that there is no reference cycle between the outer collection and its view collections. (Really, we should have used `@RetainedWith` instead of `@Weak*`, anyway, so this CL improves fixes existing issues in addition to perhaps preventing new ones. For more on `@RetainedWith`, see cl/781580713.) Parts of the `@Weak*` changes should have been done as part of https://github.com/google/guava/commit/0a8e1ec55afe1d9361ebbd0c4149adfbb32ccf9a for `Compact*HashMap`. _Not_ covered in this CL: - various other cached views that might benefit, such as `RegularImmutableSet.asList` (cl/922882558) - views that I worry at least a little more about, mainly "invertible operations" (`BiMap.inverse`, `NavigableSet.descendingSet`, etc.) - maybe migrating off `AbstractMap` in some additional cases in which it would make sense to avoid inheriting its fields - GWT/J2CL RELNOTES=n/a PiperOrigin-RevId: 973885682

Standardize racy lazy init. This addresses almost all our https://errorprone.info/bugpattern/AssignmentExpression warnings. The final(?) warnings will be addressed by cl/959862978. RELNOTES=n/a PiperOrigin-RevId: 962152630

More aggressively throw `NullPointerException` in `common.graph`. Currently, we sometimes throw `IllegalArgumentException` and/or throw later (if at all). This was hidden because `PackageSanityTest` was exiting as soon as it hit the first failure. (It exited successfully, assuming that the first failure matched what we expect.) This CL exists because that `PackageSanityTest` setup was fragile to the relative ordering of the prod and test jars on the classpath: IIUC, we were seeing the prod jar first, and that was triggering the expected failure, but later we started seeing the test jar first, so we started seeing failures related to `TestUtil`. I fixed the failures in `TestUtil` by adding `@Nullable` to some parameters there. (I could reasonably also just have excluded `TestUtil` from `PackageSanityTest` entirely, since we're almost entirely interested in testing our _prod_ code. Or I could have added `checkNotNull`, but that would arguably have been a tiny regression in the extremely unlikely event that a caller were to pass `null`.) That might have been enough to prevent the test failure in practice, but the setup was still fragile. So I configured sorting of classes across jars (cl/948465826), and I'm removing the short-circuiting behavior (this CL). That requires fixing the resulting "latent" failures. That brings us back to the main part of this CL, which is to perform null checks eagerly, as the newly comprehensive `NullPointerTest` testing requires—and as is at least arguably a good idea, anyway, since it's closer to the behavior seen today by Kotlin users and maybe closer to some future Valhalla behavior. RELNOTES=n/a PiperOrigin-RevId: 949022628

Add some more `@Nullable` annotations, mostly in null-unmarked code. Compare https://github.com/google/guava/commit/4c4b7547bc9f8da93a3e10d84417feb606d566cb and https://github.com/google/guava/commit/f42aa699a9e63c0e42045b738143bf9c470fe3ee. RELNOTES=n/a PiperOrigin-RevId: 947342855

Internally disallow weak and soft references to _most_ value classes in `MapMaker` and `CacheBuilder` (in the JRE flavor but not the Android flavor, not even for Robolectric tests). Update corresponding tests to avoid value classes. RELNOTES=n/a PiperOrigin-RevId: 973492685

Add a test that each class in a package is safe to initialize first, and apply it to Guava. This new testing detected one problem, which I addressed in https://github.com/google/guava/commit/5195255413ee7eac23a012484c9d0236c1826363. For further background, see https://github.com/google/guava/issues/8617. I hope that the additional testing doesn't slow external CI significantly. I ran Guava's tests once with this change and once without, and I saw a 13s slowdown. But I don't know what the normal level of variation is. We could consider running the test internally only, but I figure it's safer to run everywhere for now. (While making a copy of `ClassPathUtil`, address a deprecation warning there, and migrate to [the `AssertionError constructor that was added to Android in API Level 19](https://developer.android.com/reference/java/lang/AssertionError#AssertionError(java.lang.String,%20java.lang.Throwable)). We have done such similar migrations elsewhere, as in https://github.com/google/guava/commit/c497a975ba2845d553bb1c1ae76d7118320b32a5.)) RELNOTES=`testlib`: Added `AbstractClassInitializationTest`. PiperOrigin-RevId: 971504745

Eliminate caching of many collection views in many `ImmutableMap` implementations. This CL removes all fields that held cached views from the base `ImmutableMap` type and from `SingletonImmutableBiMap`. This covers common cases like `ImmutableMap.of(...)`. This CL does _push fields down_ from `ImmutableMap` into a lesser-used branch of the implementation tree, `IteratorBasedImmutableMap`. `IteratorBasedImmutableMap` continues to be used by APIs like `immutableEnumMap`, and those continue to cache their views. It would probably make sense to eliminate caching for many of those implementations, as well, but I wanted to focus on the more commonly used types first, taking care _not_ to remove caching from views that are more heavyweight to compute. (But see a point below about `asMultimap().inverse()`!) (Also: This CL also does not touch GWT/J2CL.) The main motivation for eliminating view caching is to save memory, but: - It likely also helps with J2ObjC memory management. Notably, `ImmutableMap.multimapView` has been set up to use the `@WeakOuter` class `MapViewOfValuesAsSingletonSets`, while the safer approach would likely have been `@RetainedWith`, as seen in cl/781580713. By breaking the reference cycle, this CL eliminates the need for either approach. - Cached views have been a complication for our internal memory tests, as we saw most recently in https://github.com/google/guava/commit/046468055f9a3727726e092db90cd3f4fd6a5650. If something were to go wrong with this CL, here are a couple possibilities for what it might be: - Performance might get worse in some unusually allocation-sensitive use case, perhaps involving Android. We could address any Android trouble by rolling back this CL's change to the Android flavor. But it's possible that this CL's impact on Android will be positive, and I'd rather not introduce diffs between the flavors if we don't need to. (That said, the immutable collections are one case in which significant diffs exist already.) - `someImmutableMap.asMultimap().inverse()` can be expensive (specifically, the `inverse()` part of the call chain), so anyone who is using it repeatedly on the same object might be benefiting from the caching of the multimap view. (Kudos to cl/35373012 for having documented this!) I'm hoping that this won't be a big deal, but I plan to monitor fleet-wide profiling for the cost of calls to `ImmutableMultimap.inverse()` after submission. (This CL also happened to naturally eliminate at least one https://errorprone.info/bugpattern/AssignmentExpression warning in `ImmutableMap`, and I ended up eliminating the others in the class (though not the one in `RegularImmutableBiMap`) in response to review feedback. Plus, I suppressed an old https://errorprone.info/bugpattern/PreferredInterfaceType warning on `ImmutableMap.values()`, which we've discussed before in https://github.com/google/guava/issues/3255 and b/160392629.) RELNOTES=`collect`: Changed some `ImmutableMap` implementations to create a new, lightweight object on each call to view methods like `keySet()`, rather than creating and storing that object during the first call for reuse later. This is likely to slightly improve performance for some users, but it could lead to regressions in unusual cases, such as repeatedly calling `asMultimap().inverse()` on the same `ImmutableMap` instance. PiperOrigin-RevId: 955240073

Tidy up `ExecutionSequencerTest`: - Simplify some tests that used a Google-internal test `Executor` implementation, and release them publicly. - Use unused `Future` values. - Use `TestCallable` less, and rename it to "`TestAsyncCallable`." - Replace some fields with local variables. (Currently, tests rely on the relationship between the `firstFuture` and `firstCallable` fields, even though that relationship is configured in `setUp`, which is sometimes hundreds of lines away.) - Rename `Future` variables to attempt to clarify which are the ones returned by `TestCallable` instances and which are the ones returned by `serializer.submitAsync` calls. Rename other variables more generally, including to improve consistency across tests. - Don't use transformations when a simpler `Future` would suffice. - Inline formerly Google-internal helper method `add`. - Remove stale `nullness` suppressions. - Use a couple static imports. - Use/simplify lambdas in a couple places. - Suppress an Android Lint ThreadSafe error that is produced for a lambda but that wasn't produced for the equivalent anonymous class. - Clarify some comments. - etc. Also, belatedly update a `ExecutionSequencer` comment for the migration from `&` to `&&` back in https://github.com/google/guava/commit/9d690928acb69b6158bde3371c66445c861f5b7c. RELNOTES=n/a PiperOrigin-RevId: 962845814

Migrate `graph` tests from `Parameterized` to TestParameterInjector. This is good in general, but what finally got me to do it was that `Parameterized` was generating test names that were too long for our Android infrastructure. (I haven't yet checked whether the TestParameterInjector names are short enough.) RELNOTES=n/a PiperOrigin-RevId: 978699274

Add some `@Nullable` annotations (and one `requireNonNull` call) to `LocalCache` and to `cache` tests. These classes aren't `@NullMarked`, so we don't "need" the annotations at the moment. But it seems likely that we'll increasingly want proper nullness information here over time. RELNOTES=n/a PiperOrigin-RevId: 945724763