[pull] master from google:master #247
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Bumps the github-actions group with 1 update: [github/codeql-action](https://github.com/github/codeql-action). Updates `github/codeql-action` from 3.28.12 to 3.28.13 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@5f8171a...1b549b9) Fixes #7754 RELNOTES=n/a PiperOrigin-RevId: 742743995
RELNOTES=n/a PiperOrigin-RevId: 742818259
…ev.java/learn/modules/implied-readability/), and improve module deps in `testlib`, too. See #2970 (comment) RELNOTES=Modified the `guava` module's dependency on `failureaccess` to be `transitive`. Also, modified the `guava-testlib` module to make its dependency on `guava` transitive, to remove its dependency on `failureaccess`, and to add a dependency (`transitive`) on `junit`. PiperOrigin-RevId: 742835889
We already do for `maven-source-plugin`. Timings for `mvn clean install -DskipTests`: - Before: Total time: 08:24 min - After: Total time: 02:29 min The slowness that this CL addresses only surfaces under certain configurations, perhaps those with large/remote definitions of Unix groups? I worked around it a while back for my main machine by using `chgrp`, but I was reminded of it today by unicode-org/icu#3459, and I was able to reproduce the slowness on a temporary machine that I'm using on account of some network trouble and an impending desk move. I don't expect for GitHub CI to be affected, but I do wonder if this could help with our local releases. I know that our releases are slow because they have to run a lot of tests, but maybe they have been further slowed by the problem I'm addressing here? After all, they build under `/tmp` by default, and my `chgrp` workaround wouldn't affect that. If only Maven's output weren't [so verbose that our release script has to hide most of it](https://www.club.cc.cmu.edu/~cmccabe/blog_the_bad_build_system_drinking_game.html#:~:text=Take%20three%20shots%20if%20it%20generates%20more%20than%20a%20megabyte%20of%20output)... RELNOTES=n/a PiperOrigin-RevId: 742856755
This test doesn't exercise any alternative `AtomicHelper` implementations; it merely checks that `AbstractFutureState` selects the implementation that we expect under each given environment. This CL is prompted by my discovery today that I'd gotten a Proguard config wrong. I corrected the config in cl/742724817, but this new test suggests that (at least in under our monorepo's build toolchain, which differs from typical external Android toolchains!) `AbstractFutureState` continued doing the right thing throughout, even under optimization/minification (which I tested locally), apparently because the optimizer autodetected that we were reflecting on the fields. Still, we shouldn't rely on that. I also cleaned up the existing `AtomicHelper` tests to use a package-private method instead of reading a `private` field. The motivation for that change is that my new test was failing under minification because the class name did not match the expected value. That forced me to expose something more stable about the choice of `AtomicHelper` to the tests. And once I had that, I figured that I might as well use it in the existing tests. Maybe someday I'll update `AggregateFutureStateFallbackAtomicHelperTest` similarly. This CL continues [work to migrate `guava-android` off `Unsafe`](#7742). RELNOTES=n/a PiperOrigin-RevId: 742859752
…n run under the JVM. Under Android, I still don't have us even _try_ `VarHandle`: - I'm not sure that using `VarHandle` is possible with our current internal Android build setup, as discussed in the internal commentary on cl/711733182. - Using `VarHandle` there may at least somewhat more complex: My testing suggests that some versions of Android expose `VarHandle` to our reflective check but don't actually expose `MethodHandles.Lookup.findVarHandle`. Accordingly, sgjesse@ [recommends using a check on `SDK_INT`](https://issuetracker.google.com/issues/134377851#comment4) ([33](https://developer.android.com/reference/java/lang/invoke/MethodHandles.Lookup#findVarHandle(java.lang.Class%3C?%3E,%20java.lang.String,%20java.lang.Class%3C?%3E)) or higher) instead of reflection under Android. Since `guava-android` can be used under the JVM, too, we'd need to use a combination of the SDK check _and_ reflection. And we'd need to perform the `SDK_INT` check reflectively, as [we do in `TempFileCreator`](https://github.com/google/guava/blob/266ce0022a1c54244df1af0bde81116ed7703577/guava/src/com/google/common/io/TempFileCreator.java#L75) (which, hmm, I should clean up as obsolete one of these days...). (We could at least _reduce_ the amount of reflection we need if we were to depend on the Android SDK at build time, as discussed in b/403282918.) - I have no idea whether `VarHandle` is faster or slower than `Unsafe` there (and I can't easily tell because of the build problems above). - I'm not aware of efforts to remove `Unsafe` under Android. This CL has two advantages for JVM users of `guava-android`: - It eliminates a warning about usage of `Unsafe` under newer JDKs. Note that `AbstractFuture` _already_ has run correctly if access to `Unsafe` is outright disallowed (as with `-sun-misc-unsafe-memory-access=deny`): It detects this and falls back to an alternative implementation. However, if `Unsafe` is available but produces warnings, `guava-android` would use it, triggering those warnings. This CL makes Guava not even try to use it under newer JVMs because it now tries `VarHandle` first. - `VarHandle` may be marginally faster than `Unsafe` under the JVM, as discussed in cl/711733182. (It also doesn't lead to VM crashes if you manage to pass `null` where you shouldn't, as I did back in b/397641020 :) But that's more something that's nice for us as Guava developers, not something that's nice for Guava _users_.) This CL is probably the most _prominent_ part of [our migration of `guava-android` off `Unsafe`](#7742). It is debatable whether it is the most _important_, given that [at least one class, `Striped64`, does not seem to have a fallback at all if `Unsafe` is unavailable](#6806 (comment)). Still, this CL addresses the warning that most users are seeing, and it gives us some precedent for how to deal with `Striped64`. Finally, it looks like our existing tests for `VarHandle` had [a mismatch between the context class loader and the class loader that we load `AbstractFutureTest` in](https://github.com/google/guava/blob/266ce0022a1c54244df1af0bde81116ed7703577/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java#L105-L107)? That seems fairly bad. This CL fixes it, extracting a method to guard against future such mismatches. Out of an abundance of caution, I made a similar change in `AggregateFutureStateFallbackAtomicHelperTest`, even though there's not really an opportunity for a mismatch there, given that there's only one alternative class loader. RELNOTES=`util.concurrent`: Changed the `guava-android` copy of `AbstractFuture` to try `VarHandle` before `Unsafe`, eliminating a warning under newer JDKs. PiperOrigin-RevId: 743198569
That's a safe assumption only under Android, of course. But JVM users of Guava should almost certainly also be fine because they're extremely unlikely to be running a Proguard-style optimizer over their code, given that such optimizers are normally used only for Android apps. If this does cause you trouble, though, let us know, and we can arrange to publish different Proguard configurations for our Android and JRE flavors, at which point the problem would remain only for users who use `guava-android` under the _JVM_ (as is common for Robolectric tests) _and_ use an optimizer. And for now, that problem would be only a warning (from the fallback to `Unsafe`), with the eventual fate that the code would fall all the way back to `AtomicReferenceFieldUpdater` (when `Unsafe` is disabled)—which appears to perform equivalently to `Unsafe` on the JVM, anyway, IIRC :) (Internally, we _already_ publish separate Proguard config files for our two flavors, so there, I'm changing only the Android one.) The benefit of this CL is that it lets optimizers strip away the `VarHandle` code entirely. That's nice just for the code-size savings, but it's also nice for anyone who optimizes or otherwise analyzes the _result_ of an optimizer: It eliminates any need for such a person to include appropriate `-dontwarn` lines for our `VarHandle` usages. That said, I would be nervous about trying to *re*-optimize the optimizer's output without including our Proguard configuration. (Including our Proguard configuration would prevent the error, since we already include a `-dontwarn` line. Note, though, that the optimizer may have to update the Proguard configuration so that it continues to refer to appropriate classes after renaming and other optimizations!) Optimizers are likely to do a good job with "simple" cases like the reflection in `AbtractFutureState`, but they might get things wrong in more complex classes. RELNOTES=n/a PiperOrigin-RevId: 743670573
Note that, even assuming that I'm right that our code does not need these lines, it's possible that _downstream users_ need these flags for _their own_ code. If so, they may see failures after this change, and they can solve them by adding `-dontwarn` lines like the ones that I'm removing. Details on the lines I'm removing: - The `Modifier` suppression in `annotations` was probably never necessary externally and likely became unnecessary internally after cl/510419614. That said, I can't seem to reproduce the warning they were suppressing even when I roll cl/510419614 back, so the actual "fix" may have been a change to Android or our build system. - `addSuppressed` has been part of Android [since API Level 19](https://developer.android.com/reference/java/lang/Throwable#addSuppressed(java.lang.Throwable)). - `@SafeVarargs` has been part of Android [since API Level 19](https://developer.android.com/reference/java/lang/SafeVarargs). - In both cases, I don't even think we needed to wait until our _`minSdkVersion`_ hit 19, just until we were confident that all our users were _compiling_ against 19+. That happened long ago. RELNOTES=n/a PiperOrigin-RevId: 743995869
As usual, Guava continues to be usable from Java 8; this CL changes only the build tools that we use to produce it. RELNOTES=n/a PiperOrigin-RevId: 744082109
…g after Android optimization. This is similar to cl/742859752 and cl/742922048, which did the same for `AbstractFutureState`. RELNOTES=n/a PiperOrigin-RevId: 744264888
This ensures that they are accessible to methods like `AtomicIntegerFieldUpdater.newUpdater` no matter which `util.concurrent` class those methods are called from. Previously, we were trying to arrange for those methods to be called from `AggregateFutureState` itself—specifically, through methods like `remainingCountUpdaterFromWithinAggregateFutureState`. However, we're finding that optimizers sometimes inline those methods into callers in other classes, leading to the warning "SafeAtomicHelper is broken!" Rather than try to prevent inlining with `-dontinline` directives, we instead give in and expand the fields' visibility. We already did this for `AbstractFutureState` in cl/742334547, albeit for somewhat different reasons, so we can follow the same playbook here: Rename the fields to make them harder to use by accident (as in cl/741607075), and update our Proguard config for the rename (as in cl/742724817). At that point, we might as well inline methods like `remainingCountUpdaterFromWithinAggregateFutureState` ourselves just to simplify the code, so I've done so. (Note that even `private` fields "should" be accessible to nested classes, thanks to nestmates. However, that's [not the case with `-source 8 -target 8`](https://github.com/google/guava/blob/a429676a3bb68ac9b29cea0f15ae65065bdf5a44/guava/src/com/google/common/util/concurrent/AbstractFutureState.java#L397-L402), and apparently it's not the case for Android, as well, even without `-source 8 -target 8`.) RELNOTES=`util.concurrent`: Modified our fast paths to ensure that they continue to work when run through optimizers, such as those commonly used by Android apps. This fixes problems that some users may have seen since [Guava 33.4.5](https://github.com/google/guava/releases/tag/v33.4.5). (b8dcaed) PiperOrigin-RevId: 744855670
Compare such CLs as cl/725785821 and cl/738771413. Also, eliminate some diffs between the two flavors, and improve the documentation of `ImmutableMapValues` serialization. RELNOTES=n/a PiperOrigin-RevId: 745110143
RELNOTES=n/a PiperOrigin-RevId: 745123658
RELNOTES=n/a PiperOrigin-RevId: 745214427
RELNOTES=n/a PiperOrigin-RevId: 745302254
…nings in tests. A couple notes on the `primitives` changes: - I realized that we might as well use `Double.hashCode(double)` and `Float.hashCode(float)` there instead of boxing and calling the instance `hashCode()` method. (I'm going to change the _prod_ code in another CL.) - We probably don't even need to make sure that the expected values in our assertions have the exact right runtime type, given that Truth considers, e.g., the `Integer` value `1` to be equal to the `Long` value `1L`. I still kept the types matching when it was easy to do (e.g., `1.0`, `1.0f`), but I didn't bother when it requires a cast (`(short) 1`). PiperOrigin-RevId: 745662180
RELNOTES=n/a PiperOrigin-RevId: 746063845
RELNOTES=n/a PiperOrigin-RevId: 746145356
…android` runs under a JVM. For much more discussion, see the code comments, especially in the backport copy of `AbstractFutureState`. (For a bit more on performance, see https://shipilev.net/blog/2015/faster-atomic-fu/, including its notes that `Unsafe` is not necessarily faster than `AtomicReferenceFieldUpdater`.) Now that we no longer use `VarHandle` under Android, we can remove some Proguard rules for it: - We no longer need the `-dontwarn` rule for `VarHandleAtomicHelper`, since that type no longer exists in `guava-android`. - We no longer need the `-assumevalues` rule for `mightBeAndroid`, since it served only to strip the `VarHandle` code (to hide it from optimizers that run on the optimized code). And all else being equal, we'd rather _not_ have that rule _just in case_ someone is running `guava-android` through an optimizer and using it under the JVM (in which case we'd like to follow the JVM code path so that we don't try to use `Unsafe`). (OK, maybe it would be nice to keep the rule just so that Android doesn't have to perform a check of the `java.runtime.name` system property once during `AbstractFutureState` initialization. If anyone finds this to be an issue, please let us know.) I've also updated the tests to better reflect which ones we run only under a JVM, not in an Android emulator. (I should really have done that back in cl/742859752.) Fixes #7769 RELNOTES=`util.concurrent`: Removed our `VarHandle` code from `guava-android`. While the code was never used at runtime under Android, it was causing [problems under the Android Gradle Plugin](#7769) with a `minSdkVersion` below 26. To continue to avoid `sun.misc.Unsafe` under the JVM, `guava-android` will now always use `AtomicReferenceFieldUpdater` when run there. PiperOrigin-RevId: 746800729
RELNOTES=n/a PiperOrigin-RevId: 746821437
…t JDK methods, and put `@InlineMe` on them. On the JVM, the methods have been available since Java 8. On Android, the methods weren't added [until API Level 24](https://developer.android.com/reference/java/lang/Double#hashCode(double)), but they are among [the methods that are _always_ desugared](https://r8.googlesource.com/r8/+/05a06bdb851d3c286fb69dc83e956d7947da7a19/src/main/java/com/android/tools/r8/ir/desugar/BackportedMethodRewriter.java), even for users who don't opt in to desugaring. Also, remove the warnings about different behavior under GWT: It was [fixed](gwtproject/gwt@3fc5c64) years ago, as our tests will now verify. Also, update tests to test against the JDK methods instead of boxing. (Compare cl/745579695.) RELNOTES=n/a PiperOrigin-RevId: 746833146
- Some are really about lambdas, so they apply to everyone, JVM and Android alike. I did add caveats about `Serializable`. (I could also add caveats about `equals`, but I didn't.) - Some are about methods that are [always desugared](https://r8.googlesource.com/r8/+/refs/heads/main/src/main/java/com/android/tools/r8/ir/desugar/BackportedMethodRewriter.java) under Android, even for developers who don't opt in to library desugaring, so they, too, apply to everyone, so we can `@InlineMe` them. - A few are about primitive constants, which are inlined into our own code when we use them (and into developers' own code when they use them), so they apply to everyone, as well. - There is I think just _one_ method that actually depends on _APIs_ that aren't available except to developers who opt in to library desugaring. I skipped `UnsignedInts` and `UnsignedLongs` before: As I think I discovered once previously, they use methods that aren't available under J2CL and probably other environments. RELNOTES=n/a PiperOrigin-RevId: 746896418
…opped running it internally. While at it, also remove an _`AndroidJdkLibsChecker`_ suppression from some JRE code in `AbstractFutureState` that I have given up on using from Android. RELNOTES=n/a PiperOrigin-RevId: 747424251
…ly stopped running it internally. RELNOTES=n/a PiperOrigin-RevId: 747441237
…he one in `Shorts`. (The annotations are from cl/746833146 and cl/746896418.) RELNOTES=n/a PiperOrigin-RevId: 747452059
RELNOTES=n/a PiperOrigin-RevId: 747502562
- Renamed `SetOperationsTest` to `SetViewTest` to make it easier to find. - Added `symmetricDifference` tests to the exhaustive test suite. - Refactored existing tests for `SetView`s to test independent behaviors separately. - Removed redundant tests from `SetsTest` so it's more obvious where to add new tests for `SetView`. RELNOTES=n/a PiperOrigin-RevId: 747522991
This has been unused since cl/138100073. RELNOTES=n/a PiperOrigin-RevId: 747539199
RELNOTES=`graph`: Added an `asNetwork()` view to `Graph` and `ValueGraph`. PiperOrigin-RevId: 815909867
PiperOrigin-RevId: 815956673
RELNOTES=`graph`: Added an `asNetwork()` view to `Graph` and `ValueGraph`. PiperOrigin-RevId: 816230549
… use the `Duration`-based APIs instead. RELNOTES=deprecate legacy CacheBuilder APIs PiperOrigin-RevId: 816309096
RELNOTES=n/a PiperOrigin-RevId: 816777003
… cl/305727790. (Also, newly link to `Executor` from `MoreExecutors`.) RELNOTES=n/a PiperOrigin-RevId: 816800946
RELNOTES=n/a PiperOrigin-RevId: 817332567
RELNOTES=n/a PiperOrigin-RevId: 817675087
RELNOTES=n/a PiperOrigin-RevId: 817823919
. I had thought (here _and_ during the review of 6ab3006): "Oh, maybe we could simplify this code because we don't _build under_ Java 8 anymore." But it looks like the problem we'd encountered was a problem with Java 8 _and higher_, not a problem that _went away after_ Java 8. (I found history in cl/98448037 and the threads linked from the review discussion there.) And I suspect that Java 8 is in the right. (Not that the code is "unsafe," just that we're doing things that look like they deserve the `unchecked` warnings that we're getting.) It's possible that cl/98448037 went unnecessarily far in _also_ changing `ImmutableEnumSet` itself, after which we undid that part 6ab3006. Now I'm back for `ImmutableSet.copyOf` itself, with goals like: - improve the suppression comment - localize the suppression - reduce diffs between mainline and backport - maybe prefer `unchecked` warnings over `rawtypes` warnings, as the latter seem to have given us more trouble before (as in 6ab3006) I'd originally intended to accomplish this by performing an unchecked cast to a set with arbitrary enum type (`Set<TimeUnit>`), operating on that, and then casting back to `ImmutableSet<E>` at the end. (`TimeUnit` wouldn't show up anywhere in the class file except in the local-variable table.) But after I got that working, I decided to ask Gemini, and it pointed out that we can do plenty with a wildcarded `EnumSet<?>`. I wonder if that approach didn't work at some point in the past, like when we actually did build under Java 8? Maybe I'm about to unwittingly break GWT/Eclipse? (If we encounter a problem specifically with `clone()`, I've had good results sticking with `EnumSet.copyOf` as an alternative.) RELNOTES=n/a PiperOrigin-RevId: 818031035
(followup to 909c593) (Arguably, we should backfill more `@since` tags in `AbstractGraph` and `AbstractBaseGraph` (and not just for `asNetwork()`), but I find it hard to get excited about that.) RELNOTES=n/a PiperOrigin-RevId: 819762762
PiperOrigin-RevId: 822133729
PiperOrigin-RevId: 822664336
RELNOTES=n/a PiperOrigin-RevId: 825062439
(Also, manually sneak in a change from `assertThat(foo.size()).isGreaterThan(0)` to `assertThat(foo).isNotEmpty()`.) (I also considered merging some `isLessThan`+`isGreaterThan` pairs into `isIn`, but I didn't go for it.) RELNOTES=n/a PiperOrigin-RevId: 825125053
RELNOTES=n/a PiperOrigin-RevId: 825137592
We're seeing some timeouts under Android. (I've worried for years that Truth might be too expensive in tight loops, including on Android, and this is the first time I recall hearing that it actually happened! I'm sure it's happened somewhere without my hearing about it, but still.) PiperOrigin-RevId: 825192799
…und what seems to be a long-since-fixed ART bug. The test broke after d417c5b, going undetected at presubmit time because a tool decided to skip some of our tests to save resources. RELNOTES=n/a PiperOrigin-RevId: 825701376
RELNOTES=n/a PiperOrigin-RevId: 826017956
RELNOTES=n/a PiperOrigin-RevId: 826585931
Bumps the github-actions group with 2 updates: [actions/upload-artifact](https://github.com/actions/upload-artifact) and [github/codeql-action](https://github.com/github/codeql-action). Updates `actions/upload-artifact` from 4.6.2 to 5.0.0 - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@ea165f8...330a01c) Updates `github/codeql-action` from 3.30.5 to 4.31.2 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@3599b3b...0499de3) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-version: 5.0.0 dependency-type: direct:production update-type: version-update:semver-major dependency-group: github-actions - dependency-name: github/codeql-action dependency-version: 4.31.2 dependency-type: direct:production update-type: version-update:semver-major dependency-group: github-actions ... Signed-off-by: dependabot[bot] <[email protected]>
While there, rename a parameter in a test helper method to match the production parameter name. I admit that my new test is not necessarily the best of all possible tests: It detects the problem currently, but it wouldn't detect the problem if we were to change the prod code to decompose the `Duration` only [when it actually needs to block](https://github.com/google/guava/blob/e416f493d7f6790cae859aab1ddacb456469f614/guava/src/com/google/common/collect/Queues.java#L336). I kind of doubt we'd ever bother, though. RELNOTES=n/a PiperOrigin-RevId: 828005669
RELNOTES=n/a PiperOrigin-RevId: 828573832
Most `toString()` implementations mimic the code that could have produced the instance. Notably, `someCharMatcher.negate()` produces `someCharMatcher.toString() + ".negate()"`. But the `toString()` for `.and` and `.or` used a syntax that made it look as if these were static methods rather than instance ones. Also update a couple of tests that depend on the exact `toString()` output. RELNOTES=n/a PiperOrigin-RevId: 829198847
…6f4b did for `ImmutableSet`. Plus, expand the comment on `ImmutableSet` to acknowledge a second reason for the `unchecked` warning there. RELNOTES=n/a PiperOrigin-RevId: 829442016
…dundantNullCheck`](https://errorprone.info/bugpattern/RedundantNullCheck). `RedundantNullCheck` was from google/error-prone#5121. I had already taken some of its suggestions in 125fd02, but I'd been on the fence about these ones. What pushed me over the edge was that my 34f3253 causes failures in nullness checking. I'm sure I could find another way to resolve those, but this approach probably makes more sense, anyway: If we imagine that nullness is enforced as in Kotlin (or possibly someday Java), then the call to `checkEntryNotNull` would immediately throw a `NullPointerException`, possibly with no message, instead of allowing the method to execute far enough to produce our custom `NullPointerException`, which includes both the key and the value. (I think there were a handful of other parameters like this one on which I also had declined to add `@Nullable`. I'm not revisiting that right now, but we could someday.) RELNOTES=n/a PiperOrigin-RevId: 830586416
… with `skipPublishing`. ...by rolling back 119bd9b. The bug was fixed in [release 0.9.0](https://central.sonatype.org/publish/publish-portal-maven/#090), which we upgraded to in 453a788, and we've validated as much with AutoValue cl/831014897. PiperOrigin-RevId: 831051885
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
14 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot]
Can you help keep this open source service alive? 💖 Please sponsor : )