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
And update `NavigableMapTestSuiteBuilder` to require immutable entries from those methods.
For simplicity, I'll focus on the `NavigableMap` case in the description below:
`Maps.filterKeys`, `filterValues` and `filterEntries` on a `NavigableMap` returned the backing map's live entries from their navigation methods. This caused two problems:
### Removing an `Entry` from the backing map might mutate the `Entry`
`pollFirstEntry()` and `pollLastEntry()` removed the entry through the backing `entrySet` iterator and then returned that same entry object.
When `TreeMap` removes a node with two children, it copies the successor's key and value into that node, so the returned entry could show a different mapping.
For example:
- `Maps.filterKeys(TreeMap{1=one, 2=two, 3=three}, k -> k != 1) .pollFirstEntry()` returned `3=three` instead of `2=two`.
- Draining `filterKeys(TreeMap{0..9}, odd)` with `pollFirstEntry()` returned keys [2, 4, 6, 7, 9] instead of [1, 3, 5, 7, 9].
### Navigation methods are supposed to return snapshots
`firstEntry()`, `lastEntry()`, `ceilingEntry()`, `floorEntry()`, `higherEntry()` and `lowerEntry()` returned live entries whose `setValue` wrote through to the backing map. This is bad for two reasons:
- [`NavigableMap`](https://docs.oracle.com/en/java/javase/27/docs/api/java.base/java/util/NavigableMap.html) specifies that these methods return snapshot entries that don't support `setValue`.
- Even if they were to support `setValue`, maps that are filtered by value (or by the full entry contents) should check that the predicate allows the new value.
---
Now, all these methods return snapshots.
But we continue to override `entrySet()` in a way that allows its entries to support `setValue` (with the predicate check).
Finally, I've moved `Iterables.removeFirstMatching` from `Iterables` to `Sets`. That is now the only location that it's used from, since it is no longer used from `Maps`. Along the way, I renamed it to "`pollFirstMatching`" to more closely match the methods that it's used from.
Fixes https://github.com/google/guava/pull/8692
I filed b/566253750 for a similar question in `SortedMultiset`.
RELNOTES=n/a
PiperOrigin-RevId: 988966779
`toSortedList` delegates to `Ordering.immutableSortedCopy`, which in turn
delegates to `ImmutableList.sortedCopyOf`. Both of those document that the
sort is stable, but `toSortedList` did not say so. Add the same sentence
that `ImmutableList.sortedCopyOf` uses.
Fixes#2884
Claude-Session: https://claude.ai/code/session_01X59fWmqnA4Nmb9rggkTP7n
Fixes https://github.com/google/guava/pull/8691
RELNOTES=n/a
PiperOrigin-RevId: 987463733
`TransformingRandomAccessList` and `TransformingSequentialList` previously inherited default implementations from `AbstractList`/`AbstractSequentialList`, which lost the backing list's spliterator characteristics (e.g., `IMMUTABLE`), sized/subsized properties, and custom `forEach()` optimizations.
Includes a suggestion from @kojallipalli2210-alt on #8589.
Fixes https://github.com/google/guava/issues/8165.
Closes https://github.com/google/guava/pull/8589.
RELNOTES=`collections`: In `Lists.transform`, delegate `spliterator()` and `forEach()` to the backing list.
PiperOrigin-RevId: 986925834
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
- In SetViewTest, suppress ErrorProne's `VoidMissingNullable` in `SetContainsThrows` rather than changing the type argument to `@Nullable Void`. This allows Kotlin/Native to compile `AbstractSet<Void>` successfully without incorrectly implying that the set can contain null elements.
- In TableCollectionTest, extract the `suite()` method and its associated helper methods into a new `TableCollectionTestSuite` class, and remove `extends TestCase` from `TableCollectionTest`. This properly isolates the test infrastructure and prevents test runner issues where making the original class abstract caused test suites to be silently dropped on JVM and J2KT environments.
PiperOrigin-RevId: 972700907
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
cpovirk