Nullable constraint checking outside method bodies #33261

jaredpar · 2019-02-08T21:06:55Z

Our constraint validation around nullability was only being done properly inside method bodies. This implements the checking in all other places: method signatures, base type lists, using static, etc ...

closes #32953

closes dotnet#32953

This makes nullability an explicit parameter to constraint checking rather than inferring it from the `ConversionsBase` argument. The reason is that these really represent separate operations: checking nullability in conversions and validating nullability in constraint checking.

The work for generic `unmanaged struct` constraints ended up threading through the correct `Compilation` object in many of the cases where I was using `Compilation` to calculate whether or not nullability needed to be checked for constraints. Used that whenever possible.

Don't need to be allocating them so many times during constraint checking.

jaredpar · 2019-02-08T21:08:13Z

@dotnet/roslyn-compiler for review

AlekseyTs · 2019-02-11T04:52:14Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

@@ -384,6 +384,7 @@ private bool FailsConstraintChecks(MethodSymbol method, out ArrayBuilder<TypePar
            bool constraintsSatisfied = ConstraintsHelper.CheckMethodConstraints(
                method,
                this.Conversions,
+                this.Conversions.IncludeNullability,


this.Conversions.IncludeNullability, [](start = 16, length = 36)

Should we always pass false here? Do we ever perform overload resolution during nullable flow analysis? #Closed

That's a good point.

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

AlekseyTs · 2019-02-11T04:53:02Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolutionResult.cs

@@ -974,7 +974,7 @@ private void ReportDuplicateNamedArgument(MemberResolutionResult<TMember> result
            // formal parameter type.

            TypeSymbol formalParameterType = method.ParameterTypes[result.Result.BadParameter].TypeSymbol;
-            formalParameterType.CheckAllConstraints((CSharpCompilation)compilation, conversions, location, diagnostics);
+            formalParameterType.CheckAllConstraints((CSharpCompilation)compilation, conversions, conversions.IncludeNullability, location, diagnostics);


conversions.IncludeNullability [](start = 97, length = 30)

Should we always pass false here? Do we ever perform overload resolution during nullable flow analysis? #Closed

AlekseyTs · 2019-02-11T04:57:18Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -3177,6 +3177,7 @@ private void CheckMethodConstraints(SyntaxNode syntax, MethodSymbol method)
            ConstraintsHelper.CheckMethodConstraints(
                method,
                _conversions,
+                _conversions.IncludeNullability,


_conversions.IncludeNullability [](start = 16, length = 31)

Should we simply pass true here? #Closed

AlekseyTs · 2019-02-11T05:08:05Z

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

@@ -797,6 +821,52 @@ private static bool HasDuplicateInterfaces(NamedTypeSymbol type, ConsList<TypeSy
                skipParameters);
        }

+        private struct ConversionsUtil


private struct ConversionsUtil [](start = 8, length = 30)

It looks like this mechanism is redundant. Does it duplicate what ConversionsBase._lazyOtherNullability is used for? #Closed

Indeed it does

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

AlekseyTs · 2019-02-11T05:10:50Z

internal abstract partial class TypeParameterSymbol : TypeSymbol, ITypeParameterSymbol

Is there a meaningful change in this file? Consider reverting otherwise. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeParameterSymbol.cs:15 in 9a0b8cb. [](commit_id = 9a0b8cb, deletion_comment = False)

AlekseyTs · 2019-02-11T05:11:30Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceEventSymbol.cs

@@ -668,6 +668,7 @@ protected TypeSymbolWithAnnotations BindEventType(Binder binder, TypeSyntax type
        internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions, DiagnosticBag diagnostics)
        {
            var location = this.Locations[0];
+            bool includeNullability = DeclaringCompilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes);


bool includeNullability = DeclaringCompilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes); [](start = 12, length = 109)

It looks like the local is not used. #Closed

Missed that when resolving the conflicts with Rikki PR

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

AlekseyTs · 2019-02-11T05:12:36Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs

@@ -1408,6 +1408,7 @@ internal override void ForceComplete(SourceLocation locationOpt, CancellationTok
                            {
                                var diagnostics = DiagnosticBag.GetInstance();
                                var conversions = new TypeConversions(this.ContainingAssembly.CorLibrary);
+                                bool includeNullability = DeclaringCompilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes);


bool includeNullability = DeclaringCompilation.IsFeatureEnabled(MessageID.IDS_FeatureNullableReferenceTypes); [](start = 32, length = 109)

It looks like the local is not used. #Closed

AlekseyTs · 2019-02-11T05:13:26Z

Is there a meaningful change in this file? Consider reverting otherwise. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol_Bases.cs:19 in 9a0b8cb. [](commit_id = 9a0b8cb, deletion_comment = False)

AlekseyTs · 2019-02-11T05:15:45Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

@@ -106,7 +106,7 @@ internal sealed class TupleTypeSymbol : WrappedNamedTypeSymbol
            var constructedType = Create(underlyingType, elementNames, errorPositions, locationOpt, elementLocations);
            if (shouldCheckConstraints && diagnostics != null)
            {
-                constructedType.CheckConstraints(compilation.Conversions, syntax, elementLocations, compilation, diagnostics);
+                constructedType.CheckConstraints(compilation.Conversions, compilation.Conversions.IncludeNullability, syntax, elementLocations, compilation, diagnostics);


compilation.Conversions.IncludeNullability [](start = 74, length = 42)

Should we simply pass false here? #Closed

Don't believe so. The code goes through the process of validating constraints here hence it woudl seem that nullable should be checked as well (as it was before my change)., Is the expectation that the nullable constraints are always checked elsewhere, say NullableWalker, for method bodies?

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

@AlekseyTs: @cston and I have chatted about this and I'm a bit stuck on how to proceed.

On one hand the compiler tends to special case tuple and it's questionable if tuple constraints should matter at all. But the code clearly has the intent of validating tuple constaints. Hence it seems that we should consider tuple constraints in this scenario and I can't see an obvious reason for excluding nullability ones.

As an experiment I passed false as you suggested and no tests failed. But that possibly means we're just not validating the correct scenario here. A test where the tuple had say a class constraint for every type parameter could just not be present to catch this.

Can you elaborate a bit on why you think false is the right check here?

#Resolved

Don't believe so. The code goes through the process of validating constraints here hence it woudl seem that nullable should be checked as well (as it was before my change)., Is the expectation that the nullable constraints are always checked elsewhere, say NullableWalker, for method bodies?

I doubt nullable aspect of constraints was checked here before. Is compilation.Conversions ever checks nullable? I think that your intuition is probably correct that for some cases nullbility aspect should be checked here, but probably not for all cases. For example, when this method is called from BindTupleExpression, it feels like the check for the nullability aspect should be postponed until nullable analysis, when we know nullability of expressions involved. Deconstruction is the other example. It feels like the change in this function should be scenario and unit-test driven. It would be fine to follow up on this separately.

In reply to: 255648877 [](ancestors = 255648877,255382791)

Can you elaborate a bit on why you think false is the right check here?

As I mentioned in my other response to this thread, I believe that would be consistent with the previous behavior. This method is never called from nullable analysis. I think that it would be good to back this change with targeted unit-tests that cover every call site for this function.

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

I doubt nullable aspect of constraints was checked here before. Is compilation.Conversions ever checks nullable?

This is the root of my confusion. I had mistakenly believed that compliation.Conversions.IncludeNullability was toggled based on C# 8.0 Instead looking now that is a case where it is essentially always false. Agree false is correct here.

. It feels like the change in this function should be scenario and unit-test driven.

Agree. Want to make sure I get this tested. Prefer to just take care of it now vs. waiting till later if possible.

In reply to: 255676114 [](ancestors = 255676114,255648877,255382791)

Dug into this a bit and decided to pull out this validation to a separate PR. The validation of tuple constraints is now done properly in method signatures, which is the intent of this PR. The problem is the validation within method bodies. Will follow up on that separately

#33303

In reply to: 255689761 [](ancestors = 255689761,255676114,255648877,255382791)

AlekseyTs · 2019-02-11T05:24:20Z

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

@@ -54412,6 +54684,9 @@ public void Test2(TB1 a2, TB2 b2)
            var comp1 = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue());
            // https://github.com/dotnet/roslyn/issues/29678: Constraint violations are not reported for type references outside of method bodies.


// #29678: Constraint violations are not reported for type references outside of method bodies. [](start = 12, length = 134)

Consider removing this comment. #Closed

AlekseyTs · 2019-02-11T05:29:10Z

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

@@ -56341,12 +56635,24 @@ class C4<T, U> : I<T?>, I<U?> where T : class where U : class { }";
                // (2,7): error CS0695: 'C1<T, U>' cannot implement both 'I<T>' and 'I<U>' because they may unify for some type parameter substitutions
                // class C1<T, U> : I<T>, I<U> where T : class where U : class { }
                Diagnostic(ErrorCode.ERR_UnifyingInterfaceInstantiations, "C1").WithArguments("C1<T, U>", "I<T>", "I<U>").WithLocation(2, 7),
+                // (3,7): warning CS8634: The type 'U?' cannot be used as type parameter 'T' in the generic type or method 'I<T>'. Nullability of type argument 'U?' doesn't match 'class' constraint.


// (3,7): warning CS8634: The type 'U?' cannot be used as type parameter 'T' in the generic type or method 'I'. Nullability of type argument 'U?' doesn't match 'class' constraint. [](start = 16, length = 182)

Alternatively, we could relax class constraint in interface I<T> where T : class to class?. This should reduce the unnecessary noise in the base-line. #Closed

AlekseyTs · 2019-02-11T05:33:31Z

Done with review pass (iteration 6). #Closed

jaredpar · 2019-02-11T18:57:38Z

There was originally but all of the meaningful changes got removed during merge conflict resolution. Let me try and rebase this out.

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

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol_Bases.cs:19 in 9a0b8cb. [](commit_id = 9a0b8cb, deletion_comment = False)

jaredpar · 2019-02-11T19:03:19Z

internal abstract partial class TypeParameterSymbol : TypeSymbol, ITypeParameterSymbol

There was originally but all of the meaningful changes got removed during merge conflict resolution. Let me try and rebase this out.

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

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeParameterSymbol.cs:15 in 9a0b8cb. [](commit_id = 9a0b8cb, deletion_comment = False)

jaredpar · 2019-02-11T19:18:38Z

internal abstract partial class TypeParameterSymbol : TypeSymbol, ITypeParameterSymbol

I've tried to undo this but cannot. The changes mixing with the merge conflicts are making it hard to yank this from the change.

In reply to: 462452107 [](ancestors = 462452107,462221157)

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeParameterSymbol.cs:15 in 9a0b8cb. [](commit_id = 9a0b8cb, deletion_comment = False)

jaredpar · 2019-02-11T19:19:01Z

I've tried to undo this but cannot. The changes mixing with the merge conflicts are making it hard to yank this from the change. I will remove the meaningless change from this file though.

In reply to: 462449955 [](ancestors = 462449955,462221392)

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol_Bases.cs:19 in 9a0b8cb. [](commit_id = 9a0b8cb, deletion_comment = False)

cston · 2019-02-11T20:11:51Z

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

+                // (19,25): warning CS8634: The type 'object?' cannot be used as type parameter 'T' in the generic type or method 'C<T>'. Nullability of type argument 'object?' doesn't match 'class' constraint.
+                // delegate C<object?> D(C<object?> p); // 8
+                Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterReferenceTypeConstraint, "object?").WithArguments("C<T>", "T", "object?").WithLocation(19, 25));
+        }


} [](start = 8, length = 1)

Are we testing constructors and operators? For instance:

#pragma warning disable 0649 namespace System { struct ValueTuple<T, U> where T : object { public T Item1; public U Item2; } } class C { C((object?, object?) t) { } }

AlekseyTs · 2019-02-11T20:26:45Z

Done with review pass (iteration 7). I think the issue around the tuple types needs a follow-up, doesn't have to be done in this PR though. #Closed

AlekseyTs

LGTM (iteration 7)

cston · 2019-02-11T22:25:25Z

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

@@ -10,8 +10,8 @@ namespace Microsoft.CodeAnalysis.CSharp
 {
    internal sealed class TypeConversions : ConversionsBase
    {
-        public TypeConversions(AssemblySymbol corLibrary)
-            : this(corLibrary, currentRecursionDepth: 0, includeNullability: false, otherNullabilityOpt: null)
+        public TypeConversions(AssemblySymbol corLibrary, bool includeNullability = false)


bool includeNullability = false [](start = 58, length = 31)

Is this change needed?

cston · 2019-02-11T22:49:53Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberFieldSymbol.cs

@@ -575,8 +575,7 @@ internal override bool IsDefinedInSourceTree(SyntaxTree tree, TextSpan? definedW
        internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions, DiagnosticBag diagnostics)
        {
            var options = (CSharpParseOptions)SyntaxTree.Options;


options [](start = 16, length = 7)

Not used.

cston · 2019-02-11T23:08:03Z

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

@@ -55997,9 +56271,22 @@ class B4<T> : A<T?, T?>, I<T?, T?>
 {
 }";


} [](start = 0, length = 1)

Consider adding:

class B5<T> : A<T, T>, I<T, T> where T : class? { }

cston · 2019-02-11T23:45:06Z

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

+    public static implicit operator C<string?>(D2 D2) => new C<string?>(); // 2, 3
+}
+";
+            var compilation = CreateCompilation(Tuple2NonNullable + source, targetFramework: TargetFramework.Mscorlib46);


Tuple2NonNullable [](start = 48, length = 17)

Not used.

cston · 2019-02-11T23:46:18Z

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

+    void M4(C<(string, string)> p) { }
+}
+";
+            var compilation = CreateCompilation(Tuple2NonNullable + source, targetFramework: TargetFramework.Mscorlib46);


Tuple2NonNullable + source [](start = 48, length = 26)

Consider using new[] { Tuple2NonNullable, source } here and below.

jaredpar added 6 commits February 8, 2019 10:53

Enable constraint checks in signatures

ad92327

closes dotnet#32953

Cleaned up the unit tests

32921ff

More test cases

7a7f65d

Cache ConversionBase

9a0b8cb

Don't need to be allocating them so many times during constraint checking.

jaredpar added the Area-Compilers label Feb 8, 2019

jaredpar requested a review from a team as a code owner February 8, 2019 21:06

AlekseyTs reviewed Feb 11, 2019

View reviewed changes

Respond to PR feedback

59774db

cston reviewed Feb 11, 2019

View reviewed changes

AlekseyTs approved these changes Feb 11, 2019

View reviewed changes

cston reviewed Feb 11, 2019

View reviewed changes

cston approved these changes Feb 11, 2019

View reviewed changes

PR feedback

6499be4

cston reviewed Feb 11, 2019

View reviewed changes

jaredpar added 3 commits February 11, 2019 15:52

Fix double constraint check

f4627c0

PR feedback

2d4e2d8

Fixed a test

5570bd5

jaredpar merged commit a557079 into dotnet:master Feb 12, 2019

jaredpar deleted the fix-constraints branch February 12, 2019 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nullable constraint checking outside method bodies #33261

Nullable constraint checking outside method bodies #33261

jaredpar commented Feb 8, 2019 •

edited

Loading

jaredpar commented Feb 8, 2019

AlekseyTs Feb 11, 2019 •

edited

Loading

jaredpar Feb 11, 2019

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

jaredpar Feb 11, 2019

AlekseyTs commented Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

jaredpar Feb 11, 2019

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs commented Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

jaredpar Feb 11, 2019

jaredpar Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019

AlekseyTs Feb 11, 2019

jaredpar Feb 11, 2019

jaredpar Feb 11, 2019

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs commented Feb 11, 2019 •

edited

Loading

jaredpar commented Feb 11, 2019

jaredpar commented Feb 11, 2019

jaredpar commented Feb 11, 2019

jaredpar commented Feb 11, 2019

cston Feb 11, 2019

AlekseyTs commented Feb 11, 2019 •

edited

Loading

AlekseyTs left a comment

cston Feb 11, 2019

cston Feb 11, 2019

cston Feb 11, 2019

cston Feb 11, 2019

cston Feb 11, 2019

		@@ -54412,6 +54684,9 @@ public void Test2(TB1 a2, TB2 b2)
		var comp1 = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue());
		// https://github.com/dotnet/roslyn/issues/29678: Constraint violations are not reported for type references outside of method bodies.

		@@ -55997,9 +56271,22 @@ class B4<T> : A<T?, T?>, I<T?, T?>
		{
		}";

Nullable constraint checking outside method bodies #33261

Nullable constraint checking outside method bodies #33261

Conversation

jaredpar commented Feb 8, 2019 • edited Loading

jaredpar commented Feb 8, 2019

AlekseyTs Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Feb 11, 2019 • edited Loading

AlekseyTs Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Feb 11, 2019 • edited Loading

AlekseyTs Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Feb 11, 2019 • edited Loading

jaredpar commented Feb 11, 2019

jaredpar commented Feb 11, 2019

jaredpar commented Feb 11, 2019

jaredpar commented Feb 11, 2019

Choose a reason for hiding this comment

AlekseyTs commented Feb 11, 2019 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Feb 8, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs commented Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs commented Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

jaredpar Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs Feb 11, 2019 •

edited

Loading

AlekseyTs commented Feb 11, 2019 •

edited

Loading

AlekseyTs commented Feb 11, 2019 •

edited

Loading