Nullable improvements for Microsoft.Extensions.Primitives.StringSegment №2

[maxkoshevoi]

No description provided.

[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: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

null

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

[maxkoshevoi]

cc: @eerhardt

[eerhardt]

I question how much value these MemberNotNull attributes provides on these APIs. When would a caller:

1. Call AsSpan/Substring/Subsegment on a StringSegment
2. And then afterwards try to get the Value or Buffer of the StringSegment they got a piece of above

I would assume the call to AsSpan/Substring/Subsegment would already be guarded by a HasValue check, because if it wasn't, AsSpan/Substring/Subsegment would throw when HasValue is false.

This doesn't seem like a typical code flow. Do we have evidence in ASP.NET that this is actually beneficial? If so, can you provide a link to the real-world code?

[maxkoshevoi]

I just thought giving complier more information about the code is a good thing. But you are right, I don't think there would be many (if any) places where those attributes would save someone a redundant null check.

Do you want them removed?

[dougbu]

Do we have evidence in ASP.NET that this is actually beneficial?

I got nothing but it's possible our code uses StringSegment.IsNullOrEmpty(...) and Length or HasValue checks more than is common.

[eerhardt]

If we could annotate the static IsNullOrEmpty, that would be great. But my understanding is that it isn't possible to annotate today.

Do you want them removed?

For now, I'd say let's remove them. Adding them limits the kind of changes we could make in the future. For example, if someone called AsSpan(0, 0), returning an empty span, even if the backing string is null, would be possible. With the proposed annotations, that wouldn't be possible since this is saying Buffer and Value are not null.

[maxkoshevoi]

Adding them limits the kind of changes we could make in the future

Good point. Didn't think about that removing those attributes would be a breaking change. Removed them from the PR.

But my understanding is that it isn't possible to annotate today.

Yeah, it doesn't seem to be. Currently attributes only can tell compiler about parameter, field or property not being null, but not method results (StringSegment.ToArray or implicit cast if HasValue is checked for example), nor members of parameter, field or property.
Maybe it'll be possible some day. Is there an issue for that?

[dougbu]

If we could annotate the static IsNullOrEmpty, that would be great. But my understanding is that it isn't possible to annotate today.

…

Maybe it'll be possible some day. Is there an issue for that?

@jaredpar not sure who on your team owns the nullability attributes but could you point us in the right direction please❔ Or, does an issue already exist somewhere❔

The case here is a struct passed to a static method which returns false when a couple of nullable properties on the struct are non-null. It's both the intervening struct and the fact the method is static that make the existing attributes unhelpful.

[dougbu]

@eerhardt unrelated to most of the conversation in this PR: The Exceptions thrown when Buffer or Value are null are inconsistent. ArgumentOutOfRangeException and NullReferenceException can both happen e.g. depending on which AsSpan(...) overload you call. Suggest filing an issue to reduce the inconsistencies if that feels important to you.

[eerhardt]

MacCatalyst runtime-staging leg timed out. Merging.