Then, where appropriate, make them `private`, or even inline them.
(I guess that I've set aside my "Why even write '`private`' in a test class at all?" perspective, at least for the moment :))
RELNOTES=n/a
PiperOrigin-RevId: 1008110306
(prompted by a review comment in https://github.com/google/guava/commit/37535d73f0416f30e0301efe61a8eb8ede44c1b7)
We should easily make this available publicly, but we've had reservations around our `Forwarding*` classes in general (b/5231423), so I'm reluctant to expose more there without further thought.
RELNOTES=n/a
PiperOrigin-RevId: 1008047203
- More clearly document that `StandardElementSet` is the class that we're testing _and_ where the list of legal methods is documented.
- Make methods throw by default (by virtue of calling `delegate()`), tightening the restrictions on which methods can be called.
(further evidence for https://github.com/google/guava/issues/1575)
RELNOTES=n/a
PiperOrigin-RevId: 1005714075
Currently:
- `AbstractBiMap` runs `replaceAll` on its forward map first, so if that throws in the middle, its inverse is never updated. (Additionally, if it detects a value conflict while populating the inverse later, it likewise ends up in an inconsistent state.)
- `HashBiMap` clears itself at the beginning, so if it throws in the middle, it ends up missing entries.
Fixes https://github.com/google/guava/pull/8766 (by arshsmith), which proposed a fix for the specific case of `filterValues`.
RELNOTES=n/a
PiperOrigin-RevId: 1004886559
_If_ `java.io.tmpdir` is world-writable and does not set the very important sticky bit, then an attacker has a chance to unlink the temp file and replace it with a symlink between when it is created and when it is opened. This could cause the user of the stream to write/read data to/from an unintended location.
It's not clear that it's worth defending against this, given that the problem arises only with broken temp-directory permissions, which are much more likely to open holes elsewhere than here. But given the rise of security scanning over time, I'm figuring that it's worth addressing even _some_ borderline reports (like this one from our internal scans) when it appears straightfoward and safe enough to do so.
RELNOTES=n/a
PiperOrigin-RevId: 1001464603
This prevents (for the case of `GcFinalization`, but other cases are similar):
- a `NullPointerException` if our finalization logic runs before `FinalizableReference` has assigned to `this.latch`
- indefinite retention of the `Reference` object if our finalization logic runs before `reference.add`
Fixes https://github.com/google/guava/issues/8738
RELNOTES=n/a
PiperOrigin-RevId: 998855369
- While it makes sense to have an HTML `<title>` (i.e., what shows up in the tab button or taskbar button for that page) that says (e.g.) "Truth API," it's weirder for the in-page title to say "Truth API" instead of just "Truth." Javadoc lets us set the two independently; I just chose poorly in https://github.com/google/guava/commit/49300093df01df7d7bea5f1d657ecf58fbc4f596.
- Truth was showing a copyright date of 1980, presumably some kind of default from somewhere or other. I've given it [the same value as Guava](https://github.com/google/guava/blob/1da08c4d381abf3613ad956d3aec9fa77dfc94b2/pom.xml#L100).
It's possible that [JSpecify](https://github.com/jspecify/jspecify/issues/904) would have the same issue, though I'm not sure because it uses Gradle instead of Maven. But even if it does, I'm not sure that its Javadoc ends up with a summary page at the moment, I guess because it contains only one package? So maybe there's nowhere for "JSpecify API" to appear in the actual page text, only in the `<title>` where we want it.
RELNOTES=n/a
PiperOrigin-RevId: 996717116
The config from https://github.com/google/guava/commit/c05d28807a6f0fbcb0379349884632c07180a4bd is not working how I'd hoped: We're getting PRs to [bump deps like `org.codehaus.mojo:animal-sniffer-maven-plugin`](https://github.com/google/guava/pull/8725), not only PRs for Maven and Maven Wrapper. It's possible that we should just give in and accept those, too, but I was hoping to defer that.
I'm not confident that this new config will work, either!
RELNOTES=n/a
PiperOrigin-RevId: 995193422
We still won't be able to approve these and have them automatically imported. But at least we'll get PRs that I can import manually, as I do currently for GitHub Actions.
RELNOTES=n/a
PiperOrigin-RevId: 995053987
And document why we can't use `ConcurrentHashMap.newKeySet` here.
The bogus `extends @Nullable Object` isn't detected by our nullness checking because our J2CL builds don't use `@NullMarked` classes for `java.*`.
RELNOTES=n/a
PiperOrigin-RevId: 994652789
Yes, one of these is changing the "old implementation" in a benchmark. That could in theory affect performance, but it seems extremely unlikely to. And if somehow it does, it seems only fair to compare against this improved implementation, anyway. (My change makes that code look at lot like the code in `NewExecutionListWithoutReverse` and in the prod `ExecutionList` itself.)
RELNOTES=n/a
PiperOrigin-RevId: 989830275
Because:
- While it continues to be true that `IdentityHashMap` has an `Entry` implementation that (among [other things](https://bugs.openjdk.org/browse/JDK-8352596)) throws an exception if accessed after removal, this is an implementation detail, one that could easily change in a way that would leave this test silently testing nothing.
- We already saw and suppressed an [IdentityHashMapBoxing](http://errorprone.info/bugpattern/IdentityHashMapBoxing) warning, and we're now seeing a [ValueClassIdentity](https://errorprone.info/bugpattern/ValueClassIdentity) warning.
That latter problem is solvable by using a type other than `Integer`, but the former problem could at best be solved by adding a check up front that `IdentityHashMap` behaves as we expect. Put that all together along with how exceedingly efficient we've become at implementing `Map`, and I think we should just implement `Map`.
(This test is originally from cl/13218883, which cited https://github.com/google/guava/issues/258.)
RELNOTES=n/a
PiperOrigin-RevId: 989706189
Largely, this is about enabling tests that were unlocked by our upgrade to the emulator we test under now that we are on a minSdkVersion of 24. That upgrade both fixed bugs and improved performance of some excessively slow methods.
But I also took this opportunity to be more systematic about which tests we consider "too slow" to run under Android. I believe that what I did was (keeping in mind that test runtime can vary from run to run):
- If a test takes ~30s+, add/keep `@AndroidIncompatible`. Otherwise, consider it "fast enough."
- If a test takes ~15s+, add/keep/update a comment about its approximate runtime.
- But make an unprincipled exception for `Utf8Test.testIsWellFormed_3Bytes`, which takes ~70s but felt some combination of "important" and "possible to imagine behavior differences from rogue optimizations." (Maybe vaguely compare b/262383185??)
RELNOTES=n/a
PiperOrigin-RevId: 984013883
Most of these changes were probably unlocked by our upgrade to the emulator we test under now that we are on a minSdkVersion of 24.
The complicated change is to get our `WeakReference`-related test working. Part of the fix there is to create the `WeakReference` in a separate method, similar to https://github.com/google/guava/commit/c02d13681b930d3e1585092dfff757f2d39d9dd0. Then we use `GcFinalization`, as usual for this kind of thing.
It's possible that I should be more concerned by my old "Bug?" comments in `CacheLoadingTest`. Maybe they were some kind of Android bug that got fixed, or maybe they reflect a real issue in `cache` that might still exist.
RELNOTES=n/a
PiperOrigin-RevId: 984005922
I had done some regex search and replace to insert `@Serial`, including to do so at the site of `@GwtIncompatible // serialization`. That makes sense for lots of prod members (though not all, and thankfully I seem to have avoided at least some possible mistakes, like in `ImmutableMapKeySet`), but it appears to have made sense approximately never for test members.
RELNOTES=n/a
PiperOrigin-RevId: 983985663
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
The current behavior is fragile to races and (more realistically?) to tests that use completed `Future` instances when the prod code might see in-progress ones.
RELNOTES=n/a
PiperOrigin-RevId: 977869166
This [just came up again](https://github.com/google/guava/issues/8650#issuecomment-5510207112). It is a better solution to https://github.com/google/guava/issues/2193 than https://github.com/google/guava/pull/8127 was.
(I think it makes sense for `gradle_integration_tests.sh` to continue using `install` so that the Maven-produced artifacts become visible to Gradle. But I haven't tested.)
Sadly, it's still not enough for `mvn compile` to work:
```
[ERROR] Failed to execute goal on project guava-gwt: Could not resolve dependencies for project com.google.guava:guava-gwt:jar:999.0.0-HEAD-jre-SNAPSHOT
[ERROR] dependency: com.google.guava:guava-testlib:jar:tests:999.0.0-HEAD-jre-SNAPSHOT (test)
[ERROR] Could not find artifact com.google.guava:guava-testlib:jar:tests:999.0.0-HEAD-jre-SNAPSHOT
[ERROR] dependency: com.google.guava:guava-tests:jar:tests:999.0.0-HEAD-jre-SNAPSHOT (test)
[ERROR] Could not find artifact com.google.guava:guava-tests:jar:tests:999.0.0-HEAD-jre-SNAPSHOT
```
(For all testing of such changes, it's necessary to delete/move `~/.m2/repository/com/google/guava` so that Maven doesn't just read from previous builds that are installed there—thanks to our habit of using `mvn install` instead of `mvn package`!)
RELNOTES=n/a
PiperOrigin-RevId: 976459357
`Runnables.doNothing()` exists only because Java lambdas didn't exist in 2013. `() -> {}` is shorter and more standard.
RELNOTES=n/a
PiperOrigin-RevId: 975197633
This should catch problems that would arise in Robolectric tests (or other usage of the backport on the JVM, but we otherwise non-Robolectric JVM use of the backport in our depot).
Also, tweak the implementation to be more open-source-able (by avoiding the Java-8-incompatible expression `switch`), even though we're not open-sourcing it yet.
(followup to https://github.com/google/guava/commit/f516e7587dd92720f61a56a3a1c0998c62806788)
RELNOTES=n/a
PiperOrigin-RevId: 974678606
The candidates in this batch came courtesy of IntelliJ. (I then had to get Gemini to revert problems related to reflection on Guava internals, and I reverted changes to publicly visible APIs, including those now conveniently tagged with `@J2ktPublic`.)
Also:
- Change `LineBuffer` methods from `protected` to package-private.
- Change [`AbstractClassInitializationTest`](https://github.com/google/guava/issues/8617) `getPackageName` and `isIgnored` methods from `public` to `private`. (These aren't yet in a Guava release, so the change is safe, as is the change to make `testClassesInitialize` be `final`.)
- I suspect that I could also have removed `throws Exception` from `testClassesInitialize` while I was there. But that shouldn't matter to anyone, since there should be no callers or overrides even after release, and keeping `throws Exception` is slightly more flexible for us.
RELNOTES=n/a
PiperOrigin-RevId: 974457816
(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
It's shorter, and it is even more transparent about what it does (i.e., nothing).
`doNothing()` is admittedly a singleton (though we don't promise that), while the singleton-ness of `() -> {}` depends on details of your program and toolchain. I don't expect that to be much of an issue in practice. (If anything, the thing that people ought to consider is whether to ensure that [every new usages of a do-nothing `Runnable` is a fresh object](https://github.com/google/guava/blob/1f5d40726a38b11174e0e13da78ecb9f7630cc6e/guava/src/com/google/common/util/concurrent/InterruptibleTask.java#L42).)
RELNOTES=n/a
PiperOrigin-RevId: 972573845
That initialization ordering has been impossible to trigger through public APIs since https://github.com/google/guava/issues/1977. However, we've since learned of cases in the wild in which libraries use reflection to access non-public APIs for the purpose of initializing them in basically arbitrary order:
- GraalVM does this if users request build-time initialization for a class directly or indirectly.
- At least two systems inside Google try to eagerly initialize classes during normal runtime.
- It's not hard to imagine a custom serialization framework that would do this. (Normal Java serialization, by contrast, is fine: `ImmutableList` instances are always serialized as `SerializedForm` instances, rather than as `RegularImmutableList` instances directly.) I wonder if GWT-RPC could have caused such trouble back in the day. (My quick read of our old serializer implementation suggests "no," but I didn't think about the GWT-RPC system as a whole.)
Plus, we've gotten reports of what I still suspect to be a bug in someone's Android toolchain, in which a nondescript use of `ImmutableSet.asList` appeared to be optimized to skip the `ImmutableList` initialization and instead access `RegularImmutableList` directly. (This is uncommon, so it may happen only with a combination of some toolchain and a case in which `RegularImmutableList` is specialized to handle _only_ the empty-collection case. Here's [an old example](https://issues.chromium.org/issues/40051933#comment6).) Since the first two cases above are most likely to be pulling in `guava-jre`, not `guava-android`, this is likely the biggest practical danger.
Beyond avoiding NPE, I believe that this CL even protects against [class-initialization deadlocks](https://github.com/google/guava/issues/1977) for a system that manages to try to initialize `RegularImmutableList` first in one thread and `ImmutableList` first in another—even though, again, the latter should be "impossible":
- A caller who initializes `ImmutableList` first is safe because `ImmutableList` initialization doesn't trigger `RegularImmutableList` initialization.
- A caller who somehow initializes `RegularImmutableList` first is safe because it either initializes `ImmutableList` itself or safely waits for the other thread, which can complete without dependency on `RegularImmutableList`, as discussed above.
That said, I would still worry about any system that manages to initialize `RegularImmutableList` first—not because of trouble with `RegularImmutableList` itself anymore but because these same kinds of trouble (both NPE-like problems and class-init deadlocks) could arise with other classes.
Anyway: Other than the tests that show that this change helps (cl/967775267), this CL is the only work that I have in mind for https://github.com/google/guava/issues/8617 at the moment.
(Also, undo my static importing of `RegularImmutableList.EMPTY` from cl/88131198. I remain the world's biggest fan of static imports, but I acknowledge that a class-init cycle is a case that's not necessarily best swept under the rug. Not that I'd expect for most readers to have that on their radars regardless. But I am not _totally_ unreasonable. (Compare https://github.com/google/guava/commit/ccb126df16cca8002946d8c9b4347c670d2ca6eb.))
(And remove a comment about how our `subList` declarations narrow the return type. They do, but there's enough else going on that I found the comment weird when I came across it.)
RELNOTES=n/a
PiperOrigin-RevId: 971463557
- 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
- Use erased types instead of type variables. Type variables often appear to _work_, at least with `javadoc` (and it seems Code Search), but it appears that [support for them may be accidental](https://bugs.openjdk.org/browse/JDK-8279474), and it looks like we've mostly avoided them. (My search was for `#\w+[(][^)]*\b[A-Z]\d*\b`.)
- Avoid specifying the parameter _name_ in addition to the _type_ in one case that that search happened to turn up.
- Use `@linkplain` instead of `@link` where appropriate in those links.
RELNOTES=n/a
PiperOrigin-RevId: 957376666
This became [the proper policy](https://bugs.openjdk.org/browse/JDK-8220379) in [JDK 13](https://bugs.openjdk.org/browse/JDK-8219801): At that point, `javadoc` started generating the top-level heading as (appropriately enough!) `<h1>` instead of the old `<h2>`.
The switch lets our class-level subheadings show up in the outline on the left side of the rendered documentation, and it prevents our method-level subheadings from showing up in that outline _nested under the wrong methods_. (They disappear entirely, and that seems fine.)
We may have noticed at least the `<h2>` part of this at some point, as evidenced by the `<h2>` usages in classes like https://guava.dev/CacheBuilder and in [some](https://guava.dev/releases/snapshot-jre/api/docs/com/google/common/base/package-summary.html) `package-info` files. Or maybe we just misremembered the rule :)
RELNOTES=n/a
PiperOrigin-RevId: 955458218
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
This initial CL doesn't take advantage of that by changing `guava-android` to expose more APIs that use types `Function` or `Stream`. That said, we have already been exposing _some_ such APIs for a while now. We are likely to expose more in the future.
This `minSdkVersion` bump follows [other Google foundational libraries](https://issuetracker.google.com/issues/474169350), and we expect the bump itself to have no practical impact on users.
RELNOTES=Our Android `minSdkVersion` is now 24 (Nougat). This follows the minimum of Google's foundational Android libraries, and we expect it to have no practical impact on users.
PiperOrigin-RevId: 953524131
I'd have to do more research to figure out _exactly_ when they became unnecessary, but they've always been weird.
RELNOTES=n/a
PiperOrigin-RevId: 953384319
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
This keeps tests from being fragile when multiple jars on the classpath provide classes from the package under test. Such splits have their risks, though typically they're minor, especially for a simple split between the prod and test code. And even then, the ordering in which we test classes shouldn't matter, aside from unusual cases like static state. But, well, we found a way to make things interesting by [making the test pass if the _first_ failure it encounters is of a specific form](https://github.com/google/guava/blob/5143dbe06b8a1632b50c8fe19b2870fe135e46e2/guava-tests/test/com/google/common/graph/PackageSanityTest.java#L69-L79).... I'll be fixing that test in cl/948445604.
RELNOTES=n/a
PiperOrigin-RevId: 948960851
cpovirk