Skip to content

Fix incorrect remarks for Nullable<T>.Implicit operator #2711

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 4, 2019
Merged

Fix incorrect remarks for Nullable<T>.Implicit operator #2711

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc2704f
Fix incorrect remarks for Nullable<T>.Implicit operator
MichaelJLiu Jul 8, 2019
8800522
Note that C# and VB don't use the implicit Nullable operator
MichaelJLiu Aug 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions xml/System/Nullable`1.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,11 +536,17 @@
<format type="text/markdown"><![CDATA[

## Remarks
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 equivalent method for this operator is <xref:System.Nullable%601.%23ctor%28%600%29?displayProperty=nameWithType>]]></format>
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

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.


The equivalent method for this operator is <xref:System.Nullable%601.%23ctor%28%600%29?displayProperty=nameWithType>

> [!NOTE]
> In C# and Visual Basic, an implicit conversion from `T` to <xref:System.Nullable%601> does not invoke 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.

> [!NOTE]
> In C# and Visual Basic, an implicit conversion from a `null` or `Nothing` literal to <xref:System.Nullable%601> produces a <xref:System.Nullable%601> value whose <xref:System.Nullable%601.HasValue%2A> property is initialized to `false`. The conversion occurs at compile time in these languages and does not invoke any operator.

]]></format>
</remarks>
</Docs>
</Member>
Expand Down Expand Up @@ -656,4 +662,4 @@
</Docs>
</Member>
</Members>
</Type>
</Type>