-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Address some new Roslyn nullability warnings #35782
Address some new Roslyn nullability warnings #35782
Conversation
src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallHandles.cs
Outdated
Show resolved
Hide resolved
src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallHandles.cs
Outdated
Show resolved
Hide resolved
src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallHandles.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/NullabilityHelpers.cs
Outdated
Show resolved
Hide resolved
...raries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs
Outdated
Show resolved
Hide resolved
...s/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncValueTaskMethodBuilderT.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Threading/LazyInitializer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Threading/LazyInitializer.cs
Outdated
Show resolved
Hide resolved
/// <returns>The initialized value of type <typeparamref name="T"/>.</returns> | ||
public static T EnsureInitialized<T>([AllowNull] ref T target, ref bool initialized, [NotNull] ref object? syncLock) | ||
public static T EnsureInitialized<T>([AllowNull] ref T target, ref bool initialized, [NotNullIfNotNull("syncLock")] ref object? syncLock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. This [NotNullIfNotNull("syncLock")]
on the syncLock
parameter is basically saying "the method won't set this ref parameter to null". I wonder if we have other places where we should be adding such an attribute... to my knowledge these if the first time we've used NotNullIfNotNull
to refer to the same parameter it's attributed on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, everywhere we have a ref T foo
parameter, should we be annotating it as [NotNullIfNotNull("foo")]
(just in case T was nullable)? That'd be kind of awful. @jcouv, what's the recommended approach here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along those lines, Volatile.Read<T>(ref T value)
could be a candidate for an [In]
-like attribute. Basically: "The caller must explicitly use the ref keyword to pass an address to this routine (no implicit in conversion allowed), but the routine promises not to assign a new value to the provided address." That attribute would also imply [NotNullIfNotNull("sameParam")]
by construction.
src/libraries/System.Private.CoreLib/src/System/Threading/LazyInitializer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Threading/LazyInitializer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
@stephentoub Were you waiting for your question at #35782 (comment) to be addressed before I merge? Otherwise I'll merge by EOD once CI goes green. |
Hadn't thought of such usage :-) That's clever and I think it's legal/correct. |
Thanks. FWIW, I looked through all of our ref parameters, and there are very few additional ones that would benefit from this treatment (e.g. |
@stephentoub - I wonder if those would benefit from the |
How is that different from |
"in" means that the call site will neither overwrite the target address nor call any API on the target reference which will cause mutation of the value. "in" also doesn't require you to use the "in" keyword when calling the target method, which runs the risk of making hidden copies of locals at the call site. What I'm proposing is more akin to what ECMA refers to as a controlled mutability reference. You still need to use the "ref" keyword at the call site, but the callee guarantees that the target address will not be overwritten. It may however be mutated through other means. The upshot is that such APIs implicitly have a contract "if you call me with a non null ref, I guarantee the ref will remain non null at method exit." |
That's what What you're discussing is a "I guarantee I won't change this reference", and I think the distinctions between that and (And as an aside, |
It would also be the natural return type of |
I tested the runtime build against a recent version of the Roslyn compiler (toolset version 3.7.0-1.20251.4) and saw that it's spitting out additional warnings regarding our use of nullability annotations. This PR fixes those warnings in our code base.
Note: I had to change the public-facing annotations on
LazyInitializer.EnsureInitialized<T>(...)
, as they weren't entirely correct. The original annotations had the parameter[NotNull] ref object? syncLock
, meaning that syncLock would always definitely be assigned upon method exit. This isn't always correct, as assignment may be bypassed if the target is already allocated. Instead, I changed the annotation to[NotNullIfNotNull("syncLock")] ref object? syncLock
, meaning that if syncLock is non-null on method enter, it will also be non-null on method exit.Aside from changing those public-facing annotations this PR introduces no behavioral changes.
Contributes to #34417.