Don't suggest to add null checks to nullable reference types#53249
Conversation
If a parameter is marked as nullable, then we shouldn't be adding a check for it to be non-null.
Youssef1313
left a comment
There was a problem hiding this comment.
The refactoring can still be useful in a very limited number of cases (apply it, then manually extend the check). Example:
However, it should be very rare, so I'm not opposing the change.
|
@jinujoseph: this would need your approval for going into 16.11. Easy bugfix, very low risk, and honestly kinda embarrassing we missed this. 😄 |
|
@Youssef1313: interesting case! Yep, that's rare and also in this case honestly that's only mattering in the non-generic interface which is more or less something we pretend doesn't exist. |
|
i don't think this meets any sort of bar . it's a refactoring. i think it was fine that it was offered before. it's not great, but it's nbd imo. |
|
@CyrusNajmabadi: this was in my way for the "apply to all" case in #53246 -- it meant I couldn't use "add checks for all" at the public entrypoints after annotating without immediately auditing to make sure it didn't add one I didn't want. |
I understand :) it's just not something i think meets any sort of bar here. It's in line with all our other refactorings that do something wrong some of the time. |
| if (parameter.Type.IsReferenceType) | ||
| { | ||
| // Don't add null checks to things explicitly declared nullable | ||
| if (parameter.Type.NullableAnnotation == NullableAnnotation.Annotated) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| else if (!parameter.Type.IsNullable()) |
There was a problem hiding this comment.
| if (parameter.Type.IsReferenceType) | |
| { | |
| // Don't add null checks to things explicitly declared nullable | |
| if (parameter.Type.NullableAnnotation == NullableAnnotation.Annotated) | |
| { | |
| return false; | |
| } | |
| } | |
| else if (!parameter.Type.IsNullable()) | |
| // Don't add null checks to things explicitly declared nullable | |
| if (parameter.Type is { IsReferenceType: true, NullableAnnotation: NullableAnnotation.Annotated }) | |
| return false; | |
| if (!parameter.Type.IsNullable()) |
There was a problem hiding this comment.
@CyrusNajmabadi That will change behavior -- IsNullable is specifically "is this Nullable<T>".
There was a problem hiding this comment.
(might be worth renaming the helper -- let's take that in a separate PR if desired.)
If a parameter is marked as nullable, then we shouldn't be adding a check for it to be non-null.