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

Add support for nullable references #700

Draft
wants to merge 41 commits into
base: draft-v8
Choose a base branch
from

Conversation

RexJaeschke
Copy link
Contributor

@RexJaeschke RexJaeschke commented Jan 9, 2023

Note carefully The spec in the V8 proposals folder states, “***This is a work in progress - several parts are missing or incomplete. An updated version of this document can be found in the C# 9 folder. ***”. Just what was implemented in V8 is not yet known. Even though this proposal is aimed at the V8 spec, it is based on the V9 proposal from MS.

Separately, in discussions with Bill, he thinks that TG2 should be considering two aspects of this proposal:

  1. The general topic of static analysis
  2. The fact that this proposal adds syntax via reference-type annotation to allow analysis across non-local boundaries

See also list of diagnostics

@RexJaeschke RexJaeschke added this to the C# 8.0 milestone Jan 9, 2023
@RexJaeschke RexJaeschke self-assigned this Jan 9, 2023
@RexJaeschke RexJaeschke marked this pull request as draft January 9, 2023 16:00
@RexJaeschke
Copy link
Contributor Author

RexJaeschke commented Jan 10, 2023

Re "Query expressions," the MS spec states, "Additional work needed here."

Bill's initial feedback re this was:

There’s probably a lot here. There are two major questions that need to be addressed.

  1. What type is represented by the elements in the result of a query? Is it nullable or non-nullable?
  2. What is the null-state of the elements if the type is nullable?

For example the result of First() would be not null. If it were null, the method throws an exception. By contrast , FirstOrDefault() could return a null value. What other type information should flow through a query expression?

@RexJaeschke
Copy link
Contributor Author

RexJaeschke commented Jan 10, 2023

Re "Type inference," there are numerous differences between the V8 and V9 MS Specs. Specifically,

  • The V8 sections “Type inference for var” and “Type inference for var?” have been replaced by V9 section “nullable implicitly typed local variables” with quite different content.
  • Section “Generic type inference” exists in both versions, but V9 added a bit and removed most content.

The following sections are identical in both versions:

  • The first phase
  • Exact, upper-bound and lower-bound inferences
  • Fixing

The opening paragraph of “Fixing” is not spec text, and indicates that more work is needed here.

Issue: We need to spec exactly what’s in V8. Is anything in the V9 spec that is NOT for V8 only?

Issue: What’s missing re “Fixing?”

Issue: We currently have 12.6.3, “Type inference” with sections 12.6.3.2, “The first phase”, 12.6.3.10 “Lower-bound inferences”, 12.6.3.11, “Upper-bound inferences,” and 12.6.3.12 “Fixing,” that match some of the proposed-edit homes. It would be good to get some suggestions as to where to put the other new stuff.

Bill: I admit that I get lost here as well. When you get ready to pick this up again, I can go through the history of the dotnet/csharplan repo and try to find the differences. We may also experiment using LangVersion in our csproj.

@BillWagner BillWagner force-pushed the add-support-for-nullable-references branch from 4721d85 to f3fa9f8 Compare February 6, 2023 14:06
@RexJaeschke
Copy link
Contributor Author

Reviewers: Please check my attempt to merge in to the section "Fixing" the new steps with the old. This was quite complicated.

@RexJaeschke RexJaeschke added the type: feature This issue describes a new feature label Jul 22, 2023
@BillWagner BillWagner force-pushed the add-support-for-nullable-references branch from f66c0b6 to b97c009 Compare September 25, 2023 18:11
@BillWagner BillWagner closed this Sep 25, 2023
@BillWagner BillWagner reopened this Sep 25, 2023
@BillWagner
Copy link
Member

rebased on updated draft-v8 branch on 09/25/2023

@BillWagner
Copy link
Member

BillWagner commented Sep 25, 2023

The rebase removed some sections added in draft V7. Adding them back in newer commits.

This wasn't quite true. Sections weren't removed, but some headers had their name edited.

@RexJaeschke RexJaeschke added the Review: pending Proposal is available for review label Oct 13, 2023
@RexJaeschke RexJaeschke removed their assignment Oct 22, 2023
@RexJaeschke RexJaeschke reopened this Oct 25, 2023
@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Nov 24, 2023

There is a bug report dotnet/roslyn#70958 about T? in explicit interface implementation where T is an unconstrained type parameter of the interface method being implemented. The compiler behaviour is surprising but I have not checked whether it matches the specification in this pull request.

… Apparently this is not relevant to the PR after all, because Unconstrained type parameter annotations are a C# 9 feature so the problematic case would be invalid in C# 8 anyhow.

@RexJaeschke RexJaeschke marked this pull request as ready for review January 11, 2024 14:07
@jskeet jskeet added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Feb 7, 2024
@jskeet
Copy link
Contributor

jskeet commented Feb 7, 2024

Intention of adding the "meeting: discuss" label is that the working group will review this before the next meeting only in enough depth to gauge whether the approach is suitable, and to think about how we can make progress perhaps without trying to do it all in one PR.

@@ -3494,6 +3752,15 @@ The term “correct grammar” above means only that the sequence of tokens shal
<!-- markdownlint-enable MD028 -->
> *Note*: From the disambiguation rule, it follows that, if `x` and `y` are identifiers, `(x)y`, `(x)(y)`, and `(x)(-y)` are *cast_expression*s, but `(x)-y` is not, even if `x` identifies a type. However, if `x` is a keyword that identifies a predefined type (such as `int`), then all four forms are *cast_expression*s (because such a keyword could not possibly be an expression by itself). *end note*

If a cast expression `(T)E` invokes a user-defined conversion, then the null state (§Nullabilities-And-Null-States) of the expression is the default null state for the type of the user-defined conversion. Otherwise:

- If `T` is a non-nullable value type then `T` has the null state “not null”
Copy link
Contributor

Choose a reason for hiding this comment

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

Is something mixed up here? It is saying that T (a type), not the expression, has a null state.

> // provided for a normal string parameter
> void M<T>(T t)
> {
> // There is no guarantee that default(T) is within the valid values for T hence the
Copy link
Contributor

Choose a reason for hiding this comment

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

Humpty Dumpty is that you? (Sorry ;-))

Comment on lines +5942 to +5944
The null state (§Nullabilities-And-Null-States) of an *anonymous_method_expression* or *lambda_expression* is “not null.”

An anonymous function is treated like a method, except in regard to its captured variables. The initial state of a captured variable inside an anonymous function is the intersection of the nullable state of the variable at all the uses of that anonymous function. The use of an anonymous function is the point at which it is defined in source.
Copy link
Contributor

@Nigel-Ecma Nigel-Ecma Mar 14, 2024

Choose a reason for hiding this comment

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

Is this bit meant to be in the previous clause? (§12.19 Anonymous function expressions)

@jskeet jskeet marked this pull request as draft April 24, 2024 21:24
@jskeet
Copy link
Contributor

jskeet commented Apr 24, 2024

Marking this as a draft, as we're going to try to specify this in small PRs (leaving a "known incomplete" standard until we're done).

@BillWagner is going to start by trying to specify #nullable.

BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request May 9, 2024
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.
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request May 9, 2024
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.
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request May 10, 2024
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.

fix build warnings

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.

Revert "fix build warnings"

This reverts commit dcb3731.

test for nullable annotations

If the samples compile cleanly in this mode, I can continue in this PR without breaking all the samples.

revert changes for type system.
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request May 10, 2024
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.

In addition, it fixes build warnings
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request May 10, 2024
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.

In addition, it fixes build warnings
@KalleOlaviNiemitalo
Copy link
Contributor

Re query expressions in #700 (comment)

For example the result of First() would be not null. If it were null, the method throws an exception.

If Enumerable.First<T>(this IEnumerable<T>) is implemented in C#, then it cannot detect at run time whether T is a nullable reference type, and cannot use that information to decide whether to throw instead of returning null.

I was looking at dotnet/csharplang#8383 and a similar case (tried at sharplab.io, not with a C#-8-only compiler):

using System;

// Syntactically similar to IEnumerable<T> as far as query expressions
// are concerned, but does not have any standard semantics.
class Seq<T> {
}

// Query expression patterns for Seq<T>.
static class SeqExtensions
{
    // Using Func<int, int> to be clear that func does not affect nullability of the result.
    public static Seq<T>? Select<T>(this Seq<T> sequence, Func<int, int> func)
    {
        throw new NotImplementedException();
    }
}

static class C {
    static Seq<T> UsingQueryExpression<T>(Seq<T?> sequence) 
        where T : class
    {
        // no warnings
        return from item in sequence select item;
    }

    static Seq<T> UsingExtensionMethod<T>(Seq<T?> sequence)
        where T : class
    {
        // warning CS8603: Possible null reference return.
        // warning CS8619: Nullability of reference types in value of type 'IEnumerable<T?>' doesn't match target type 'IEnumerable<T>'.
        return sequence.Select(item => item);
            
        // Perhaps §12.20.3 (Query expression translation) should specify:
        return sequence.Select(item => item)!;
    }
}

BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request May 29, 2024
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.
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request May 29, 2024
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.

In addition, it fixes build warnings
@jskeet jskeet removed the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Jun 12, 2024
@jskeet
Copy link
Contributor

jskeet commented Jun 12, 2024

Dropped the meeting discuss label as we'll be discussing smaller ones.

BillWagner added a commit that referenced this pull request Jul 10, 2024
…llable context. (#1123)

* Port text from #700

The first commit ports in the text from #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.

In addition, it fixes build warnings

* Remove link (which will need to get added)

Remove an Xref to a section that's not in this PR.

* Define a single nullable context

Alternative PR to #1108.

This branch builds on the work in #1108. This is an alternative approach that defines a single `nullable` context. The nullable context has two properties: `annotations` and `warnings` that can be enabled or disabled separately.

* Update per meeting comments in review

This commit addresses all online feedback.

* Respond to feedback from June meeting

I listened to the June meeting. I updated a bit of text, and added Jon's example  from the conversation.

* Update lexical-structure.md

* Apply suggestions from code review

* Update standard/lexical-structure.md
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request Jul 11, 2024
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.
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request Sep 16, 2024
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.
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request Sep 18, 2024
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.
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request Sep 18, 2024
Bring in the normative text from dotnet#700.

Some text is removed because of the decision on normative language in our September meeting.
BillWagner added a commit that referenced this pull request Oct 9, 2024
* Port text from #700

The first commit ports in the text from #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.

* fix build warnings

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.

* Revert "fix build warnings"

This reverts commit dcb3731.

* test for nullable annotations

If the samples compile cleanly in this mode, I can continue in this PR without breaking all the samples.

* fix NRT grammar

* revert non-type changes.

* In progress draft

First draft at language for variations of nullability and reference types.

* Edit pass

* Apply suggestions from code review

Co-authored-by: Jon Skeet <[email protected]>

* remove null-oblivious type

* a fair amount of cleanup

* fix build issues.

* fix preprocessor symbols and test messages

* one more warning fix.

* respond to feedback.

* respond to feedback.

* Apply suggestions from code review

Co-authored-by: Jon Skeet <[email protected]>

* Notes for next updates

* fix warnings

* rebase and warnings

* word converter warnings

* word converter fx

* fix merge issues

* Updates from September meeting

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.

* typo

* lint issue

* Add description of the `!` operator

This makes sense to add to this PR logically.

* null forgiveness can't be on array creation

I think this may fix the left recursion error as well. Even if it doesn't, it's still correct.

* Update for implicit declarations

State the nullable type for implicitly typed variables

* Apply suggestions from code review

Co-authored-by: Jon Skeet <[email protected]>

* final updates from Oct 2nd meeting

Additional edits from comments at the Oct 2 meeting. This resolves all open concerns.

* Apply suggestions from code review

Co-authored-by: Nigel-Ecma <[email protected]>

---------

Co-authored-by: Jon Skeet <[email protected]>
Co-authored-by: Nigel-Ecma <[email protected]>
BillWagner added a commit to BillWagner/csharpstandard that referenced this pull request Oct 9, 2024
Bring in the normative text from dotnet#700.

Some text is removed because of the decision on normative language in our September meeting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: pending Proposal is available for review type: feature This issue describes a new feature
Projects
No open projects
Status: ✅ Done
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

5 participants