An issue in TypeDescriptor.GetConverter()

[iSazonov]

https://github.com/dotnet/corefx/blob/3843042d560549baa492f2c99441fd4571655422/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs#L204-L216

In line 206 typeAttr is never null because GetAttributes() returns always an empty collection. Also indexer returns "empty" TypeConverter if the item does not present in the AttributeCollection.
Perhaps code above (for instance) is broken too.

Discovered in PowerShell repo.

[iSazonov]

@ericstj Could you please look the issue?

[ericstj]

Can you let us know what issue you're facing because of this? I don't see any obvious bug here, perhaps some overly defensive code, but that's not a problem in and of itself.

Please rephrase this issue describing:
repro-steps
expected behavior
actual behavior

Thanks!
Eric

[iSazonov]

@ericstj I can not say that it is "bug" but obviously the code is not full correct. It is a bad pattern. I caught this while I played with TypeDescripter.GetConverter in PowerShell Core repo. I copy-pasted line 206 of the code and it did not work as expected.
My report is that typeAttr is never null and it is better to remove the bad pattern because it confuses code readers.

[ericstj]

I see what you mean. That's true for the normal case, however I see the call to TypeDescriptor.GetAttributes can be extended with a custom TypeDescriptor which can provide a derived AttributeCollection with indexer implementation that returns null. We have also supported AOT platforms in the past that strip metadata which would make the reflection call to TypeConverterAttribute.Default in GetDefaultValue fail even though we define that property. This is enough to give me pause. One could say that this is an unlikely combination of circumstances, but what should this code do if that API does return null? Certainly not null-ref: throw or just handle the null as it does now? The current behavior seems most correct.

How about we tackle this type of issue when we add nullable annotations to this assembly? We're going through all of the libraries and doing this now (/cc @buyaa-n). That should uncover through compile-time analysis the cases where things can and cannot be null and we will naturally clean up cases where we were doing unnecessary null checks.

[iSazonov]

@ericstj Thanks for clarify! I think nullable annotations will resolve the issue. Although I don't know whether they work well for interfaces.

[buyaa-n]

How about we tackle this type of issue when we add nullable annotations to this assembly?

Sure we can evaluate this case during nullabliity analysis, might be not in near future as this assembly is in the group 9 of dependency list https://github.com/dotnet/corefx/issues/40623.

That should uncover through compile-time analysis the cases where things can and cannot be null and we will naturally clean up cases where we were doing unnecessary null checks.

Compiler wouldn't really warn for such unnessesary null check cases, as you mentioned it is not errornous case. Compiler analysis more devoted to detect possible null references errors, but of course we can do this kind of clean up adding our human analysis to the compiler analysis CC @stephentoub @safern

[ericstj]

Curious: the compiler won't warn if you check a non-nullable reference type for null? That seems like an opportunity for an analyzer (or compiler improvement).