Runtime: CultureInfoConverter.GetCultureName(null) throws NRE

[hughbe]

Should return null

public static IEnumerable<object[]> GetCultureName_TestData()
{
    yield return new object[] { new CultureInfo("fr-FR"), new CultureInfo("fr-FR").Name };
    yield return new object[] { new CultureInfo("es-MX"), new CultureInfo("es-MX").Name };
    yield return new object[] { null, null };
}

[Theory]
[MemberData(nameof(GetCultureName_TestData))]
public void GetCultureName_Invoke_ReturnsExpected(CultureInfo culture, string expected)
{
    var converter = new SubCultureInfoConverter();
    Assert.Equal(expected, converter.GetCultureName(culture));
}

private class SubCultureInfoConverter : CultureInfoConverter
{
    public new string GetCultureName(CultureInfo culture)
    {
        return base.GetCultureName(culture);
    }
}

[maryamariyan]

Just checked to see if we throw or return null in .NET Framework. The behavior there is to throw NRE.

Also looking at e.g. TypeConverter.ConvertFromInvariantString method here shows that can throw with NSE if it couldn't do the operation.

Also note the remarks at the same docs page:

An exception is raised if the string cannot be converted into the appropriate object. The default implementation always returns null.

Use the context parameter to extract additional information about the environment from which this converter is invoked. This parameter can be null, so always check it. Also, properties on the context object can return null.

See also
ITypeDescriptorContext

cc: @tarekgh @safern

[maryamariyan]

As I spoke offline with @tarekgh it might seem like a good idea to continue throwing so as to differentiate from default behavior which returns null.

[hughbe]

There is precedent to fix things like NREs in .Net core

[maryamariyan]

That makes sense. I think though, the question is whether to return null or rather NSE.

[safern]

Yes I also agree we should throw an ANE instead. Even though .NET Framework throws a NRE it is better to instead throw an ANE from the user perspective.

We’ve decided to throw NRE only where in hot paths where perf matters much and we want to avoid doing the null check.

[hughbe]

I think the question is to return null or to throw ANE

[maryamariyan]

Yes.

[safern]

As part of the nullable annotations effort we've been trying to avoid more nulls as part of the framework, being said that, I don't think we should return null. I think we should instead throw ANE.

cc: @stephentoub thoughts?

[maryamariyan]

@danmosemsft we're inclined towards throwing ANE. What are your thoughts?

[danmosemsft]

Makes sense given that brings us closer to .NET Framework and documented behavior.

[tarekgh]

I agree throwing is better here than returning null. if the caller need to check null after the call, the caller can still check null before the call. or try catch the exception.

[hughbe]

Fix to ANE instead of NRE is in https://github.com/dotnet/corefx/pull/38980