Add BoundExpression.IsSuppressed

docs/features/nullable-reference-types.md

@@ -158,7 +158,6 @@ A warning is reported when using the `!` operator absent a `NonNullTypes` contex
 Unnecessary usages of `!` do not produce any diagnostics, including `!!`.
 
 A suppressed expression `e!` can be target-typed if the operand expression `e` can be target-typed.
-A suppressed expression `e!` contributes to method type inference just how `e` does.
 
 ### Explicit cast
 Explicit cast to `?` changes top-level nullability.

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

@@ -204,13 +204,6 @@ private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind
                         }
                     }
                     break;
-
-                case BoundKind.SuppressNullableWarningExpression:
-                    {
-                        var outer = (BoundSuppressNullableWarningExpression)expr;
-                        var inner = CheckValue(outer.Expression, valueKind, diagnostics);
-                        return outer.Update(inner, inner.Type);
-                    }
             }
 
             bool hasResolutionErrors = false;
@@ -291,6 +284,23 @@ internal static bool IsTypeOrValueExpression(BoundExpression expression)
             }
         }
 
+        private static bool IsLegalSuppressionValueKind(BindValueKind valueKind)
+        {
+            // Need to review allowed uses of the suppression operator
+            // Tracked by https://github.com/dotnet/roslyn/issues/31297
+
+            switch (valueKind)
+            {
+                case BindValueKind.RValue:
+                case BindValueKind.RValueOrMethodGroup:
+                case BindValueKind.RefOrOut:
+                    return true;
+            }
+
+            // all others are illegal
+            return false;
+        }
+
         /// <summary>
         /// The purpose of this method is to determine if the expression satisfies desired capabilities. 
         /// If it is not then this code gives an appropriate error message.
@@ -313,6 +323,12 @@ internal bool CheckValueKind(SyntaxNode node, BoundExpression expr, BindValueKin
                 return false;
             }
 
+            if (expr.IsSuppressed && !IsLegalSuppressionValueKind(valueKind))
+            {
+                Error(diagnostics, ErrorCode.ERR_IllegalSuppression, node);
+                return false;
+            }
+
             switch (expr.Kind)
             {
                 // we need to handle properties and event in a special way even in an RValue case because of getters
@@ -322,11 +338,6 @@ internal bool CheckValueKind(SyntaxNode node, BoundExpression expr, BindValueKin
 
                 case BoundKind.EventAccess:
                     return CheckEventValueKind((BoundEventAccess)expr, valueKind, diagnostics);
-
-                case BoundKind.SuppressNullableWarningExpression:
-                    // https://github.com/dotnet/roslyn/issues/29710 We can reach this assertion
-                    Debug.Assert(false);
-                    return CheckValueKind(node, ((BoundSuppressNullableWarningExpression)expr).Expression, valueKind, checkingReceiver, diagnostics);
             }
 
             // easy out for a very common RValue case.

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

@@ -300,6 +300,10 @@ private static BoundExpression CreateAnonymousFunctionConversion(SyntaxNode synt
 
             var unboundLambda = (UnboundLambda)source;
             var boundLambda = unboundLambda.Bind((NamedTypeSymbol)destination);
+            if (unboundLambda.IsSuppressed)
+            {
+                boundLambda = boundLambda.WithSuppression();
+            }
             diagnostics.AddRange(boundLambda.Diagnostics);
 
             return new BoundConversion(

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

@@ -238,10 +238,12 @@ private BoundExpression BindInvocationExpression(
                 // Either we have a dynamic method group invocation "dyn.M(...)" or 
                 // a dynamic delegate invocation "dyn(...)" -- either way, bind it as a dynamic
                 // invocation and let the lowering pass sort it out.
+                reportSuppressionIfNeeded();
                 result = BindDynamicInvocation(node, boundExpression, analyzedArguments, ImmutableArray<MethodSymbol>.Empty, diagnostics, queryClause);
             }
             else if (boundExpression.Kind == BoundKind.MethodGroup)
             {
+                reportSuppressionIfNeeded();
                 result = BindMethodGroupInvocation(
                     node, expression, methodName, (BoundMethodGroup)boundExpression, analyzedArguments,
                     diagnostics, queryClause, allowUnexpandedForm: allowUnexpandedForm, anyApplicableCandidates: out _);
@@ -268,6 +270,14 @@ private BoundExpression BindInvocationExpression(
             CheckRestrictedTypeReceiver(result, this.Compilation, diagnostics);
 
             return result;
+
+            void reportSuppressionIfNeeded()
+            {
+                if (boundExpression.IsSuppressed)
+                {
+                    Error(diagnostics, ErrorCode.ERR_IllegalSuppression, boundExpression.Syntax);
+                }
+            }
         }
 
         private BoundExpression BindDynamicInvocation(

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)
+            {
+                case BoundKind.NamespaceExpression:
+                case BoundKind.TypeExpression:
+                    Error(diagnostics, ErrorCode.ERR_IllegalSuppression, expr.Syntax);
+                    break;
+                default:
+                    break;
+            }
+
+            return expr.WithSuppression();
         }
 
         // Based on ExpressionBinder::bindPtrIndirection.

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

@@ -1994,10 +1994,6 @@ protected void GenerateImplicitConversionError(
             {
                 return;
             }
-            while (operand.Kind == BoundKind.SuppressNullableWarningExpression)
-            {
-                operand = ((BoundSuppressNullableWarningExpression)operand).Expression;
-            }
 
             switch (operand.Kind)
             {
@@ -2934,6 +2930,11 @@ private static bool IsValidStatementExpression(SyntaxNode syntax, BoundExpressio
                 return false;
             }
 
+            if (expression.IsSuppressed)
+            {
+                return false;
+            }
+
             // It is possible that an expression is syntactically valid but semantic analysis
             // reveals it to be illegal in a statement expression: "new MyDelegate(M)" for example
             // is not legal because it is a delegate-creation-expression and not an

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

@@ -834,17 +834,6 @@ private Conversion ClassifyImplicitBuiltInConversionFromExpression(BoundExpressi
                     }
                     break;
 
-                case BoundKind.SuppressNullableWarningExpression:
-                    {
-                        var innerExpression = ((BoundSuppressNullableWarningExpression)sourceExpression).Expression;
-                        var innerConversion = ClassifyImplicitBuiltInConversionFromExpression(innerExpression, innerExpression.Type, destination, ref useSiteDiagnostics);
-                        if (innerConversion.Exists)
-                        {
-                            return innerConversion;
-                        }
-                        break;
-                    }
-
                 case BoundKind.ExpressionWithNullability:
                     {
                         var innerExpression = ((BoundExpressionWithNullability)sourceExpression).Expression;

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

@@ -3,15 +3,15 @@
 using System.Collections.Immutable;
 using System.Diagnostics;
 using Microsoft.CodeAnalysis.CSharp.Symbols;
-using Microsoft.CodeAnalysis.CSharp.Syntax;
-using Microsoft.CodeAnalysis.Text;
 using Roslyn.Utilities;
 using System;
 
 namespace Microsoft.CodeAnalysis.CSharp
 {
     internal partial class BoundExpression
     {
+        public abstract BoundExpression WithSuppressionCore();
+
         public virtual ConstantValue ConstantValue
         {
             get
@@ -356,14 +356,6 @@ public override Symbol ExpressionSymbol
         public ImmutableArray<MethodSymbol> OriginalUserDefinedOperatorsOpt { get; }
     }
 
-    internal partial class BoundSuppressNullableWarningExpression
-    {
-        public override ConstantValue ConstantValue
-        {
-            get { return this.Expression.ConstantValue; }
-        }
-    }
-
     internal partial class BoundLiteral
     {
         public override ConstantValue ConstantValue

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

@@ -13,6 +13,11 @@ namespace Microsoft.CodeAnalysis.CSharp
 {
     internal static partial class BoundExpressionExtensions
     {
+        public static T WithSuppression<T>(this T node) where T : BoundExpression
+        {
+            return (T)node.WithSuppressionCore();
+        }
+
         /// <summary>
         /// Returns the RefKind if the expression represents a symbol
         /// that has a RefKind, or RefKind.None otherwise.
@@ -234,8 +239,6 @@ private static NullableAnnotation GetNullableAnnotation(this BoundExpression exp
         {
             switch (expr.Kind)
             {
-                case BoundKind.SuppressNullableWarningExpression:
-                    return NullableAnnotation.Unknown;
                 case BoundKind.Local:
                     {
                         var local = (BoundLocal)expr;

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

@@ -29,6 +29,7 @@ private enum BoundNodeAttributes : byte
             /// </summary>
             WasCompilerGeneratedIsChecked = 1 << 2,
 #endif
+            IsSuppressed = 1 << 4,
         }
 
         protected BoundNode(BoundKind kind, SyntaxNode syntax)
@@ -149,6 +150,25 @@ public void ResetCompilerGenerated(bool newCompilerGenerated)
             }
         }
 
+        public bool IsSuppressed
+        {
+            get
+            {
+                return (_attributes & BoundNodeAttributes.IsSuppressed) != 0;
+            }
+            internal set
+            {
+
+                if (value)
+                {
+                    _attributes |= BoundNodeAttributes.IsSuppressed;
+                }
+                else
+                {
+                    Debug.Assert((_attributes & BoundNodeAttributes.IsSuppressed) == 0, "flag should not be reset");
+                }
+            }
+        }
 
         public BoundKind Kind
         {

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -272,11 +272,6 @@
     <Field Name="ResultKind" PropertyOverrides="true" Type="LookupResultKind"/>
   </Node>
 
-  <Node Name="BoundSuppressNullableWarningExpression" Base="BoundExpression">
-    <Field Name="Type" Type="TypeSymbol" Override="true" Null="allow"/>
-    <Field Name="Expression" Type="BoundExpression"/>
-  </Node>
-
   <Node Name="BoundIncrementOperator" Base="BoundExpression">
     <!-- x++ might need a conversion from the type of x to the operand type
          of the ++ operator (that produces the incremented value) and a 

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

@@ -38,11 +38,6 @@ public override object Display
         }
     }
 
-    internal sealed partial class BoundSuppressNullableWarningExpression
-    {
-        public override object Display => Expression.Display;
-    }
-
     internal sealed partial class BoundLambda
     {
         public override object Display

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

@@ -545,10 +545,6 @@ protected void VisitLvalue(BoundExpression node)
                     ((BoundTupleExpression)node).VisitAllElements((x, self) => self.VisitLvalue(x), this);
                     break;
 
-                case BoundKind.SuppressNullableWarningExpression:
-                    VisitLvalue(((BoundSuppressNullableWarningExpression)node).Expression);
-                    break;
-
                 default:
                     VisitRvalue(node);
                     break;
@@ -2631,12 +2627,6 @@ public override BoundNode VisitNameOfOperator(BoundNameOfOperator node)
             return null;
         }
 
-        public override BoundNode VisitSuppressNullableWarningExpression(BoundSuppressNullableWarningExpression node)
-        {
-            VisitRvalue(node.Expression);
-            return null;
-        }
-
         public override BoundNode VisitAddressOfOperator(BoundAddressOfOperator node)
         {
             VisitAddressOfOperand(node.Operand, shouldReadOperand: false);

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

@@ -1216,11 +1216,6 @@ protected virtual void AssignImpl(BoundNode node, BoundExpression value, bool is
                     ((BoundTupleExpression)node).VisitAllElements((x, self) => self.Assign(x, value: null, isRef: isRef), this);
                     break;
 
-                case BoundKind.SuppressNullableWarningExpression:
-                    // for example, assigning to `x!` in `M(out x!)` assigns to `x`
-                    AssignImpl(((BoundSuppressNullableWarningExpression)node).Expression, value, isRef, written, read);
-                    break;
-
                 default:
                     // Other kinds of left-hand-sides either represent things not tracked (e.g. array elements)
                     // or errors that have been reported earlier (e.g. assignment to a unary increment)