Use flag to represent suppression in bound tree #32850

jcouv · 2019-01-27T02:37:24Z

This PR addresses some issues with the suppression operator. It does not address tuple scenarios or the semantic model.
Compared to #32699 which added many special cases based on bound node for suppression, this PR drops the suppression node from the bound tree and represents suppression as a flag.

Fixed a number of crashes
Fixes #29862 (top-level nullability of !)
Fixes #29903 (t! = s should be an error)
I also noticed that we skipped some warnings in cast scenarios (latest LDM notes on the topic)
Fixes #31294 (throw null!; isn't allowed)
Fixes #29905
Fixes #29906

Follow-ups:

We'll need to refine where suppressions are allowed (I probably made more cases illegal than strictly necessary, for now)
Hit assertion in CheckValueKind related to suppression operator #29710 (analysis of ref re-assignment and CheckValueKind)
error CS8374: Cannot ref-assign 'x!' to 'x' because 'x!' has a narrower escape scope than 'x'. #31297 (escape rules and suppressions)
Handle tuple conversion on tuple with suppression #32553 (suppressions on tuple literals)
Nullable + dynamic #32364 (nullable and dynamic)
Verify semantic model on suppressed expressions #32661 (follow-up on semantic model on suppressed expressions)
Need to re-analyze method groups #32697 (need to re-analyze method group conversions)
Need nullability analysis of delegate creation expressions #32698 (need to analyze delegate creations)
Nullable analysis of default parameter values #32844 (need to analyze default parameter values better)
Representation of suppression in IOperation #32881 (represent suppressions in IOperation)

Notes:

Although I ended up removing the IOperation test I'd added, I adjusted the comparison logic for IOperation assertions to tolerate whitespaces

jaredpar · 2019-01-27T03:09:18Z

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

+            if (unboundLambda.IsSuppressed)
+            {
+                boundLambda = boundLambda.WithSuppression();
+            }


Is there a reason the implementation of Bind shouldn't be handling this? #Resolved

That's a good idea. #Resolved

jaredpar · 2019-01-27T03:10:05Z

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

@@ -268,6 +270,14 @@ private BoundExpression BindArgListOperator(InvocationExpressionSyntax node, Dia
            CheckRestrictedTypeReceiver(result, this.Compilation, diagnostics);

            return result;
+
+            void reportSuppressionIfNeeded()


reportSuppressionIfPresent? #Resolved

jaredpar · 2019-01-27T03:11:01Z

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

@@ -2041,7 +2041,17 @@ private BoundExpression BindIncrementOperator(CSharpSyntaxNode node, ExpressionS
        private BoundExpression BindSuppressNullableWarningExpression(PostfixUnaryExpressionSyntax node, DiagnosticBag diagnostics)
        {
            var expr = BindExpression(node.Operand, diagnostics);
-            return new BoundSuppressNullableWarningExpression(node, expr, expr.Type);
+            switch (expr.Kind)


Why not use IsLegalSuppressionValueKind here? Isn't that the same logic? #Resolved

They are different logic.

Maybe IsLegalSuppressionValueKind is not the best name, but it checks which ValueKinds can be used to access a suppression expression.
For instance, ValueKind.IncrementDecrement is disallowed. That means we're blocking some cases where suppression could appear (x!++).

On the other hand, here, we're blocking what could appear underneath a suppression. #Resolved

jaredpar · 2019-01-27T03:17:49Z

            Diagnostic(ErrorCode.ERR_IllegalSuppression, "y").WithLocation(15, 20)

Why is this context illegal? I can imagine that ref returns will also hit nullable type mismatches #Resolved

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

jaredpar · 2019-01-27T03:23:08Z

Done with review pass (iteration 3). Really like this approach. I think it's much cleaner. #Resolved

jcouv · 2019-01-28T04:33:17Z

Why is this context illegal? (return ref y!;) I can imagine that ref returns will also hit nullable type mismatches

I have a follow-up issue to refine the list of ValueKinds that can be used to access a suppressed value.
I agree the ref return should be allowed. I just didn't want to bloat this PR (especially in its initial form).

        private static bool IsLegalSuppressionValueKind(BindValueKind valueKind)
        {
            // Need to review allowed uses of the suppression operator
            // Tracked by https://github.com/dotnet/roslyn/issues/31297
``` #Resolved

jaredpar

cston · 2019-01-28T17:03:23Z

src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs

+                }
+                else
+                {
+                    Debug.Assert((_attributes & BoundNodeAttributes.IsSuppressed) == 0, "flag should not be reset");


Debug.Assert((_attributes & BoundNodeAttributes.IsSuppressed) == 0, "flag should not be reset"); [](start = 20, length = 96)

Should we assert this always (before the if), to ensure the bit is not set twice? #Closed

cston · 2019-01-28T17:21:13Z

src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs

+        {
+            if (expression.IsSuppressed)
+            {
+                Error(ErrorCode.ERR_ExpressionTreeCantContainSuppressNullableWarning, expression);


ERR_ExpressionTreeCantContainSuppressNullableWarning [](start = 32, length = 52)

Why disallow ! in an expression tree?

Won't we report a nullability warning for => x?

static void F(object? x) { Expression<Func<object>> e = () => x; } ``` #Closed

When suppressions were represented as a bound node, I think it made more sense to disallow. Now I could go either way, but I'm still leaning on disallowing since it serves no purpose. That reserves our options in the future.

Why do you think we should allow?

Update: Chuck convinced me offline :-)

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

cston · 2019-01-28T17:52:20Z

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

@@ -5317,7 +5314,11 @@ private void CheckPossibleNullReceiver(BoundExpression receiverOpt, bool checkNu
                    {
                        return;
                    }
-                    ReportSafetyDiagnostic(isValueType ? ErrorCode.WRN_NullableValueTypeMayBeNull : ErrorCode.WRN_NullReferenceReceiver, syntaxOpt ?? receiverOpt.Syntax);
+
+                    if (!receiverOpt.IsSuppressed)


if (!receiverOpt.IsSuppressed) [](start = 20, length = 30)

Is this check necessary? Won't _resultType have been set appropriately by the caller? #Closed

Thanks. Indeed. I think I added this before I fixed the result type from VisitExpressionWithoutStackGuard and should have revisited this afterwards.

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

cston · 2019-01-28T17:55:12Z

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

@@ -238,6 +238,7 @@ protected ConstantValue MakeDefaultExpression(DiagnosticBag diagnostics, Binder
                parameterType.NullableAnnotation.IsAnyNotNullable() &&
                convertedExpression.ConstantValue?.IsNull == true &&
                !suppressNullableWarning(convertedExpression) &&
+                !convertedExpression.IsSuppressed &&


convertedExpression.IsSuppressed [](start = 17, length = 32)

Should this be handled in suppressNullableWarning()? #Closed

gafter · 2019-01-28T18:22:08Z

src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs

+                return (_attributes & BoundNodeAttributes.IsSuppressed) != 0;
+            }
+            internal set
+            {


The existence of this internal setter is inherently dangerous. Can you make it protected? Or better yet remove it and instead add a protected method whose purpose is to set suppressed to true. #Resolved

gafter · 2019-01-28T18:30:48Z

internal sealed partial class BoundQueryClause : BoundExpression

BoundQueryClause has three fields declared in BoundQueryClause.cs. Does that need to be handled? #Resolved

Refers to: src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs:8064 in eaa8738. [](commit_id = eaa8738, deletion_comment = False)

gafter · 2019-01-28T18:32:55Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

@@ -69,6 +69,13 @@ public BoundLambda(SyntaxNode syntax, UnboundLambda unboundLambda, BoundBlock bo
            );
        }

+        public BoundLambda WithSuppression()


WithSuppression [](start = 27, length = 15)

This method does not override anything virtual. Where is it called?

I recommend making WithSuppression a virtual method in BoundExpression (rather than an extension method) so that it can be overridden.

Structured that way, you might consider renaming WithSuppressionCore to ShallowClone and move the setting of the suppression bit to the WithSuppression method. #Resolved

gafter · 2019-01-28T18:39:25Z

src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs

            {
-                Error(ErrorCode.ERR_ExpressionTreeCantContainRefStruct, node, type.Name);
+                ReportSuppressionIfNeeded(expression);
+


I don't see a problem with suppression in expression trees. Like parens, they would be applied at compile-time only. #Resolved

gafter · 2019-01-28T18:40:54Z

src/Compilers/CSharp/Portable/BoundTree/BoundExpressionExtensions.cs

+        {
+            return (T)node.WithSuppressionCore();
+        }
+


Does any caller need this to be generic? #Resolved

cston · 2019-01-29T19:09:33Z

src/Tools/Source/CompilerGeneratorTools/Source/BoundTreeGenerator/BoundNodeClassWriter.cs

@@ -1594,6 +1645,11 @@ private static bool SkipInVisitor(Field f)
            return f.SkipInVisitor != null && string.Compare(f.SkipInVisitor, "true", true) == 0;
        }

+        private static bool SkipShallowClone(Node n)
+        {
+            return n.SkipShallowClone != null && string.Compare(n.SkipShallowClone, "true", true) == 0;


The n.SkipShallowClone != null check seems unnecessary. (Same comment for similar existing methods above.) #Closed

cston · 2019-01-29T19:55:47Z

Consider testing nameof(a!) as well. #Resolved

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

gafter · 2019-01-29T20:45:58Z

src/Tools/Source/CompilerGeneratorTools/Source/BoundTreeGenerator/BoundNodeClassWriter.cs

@@ -885,11 +885,49 @@ private void WriteType(TreeType node)
            {
                WriteAccept(node.Name);
                WriteUpdateMethod(node as Node);
+                WriteWithSuppressionMethodIfNeeded(node as Node);


WriteWithSuppressionMethodIfNeeded [](start = 16, length = 34)

Should be WriteShallowCloneMethodIfNeeded #Resolved

jcouv · 2019-01-29T20:59:43Z

Consider testing nameof(a!) as well.

That's covered in SuppressNullableWarning_MemberAccess and SymbolInfoForMethodGroup03 #Resolved

cston · 2019-01-29T21:30:49Z

            Assert.True(expectedType == actualType, $"Unexpected TypeInfo.Type '{actualType}'");

Consider using Assert.Equal(expectedType, actualType) here and below since Assert.Equal() reports both expected and actual values. #WontFix

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

cston · 2019-01-29T21:44:26Z

}";

Consider testing:

static void Main()
{
    _ = Main()!;
}
``` #Closed

---
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:1476 in 5bc7a16. [](commit_id = 5bc7a161b6fc688e8b728d306f9afaa45e838674, deletion_comment = False)

cston · 2019-01-29T21:45:07Z

        CreateCompilation(text).VerifyDiagnostics(

Please include comments. #Closed

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

cston · 2019-01-29T21:46:42Z

}

Consider reducing down to:

static void Main()
{
    var x = (() => { return 1; } !) ?.ToString();
} #Closed

---
Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:1537 in 5bc7a16. [](commit_id = 5bc7a161b6fc688e8b728d306f9afaa45e838674, deletion_comment = False)

cston · 2019-01-29T21:49:03Z

/*<bind>*/{

Not used. #Closed

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

cston · 2019-01-29T21:59:38Z

src/Compilers/Test/Utilities/CSharp/CompilationTestUtils.cs

-                        (type.IsNull ? "<null>" : type.ToDisplayString(TypeSymbolWithAnnotations.TestDisplayFormat)) :
-                        null;
+                    // We don't support VerifyTypes on suppressions at the moment
+                    Debug.Assert(syntaxOpt.Kind() != SyntaxKind.SuppressNullableWarningExpression);


Assert.NotEqual(syntaxOpt.Kind(), SyntaxKind.SuppressNullableWarningExpression); #Closed

This was intentional. It's documenting the contract of the local function, rather than a test assertion. But I don't mind either way...

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

jcouv · 2019-01-29T22:16:41Z

            Assert.True(expectedType == actualType, $"Unexpected TypeInfo.Type '{actualType}'");

This is on purpose because I found the test output more useful this way. I don't like to click on the stack trace to figure out if the problem is with Type or ConvertedType.
Unfortunately, Assert.Equal doesn't allow for a message...

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

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

cston · 2019-01-29T22:29:41Z

src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs

@@ -100,6 +100,20 @@ public SyntaxTree SyntaxTree
            }
        }

+        protected void CopyAttributes(BoundNode original)
+        {
+            if (this.WasCompilerGenerated)


Shouild this be if (original.WasCompilerGenerated)? #Closed

D'uh. Sorry :-(

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

cston · 2019-01-29T22:34:01Z

src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs

+            if (original.IsSuppressed)
+            {
+                this.IsSuppressed = original.IsSuppressed;
+            }


I realize each attribute checks the value is not already set, but ignoring that, it seems strange that the contract of this method is to only set the attributes if the original attributes were set.

Consider setting the attributes unconditionally:

this.WasCompilerGenerated = original.WasCompilerGenerated; this.IsSuppressed = original.IsSuppressed; ``` #Resolved

What about VB?
I tried to align the two implementations since it is possible.

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

The two implementations will be distinct since VB uses SetWasCompilerGenerated() that does not allow setting False.

In reply to: 252049465 [](ancestors = 252049465,252046440)

The C# WasCompilerGenerated setter also does not allow setting to false

In reply to: 252051070 [](ancestors = 252051070,252049465,252046440)

jcouv and others added 2 commits January 26, 2019 15:06

Handle special cases with suppression operator

155c367

Revert most code changes

6c222d9

jcouv added Area-Compilers New Language Feature - Nullable Reference Types Nullable Reference Types labels Jan 27, 2019

jcouv added this to the 16.1.P1 milestone Jan 27, 2019

jcouv self-assigned this Jan 27, 2019

jcouv requested review from a team as code owners January 27, 2019 02:37

Add BoundExpression.IsSuppressed

edd850f

jcouv force-pushed the lambda-bang branch from 9fb87b6 to edd850f Compare January 27, 2019 02:58

jaredpar reviewed Jan 27, 2019

View reviewed changes

Address feedback from Jared

eaa8738

jaredpar approved these changes Jan 28, 2019

View reviewed changes

cston reviewed Jan 28, 2019

View reviewed changes

gafter reviewed Jan 28, 2019

View reviewed changes

cston reviewed Jan 29, 2019

View reviewed changes

gafter reviewed Jan 29, 2019

View reviewed changes

cston reviewed Jan 29, 2019

View reviewed changes

Use CopyAttributes

1b270a5

cston reviewed Jan 29, 2019

View reviewed changes

Fix CopyAttributes

489ed4b

cston reviewed Jan 29, 2019

View reviewed changes

Remove unnecessary null check

e9121d9

jcouv force-pushed the lambda-bang branch from 41418be to e9121d9 Compare January 29, 2019 22:52

Tweak

908f7e4

cston approved these changes Jan 29, 2019

View reviewed changes

jcouv merged commit d89223c into dotnet:master Jan 30, 2019

jcouv deleted the lambda-bang branch January 30, 2019 01:35

jcouv modified the milestones: 16.1.P1, 16.0.P4 Jan 30, 2019

xoofx pushed a commit to stark-lang/stark-roslyn that referenced this pull request Apr 16, 2019

Use flag to represent suppression in bound tree (dotnet#32850)

ced028e

Use flag to represent suppression in bound tree #32850

Use flag to represent suppression in bound tree #32850

Conversation

jcouv commented Jan 27, 2019 • edited Loading

jaredpar Jan 27, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

jaredpar Jan 27, 2019 • edited Loading

Choose a reason for hiding this comment

jaredpar Jan 27, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

jaredpar commented Jan 27, 2019 • edited by jcouv Loading

jaredpar commented Jan 27, 2019 • edited by jcouv Loading

jcouv commented Jan 28, 2019 • edited Loading

jaredpar left a comment

Choose a reason for hiding this comment

cston Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

cston Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

cston Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 28, 2019

Choose a reason for hiding this comment

cston Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

gafter Jan 28, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter commented Jan 28, 2019 • edited by jcouv Loading

gafter Jan 28, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter Jan 28, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter Jan 28, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

cston commented Jan 29, 2019 • edited by jcouv Loading

gafter Jan 29, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Jan 29, 2019 • edited Loading

cston commented Jan 29, 2019 • edited by jcouv Loading

cston commented Jan 29, 2019 • edited Loading

cston commented Jan 29, 2019 • edited Loading

cston commented Jan 29, 2019 • edited Loading

cston commented Jan 29, 2019 • edited Loading

cston Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 29, 2019

Choose a reason for hiding this comment

jcouv commented Jan 29, 2019 • edited Loading

cston Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 29, 2019

Choose a reason for hiding this comment

cston Jan 29, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jan 29, 2019

Choose a reason for hiding this comment

cston Jan 29, 2019

Choose a reason for hiding this comment

jcouv Jan 29, 2019

Choose a reason for hiding this comment

jcouv commented Jan 27, 2019 •

edited

Loading

jaredpar Jan 27, 2019 •

edited by jcouv

Loading

jcouv Jan 28, 2019 •

edited

Loading

jaredpar Jan 27, 2019 •

edited

Loading

jaredpar Jan 27, 2019 •

edited by jcouv

Loading

jcouv Jan 28, 2019 •

edited

Loading

jaredpar commented Jan 27, 2019 •

edited by jcouv

Loading

jaredpar commented Jan 27, 2019 •

edited by jcouv

Loading

jcouv commented Jan 28, 2019 •

edited

Loading

cston Jan 28, 2019 •

edited

Loading

cston Jan 28, 2019 •

edited

Loading

jcouv Jan 28, 2019 •

edited

Loading

cston Jan 28, 2019 •

edited

Loading

cston Jan 28, 2019 •

edited

Loading

gafter Jan 28, 2019 •

edited by jcouv

Loading

gafter commented Jan 28, 2019 •

edited by jcouv

Loading

gafter Jan 28, 2019 •

edited by jcouv

Loading

gafter Jan 28, 2019 •

edited by jcouv

Loading

gafter Jan 28, 2019 •

edited by jcouv

Loading

cston Jan 29, 2019 •

edited

Loading

cston commented Jan 29, 2019 •

edited by jcouv

Loading

gafter Jan 29, 2019 •

edited by jcouv

Loading

jcouv commented Jan 29, 2019 •

edited

Loading

cston commented Jan 29, 2019 •

edited by jcouv

Loading

cston commented Jan 29, 2019 •

edited

Loading

cston commented Jan 29, 2019 •

edited

Loading

cston commented Jan 29, 2019 •

edited

Loading

cston commented Jan 29, 2019 •

edited

Loading

cston Jan 29, 2019 •

edited

Loading

jcouv commented Jan 29, 2019 •

edited

Loading

cston Jan 29, 2019 •

edited

Loading

cston Jan 29, 2019 •

edited by jcouv

Loading