You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I would have commented in the closed NRT PRs but they have all been locked for discussion for some reason.
However I'd like to point out some issues with the PRs: If you mark a property or parameter as non-nullable, but you don't actually throw when setting something to null, the annotation is wrong.
Consider that before an NRT annotation is added, if I can set something to null before, an annotation shouldn't say "you can't set this to null". IE you should only mark something non-nullable if, and only if, inputting a null would cause an exception to occur. If you do say "this value is non-nullable" you still have to check and throw ArgumentNullException on ALL public entry-points into that, to ensure the internals stay true to the promise of something not being null, since null annotations are just warnings, and users might not even have them turned on. You should always assume non-nullable properties might get a null value passed in anyway. If you don't do this, all your internal code paths can't trust that they are getting non-null values, and we'll ultimately end up with even more NullReferenceExceptions than before.
One good example is the following change in this PR: #8676
This bit of code has always allowed nulls, and never validated it couldn't be null. The above change is incorrect. If you want no behavioral change, this annotation should clearly state the Name parameter can be null.
However, if you really wanted to make this non-nullable, well then you MUST validate the input is in fact not null and throw an ArgumentNullException, but now you're likely also introducing a behavioral change (the exception would be that other code would throw a null-reference exception, so now you're merely improving the thrown error message).
The text was updated successfully, but these errors were encountered:
Description
I would have commented in the closed NRT PRs but they have all been locked for discussion for some reason.
However I'd like to point out some issues with the PRs: If you mark a property or parameter as non-nullable, but you don't actually throw when setting something to null, the annotation is wrong.
Consider that before an NRT annotation is added, if I can set something to null before, an annotation shouldn't say "you can't set this to null". IE you should only mark something non-nullable if, and only if, inputting a null would cause an exception to occur. If you do say "this value is non-nullable" you still have to check and throw ArgumentNullException on ALL public entry-points into that, to ensure the internals stay true to the promise of something not being null, since null annotations are just warnings, and users might not even have them turned on. You should always assume non-nullable properties might get a null value passed in anyway. If you don't do this, all your internal code paths can't trust that they are getting non-null values, and we'll ultimately end up with even more NullReferenceExceptions than before.
One good example is the following change in this PR: #8676
![image](https://private-user-images.githubusercontent.com/1378165/312710726-8b9a4cf1-3edf-462d-abe6-0ca2378e5540.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTkxNTI4NzksIm5iZiI6MTcxOTE1MjU3OSwicGF0aCI6Ii8xMzc4MTY1LzMxMjcxMDcyNi04YjlhNGNmMS0zZWRmLTQ2MmQtYWJlNi0wY2EyMzc4ZTU1NDAucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDYyMyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA2MjNUMTQyMjU5WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NzM0MWQ3MWQ1YzA1MzZjOTU4NmUzOWVlNGY0OWUwMTQyYjBjMTVlOTI2ZWUzMjYwMmFhNDc3MDE4MWNmMzE0MSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.-W7cZ7iqeTJyBhLskCEuFQM9RV_RzdGIV2yPwkprDYU)
This bit of code has always allowed nulls, and never validated it couldn't be null. The above change is incorrect. If you want no behavioral change, this annotation should clearly state the Name parameter can be null.
However, if you really wanted to make this non-nullable, well then you MUST validate the input is in fact not null and throw an ArgumentNullException, but now you're likely also introducing a behavioral change (the exception would be that other code would throw a null-reference exception, so now you're merely improving the thrown error message).
The text was updated successfully, but these errors were encountered: