[Group 1] Enable nullable annotations for Microsoft.Extensions.Primitives

[maxkoshevoi]

Related to #43605

[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.

[stephentoub]

Doesn't this throw for null?

[maxkoshevoi]

It doesn't allow me to make it non-nullable...

[maxkoshevoi]

Interestingly, in implementation text is non-nullable and everything compiles fine

[stephentoub]

It doesn't allow me to make it non-nullable...

You need to change the type of the interface being implemented to IEquatable<string?>.

in implementation text is non-nullable and everything compiles fine

Yes, because the code never tries to dereference a maybe-null value. The first thing the code does is check whether it's null and throw if it is.

[maxkoshevoi]

You need to change the type of the interface being implemented to IEquatable<string?>

Still doesn't allow non-nullable text

[stephentoub]

Sorry, I misread what you'd written. IEquatable<T> is defined to expect Equals always allows null (it accepts a T?), which is why this is complaining about trying to specify a non-nullable value. I think the implementation should be changed to return false rather than throwing, and then typed as string?, but I'll defer here to @dotnet/area-microsoft-extensions area owners.

[maxkoshevoi]

Made it return false.

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Related to #43605

Author:	maxkoshevoi
Assignees:	-
Labels:	new-api-needs-documentation, area-Extensions-Primitives, community-contribution
Milestone:	-

[krwq]

LGTM, thank you a lot for this contribution!

[eerhardt]

LGTM. Thanks for the contribution!

[BrennanConroy]

I think StringSegment needs another look, Buffer is allowed to be null so you can distinguish between string.Empty and a null string.
You can see that null is a valid value because there is code specific to Buffer being null

runtime/src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

Line 86 in 8f2408c

public bool HasValue => Buffer != null;

runtime/src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

Line 30 in 8f2408c

Length = buffer?.Length ?? 0;

runtime/src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

Line 80 in 8f2408c

public string? Value => HasValue ? Buffer.Substring(Offset, Length) : null;

This is causing issues in AspNetCore when trying to consume the Runtime update because we expect to be able to pass null to StringSegment. dotnet/aspnetcore#35547

[maxkoshevoi]

Oh, I see. Second constructor thrown me off (it throws if buffer is null, but that's because we are trying to get only part of it).
I'll create a PR to make buffer nullable shortly.

[maxkoshevoi]

Shouldn't new StringSegment(null).Equals((string?)null); return true than?
In old implementation it throws, and after this PR it returns false (which might need to be changed back to throwing now that buffer can be null).