Refactor DesignerSerializationManager dictionary usage to use TryGetValue
#8702
Conversation
|
Would this deserve a test too? |
I'll look into it when I have more time. I am not sure how to go about it. |
| serializer = serializers[objectType]; | ||
| if (serializer is not null && !serializerType.IsAssignableFrom(serializer.GetType())) | ||
| // I don't double hash here. It will be a very rare day where multiple types of serializers will be used in the same scheme. We still handle it, but we just don't cache. | ||
| if (serializers.TryGetValue(objectType, out serializer) && !serializerType.IsAssignableFrom(serializer.GetType())) |
There was a problem hiding this comment.
Is it possible for objectType to map to a null value in serializers? If so, we should check if serializer is null before calling serializer.GetType()
There was a problem hiding this comment.
We check objectType for null just above on line 416.
There was a problem hiding this comment.
I think @lonitra meant, if serializers contained an entry in which the key (i.e. objectType) is not null but the value (i.e. serializer) is null, that could cause a NullReferenceException in serializer.GetType(). Although the code checks objectType is not null, that doesn't help against this risk.
Anyway, it won't happen, because the only code that adds anything to serializers is here and it does not add null values:
| ArgumentNullException.ThrowIfNull(value); | ||
|
|
||
| CheckSession(); | ||
| // Check our local nametable first | ||
| if (namesByInstance is not null) | ||
| if (namesByInstance is not null && namesByInstance.TryGetValue(value, out string? name)) |
There was a problem hiding this comment.
Ditto here. If value can map to a null value in namesByInstance, we should check if name is not null before returning it.
There was a problem hiding this comment.
namesByInstance/instancesByName do not accept a nullable value.
Off the top of my head I am not sure how complicated our unit tests for |
lonitra
added
the
waiting-author-feedback
|
@elachlan, just checking if this is still on your radar. |
|
Yes. Work has taken over a bit due to some deadlines. I'll see what I can do. |
ghost
removed
the
waiting-author-feedback
dreddy-work
added
the
waiting-author-feedback
…used. serializers is only used in a session and only exists after the first call.
ghost
removed
the
waiting-author-feedback
|
I have tested locally and it fails without the fix. I've only added a test change for the Sorry this took so long. I imagined it would take me a lot longer and had been busy with work. |
|
@elachlan, did you get chance to respond on couple of open comment above? |
|
I'll look today. |
|
@dreddy-work I believe its addressed. But feel free to add to it if I have missed something. |
Refactor dictionary usage to use
TryGetValue.Addresses feedback from:
#8353 (comment)
Microsoft Reviewers: Open in CodeFlow