cpovirk

Address some more counterfactual [CheckedExceptionNotThrown](https://errorprone.info/bugpattern/CheckedExceptionNotThrown) findings. I suspect that IntelliJ flags most of these, especially since its analyses seem happy to treat Guava as a "closed world" with no callers/overrides outside the project, if I'm remembering right. Anyway, that makes them nice to fix so that F2 doesn't jump to them. We don't normally worry much about a stray `throws Exception` on a `test*` method in particular, but it was easier to have the tooling remove them (and then glance over the results) than to make it smarter, and it's a nice bonus to remove such `throws` clauses, anyway. It's possible that I could rerun this cleanup until I reach a fixed point, since each removal may unlock removals downstream. I think that I made one follow-up change somewhere in `util.concurrent`. Maybe I'll do another "full" round, maybe using IntelliJ in case it catches anything that Error Prone did not. I made no effort to look at `abstract` methods that might have no implementations that actually throw a declared exception. I made no effort to remove calls to `super.setUp`/`tearDown` when those calls are no-ops (because they call the methods on plain `TestCase`). If I were to do so, that might unlock more `throws` removals on the `setUp` and `tearDown` methods that are making those calls. There were some cases that I had to revert: - `TypeTokenTest.CannotConstruct` constructor - `AbstractIdleService`: `DefaultService` and `TestService`, both `startUp` and `shutDown` (though I may or may not have needed to revert all four) - `AbstractExecutionThreadServiceTest.FakeServices.startUp` Finally, I made `ListeningExecutorServiceTest.FakeExecutorService` `final` while I was in the area. RELNOTES=n/a PiperOrigin-RevId: 938063962

Address various findings, mostly from IntelliJ. Most of the findings are around Javadoc links to non-visible APIs. The mildly annoying case is when we write something like `{@link #keySet}`: If we were building Javadoc with the `-private` flag, then these references would be to the private field of that name. And that's... probably fine, since the rendered Javadoc is in fact going to include private members? But I guess it's questionable to refer to private members of another class at all, even from within Javadoc? Anyway, when we run plain old Javadoc, the links get resolved to `keySet()`, as desired. So the changes of that nature are about making IntelliJ happy (so that I can jump from "real error" to "real error" more easily in cl/934353699) and maybe being slightly more principled about things, rather than fixing errors in our rendered Javadoc. (Also notice that plenty of the findings were in `javatests`, which we don't normally generate Javadoc for.) A couple other notes on Javadoc: - I stopped trying to link to `EmptyImmutableTable`, which was deleted in cl/43207266. There may well have been some other deletions (or at least moves, like `hashFloodingDetected`, mentioned in `ImmutableSetHashFloodingDetectionBenchmark`) that I didn't dig into or at least didn't save notes on. - I made the Android flavor of `TopKSelector` refer to `Comparators.least`, eliminating a diff from the mainline, since we added `least` to the Android flavor a while back. The other notable class of findings is unnecessary casts. RELNOTES=n/a PiperOrigin-RevId: 934979370

Enable translation of actual visibility modifiers in `com.google.common` (excluding Truth). For `protected` methods which are not accessible elsewhere in the package/module under Kotlin, declare a bridge `internal` method, and updates call sites to use it instead. (It's possible that I've made updates in some places that weren't strictly necessary.) RELNOTES=n/a PiperOrigin-RevId: 937492418

Enable translation of actual visibility modifiers in `com.google.common` (excluding Truth). For `protected` methods which are not accessible elsewhere in the package/module under Kotlin, declare a bridge `internal` method, and updates call sites to use it instead. (It's possible that I've made updates in some places that weren't strictly necessary.) RELNOTES=n/a PiperOrigin-RevId: 936234321

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

Address some https://errorprone.info/bugpattern/ReferenceEquality warnings. There are many others left, but this is a start. Plus, sneak in a migration to `SimpleImmutableEntry`, which we had avoided long ago for GWT reasons. RELNOTES=n/a PiperOrigin-RevId: 922103048

Address places where [CheckedExceptionNotThrown](https://errorprone.info/bugpattern/CheckedExceptionNotThrown) *would* produce warnings if it were to operate on test code. I acknowledge that we are removing a checked exception from `EscaperAsserts.assertBasic`. This could lead to source incompatibilities. For a `guava-testlib` API whose only callers inside Google are in Guava itself, I don't worry about fallout from that. RELNOTES=n/a PiperOrigin-RevId: 937536463

Use yet more `assertThrows` (and a tiny bit more Truth). AssertThrowsMinimizer might again not be happy (though, surprisingly enough, I haven't seen it yet). If so, that is a problem for another day. RELNOTES=n/a PiperOrigin-RevId: 938810532

Modernize a few tests, including migrating off Mockito (to improve cross-platform compatibility) and also onto Truth and `assertThrows`. RELNOTES=n/a PiperOrigin-RevId: 918097212

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

Avoid writing "`Foo.Bar`" inside a `Foo` in some cases. RELNOTES=n/a PiperOrigin-RevId: 936730586

Remove redundant nested annotations. For example, `@SuppressWarnings("foo")` is a no-op inside another `@SuppressWarnings("foo")`, as is `@GwtIncompatible` inside another `@GwtIncompatible`. Some of the annotations here are mistakes from https://github.com/google/guava/commit/16d1e071ae555e67577d5c69aa543ea3cd556cd2 and https://github.com/google/guava/commit/8b7542382fff13094a880d510466307a50530eaf. Looking at the results, I see some `SerializedForm` classes that should probably be made `@GwtIncompatible`. I'm planning to not bite that off right now. RELNOTES=n/a PiperOrigin-RevId: 914329736

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

Produce `<T extends Foo & Bar>` instead of `<T extends Foo extends Bar>`. And produce `<T extends Object & Foo>` instead of `<T extends Foo>`, since the `T` [matters for erasure](https://docs.oracle.com/javase/tutorial/extra/generics/convert.html). RELNOTES=n/a PiperOrigin-RevId: 926231624

Run `@LargeTest` tests under Android emulators by default. Add `@AndroidIncompatible` to some large tests that would start running under Android but that we don't want. (Ideally, we will someday make some of them run but with fewer iterations.) Update documentation on `AndroidIncompatible` to reflect not only this change but also the removal of our direct usage of `FpUtils` in cl/158434703 (later improved upon by https://github.com/google/guava/commit/caf8221758831fc44839c46aa1ef6bb9c0742abb). RELNOTES=n/a PiperOrigin-RevId: 923727864

Defer potentially expensive `ClosingFuture` string conversions during logging. The string conversions can be expensive if the underlying `Future` has an expensive `toString()`, as many Guava `Future` implementations do when they contain, say, a large proto instance. (See cl/158789362 and cl/165925433.) The conversion to `String` used to be lazy, but I unwittingly made it eager in https://github.com/google/guava/commit/aeef90988dec22f87430c822e27315d8f41a90d2. (I can't remember whether that crossed my mind at some point or not, but if it did, I forgot. Sorry.) RELNOTES=n/a PiperOrigin-RevId: 944596146

Use `assertThrows` more in `NullPointerTesterTest`. (And apply some random other cleanups, including a couple [DoNotCallSuggester](https://errorprone.info/bugpattern/DoNotCallSuggester) suggestions, for good measure.) I had hoped not to bother with preserving _any_ custom failure messages, but I was wrong: We can't rely on `NullPointerTester` to include the name of the method under test in its `AssertionError` because we're providing a message for the case in which it _doesn't_ throw! RELNOTES=n/a PiperOrigin-RevId: 938167845

Remove some redundant `isInstanceOf` assertions. Replace the assertion in `testConcatWithMoreGeneralType` (which acted as a non-null assertion) with an assertion that shows that `concat` uses the provided type and not the more specific type that the input arrays have. I had made that assertion worse back in https://github.com/google/guava/commit/156c24efe69ca0bf858cfd7dc450d3d30c60b875. But don't run that assertion under J2KT / Kotlin/Native, where it would fail. To keep some level of testing under that platform, introduce a similar test that at least checks that the result is empty (and, again, implicitly checks that it's non-null). Finally, standardize on "`array`" as the name of the result of the call to the `ObjectArrays` method under test, rather than mixing "`array`," "`result`," and "`empty`." (OK, the test class has some methods that are more complicated than that. But for most of the methods, a single variable is good enough, and "`array`" seems like a fine name. I'm not opposed to "`result`" if you prefer that. RELNOTES=n/a PiperOrigin-RevId: 940131612

Continue tearing out parts of `JSR166TestCase`. The immediate motivation is some [`throws` clauses for exceptions that aren't thrown](https://errorprone.info/bugpattern/CheckedExceptionNotThrown), this time detected by IntelliJ. RELNOTES=n/a PiperOrigin-RevId: 938715872