Skip to content

Commit 5ebd9f3

Browse files
cpovirkGoogle Java Core Libraries
authored andcommitted
Make nullness comment slightly less ponderous.
There have been an increasing number of reasons to avoid "lying" about nullness, notably including J2KT. We don't need to get into that here anymore, if ever we did in the first place. Also, use `unsafeNull()` instead of a more broadly scoped suppression. `unsafeNull()` better emphasizes that we know we're violating proper nullness, rather than suppressing a false positive. RELNOTES=n/a PiperOrigin-RevId: 784621004
1 parent 59e3f8a commit 5ebd9f3

File tree

2 files changed

+22
-38
lines changed

2 files changed

+22
-38
lines changed

android/guava/src/com/google/common/collect/LinkedHashMultimap.java

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import static com.google.common.base.Preconditions.checkState;
2020
import static com.google.common.collect.CollectPreconditions.checkNonnegative;
21+
import static com.google.common.collect.NullnessCasts.unsafeNull;
2122
import static java.util.Objects.requireNonNull;
2223

2324
import com.google.common.annotations.GwtCompatible;
@@ -169,23 +170,10 @@ static final class ValueEntry<K extends @Nullable Object, V extends @Nullable Ob
169170
*
170171
* The exception is the *InValueSet fields of multimapHeaderEntry, which are never set. (That
171172
* works out fine as long as we continue to be careful not to try to delete them or iterate
172-
* past them.)
173-
*
174-
* We could consider "lying" and omitting @CheckNotNull from all these fields. Normally, I'm not
175-
* a fan of that: What if we someday implement (presumably to be enabled during tests only)
176-
* bytecode rewriting that checks for any null value that passes through an API with a
177-
* known-non-null type? But that particular problem might not arise here, since we're not
178-
* actually reading from the fields in any case in which they might be null (as proven by the
179-
* requireNonNull checks below). Plus, we're *already* lying here, since newHeader passes a null
180-
* key and value, which we pass to the superconstructor, even though the key and value type for
181-
* a given entry might not include null. The right fix for the header problems is probably to
182-
* define a separate MultimapLink interface with a separate "header" implementation, which
183-
* hopefully could avoid implementing Entry or ValueSetLink at all. (But note that that approach
184-
* requires us to define extra classes -- unfortunate under Android.) *Then* we could consider
185-
* lying about the fields below on the grounds that we always initialize them just after the
186-
* constructor -- an example of the kind of lying that our hypothetical bytecode rewriter would
187-
* already have to deal with, thanks to DI frameworks that perform field and method injection,
188-
* frameworks like Android that define post-construct hooks like Activity.onCreate, etc.
173+
* past them.) The right fix for that problem is probably to define a separate MultimapLink
174+
* interface with a separate "header" implementation, which hopefully could avoid implementing
175+
* Entry or ValueSetLink at all. (But note that that approach requires us to define extra
176+
* classes -- unfortunate under Android.)
189177
*/
190178

191179
private @Nullable ValueSetLink<K, V> predecessorInValueSet;
@@ -204,9 +192,13 @@ static final class ValueEntry<K extends @Nullable Object, V extends @Nullable Ob
204192
this.nextInValueBucket = nextInValueBucket;
205193
}
206194

207-
@SuppressWarnings("nullness") // see the comment on the class fields, especially about newHeader
208195
static <K extends @Nullable Object, V extends @Nullable Object> ValueEntry<K, V> newHeader() {
209-
return new ValueEntry<>(null, null, 0, null);
196+
/*
197+
* We pass a null key and value, even though the key and value type for this multimap might
198+
* not include null. That's likely to be safe as long as no one tries to operate on the key
199+
* and value of a header entry. (But who knows if Valhalla will someday change that.)
200+
*/
201+
return new ValueEntry<>(unsafeNull(), unsafeNull(), 0, null);
210202
}
211203

212204
boolean matchesValue(@Nullable Object v, int smearedVHash) {

guava/src/com/google/common/collect/LinkedHashMultimap.java

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static com.google.common.base.Preconditions.checkNotNull;
2020
import static com.google.common.base.Preconditions.checkState;
2121
import static com.google.common.collect.CollectPreconditions.checkNonnegative;
22+
import static com.google.common.collect.NullnessCasts.unsafeNull;
2223
import static java.util.Objects.requireNonNull;
2324

2425
import com.google.common.annotations.GwtCompatible;
@@ -173,23 +174,10 @@ static final class ValueEntry<K extends @Nullable Object, V extends @Nullable Ob
173174
*
174175
* The exception is the *InValueSet fields of multimapHeaderEntry, which are never set. (That
175176
* works out fine as long as we continue to be careful not to try to delete them or iterate
176-
* past them.)
177-
*
178-
* We could consider "lying" and omitting @CheckNotNull from all these fields. Normally, I'm not
179-
* a fan of that: What if we someday implement (presumably to be enabled during tests only)
180-
* bytecode rewriting that checks for any null value that passes through an API with a
181-
* known-non-null type? But that particular problem might not arise here, since we're not
182-
* actually reading from the fields in any case in which they might be null (as proven by the
183-
* requireNonNull checks below). Plus, we're *already* lying here, since newHeader passes a null
184-
* key and value, which we pass to the superconstructor, even though the key and value type for
185-
* a given entry might not include null. The right fix for the header problems is probably to
186-
* define a separate MultimapLink interface with a separate "header" implementation, which
187-
* hopefully could avoid implementing Entry or ValueSetLink at all. (But note that that approach
188-
* requires us to define extra classes -- unfortunate under Android.) *Then* we could consider
189-
* lying about the fields below on the grounds that we always initialize them just after the
190-
* constructor -- an example of the kind of lying that our hypothetical bytecode rewriter would
191-
* already have to deal with, thanks to DI frameworks that perform field and method injection,
192-
* frameworks like Android that define post-construct hooks like Activity.onCreate, etc.
177+
* past them.) The right fix for that problem is probably to define a separate MultimapLink
178+
* interface with a separate "header" implementation, which hopefully could avoid implementing
179+
* Entry or ValueSetLink at all. (But note that that approach requires us to define extra
180+
* classes -- unfortunate under Android.)
193181
*/
194182

195183
private @Nullable ValueSetLink<K, V> predecessorInValueSet;
@@ -208,9 +196,13 @@ static final class ValueEntry<K extends @Nullable Object, V extends @Nullable Ob
208196
this.nextInValueBucket = nextInValueBucket;
209197
}
210198

211-
@SuppressWarnings("nullness") // see the comment on the class fields, especially about newHeader
212199
static <K extends @Nullable Object, V extends @Nullable Object> ValueEntry<K, V> newHeader() {
213-
return new ValueEntry<>(null, null, 0, null);
200+
/*
201+
* We pass a null key and value, even though the key and value type for this multimap might
202+
* not include null. That's likely to be safe as long as no one tries to operate on the key
203+
* and value of a header entry. (But who knows if Valhalla will someday change that.)
204+
*/
205+
return new ValueEntry<>(unsafeNull(), unsafeNull(), 0, null);
214206
}
215207

216208
boolean matchesValue(@Nullable Object v, int smearedVHash) {

0 commit comments

Comments
 (0)