Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Map.replace(K, V, V): Make oldValue @Nullable #118

Open
cpovirk opened this issue Feb 3, 2025 · 1 comment
Open

Map.replace(K, V, V): Make oldValue @Nullable #118

cpovirk opened this issue Feb 3, 2025 · 1 comment

Comments

@cpovirk
Copy link
Collaborator

cpovirk commented Feb 3, 2025

We have it as parametric nullness in Map itself and in ConcurrentMap and presumably other subtypes:

Android has it as @Nullable: https://cs.android.com/android/platform/superproject/main/+/main:libcore/ojluni/annotations/sdk/nullability/java/util/Map.annotated.java;l=72;drc=31fc4306a5baa196245ae16f79009b632ccbe667

As a result of that difference, an Android nullness check in Google's depot gets upset.

It seems like @Nullable would make sense, given the concept of the method and the default implementation: You can't hurt anything by passing null.

But that might be trouble for Kotlin? https://github.com/JetBrains/kotlin/blob/e32761ba660d67e60ddcd3cd1e798331b9f6d9d3/core/compiler.common.jvm/src/org/jetbrains/kotlin/load/java/typeEnhancement/predefinedEnhancementInfo.kt#L169-L174

And we'd want to see if at least some implementations would reject null, similar to contains. (That likely wouldn't change our direction for the interface, but we'd want to keep the type with parametric nullness (or make it non-null) for any such implementations.)

@cpovirk cpovirk changed the title Map.replace(K, V, V): Make oldValue @Nullable Map.replace(K, V, V): Make oldValue @Nullable Feb 3, 2025
@cpovirk
Copy link
Collaborator Author

cpovirk commented Feb 3, 2025

But that might be trouble for Kotlin?

Then again, the existing Android annotation doesn't seem to be causing trouble.

(We had trouble with mismatches between our annotations and Kotlin's idea of how some JDK methods should be annotated before. But I think that was under K1, and I'm not sure if I ever figured out for sure whether there was actually a bug in K1 that made things worse than they were meant to be.)

copybara-service bot pushed a commit to google/guava that referenced this issue Feb 18, 2025
One is from cl/725859649 (I _thought_ I had tested that change with nullness checking on), and the other is from [a change to the annotations for `Map.replace` used by Android](jspecify/jdk#118). (I think that the latter change should actually be leading to _more_ errors in our _other_ implementations of the 3-arg `Map.replace`, like in `Maps` and `Synchronized`. The lack of errors is probably a bug in the checker.)

RELNOTES=n/a
PiperOrigin-RevId: 728167290
copybara-service bot pushed a commit to google/guava that referenced this issue Feb 18, 2025
One is from cl/725859649 (I _thought_ I had tested that change with nullness checking on), and the other is from [a change to the annotations for `Map.replace` used by Android](jspecify/jdk#118). (I think that the latter change should actually be leading to _more_ errors in our _other_ implementations of the 3-arg `Map.replace`, like in `Maps` and `Synchronized`. The lack of errors is probably a bug in the checker.)

RELNOTES=n/a
PiperOrigin-RevId: 728229469
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant