-
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
Use [NotNullWhen(true)] in more places #47598
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
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.
Left a few comment, overall looks good to me
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector128_1.cs
Show resolved
Hide resolved
...Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSObject.cs
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.
LGTM with minor comment
Did a quick search/audit for methods that returned bool and took nullable object as the first input, and added [NotNullWhen(true)] where it was obviously correct.
ce82919
to
608d70c
Compare
The issue fixed by #47579 inspired me to search for other places the consuming experience could be improved by using
[NotNullWhen(true)]
, and found a lot. Most of these are overrides of object.Equals, but some are TryParse methods, other Equals overloads, and a smattering of other APIs. I essentially just did a quick regex search for methods that returned bool and took nullable reference type as the first input, and then added [NotNullWhen(true)] where it was obviously correct (I skipped over a few large areas, namely reflection, as this was already getting a bit out of hand).For reference, the benefit of applying
[NotNullWhen(true)]
to a nullable argument is it lets the compiler know that if the method returns true then that argument wasn't null when the method was called, which lets you write code like:Note that for the most part I stayed away from modifying virtual methods, but obviously some of the overrides I modified aren't sealed, so technically this could be a breaking change (if someone derives from one of those types and overrides Equals and has nullable enabled and doesn't have [NotNullWhen(true)] on the argument). I can undo any folks feel queasy about.
cc: @jeffhandley, @krwq, @buyaa-n