Skip to content

Annotate ListStringTypeConverter for nullability#28217

Closed
MartyIX wants to merge 1 commit intodotnet:mainfrom
MartyIX:feature/2025-03-06-Convert-nullability-1-n
Closed

Annotate ListStringTypeConverter for nullability#28217
MartyIX wants to merge 1 commit intodotnet:mainfrom
MartyIX:feature/2025-03-06-Convert-nullability-1-n

Conversation

@MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Mar 6, 2025

Description of Change

Recently, a PR was merged (#27984) where nullability annotations for ShadowTypeConverter were added. However, I believe that there are minor errors.

Based on dotnet/runtime#63874 which corrected nullable types for .NET runtime (e.g. DateTimeConverter), I believe that the correct method synopses are:

+public override bool CanConvertFrom(ITypeDescriptorContext? context, Type sourceType)
+public override bool CanConvertTo(ITypeDescriptorContext? context, Type? destinationType)
+public override object? ConvertFrom(ITypeDescriptorContext? context, CultureInfo? culture, object value)
+public override object? ConvertTo(ITypeDescriptorContext? context, CultureInfo? culture, object? value, Type destinationType)

This PR attempts to fix the annotations for a single converter to validate my beliefs.


If you wonder, what "~" character (called oblivious marker) denotes in PublicAPI.Shipped.txt and PublicAPI.Unshipped.txt files mean, then look here: https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md#nullable-reference-type-support

Any public API that haven't been annotated (i.e. uses an oblivious reference type) will be tracked with a ~ marker.
The marker lets you track how many public APIs still lack annotations. For instance, ~C.ObliviousMethod() -> string.

Note: See explanation of "~" character here: https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md#nullable-reference-type-support

> Any public API that haven't been annotated (i.e. uses an oblivious reference type) will be tracked with a `~` marker.
> The marker lets you track how many public APIs still lack annotations. For instance, `~C.ObliviousMethod() -> string`.

See also: dotnet/runtime#63874 which corrected nullable types for .NET runtime (e.g. https://github.com/dotnet/runtime/blob/78142b0e78681a3be581040c7b988bc91d5a8169/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/DateTimeConverter.cs)
@MartyIX MartyIX requested a review from a team as a code owner March 6, 2025 11:26
@MartyIX MartyIX requested review from PureWeen and tj-devel709 March 6, 2025 11:26
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Mar 6, 2025
@dotnet-policy-service
Copy link
Contributor

Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@bhavanesh2001
Copy link
Contributor

This should be targeting main?

@MartyIX
Copy link
Contributor Author

MartyIX commented Mar 6, 2025

This should be targeting main?

I can retarget if it is not good for main.

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jfversluis
Copy link
Member

Not sure to what extend this is actually breaking, but just to be sure lets make this target .NET 10 since it does touch public APIs

@MartyIX
Copy link
Contributor Author

MartyIX commented Mar 7, 2025

I believe it's not a breaking change but I'm not 100 per cent sure.

I created #28244 for NET 10.0, given that tests run and I'm interested if there is an issue or not.

@jfversluis
Copy link
Member

Lets keep this for .NET 10!

@jfversluis jfversluis closed this Mar 7, 2025
@MartyIX MartyIX deleted the feature/2025-03-06-Convert-nullability-1-n branch March 7, 2025 14:42
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

community ✨ Community Contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants