csharpstandard icon indicating copy to clipboard operation
csharpstandard copied to clipboard

Add support for nullable references

Open RexJaeschke opened this issue 3 years ago • 11 comments

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 avatar Jan 09 '23 16:01 RexJaeschke

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 avatar Jan 10 '23 11:01 RexJaeschke

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.

RexJaeschke avatar Jan 10 '23 11:01 RexJaeschke

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

RexJaeschke avatar Feb 07 '23 11:02 RexJaeschke

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

BillWagner avatar Sep 25 '23 18:09 BillWagner

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

BillWagner avatar Sep 25 '23 19:09 BillWagner

There is a bug report https://github.com/dotnet/roslyn/issues/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.

KalleOlaviNiemitalo avatar Nov 24 '23 18:11 KalleOlaviNiemitalo

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.

jskeet avatar Feb 07 '24 21:02 jskeet

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

Associated WorkItem - 209385

Serious question: does this need to go into v8 at all?

It is not unknown for compilers to introduce prototype implementations of features being considered for future versions of a language and the above comment makes me wonder whether Roslyn’s support in v8 should be seen as that, and hence not in the Standard, and the feature should go into v9.

Nigel-Ecma avatar Mar 04 '24 17:03 Nigel-Ecma

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.

jskeet avatar Apr 24 '24 21:04 jskeet

Re query expressions in https://github.com/dotnet/csharpstandard/pull/700#issuecomment-1377099339

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 https://github.com/dotnet/csharplang/issues/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)!;
    }
}

KalleOlaviNiemitalo avatar May 27 '24 18:05 KalleOlaviNiemitalo

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

jskeet avatar Jun 12 '24 20:06 jskeet