Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have bool and string implement ISpanParsable<T> #82836

Merged
merged 2 commits into from Mar 2, 2023

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Mar 1, 2023

This resolves #78842 and resolves #78523

@dotnet-issue-labeler
Copy link

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
Copy link

ghost commented Mar 1, 2023

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

Issue Details

This resolves #78842 and resolves #78523

Author: tannergooding
Assignees: tannergooding
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

@ghost
Copy link

ghost commented Mar 1, 2023

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

Issue Details

This resolves #78842 and resolves #78523

Author: tannergooding
Assignees: tannergooding
Labels:

area-System.Runtime, new-api-needs-documentation

Milestone: -

@tannergooding tannergooding marked this pull request as ready for review March 1, 2023 17:05
return s.ToString();
}

static bool ISpanParsable<string>.TryParse(ReadOnlySpan<char> s, IFormatProvider? provider, [MaybeNullWhen(returnValue: false)] out string result)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really matter other than for consistency with other use, but whereas for a generic T we'll use [MaybeNullWhen(false)] out T, for a non-generic we'll use [NotNullWhen(true)] out string? .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with this since its what the interface declaration was.

Is there a preference on updating to NotNullWhen(true) instead or is there any conflict in doing that given the usage via generics?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a preference on updating to NotNullWhen(true) instead

From a functionality perspective, it doesn't matter: [MaybeNullWhen(false)] out string and [NotNullWhen(true)] out string? are identical. We prefer the latter form in general because it more closely aligns with syntax of the language: the out is in fact nullable, hence the ?, and it's just that in certain circumstances we can use attributes to provide more information to the flow analysis, that we know even though it's nullable it won't be when the method returns true.

We can't, however, use [NotNullWhen(true)] out T, because the consumer might instantiate with a nullable T such that null is a valid value in the domain, such that NotNullWhen(true) would potentially be a lie. Hence, even though we prefer [NotNullWhen(true)], we use [MaybeNullWhen(false)] with unconstrained generics, since it ends up functionally being the same and is correct.

So, we do have a preference for [NotNullWhen(true)] when it's possible to use it. In this particular case, it's even less impactful because we're explicitly implementing the interface, so effectively no consumer will see the annotations here (outside of via reflection, which is really remote).

@tannergooding
Copy link
Member Author

@nietras
Copy link
Contributor

nietras commented Mar 10, 2023

Awesome I've been missing these. 👍

@jeffhandley jeffhandley added the blog-candidate Completed PRs that are candidate topics for blog post coverage label Mar 14, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Apr 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime blog-candidate Completed PRs that are candidate topics for blog post coverage new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: String should implement IParsable<string> bool does not implement IParsable<TSelf>
4 participants