Repair inconsistent nullability nullability on TypeConverter (#63186)

[MichaelKetting]

Fixes #63186

• new ticket to update the docs (see Repair inconsistent nullability nullability on TypeConverter (#63186) #63874 (comment))

• new ticket to discuss if a change to TypeConverter.CanConvertFrom(Type sourceType) to nullable with NotNullWhen(true) is also wanted. Note: I've also found one change on src/ImageConverter.CanConvertFrom and ref/ImageFormatConverter.CanConvertFrom and placed them in a dedicated commit. I can create a separate ticket for this if you prefer.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-componentmodel
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #63186

Note: I've also found one change on src/ImageConverter.CanConvertFrom and ref/ImageFormatConverter.CanConvertFrom and placed them in a dedicated commit. I can create a separate ticket for this if you prefer.

Author:	MichaelKetting
Assignees:	-
Labels:	area-System.ComponentModel, new-api-needs-documentation
Milestone:	-

[MichaelKetting]

@maryamariyan I believe my PR is running into #63439 (DllImporter fails on MacOS for Arm64) but is otherwise I'm ready for review. I'll rebase on top of main once #63439 is done and check again that my changes didn't also break something but figured I'll let you know in case there's something else to be done before that.

[eerhardt]

I looked into this more, and I think the advice I gave on #63186 (comment) wasn't 100% correct. Now that I look at it again, I think this should be annotated as:

public virtual bool CanConvertTo(ITypeDescriptorContext? context, [NotNullWhen(true)] Type? destinationType)

And then to fix the inconsistency, the above CanConvertTo overload should be annotated the same:

public bool CanConvertTo([NotNullWhen(true)] Type? destinationType)

Note that ConvertTo doesn't need to change. It doesn't allow for null, and will throw if someone passes null.

This way, people can write code like:

Type? someType = ...;

if (typeConverter.CanConvertTo(someType))
{
    var converted = typeConverter.ConvertTo(someType);
}

And they won't get any nullability warnings.

@krwq - since you originally added nullability here - do you have any thoughts?

[MichaelKetting]

@eerhardt Thanks for the input. That does make sense.

Should I expand the original ticket to also cover the sourceType parameter on CanConvertFrom?

[eerhardt]

Should I expand the original ticket to also cover the sourceType parameter on CanConvertFrom?

No. Let's open a new issue for CanConvertFrom. The scenario there is a bit different because presumably you've gotten the type from calling something.GetType(). ("From" implies you have something already that you are going to convert from.) I'd rather keep these separate, so we can get the CanConvertTo changes in without being held up by any issues with CanConvertFrom.

[MichaelKetting]

@eerhardt Sure thing. Just let me know how you want to handle the ImageConverter-fix for CanConvertFrom.

[MichaelKetting]

@eerhardt and @krwq Do you already have an update on the final decision on how to proceed with CanConvertTo (see #63874 (comment) , syntax update with NotNullWhen instead of not null)?

[eerhardt]

@MichaelKetting - I think we can go forward with my above suggestion. Any plans on moving this forward?

[MichaelKetting]

@eerhardt Great, yes, will be updating the PR in the next couple of days / the weekend to go with the NotNullWhen solution.

Since the original null from way back changes did not update the docs, should I take the opportunity and do something there as well by adding parameter documentation and null guides?

[eerhardt]

You can, but those docs are contained in a separate repo. Updating the xml doc above doesn't make it into the official docs.

Here are the official docs:
https://github.com/dotnet/dotnet-api-docs/blob/d90566928a7c5f6070f942a89d9be588647a7105/xml/System.ComponentModel/TypeConverter.xml#L349-L407

[MichaelKetting]

@eerhardt Ah, okay, there they are :) This would then be a separate ticket and PR, correct, since they're separate repos? Which means I'll do the code first and once that's done and approved, I'll check out the docs.

[eerhardt]

Yes, that would be a separate PR

[MichaelKetting]

@eerhardt I'll update the PR from Draft to Ready-for-Review in the evening. Want to update the docs in the PR first. Until then: There's some crashes / failures in helix but I can't pinpoint (yet) if that's my changes or a general instability.

[MichaelKetting]

@eerhardt I've rebased on the latest main and should be done. The errors in the pipeline / helix seem to be unrelated to the changes in this PR and I've found the same issues also in at least one other PR (https://github.com/dotnet/runtime/pull/65944/checks?check_run_id=5365597384)

[eerhardt]

Failures are #65890.

[eerhardt]

Thanks for the contribution, @MichaelKetting!