Skip to content

Conversation

MichaelJLiu
Copy link
Contributor

The implicit operator's value parameter is of type T (not T?) and will never be null, so the remarks shouldn't say "If the value parameter is (not) null...".

The implicit operator's value parameter is of type T (not T?) and will never be null.
@mairaw mairaw added this to the July 2019 milestone Jul 24, 2019
@mairaw mairaw requested a review from BillWagner July 26, 2019 00:04
@mairaw
Copy link
Contributor

mairaw commented Aug 9, 2019

@safern @stephentoub can you please review these changes to System.Nullable<T>?

@mairaw mairaw requested review from stephentoub and safern August 9, 2019 00:12
If the `value` parameter is not `null`, the <xref:System.Nullable%601.Value%2A> property of the new <xref:System.Nullable%601> value is initialized to the `value` parameter and the <xref:System.Nullable%601.HasValue%2A> property is initialized to `true`.

If the `value` parameter is `null`, the <xref:System.Nullable%601.Value%2A> property of the new <xref:System.Nullable%601> value is initialized to the default value, which is the value that is all binary zeroes, and the <xref:System.Nullable%601.HasValue%2A> property is initialized to `false`.
The <xref:System.Nullable%601.Value%2A> property of the new <xref:System.Nullable%601> value is initialized to the `value` parameter and the <xref:System.Nullable%601.HasValue%2A> property is initialized to `true`.
Copy link
Member

Choose a reason for hiding this comment

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

While it's true that this defined implicit operator accepts a T and not a T?, it's also true that the compiler let's you assign null to a Nullable<T>, in which case it generates the equivalent of setting HasValue to false and Value to default. From that perspective, what was written was correct. So I think the question here is whether the docs should be from the perspective of the actual raw API or the syntax the compiler enables; I'd be inclined to stick with what was there, since implicit operators are all about being used by the compiler. But I don't have a strong opinion; if you want to go with strictly what the API declares, then the new version looks fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that if you wanted to describe how the C# compiler converts a null literal to T?, it should be done in a separate paragraph that emphasizes how this implicit operator is not involved.

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

I agree that the new version is more accurate. But, as Stephen points out, it is also misleading. What do you think of the proposed suggestion?

If the `value` parameter is not `null`, the <xref:System.Nullable%601.Value%2A> property of the new <xref:System.Nullable%601> value is initialized to the `value` parameter and the <xref:System.Nullable%601.HasValue%2A> property is initialized to `true`.

If the `value` parameter is `null`, the <xref:System.Nullable%601.Value%2A> property of the new <xref:System.Nullable%601> value is initialized to the default value, which is the value that is all binary zeroes, and the <xref:System.Nullable%601.HasValue%2A> property is initialized to `false`.
The <xref:System.Nullable%601.Value%2A> property of the new <xref:System.Nullable%601> value is initialized to the `value` parameter and the <xref:System.Nullable%601.HasValue%2A> property is initialized to `true`.
Copy link
Member

Choose a reason for hiding this comment

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

How about this as an alternative:

Suggested change
The <xref:System.Nullable%601.Value%2A> property of the new <xref:System.Nullable%601> value is initialized to the `value` parameter and the <xref:System.Nullable%601.HasValue%2A> property is initialized to `true`.
The <xref:System.Nullable%601.Value%2A> property of the new <xref:System.Nullable%601> value is initialized to the `value` parameter and the <xref:System.Nullable%601.HasValue%2A> property is initialized to `true`.
Note that assignment to `null` sets the <xref:System.Nullable%601.Value%2A> property to the default value of all binary 0, and the <xref:System.Nullable%601.HasValue%2A> property to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand how my proposed change is misleading. This documentation topic is not about implicit nullable conversions in general (such as null to int? or int? to long?). Instead, as indicated by its title, this topic is specifically about the implicit T to Nullable<T> operator that's implemented in the .NET Framework as follows:

public static implicit operator Nullable<T>(T value) {
    return new Nullable<T>(value);
}

The operand is of type T, not T?, so it can't possibly be null. Any discussion in this topic about how null is converted to T? would, I believe, misleadingly imply that this operator is used to perform that conversion, which isn't the case. The appropriate place to talk about how null is converted to T? would be in the Remarks for the Nullable<T> structure itself, not here.

Copy link
Member

Choose a reason for hiding this comment

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

@MichaelJLiu The reason I say it's "misleading" is that it implies this wouldn't be allowed:

int? a = null;

Yes, it's true that the above code does not use the implicit conversion operator. But, readers will make the improper assumption. Adding the one sentence removes that assumption, but doesn't try to expand the article to cover the compiler's behavior to do the assignment.

I hope that helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked @ericlippert for his opinion, which I share here with his permission:

I agree with you. This is the documentation of a method, not of a conversion feature of the C# language.

What I'd like to see here is that (1) the documentation documents the semantics of the method, and (2) the documentation make clear that C# does not call this method when converting from T to nullable T; this method is provided for the use of programming languages that do not have special rules for conversions to Nullable<T>.

While we're making picky critiques I note that Bill's suggestion first, discusses an implementation detail rather than describing semantics, and second, assumes that conversions are in the context of assignments, which is not necessarily true.

I believe (1) is addressed by my original PR. This operator creates a Nullable<T> instance whose Value property is initialized to value and whose HasValue property is initialized to true. No more and no less.

For (2), how about the following?

Note

In C# and Visual Basic, an implicit conversion from T to Nullable<T> does not call this operator because these languages have special rules for the conversion. This operator is provided for the benefit of languages that do not have such special rules.

In C# and Visual Basic, an implicit conversion from a null or Nothing literal to Nullable<T> produces a Nullable<T> value whose HasValue property is false. The conversion is a special compile-time feature of these languages and does not call any operator.

@dnfclas
Copy link

dnfclas commented Aug 22, 2019

CLA assistant check
All CLA requirements met.

@MichaelJLiu
Copy link
Contributor Author

@BillWagner, @stephentoub - I've submitted changes to my PR. I'm not sure if I need to do anything to have the changes reviewed.

@BillWagner
Copy link
Member

Thanks for working with us on the set of changes. I'll :shipit: now.

@BillWagner BillWagner merged commit 5ff05c9 into dotnet:master Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants