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
`rotate(array, distance, fromIndex, toIndex)` computed `length` and `-distance % length` without guarding an empty range, so a call with `fromIndex == toIndex` threw `ArithmeticException`. Return early when the range has fewer than two elements.
Fixes https://github.com/google/guava/pull/8772
Fixes https://github.com/google/guava/pull/8774
RELNOTES=`primitives`: Fixed `ArithmeticException` when calling `rotate` on an empty subrange of a multi-element array.
PiperOrigin-RevId: 1006325060
- 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
`ClassInfo.isTopLevel()` (and so `ClassPath.getTopLevelClasses()`) assumed that any `$` in a class name marks a nested class, so generated top-level classes like `$AutoValue_Foo` were silently dropped. `getSimpleName()` had the same problem, returning `AutoValue_Foo`.
_Consecutive_ `$` characters generally appear in runtime-generated classes, in which case they are outside the domain of `ClassPath`. However, they can appear at compile time, too, such as in desugared Android code—though, in the specific example of Android, `ClassPath` may well not work anyway. In any case, such classes don't represent true nested classes.
This new logic is all still based on heuristics, but it handles the common cases of generated class names.
Fixes https://github.com/google/guava/pull/8740
Fixes https://github.com/google/guava/issues/3349 well enough in practice.
RELNOTES=n/a
PiperOrigin-RevId: 1004273644
`decodingStream` methods to synchronize their methods as typical of
`InputStream` and `OutputStream` implementations.
RELNOTES=n/a
PiperOrigin-RevId: 1004050756
`Double.MIN_VALUE` and `Float.MIN_VALUE` represent the smallest positive nonzero values (`2^-1074` and `2^-149`) rather than the most negative finite values, unlike `Integer.MIN_VALUE` and `Long.MIN_VALUE`.
RELNOTES=`primitives`: Added `Doubles.MIN_POSITIVE_VALUE` and `Floats.MIN_POSITIVE_VALUE` as more clearly named aliases for `Double.MIN_VALUE` and `Float.MIN_VALUE`.
PiperOrigin-RevId: 1004024484
`FilteredEntryMap.EntrySet` wrapped only the entries returned by `iterator()`, so the entries returned by `toArray()` were the backing map's own entries, and their `setValue` bypassed the filter.
The fix is to override `toArray`. But instead of using `standardToArray` like `AbstractBiMap.EntrySet` and `UnmodifiableEntries` do, we use `newArrayList(iterator()).toArray(...)` like `FilteredMapValues` and `FilteredEntryMap.KeySet` do so that we avoid evaluating the predicate against all elements twice (once in `size()` and once in `iterator()`).
- Fixes https://github.com/google/guava/pull/8758
- Support for https://github.com/google/guava/issues/1575
RELNOTES=n/a
PiperOrigin-RevId: 1003049749
`ThreadFactory.newThread` is permitted to return `null` to reject a request to create a thread. Previously, a factory built by `ThreadFactoryBuilder` threw `NullPointerException` if its backing factory returned `null`.
Fixes https://github.com/google/guava/issues/3568.
Closes https://github.com/google/guava/pull/8742.
RELNOTES=In the `ThreadFactory` returned by `ThreadFactoryBuilder`, a null return from `newThread` on the backing `ThreadFactory` is now propagated to the caller. Previously it triggered a NullPointerException.
PiperOrigin-RevId: 1001877586
_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
inconsistent with concurrent updates.
`SimpleStatsCounter` is documented as thread-safe, but it updates its counters independently
and without locking, so a snapshot taken during an update can include part of it (for example,
a load-success count without the corresponding load time). That's intentional for performance,
so document it rather than adding synchronization.
Fixes https://github.com/google/guava/pull/8741
RELNOTES=n/a
PiperOrigin-RevId: 999541957
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
But (for better or for worse) preserve the existing lenient behavior of `setCount(element, 0, 0)`.
Fixes https://github.com/google/guava/pull/8720
RELNOTES=n/a
PiperOrigin-RevId: 995897499
`LimitedInputStream.skip` passed a negative count on to the delegate. `FileInputStream` then seeks backward and returns a negative value, which was subtracted from the bytes left, so the stream could move before the position where the limit began and supply more than the limit.
Fixes https://github.com/google/guava/pull/8712
RELNOTES=n/a
PiperOrigin-RevId: 994451753
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
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
cpovirk