-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Learn from non-null test through suppression operator #32078
Conversation
slotBuilder.Free(); | ||
var slotBuilder = ArrayBuilder<int>.GetInstance(); | ||
GetSlotsToMarkAsNotNullable(receiverOpt, slotBuilder); | ||
if (slotBuilder.Count > 0) |
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.
if (slotBuilder.Count > 0) [](start = 16, length = 26)
Consider removing the check since it looks unnecessary.
x!.F1(x); // 1 | ||
x.F1(x); // 2 | ||
x!.F1(x); | ||
x.F1(x); | ||
x.F1(x); |
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.
Perhaps remove the last x.F1(x);
since it's now redundant.
{ | ||
void M(int? i) | ||
{ | ||
i!!.Value.ToString(); |
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.
!! [](start = 9, length = 2)
Is use of !
twice intentional?
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.
Yes, to verify that we see through two layers
Consider expanding testing to other scenarios that use |
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 (iteration 1)
approved |
public string? field; | ||
void M(C? c) | ||
{ | ||
c?.field!.ToString(); |
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.
📝 This scenario makes me wonder if we should warn about unnecessary null-conditional dereference (you should remove the ?
).
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.
Filed #32105
Fixes #31733