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

Different reference types: nullable and non-nullable #1124

Merged
merged 33 commits into from
Oct 9, 2024

Conversation

BillWagner
Copy link
Member

@BillWagner BillWagner commented May 29, 2024

Fixes #1089
Fixes #1090

This is an alternative to #1105

The changes were done on top of the branch for #1105. The final commit has the updates to remove null oblivious types.

It removes the text for a "null-oblivious" type. Instead, it relies on the nullable context for how to interpret the nullability of a reference type T.

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
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.

More comments, but I think we're making progress :)

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/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
Copy link
Member Author

@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.

Comments on updates per feedback.

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/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.

Getting there :) Sorry to keep picking nits, but I'm definitely keen on getting this one over the line rather than the alternative that has null-oblivious.

standard/types.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

@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.

I think I like this approach better than #1105, but do see some issues.

standard/types.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
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/types.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

@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.

A few language changes, some comments to consider.

As with #1123 I am recommending we don’t merge this, we get it to a state we’re currently happy with, move on to others in the group, circle back around as needed, and then merge/reject the group as a whole.

standard/types.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
@gafter
Copy link
Member

gafter commented Jul 10, 2024

We may need three types, not two. For example:

void f(
#nullable disable
string x, // could x (oblivious) be seen as nullable type but non-nullable initial state?
#nullable enable
string y,
string? z)
{
 if (...) {
   // x state is not null? or oblivious?
   g1(ref x); // okay
   g1(ref y); // not okay
   g1(ref z); // okay
   // x state is possibly null
 } else {
   // x state is not null? or oblivious?
   g2(ref x); // okay
   g2(ref y); // okay
   g2(ref z); // not okay
   // x state is possibly null
 }
}
void g1(ref string? x) {}
void g2(ref string x) {}

@BillWagner
Copy link
Member Author

Very short version:

f("x", "y", "z");
void f(
#nullable disable
    string x,
#nullable enable
    string y,
    string? z)
{
    y = x; // no warning
    y = z; // warning. Assignment of null to non-nullable variable.
}

@jskeet
Copy link
Contributor

jskeet commented Jul 10, 2024

Example of why I believe it makes sense to view a variable of type T declared in a nullable-disabled context as a nullable type:

# nullable disable
string x;
# nullable enable
x = "some value";
x = null;

This does not issue a warning, even for the last line. The state of x just before the last line is surely "definitely not null" as we've assigned a string literal to it... so to be able to assign null to it means its type can't be a not-null type.

@BillWagner
Copy link
Member Author

BillWagner commented Jul 10, 2024

#nullable enable
string s = M();

#nullable disable
// What is the type of the return value?
string M() => null;

@RexJaeschke
Copy link
Contributor

Note that from 2024-8-20 through 2024-09-03, there was a lengthy thread on email re this topic, under the subject "ECMA TC49-TG2 agenda for September 4th."

The first commit ports in the text from dotnet#700 substantially without change. The only editorial change is to move the clause on "Nullable directives" in lexical-structure before the clause on Pragma directives.
This will likely cause new failures, as I turned on nullable annotations and warnings in one of the templates.

Update code samples for the template in use to remove nullable warnings.

I did this as we plan to have nullable annotations on by default in this version of the spec.
At our September meeting, we decided that the standard should limit its normative text to the syntax required for nullable reference types, and a basic description explaining that the purpose of the annotations is to provide diagnostics.
This makes sense to add to this PR logically.
I think this may fix the left recursion error as well. Even if it doesn't, it's still correct.
State the nullable type for implicitly typed variables
standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/statements.md Outdated Show resolved Hide resolved
standard/types.md Show resolved Hide resolved
standard/types.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
standard/types.md Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
Copy link
Member Author

@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.

comments during our meeting

standard/expressions.md Outdated Show resolved Hide resolved
standard/expressions.md Outdated Show resolved Hide resolved
standard/types.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
standard/types.md Outdated Show resolved Hide resolved
Co-authored-by: Jon Skeet <skeet@pobox.com>
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.

Thanks! Big step forward :)

Additional edits from comments at the Oct 2 meeting. This resolves all open concerns.
@BillWagner
Copy link
Member Author

I've addressed all open comments, and resolved all conversations. This is ready for a final (asynchronous) pass and approval.

@jskeet @Nigel-Ecma @MadsTorgersen @ericlippert @RexJaeschke @gafter

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.

Just a missing word and a one word change suggestion.

standard/statements.md Outdated Show resolved Hide resolved
standard/types.md Outdated Show resolved Hide resolved
Co-authored-by: Nigel-Ecma <perryresearch@zoot.net.nz>
@BillWagner BillWagner merged commit 64cfcec into dotnet:draft-v8 Oct 9, 2024
8 checks passed
@BillWagner BillWagner deleted the nullable-type-definitions branch October 9, 2024 14:40
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
5 participants