Skip to content

What about Nullable struct? #11834

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

Closed
gpetrou opened this issue Apr 15, 2019 — with docs.microsoft.com · 3 comments · Fixed by #11983
Closed

What about Nullable struct? #11834

gpetrou opened this issue Apr 15, 2019 — with docs.microsoft.com · 3 comments · Fixed by #11983
Assignees
Milestone

Comments

Copy link

gpetrou commented Apr 15, 2019

Should there be some information here on whether to use or not the in-modifier with Nullable struct?
For example should we have
public void Method(int? arg) {}
or
public void Method(in int? arg) {}
?
ErrorProne.NET for example flags this as a special case: https://github.com/SergeyTeplyakov/ErrorProne.NET/blob/5cc962abfaf0005890dd0a9fba46a9e304a8a450/src/ErrorProne.NET.StructAnalyzers/TypeExtensions.cs#L28


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@BillWagner BillWagner added the waiting-on-feedback Waiting for feedback from SMEs before they can be merged label Apr 22, 2019
@BillWagner
Copy link
Member

That's a great question @gpetrou

I'd have to benchmark that case.

Ping @jaredpar if the C# team has any general guidance on nullable structs as in parameters.

@jaredpar
Copy link
Member

They should not be marked as readonly because a Nullable<T> is a struct which is non-readonly. There was an attempt to make Nullable<T> into a readonly struct but that was a breaking change. The reasoning is detailed here:

dotnet/corefx#24997 (comment)

In short though the problem is the backing T was never marked readonly and making it readonly now can cause observable side effects. Consider for example a ToString implementation that memoized the result. Before that was cached in the Nullable<T> and now it's not. Hence it remains a standard struct and should follow the same rules.

@BillWagner BillWagner added P1 and removed ⌚ Not Triaged Not triaged waiting-on-feedback Waiting for feedback from SMEs before they can be merged labels Apr 22, 2019
@BillWagner BillWagner added this to the May 2019 milestone Apr 22, 2019
@gpetrou
Copy link
Author

gpetrou commented Apr 23, 2019

Thanks for the explanation. I will open an issue in Error.Prone repository to fix this.

BillWagner added a commit that referenced this issue Apr 23, 2019
@BillWagner BillWagner modified the milestones: May 2019, April 2019 Apr 23, 2019
BillWagner added a commit that referenced this issue Apr 25, 2019
* Add note about nullable value types

Fixes #11834

* Update docs/csharp/write-safe-efficient-code.md

Co-Authored-By: BillWagner <[email protected]>
@dotnet-bot dotnet-bot added the ⌚ Not Triaged Not triaged label Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants