Adding annotations based on MS guidance #4387
Conversation
|
I'm sure there are people out there that have maps that return null for non-null source. You'll just lie to them? :) Does MS do that anywhere? I've checked System.Text.Json and it does have nullable both on the source and the result. |
| public TDestination Map<TDestination>(object source, Action<IMappingOperationOptions<object, TDestination>> opts) => Map(source, default, opts); | ||
| public TDestination Map<TDestination>(object source) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(source); |
There was a problem hiding this comment.
Ha yes. We should be checking for nulls here when we rely on a not-null source object for getting the type.
|
This is not good :) If you don't want the headache, let's just revert the whole thing. We can simply be nullable oblivious as before. |
|
Then I'd just vote revert. I just tried this on a codebase and had similar headaches - it's not adding anything meaningful in the vast majority of cases. This captures the vast majority of cases that I can think of, as well as our expected built-in behavior. You can get around all the compile-time null checks anyway but this is about describing expected behaviors. But it's not 100% accurate. |
|
It's not worth the trouble. We can simply revert and let people write their own stuff if they must. |
Reviewed the changes and added tests basically to confirm the behavior based on this guidance:
https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/api-guidelines/nullability.md
This should cover the 99.99% scenarios. People can break it intentionally but this at least shows the default behaviors?