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

Specify generic constraints added to support nullable reference types in C# 8 #1178

Merged
merged 25 commits into from
Nov 20, 2024

Conversation

BillWagner
Copy link
Member

Add the class? and notnull constraint.

The default constraint is added in C# 9, so is not included.

@BillWagner BillWagner marked this pull request as ready for review September 18, 2024 20:44
@BillWagner BillWagner added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Sep 18, 2024
@BillWagner
Copy link
Member Author

@jskeet I added the meeting discuss label on this. I doubt that it's final, but it's worth a first look.

standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
@jskeet
Copy link
Contributor

jskeet commented Sep 19, 2024

(I'll wait for responses to @KalleOlaviNiemitalo's questions before reviewing, if that's okay.)

Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

Some comments plus ANTLRizing some grammar rules.

I haven’t considered per se what should (not) be in the Standard in regards to the specification of NRT.

standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

I like Nigel's suggestions in general. My guess is that the currently-unused rules are intended to be used in the future? Would be good to know.

standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
@BillWagner
Copy link
Member Author

@jskeet

My guess is that the currently-unused rules are intended to be used in the future? Would be good to know.

Which rules do you think are unused? I didn't see any.

@jskeet
Copy link
Contributor

jskeet commented Sep 25, 2024

@jskeet

My guess is that the currently-unused rules are intended to be used in the future? Would be good to know.

Which rules do you think are unused? I didn't see any.

non_nullable_non_value_type_parameter and nullable_type_parameter - see Nigel's comments.

@BillWagner
Copy link
Member Author

@jskeet

My guess is that the currently-unused rules are intended to be used in the future? Would be good to know.

Which rules do you think are unused? I didn't see any.

non_nullable_non_value_type_parameter and nullable_type_parameter - see Nigel's comments.

Got it. I'd missed some edits. See latest commit.

standard/types.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
@BillWagner
Copy link
Member Author

Example discussed in the meeting:

M("Hello", null);
M(7, 3);
M<int?>(8, null);    // Warning

void M<T>(T t, T? n) where T : notnull 
{
    T t1 = t;
    T t2 = n;        // Warning
    T t3 = default;  // Warning
    T? n1 = t;
    T? n2 = n;
    T? n3 = default;
    T? n4 = null;    // Error

}

BillWagner and others added 6 commits October 9, 2024 14:28
Bring in the normative text from dotnet#700.

Some text is removed because of the decision on normative language in our September meeting.
Co-authored-by: Nigel-Ecma <[email protected]>
@jskeet
Copy link
Contributor

jskeet commented Oct 22, 2024

@BillWagner Is this ready for another round of review?

@BillWagner
Copy link
Member Author

BillWagner commented Oct 22, 2024

Is this ready for another round of review?

@jskeet Not yet. But on my list for this week.

Edits made and pushed this morning (my time). It's ready for review.

We'd used `'?'` and `nullable_type_attribute` in different places for the `?` annotation. Define `nullable_type_attribute` at first use, and use that consistently.
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Just one comment for now. I need to have another look when more awake...

standard/types.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

I think I'm starting to get my head round this again...

standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
BillWagner and others added 2 commits October 28, 2024 13:31
@jskeet
Copy link
Contributor

jskeet commented Oct 30, 2024

As noted by Neal, this is an error:

public partial class PartialList : List<string>
{
}

public partial class PartialList : List<string?>
{
}

Error: CS0263 Partial declarations of 'PartialList' must not specify different base classes

@jskeet
Copy link
Contributor

jskeet commented Oct 30, 2024

Discussions:

  • Using T? when T is unconstrained should be forbidden in C# 8. (For C# 9 we'll need to explain some oddities.)
  • where T : class should be described as T being a non-nullable reference type, and class? is "any reference type" - although the non-nullable reference constraint only generates a warning. (We're debating how to specify this.)
  • We need to explain what T? means when T itself is nullable

standard/classes.md Outdated Show resolved Hide resolved
standard/classes.md Outdated Show resolved Hide resolved
@jskeet
Copy link
Contributor

jskeet commented Oct 30, 2024

We observe that Roslyn allows both of these:

public class Foo<T> where T : notnull, Stream
public class Foo<T> where T : notnull, Stream?

This PR prohibits both. @BillWagner will file a bug against Roslyn (which we suspect will be closed).

@Nigel-Ecma
Copy link
Contributor

@BillWagner – I made some changes in PR #1195 thinking this PR was already merged, oops. I've removed them from 1195 but cannot make direct suggestions here as the changes are on lines this PR has not modified. SO here they are as text blocks.

The changes centred around changing ? in the text to *nullable_type_attribute* – the latter is introduced in this PR so these changes should either be here or in a subsequent PR.

In types.md §8.9.1, first para, line 729 there are a couple of ?’s, I suggest:

A nullable reference type is denoted by appending a nullable_type_attribute (?) to a non-nullable reference type. There is no semantic difference between a non-nullable reference type and its corresponding nullable type, both can either be a reference to an object or null. The presence or absence of the nullable_type_attribute declares whether an expression is intended to permit null values or not. A compiler may provide diagnostics when an expression is not used according to that intent. The null state of an expression is defined in §8.9.5. An identity conversion exists among a nullable reference type and its corresponding non-nullable reference type (§10.2.2).

I made a few other word changes, e.g. can -> may.

I do think this para needs some work on its clarity, but I’ve no suggestions right now – and that can come in a subsequent PR as its nothing to do with this one per se.

For line 738 I suggest:

The syntactic distinction between a nullable reference type and its corresponding non-nullable reference type enables a compiler to generate diagnostics. A compiler must allow the nullable_type_attribute as defined in §8.2.1. The diagnostics must be limited to warnings. Neither the presence or absence of nullable annotations, nor the state of the nullable context can change the compile time or runtime behavior of a program except for changes in any diagnostic messages generated at compile time.

On line 746 there is another ? but as it is in reference to a preceding T? I think that is fine.

There might be other ?s which would be better as nullable_type_attribute (or nullable type attribute?), I haven’t done an exhaustive search.

(Note: There is a simple typo on line 728, I’ve added PR #1199 to fix it.)

Before I removed the changes from #1195 @KalleOlaviNiemitalo raised a comment on the naming of nullable_type_attribute – I've asked them to make the comment in this PR.

This covers part 1, the comments in the files tab
This commit addresses the comments in the conversation tab from the 10/30 meeting.
This commit incorporates the comments on the conversation tab.
standard/classes.md Outdated Show resolved Hide resolved
@jskeet jskeet added this to the C# 8.0 milestone Nov 20, 2024
with nullable_type_annotation
@BillWagner BillWagner closed this Nov 20, 2024
@BillWagner BillWagner reopened this Nov 20, 2024
@BillWagner BillWagner merged commit a7525ff into dotnet:draft-v8 Nov 20, 2024
6 checks passed
@BillWagner BillWagner deleted the nullable-type-parameters branch November 20, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting: discuss This issue should be discussed at the next TC49-TG2 meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants