Add initial support for 'notnull' generic type constraint. by AlekseyTs · Pull Request #36109 · dotnet/roslyn

AlekseyTs · 2019-06-01T00:14:48Z

See https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-15.md
Also fixes #36005.

See https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-15.md Also fixes dotnet#36005.

AlekseyTs · 2019-06-01T00:15:30Z

@dotnet/roslyn-compiler Please review.

AlekseyTs · 2019-06-03T16:49:01Z

@dotnet/roslyn-compiler, @cston, @jcouv Please review.

jcouv · 2019-06-03T16:59:08Z

docs/features/nullable-reference-types.md

 NullableAttribute(1) should be placed on a type parameter definition that has a `class!` constraint.
 NullableAttribute(2) should be placed on a type parameter definition that has a `class?` constraint.
-NullableAttribute(2) should be placed on a type parameter definition that has no type constraints, `class`, `struct` and `unmanaged` constraints and
+NullableAttribute(2) should be placed on a type parameter definition that has no type constraints, `notnull`, `class`, `struct` and `unmanaged` constraints and


constraints [](start = 86, length = 11)

nit: I had trouble parsing this sentence, so was confused. Consider removing the word "constraints" here ("... that has no type, notnull, class ... constraints"). Also typo on next line (equivalent). #Resolved

jcouv · 2019-06-03T17:29:14Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs

        internal override bool? IsNotNullableIfReferenceType
        {
            get
            {


{ [](start = 12, length = 1)

Could we do if (HasNotnullConstraint) { return true; }? #Resolved

Could we do if (HasNotnullConstraint) { return true; }?

We could (as in other implementations of IsNotNullableIfReferenceType), but I decided against duplicating the logic around that and keep it in CalculateIsNotNullableIfReferenceType.

In reply to: 289953856 [](ancestors = 289953856)

jcouv · 2019-06-03T18:31:04Z

            // (8,59): error CS0449: The 'class' or 'struct' constraint must come before any other constraints

Consider consolidating the error messages CS8713 and CS0449 together. "The class, struct, notnull constraints must come before ..." #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:61204 in e463d06. [](commit_id = e463d06, deletion_comment = False)

AlekseyTs · 2019-06-03T18:37:20Z

            // (8,59): error CS0449: The 'class' or 'struct' constraint must come before any other constraints

We already have a specialized error for unmanaged. I'll open an issue to suggest to consolidate them all.

In reply to: 498373294 [](ancestors = 498373294)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:61204 in e463d06. [](commit_id = e463d06, deletion_comment = False)

cston · 2019-06-03T19:01:51Z

src/Compilers/CSharp/Portable/Syntax/TypeSyntax.cs


        public bool IsUnmanaged => ((InternalSyntax.TypeSyntax)this.Green).IsUnmanaged;
+
+        public bool IsNotnull => ((InternalSyntax.TypeSyntax)this.Green).IsNotnull;


IsNotnull [](start = 20, length = 9)

Should this be "IsNotNull" (uppercase "N")? It looks like other members of the public API use uppercase for compound keywords ("IsReadOnly", "NameOf", "OrderBy", etc.). #Resolved

cston · 2019-06-03T19:14:54Z

src/Compilers/CSharp/Portable/Symbols/TypeParameterSymbol.cs

        /// </summary>
        internal abstract bool? ReferenceTypeConstraintIsNullable { get; }

+        public abstract bool HasNotnullConstraint { get; }


Notnull [](start = 32, length = 7)

Consider using "NotNull" (uppercase "N") throughout. #Resolved

cston · 2019-06-03T19:21:26Z

src/Compilers/CSharp/Portable/Binder/Binder_Constraints.cs

                    }

+                    // "Constraint cannot be special class '{0}'"
+                    Error(diagnostics, ErrorCode.WRN_SpecialTypeAsBound, syntax, typeWithAnnotations);


WRN_SpecialTypeAsBound [](start = 49, length = 22)

Should an object constraint be reported as an error rather than a warning, as in C#7? #Resolved

I thought we were going to keep this as for the moment, and disallow it later to allow corefx to migrate.

In reply to: 289997980 [](ancestors = 289997980)

Should an object constraint be reported as an error rather than a warning, as in C#7?

If I remember correctly, removal of support for object constraint is in the second phase. The reason is to avoid breaking CoreFx right from the first compiler upgrade.

In reply to: 290001495 [](ancestors = 290001495,289997980)

cston · 2019-06-03T19:27:58Z

            //     public void F1<TF1A>() where TF1A : object

Comments are stale. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:62001 in e463d06. [](commit_id = e463d06, deletion_comment = False)

cston · 2019-06-03T19:30:37Z

            // partial void M1<TF1>() where TF1 : object

notnull #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:65653 in e463d06. [](commit_id = e463d06, deletion_comment = False)

jcouv

LGTM Thanks (iteration 3)

cston · 2019-06-03T19:39:19Z

Consider testing the current constraints with M<notnull?>(), and also testing where T : notnull?. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:66945 in e463d06. [](commit_id = e463d06, deletion_comment = False)

cston · 2019-06-03T19:41:23Z

Consider testing: M<notnull>() where notnull : notnull

In reply to: 498396946 [](ancestors = 498396946)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:66945 in e463d06. [](commit_id = e463d06, deletion_comment = False)

cston · 2019-06-03T20:00:59Z

Are we testing cases where the notnull constraint is on a declaration from metadata? #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:68742 in e463d06. [](commit_id = e463d06, deletion_comment = False)

AlekseyTs · 2019-06-03T20:20:09Z

Are we testing cases where the notnull constraint is on a declaration from metadata?

To the same degree as we tested that for object constraint. For example, Constraints_46, Constraints_47, Constraints_59, etc.

In reply to: 498404345 [](ancestors = 498404345)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:68742 in e463d06. [](commit_id = e463d06, deletion_comment = False)

cston · 2019-06-03T20:28:16Z

Do any of those test the check in ConstraintsHelper?

In reply to: 498410871 [](ancestors = 498410871,498404345)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:68742 in e463d06. [](commit_id = e463d06, deletion_comment = False)

AlekseyTs · 2019-06-03T20:30:03Z

Do any of those test the check in ConstraintsHelper?

I do not think so, they test APIs that those changes use.

In reply to: 498413865 [](ancestors = 498413865,498410871,498404345)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:68742 in e463d06. [](commit_id = e463d06, deletion_comment = False)

cston · 2019-06-03T20:37:55Z

In short, what is the error reported for the following? Does it refer to notnull?
a.cs:

public interface I<T> where T : notnull { }

b.cs:

class Program
{
    static void M<T>()
    {
        I<T> i;
    }
}

In reply to: 498414534 [](ancestors = 498414534,498413865,498410871,498404345)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:68742 in e463d06. [](commit_id = e463d06, deletion_comment = False)

AlekseyTs · 2019-06-03T20:43:48Z

In short, what is the error reported for the following? Does it refer to notnull?

Yes, there is an example several lines above:

                // (8,18): warning CS8714: The type 'TIB' cannot be used as type parameter 'TA' in the generic type or method 'IA<TA>'. Nullability of type argument 'TIB' doesn't match 'notnull' constraint.
                // public interface IB<TIB> : IA<TIB> // 1
                Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterNotnullConstraint, "IB").WithArguments("IA<TA>", "TA", "TIB").WithLocation(8, 18),

In reply to: 498417252 [](ancestors = 498417252,498414534,498413865,498410871,498404345)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:68742 in e463d06. [](commit_id = e463d06, deletion_comment = False)

AlekseyTs · 2019-06-04T00:11:51Z

@jcouv, @cston The feedback has been addressed.

cston · 2019-06-04T00:21:34Z

src/EditorFeatures/CSharpTest2/Recommendations/NotnullKeywordRecommenderTests.cs

+
+namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Recommendations
+{
+    public class NotnullKeywordRecommenderTests : RecommenderTests


Notnull [](start = 17, length = 7)

Consider using "NotNull". #Resolved

cston · 2019-06-04T00:21:43Z

src/Features/CSharp/Portable/Completion/KeywordRecommenders/NotnullKeywordRecommender.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.Completion.KeywordRecommenders
+{
+    internal class NotnullKeywordRecommender : IKeywordRecommender<CSharpSyntaxContext>


Notnull [](start = 19, length = 7)

Consider using "NotNull". #Resolved

jcouv

LGTM Thanks (iteration 6)

AlekseyTs · 2019-06-04T20:23:10Z

            // (8,59): error CS0449: The 'class' or 'struct' constraint must come before any other constraints

Opened #36160

In reply to: 498375369 [](ancestors = 498375369,498373294)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:61204 in e463d06. [](commit_id = e463d06, deletion_comment = False)

Add initial support for 'notnull' generic type constraint.

b7f7299

See https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-15.md Also fixes dotnet#36005.

AlekseyTs added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jun 1, 2019

AlekseyTs requested review from a team as code owners June 1, 2019 00:14

AlekseyTs added 2 commits June 2, 2019 10:46

Merge remote-tracking branch 'dotnet/master' into NotNull_01

9334532

Follow up on merge from master.

e463d06

jcouv reviewed Jun 3, 2019

View reviewed changes

cston reviewed Jun 3, 2019

View reviewed changes

jcouv added this to the 16.2.P3 milestone Jun 3, 2019

jcouv self-assigned this Jun 3, 2019

jcouv approved these changes Jun 3, 2019

View reviewed changes

Merge remote-tracking branch 'dotnet/master' into NotNull_01

d68d0e3

AlekseyTs added 2 commits June 3, 2019 15:23

Rename Notnull to NotNull

c039e4a

PR feedback

f78190f

cston reviewed Jun 4, 2019

View reviewed changes

cston approved these changes Jun 4, 2019

View reviewed changes

jcouv approved these changes Jun 4, 2019

View reviewed changes

More renames

1726f4f

AlekseyTs merged commit 10e0d22 into dotnet:master Jun 4, 2019

jaredpar mentioned this pull request Jun 6, 2019

Nullable Reference Type Changes #35816

Closed

23 tasks

AlekseyTs mentioned this pull request Jun 17, 2019

Fixed IDE services touching notnull constraint #36508

Merged


		public bool IsUnmanaged => ((InternalSyntax.TypeSyntax)this.Green).IsUnmanaged;

		public bool IsNotnull => ((InternalSyntax.TypeSyntax)this.Green).IsNotnull;

Conversation

AlekseyTs commented Jun 1, 2019

Uh oh!

AlekseyTs commented Jun 1, 2019

Uh oh!

AlekseyTs commented Jun 3, 2019

Uh oh!

jcouv Jun 3, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 3, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jun 3, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Jun 3, 2019

Uh oh!

cston Jun 3, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jun 3, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jun 3, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

cston commented Jun 3, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jun 3, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Jun 3, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jun 3, 2019

Uh oh!

cston commented Jun 3, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Jun 3, 2019

Uh oh!

cston commented Jun 3, 2019

Uh oh!

AlekseyTs commented Jun 3, 2019

Uh oh!

cston commented Jun 3, 2019

Uh oh!

AlekseyTs commented Jun 3, 2019

Uh oh!

AlekseyTs commented Jun 4, 2019

Uh oh!

cston Jun 4, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jun 4, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

jcouv Jun 3, 2019 •

edited by AlekseyTs

Loading

jcouv Jun 3, 2019 •

edited by AlekseyTs

Loading

jcouv commented Jun 3, 2019 •

edited by AlekseyTs

Loading

cston Jun 3, 2019 •

edited by AlekseyTs

Loading

cston Jun 3, 2019 •

edited by AlekseyTs

Loading

cston Jun 3, 2019 •

edited by AlekseyTs

Loading

cston commented Jun 3, 2019 •

edited by AlekseyTs

Loading

cston commented Jun 3, 2019 •

edited by AlekseyTs

Loading

cston commented Jun 3, 2019 •

edited by AlekseyTs

Loading

cston commented Jun 3, 2019 •

edited by AlekseyTs

Loading

cston Jun 4, 2019 •

edited by AlekseyTs

Loading

cston Jun 4, 2019 •

edited by AlekseyTs

Loading