System.ComponentModel.ICustomTypeDescriptor.GetConverter have incorrect nullable annotation

Interface System.ComponentModel.ICustomTypeDescriptor have nullable annotations, but method GetConverter is wrongly annotated to return non-null value, while documentation says it could return null:

Returns
A TypeConverter that is the converter for this object, or null if there is no TypeConverter for this object

I propose to mark return value as nullable. It will be breaking change, but people would get NullReferenceException anyway if they wont check for null

@krwq

@krwq - can you remember a reason why this was annotated this way in #54961?

Looking through the code, I see some conflicting things:

The following places look like they don't accept null:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs

Lines 3239 to 3240 in 6ca8c9b

    
           TypeConverter converter = desc.GetConverter(); 
        
           if (converter == null) throw new InvalidOperationException(SR.Format(SR.TypeDescriptorProviderError, _node.Provider.GetType().FullName, "GetConverter"));

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs

Lines 3549 to 3551 in 6ca8c9b

    
           converter = desc.GetConverter(); 
        
           if (converter == null) 
        
               throw new InvalidOperationException(SR.Format(SR.TypeDescriptorProviderError, _node.Provider.GetType().FullName, "GetConverter"));

These places seem to think returning null is acceptable:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs

Lines 2901 to 2904 in 6ca8c9b

    
           TypeConverter converter = _primary.GetConverter() ?? _secondary.GetConverter(); 
        
           Debug.Assert(converter != null, "Someone should have handled this"); 
        
           return converter;

runtime/src/libraries/System.Data.Common/src/System/Data/Common/DbDataRecord.cs

Line 80 in 6ca8c9b

TypeConverter ICustomTypeDescriptor.GetConverter() => null!;

runtime/src/libraries/System.Data.Common/src/System/Data/Common/DataRecordInternal.cs

Lines 321 to 324 in 6ca8c9b

    
           TypeConverter ICustomTypeDescriptor.GetConverter() 
        
           { 
        
               return null!; 
        
           }

runtime/src/libraries/System.Data.Common/src/System/Data/DataRowView.cs

Line 237 in 6ca8c9b

TypeConverter ICustomTypeDescriptor.GetConverter() => null!;

Since this is an interface that is implemented by external code, we should annotate it to be in-line with the documentation. The documentation indicates that null can be returned, so the API should be updated accordingly.

This seems incorrect - I suspect all internal implementations have returned non-null and I was looking only at the XML docs which was missing null remark (xml docs says only Gets a type converter for this object.) and with all large API surface I missed it.

I think unless implementation completely disagrees we should change it to match the docs.

Correct nullable annotation on ICustomTypeDescriptor.GetConverter

Correct nullable annotation on ICustomTypeDescriptor.GetConverter (#7…

Navigation Menu

Repository navigation

System.ComponentModel.ICustomTypeDescriptor.GetConverter have incorrect nullable annotation #69229

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	TypeConverter converter = desc.GetConverter();
	if (converter == null) throw new InvalidOperationException(SR.Format(SR.TypeDescriptorProviderError, _node.Provider.GetType().FullName, "GetConverter"));

	converter = desc.GetConverter();
	if (converter == null)
	throw new InvalidOperationException(SR.Format(SR.TypeDescriptorProviderError, _node.Provider.GetType().FullName, "GetConverter"));

	TypeConverter converter = _primary.GetConverter() ?? _secondary.GetConverter();

	Debug.Assert(converter != null, "Someone should have handled this");
	return converter;

	TypeConverter ICustomTypeDescriptor.GetConverter()
	{
	return null!;
	}

System.ComponentModel.ICustomTypeDescriptor.GetConverter have incorrect nullable annotation #69229

Description

Activity

eerhardt commented on May 12, 2022

eerhardt commented on May 18, 2022

krwq commented on May 19, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions