Include nullability in CheckConstraints by cston · Pull Request #28959 · dotnet/roslyn

cston · 2018-07-31T06:06:30Z

No description provided.

…s' into check-constraints

cston · 2018-07-31T17:59:05Z

@dotnet/roslyn-compiler please review.

333fred · 2018-08-01T17:37:16Z

@cston test failure appears legitimate

333fred · 2018-08-01T21:48:47Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs

 {
    public class AttributeTests_Nullable : CSharpTestBase
    {
+        private const string NonNullTypesAttributesDefinition = @"


NonNullTypesAttributesDefinition [](start = 29, length = 32)

We should consider making this available on CSharpTestBase, it's likely that many more tests will need the attribute.

Added a // PROTOTYPE comment for now.

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

333fred · 2018-08-01T21:51:21Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

-                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "F(x, y)").WithLocation(7, 9));
+                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "F(x, y)").WithLocation(7, 9),
+                // (8,9): warning CS8631: The type 'C?' cannot be used as type parameter 'U' in the generic type or method 'C.F<T, U>(T, U)'. Nullability of type argument 'C?' doesn't match constraint type 'C'.
+                //         F(y, x).ToString();


Consider adding some comments to the test to help read the errors. #Resolved

333fred · 2018-08-01T22:03:15Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

+        protected ConversionsBase(AssemblySymbol corLibrary, int currentRecursionDepth, bool includeNullability, ConversionsBase otherNullabilityOpt)
        {
            Debug.Assert((object)corLibrary != null);
+            Debug.Assert(otherNullabilityOpt == null || includeNullability == !otherNullabilityOpt.IncludeNullability);


Why not just !=? #Closed

333fred

Done review pass (commit 4)

cston · 2018-08-02T05:07:03Z

@jcouv, @dotnet/roslyn-compiler, please review.

…s' into check-constraints

333fred

LGTM (commit 7)

jcouv · 2018-08-02T21:06:55Z

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

            {
-                type.CheckConstraintsForNonTuple(this.Conversions, typeSyntax, typeArgumentsSyntax, this.Compilation, basesBeingResolved, diagnostics);
+                bool includeNullability = Compilation.IsFeatureEnabled(MessageID.IDS_FeatureStaticNullChecking);
+                type.CheckConstraintsForNonTuple(this.Conversions.WithNullability(includeNullability), typeSyntax, typeArgumentsSyntax, this.Compilation, basesBeingResolved, diagnostics);


CheckConstraintsForNonTuple [](start = 21, length = 27)

Why are tuples vs. non-tuples relevant here?
Do we need to test a ValueTuple type with constraints?

This is not specific to nullable - this call is in master. And it looks like the transform to TupleTypeSymbol occurs below, after constraints are checked.

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

jcouv · 2018-08-02T21:08:44Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

-        protected ConversionsBase(AssemblySymbol corLibrary, int currentRecursionDepth, bool includeNullability)
+        private ConversionsBase _lazyOtherNullability;
+
+        protected ConversionsBase(AssemblySymbol corLibrary, int currentRecursionDepth, bool includeNullability, ConversionsBase otherNullabilityOpt)


ConversionsBase [](start = 18, length = 15)

xml doc would be useful

Added comments to field and WithNullability.

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

jcouv · 2018-08-02T21:12:16Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

+            _lazyOtherNullability = otherNullabilityOpt;
        }

+        internal ConversionsBase WithNullability(bool includeNullability)


WithNullability [](start = 33, length = 15)

doc on this would be useful too, since there is some trickery to minimize allocations.
Maybe also on the _lazyOtherNullability field. #Resolved

jcouv · 2018-08-02T21:14:22Z

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

        /// <param name="typeArguments">Containing symbol type arguments.</param>
        /// <param name="currentCompilation">Improves error message detail.</param>
        /// <param name="diagnosticsBuilder">Diagnostics.</param>
+        /// <param name="warningsBuilderOpt">Nullability warnings.</param>


warningsBuilderOpt [](start = 25, length = 18)

Why do we need to keep warnings separate from the rest of diagnostics?

To allow NullableWalker to drop all but the nullability diagnostics.

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

jcouv · 2018-08-02T21:21:40Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

            comp.VerifyDiagnostics();
        }

-        [Fact(Skip = "PROTOTYPE(NullableReferenceTypes): hits an assertion in AsObliviousReferenceType")]


Skip [](start = 14, length = 4)

Thanks! It looks like I'd forgotten to unskip a bunch of NNT tests.

jcouv · 2018-08-02T21:27:01Z

Do we have tests where the nullability warning occurs within the constraints declaration itself?
For instance:

class C<TBase, TDerived> where TDerived : TBase { }
class D<U> where U : C<U, U?> { } // warn

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

jcouv · 2018-08-02T21:34:48Z

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

+                        Debug.Assert(conversions.IncludeNullability);
+                        if (!SatisfiesConstraintType(conversions, typeArgument, constraintType, ref useSiteDiagnostics))
+                        {
+                            warningsBuilderOpt.Add(new TypeParameterDiagnosticInfo(typeParameter, new CSDiagnosticInfo(ErrorCode.WRN_NullabilityMismatchInTypeParameterConstraint, containingSymbol.ConstructedFrom(), constraintType, typeParameter, typeArgument)));


warningsBuilderOpt [](start = 28, length = 18)

nit: Consider breaking long line #Resolved

jcouv · 2018-08-02T21:36:02Z

        // Spec 4.4.4 describes the valid conversions from

Please update the feature doc to reflect the implemented rules.

Refers to: src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs:1016 in 1206b86. [](commit_id = 1206b86, deletion_comment = False)

jcouv · 2018-08-02T21:36:54Z

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

            // "An identity conversion (6.1.1).
            // An implicit reference conversion (6.1.6). ..."
-            if (conversions.HasIdentityOrImplicitReferenceConversion(typeArgument.TypeSymbol, constraintType.TypeSymbol, ref useSiteDiagnostics))
+            if ((!conversions.IncludeNullability || ConversionsBase.HasTopLevelNullabilityImplicitConversion(typeArgument, constraintType)) &&


if [](start = 12, length = 2)

Could you add a comment? #Resolved

jcouv

Done with review pass (iteration 7). Mostly some documentation requests. Thanks

cston · 2018-08-03T06:20:29Z

Added test, currently with // PROTOTYPE comment.

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

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

… into unconstrained-type-parameter-nullability * dotnet/features/NullableReferenceTypes: Include nullability in CheckConstraints (dotnet#28959) Use struct for TypeSymbolWithAnnotations (dotnet#28943) Publish Linux Test Results Force retest Migrate Linux tests to VSTS Port determinism fixes Make sure TLS 1.2 is used to fetch from https://dot.net Move to language version 8 Make mutex creation more robust Disable icacls use on Helix Disable leak detection tests on x64 VSTS YAML file

Include nullability in CheckConstraints

14001cc

cston requested a review from a team as a code owner July 31, 2018 06:06

cston added 2 commits July 31, 2018 08:49

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

84badd1

…s' into check-constraints

Cache Conversions.WithNullability()

f63475e

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jul 31, 2018

jcouv added this to the 16.0 milestone Jul 31, 2018

Misc.

1aed4a8

333fred reviewed Aug 1, 2018

View reviewed changes

PR feedback

27ffe07

cston added 2 commits August 1, 2018 22:35

Enable tests

990a1a1

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

1206b86

…s' into check-constraints

333fred approved these changes Aug 2, 2018

View reviewed changes

jcouv reviewed Aug 2, 2018

View reviewed changes

PR feedback

cec0d9f

cston merged commit 0ece26c into dotnet:features/NullableReferenceTypes Aug 3, 2018

cston deleted the check-constraints branch August 3, 2018 14:36

cston mentioned this pull request Aug 3, 2018

No warning for mismatched nullability in constraint #27742

Closed

Conversation

cston commented Jul 31, 2018

Uh oh!

cston commented Jul 31, 2018

Uh oh!

333fred commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Aug 1, 2018 • edited by cston 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 Aug 1, 2018 • edited 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 left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Aug 2, 2018

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 2, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Aug 2, 2018

Uh oh!

jcouv Aug 2, 2018 • edited by cston 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 commented Aug 2, 2018

Uh oh!

jcouv Aug 2, 2018 • edited by cston 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

Uh oh!

cston commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

333fred Aug 1, 2018 •

edited by cston

Loading

333fred Aug 1, 2018 •

edited

Loading

jcouv Aug 2, 2018 •

edited by cston

Loading

jcouv Aug 2, 2018 •

edited by cston

Loading

jcouv Aug 2, 2018 •

edited by cston

Loading