Add support for nullable reference types - Utils#7862
Conversation
| } | ||
|
|
||
| return Type.GetTypeFromHandle(typeHandle)?.Name; | ||
| return Type.GetTypeFromHandle(typeHandle)?.Name ?? t.Name; |
There was a problem hiding this comment.
This should never happen with types in the CLR but does guard against edge cases where custom types have not been fully defined.
| public static string SanitizedPath(string commandLine) | ||
| public static string SanitizedPath(string? commandLine) | ||
| { | ||
| if (commandLine is null) |
There was a problem hiding this comment.
All calls in this code base to this method pass in Environment.CommandLine which will never be null but could be an empty string. Based on that, if we do get passed a null here from some outside call returning an empty string is logical.
There was a problem hiding this comment.
The intent of this comment is not to disagree. I merely want to provide another perspective in case it hasn't been discussed. One of the things we tried to do in the past for things that are considered utils is to design the API shape independent from how it is being called. So looking at it as an independent library tool and it's intended/desirable behavior from that perspective.
Again I think both approaches are valid and we have no hard rules that enforces one or the other
There was a problem hiding this comment.
More perspective is always appreciated. I left the comment because I think it could go either way and if someone has a different justification I want to know. And I didn't want to forget why I made this decision.
I had to make a call on if we should error with a null or return an empty string. Based on the usage an empty string feels like it matches with that pattern so it would be more intuitive behavior for the caller.
| { | ||
| if (!PropertyInfoToLateBoundProperty.TryGetValue(property, out var lateBoundPropertyGet)) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(property.DeclaringType); |
There was a problem hiding this comment.
The Expression.Convert method on line 21 below will throw if property.DeclaringType is null. This moves the failure higher up the stack.
This is true for all of Expression.Convert calls in this class
| { | ||
| if (!FieldInfoToLateBoundFieldSet.TryGetValue(field, out var callback)) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(field.DeclaringType); |
There was a problem hiding this comment.
gen.Emit on line 75 below will throw if the sourceType is null. This simply moves the exception up the call stack. https://github.com/dotnet/runtime/blob/main/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicILGenerator.cs line 132
This is true for all other gen.Emit calls in this class
3 participants
#6257
This PR add
#nullable enabledfor consistency with the rest of the folder. NewThrowIfArgumentNullchecks are now required that result in the null exception being thrown earlier up the stack.