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

Remove unnecessary type arguments. This CL comes courtesy of IntelliJ. (I did revert a few files that ended up with errors, probably often from non-javac tools like our transpilers, and I reverted one tiny part of `OrderingTest`. I may have been able to get away with reverting less.) (previously: https://github.com/google/guava/commit/cb0e7e0cb1c62012934d998ca4d0222a758a9831, https://github.com/google/guava/commit/c1ffb313fd084c865e539c9b6dc795edade21d6a, https://github.com/google/guava/commit/35a4ccbcbb86952f1c021424acd70c7526e8964d, https://github.com/google/guava/commit/08f213923dab324b53a5ee93151a2a2c04be0784, https://github.com/google/guava/commit/274062cdc2a3819ff4b0286e57973b1bbe8c8519, which together explain why I'm not seeing hits in the prod code for packages like `collect`) RELNOTES=n/a PiperOrigin-RevId: 906444162

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

Use some collection constructors (and a few more static imports). And address some other warnings and oddities. RELNOTES=n/a PiperOrigin-RevId: 909294379

Inside `Foo`, refer to "`bar()`" instead of to "`Foo.bar()`." Also, in `ImmutableSortedMap`, belatedly remove a stale comment about explicit type arguments that were added in https://github.com/google/guava/commit/8a1c90e7b34765d720b3711d4180526dde24c124 and removed in https://github.com/google/guava/commit/c1ffb313fd084c865e539c9b6dc795edade21d6a. Apparently the javac flakiness has not returned. I wouldn't be surprised if it were to do so later, but we should have either the type arguments and the comment or else neither. Finally, remove an unnecessary type argument in `ImmutableMultiset` in the backport. RELNOTES=n/a PiperOrigin-RevId: 907153931

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

Run more tests under J2KT/Native. I also cleaned up `ListenableFutureTester` a little, including making its users actually call `tearDown` (oops). And I eliminated another helper from `JSR166TestCase` in favor of a new, simpler version in `MoreExecutorsTest`. Then, as a bonus, I fixed a http://errorprone.info/bugpattern/JUnitMethodInvoked warning in `ExecutionListTest`. PiperOrigin-RevId: 909884313

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

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

Improve `cache` testing under J2CL and J2KT. Things here were more complex than I'd realized. Here are some notes. Previously, both J2CL and J2KT were running only two tests: - CacheBuilderGwtTest - CacheBuilderTest Their `srcs` values reveal that they were also _building_ some support classes: - TestingCacheLoaders - TestingRemovalListeners I identified some cases in which a class `Foo` was available to J2CL and/or J2KT but we were not running `FooTest` under that environment. The next few sections get into that. This CL enables the following classes under both environments: - AbstractCacheTest - CacheStatsTest - RemovalNotificationTest It also enables one more under J2CL only, since the corresponding prod class is available under J2CL only: - MapCacheTest And it enables two under J2KT only for the similar reason: - CacheLoaderTest (which test only `asyncReloading`, which J2CL doesn't support) - CachesTest (which contains only `getCleanupTask`, which J2CL doesn't support, though it easily could be made to). I had to port this one off Mockito. Additionally, I have separate CLs out to attempt to hide some APIs from J2KT because we aren't testing them under J2KT: - cl/907699872: CacheBuilder.weigher, CacheBuilderSpec, and associated methods - cl/907732362: AbstractLoadingCacheTest, ForwardingCache, ForwardingLoadingCache I also performed some minor cleanups here and there, both ones that were necessary for the additional nullness checking that I enabled and also others. The biggest are probably in the `Forwarding*CacheTest` classes (even though I didn't enable those under J2CL/J2KT), but I didn't migrate _them_ off Mockito. After all that, it's worth noticing that most of the "real" `cache` tests don't run under other J2CL/J2KT. We do have some basic coverage of caches in `CacheBuilderGwtTest`. (Despite its name, it does test the built caches, not just the building process. And it's not run only under GWT.) But more sophisticated tests remain un-run. Some of them are likely hard, like `LocalCacheTest`, which digs into the internals of the "main" implementation. Others might not be too hard, like `CacheManualTest` (which is a problem for J2KT because of `asMap()` and a problem for J2CL because of `stats()`, which apparently throws UOE instead of being `@GwtIncompatible`, oops). And probably a whole bunch are in between. Notice also that some use utility classes, including `CacheBuilderFactory` and `CacheTesting` (which appears to also depend on `LocalCache` internals). For the sake of being concrete, here is a list (exhaustive or not) of some tests that we don't run under J2CL/J2KT: - CacheEvictionTest - CacheExpirationTest - CacheLoadingTest - CacheReferencesTest - CacheRefreshTest - EmptyCachesTest - LocalCacheMapComputeTest - LocalLoadingCacheTest - PopulatedCachesTest PiperOrigin-RevId: 908043537

More simplifications based on recent review comments: - `FunctionalEquivalence`: Migrate from `c.g.common.base.Objects.hashCode` to `j.u.Objects.hash`. - (We can't static import it because `Equivalance` declares its own `hash` method.) - `Predicate`: Migrate Javadoc from `c.g.common.base.Object.equal` to `j.u.Objects.equals`. - `Predicates`: Tweak generics, at which point we can eliminate our local `asList` method and some explicit type arguments - various files: Rename+move `Iterators.checkNonnegative` to `CollectPreconditions.checkNonnegativeIndex` to clarify how it differs from `CollectPreconditions.checkNonnegative`—namely, by throwing `IndexOutOfBoundsException` instead of `IllegalArgumentException`. - Thanks to Gemini for catching my mistake of trying to use the existing `CollectPreconditions.checkNonnegative`! - This change also changes the error message slightly. I could have left it alone, but I figured it might as well match the format we're using for the similar methods in `CollectPreconditions`. - I had thought that perhaps we could use `Preconditions.checkPositionIndex`, but (a) that requires a size (which appears in the exception message, so we shouldn't just pass `MAX_VALUE`) and (b) that [throws `IllegalArgumentException`](https://github.com/google/guava/blob/b9d1fd6875d9267c14dc4063e13af85894ea35d1/guava/src/com/google/common/base/Preconditions.java#L1505), too, wait, seriously? What were we thinking in cl/9392102 back in 2008? - I'll bet that many (most?) of our usages of `checkPositionIndex` are wrong. Certainly that's true of [the first one that I looked at](https://github.com/google/guava/blob/b9d1fd6875d9267c14dc4063e13af85894ea35d1/guava/src/com/google/common/collect/Lists.java#L355). - various files: Remove suppressions of StaticImportPreferred for `min`/`max` methods that are in scope. The suppressions are no longer necessary as of cl/897308244, which avoids trying to static import when a method of the same name is already in scope. - Or actually, they're not necessary as of the earlier cl/811482021, which removed those suggestions from StaticImportPreferred entirely. - I kept the suppression in `Booleans`. That one exists because `Comparator.min` does not exist under all versions, so the fix of cl/897308244 is presumably not always sufficient. - Again, that's technically not necessary because StaticImportPreferred doesn't make the suggestion at all anymore (for exactly this reason). I suppose that I could tweak it to suggest `min`/`max` again as long as we're not inside a `Comparator`, but StaticImportPreferred is controversial in general, so I don't think that's necessarily worthwhile. - various files: Static import `min`/`max` where we can (by locally undoing the exclusion for those methods :)). - various files in the backport: Static import `CollectPreconditions` methods in a few more cases. RELNOTES=n/a PiperOrigin-RevId: 907127863

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