Report not ... or <redundant> patterns

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md

@@ -87,3 +87,18 @@ class C
     }
 }
 ```
+
+## Warn for redundant pattern in `not ... or <redundant>` pattern
+
+***Introduced in Visual Studio 2022 version 17.13***
+
+In a disjunctive `or` pattern such as `is not null or 42` or `is not int or string`
+the second pattern is redundant and likely results from misunderstanding the precedence order
+of `not` and `or` pattern combinators.  
+The compiler will provide a warning in such cases:
+```
+_ = o is not null or 42; // warning: pattern "42" is redundant
+_ = o is int or string; // warning: pattern "string" is redundant
+```
+It is likely that the user meant `is not (null or 42)` or `is not (int or string)` instead.
+

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

@@ -1076,7 +1076,7 @@ private void SplitCase(
             out StateForCase whenFalse,
             ref bool foundExplicitNullTest)
         {
-            stateForCase.RemainingTests.Filter(this, test, state, whenTrueValues, whenFalseValues, out Tests whenTrueTests, out Tests whenFalseTests, ref foundExplicitNullTest);
+            stateForCase.RemainingTests.Filter(this, test, state, whenTrueValues, whenFalseValues, out Tests whenTrueTests, out Tests whenFalseTests, ref foundExplicitNullTest, this._diagnostics);
             whenTrue = stateForCase.WithRemainingTests(whenTrueTests);
             whenFalse = stateForCase.WithRemainingTests(whenFalseTests);
         }
@@ -2072,7 +2072,8 @@ public abstract void Filter(
                 IValueSet? whenFalseValues,
                 out Tests whenTrue,
                 out Tests whenFalse,
-                ref bool foundExplicitNullTest);
+                ref bool foundExplicitNullTest,
+                BindingDiagnosticBag diagnostics);
             public virtual BoundDagTest ComputeSelectedTest() => throw ExceptionUtilities.Unreachable();
             public virtual Tests RemoveEvaluation(BoundDagEvaluation e) => this;
             /// <summary>
@@ -2096,7 +2097,8 @@ public override void Filter(
                     IValueSet? whenFalseValues,
                     out Tests whenTrue,
                     out Tests whenFalse,
-                    ref bool foundExplicitNullTest)
+                    ref bool foundExplicitNullTest,
+                    BindingDiagnosticBag diagnostics)
                 {
                     whenTrue = whenFalse = this;
                 }
@@ -2117,7 +2119,8 @@ public override void Filter(
                     IValueSet? whenFalseValues,
                     out Tests whenTrue,
                     out Tests whenFalse,
-                    ref bool foundExplicitNullTest)
+                    ref bool foundExplicitNullTest,
+                    BindingDiagnosticBag diagnostics)
                 {
                     whenTrue = whenFalse = this;
                 }
@@ -2141,7 +2144,8 @@ public override void Filter(
                     IValueSet? whenFalseValues,
                     out Tests whenTrue,
                     out Tests whenFalse,
-                    ref bool foundExplicitNullTest)
+                    ref bool foundExplicitNullTest,
+                    BindingDiagnosticBag diagnostics)
                 {
                     SyntaxNode syntax = test.Syntax;
                     BoundDagTest other = this.Test;
@@ -2308,9 +2312,10 @@ public override void Filter(
                     IValueSet? whenFalseValues,
                     out Tests whenTrue,
                     out Tests whenFalse,
-                    ref bool foundExplicitNullTest)
+                    ref bool foundExplicitNullTest,
+                    BindingDiagnosticBag diagnostics)
                 {
-                    Negated.Filter(builder, test, state, whenTrueValues, whenFalseValues, out var whenTestTrue, out var whenTestFalse, ref foundExplicitNullTest);
+                    Negated.Filter(builder, test, state, whenTrueValues, whenFalseValues, out var whenTestTrue, out var whenTestFalse, ref foundExplicitNullTest, diagnostics);
                     whenTrue = Not.Create(whenTestTrue);
                     whenFalse = Not.Create(whenTestFalse);
                 }
@@ -2335,20 +2340,79 @@ public sealed override void Filter(
                     IValueSet? whenFalseValues,
                     out Tests whenTrue,
                     out Tests whenFalse,
-                    ref bool foundExplicitNullTest)
+                    ref bool foundExplicitNullTest,
+                    BindingDiagnosticBag diagnostics)
                 {
                     var trueBuilder = ArrayBuilder<Tests>.GetInstance(RemainingTests.Length);
                     var falseBuilder = ArrayBuilder<Tests>.GetInstance(RemainingTests.Length);
-                    foreach (var other in RemainingTests)
+                    for (int i = 0; i < RemainingTests.Length; i++)
                     {
-                        other.Filter(builder, test, state, whenTrueValues, whenFalseValues, out Tests oneTrue, out Tests oneFalse, ref foundExplicitNullTest);
+                        var other = RemainingTests[i];
+                        other.Filter(builder, test, state, whenTrueValues, whenFalseValues, out Tests oneTrue, out Tests oneFalse, ref foundExplicitNullTest, diagnostics);
                         trueBuilder.Add(oneTrue);
                         falseBuilder.Add(oneFalse);
+
+                        if (i > 0
+                            && oneTrue is False
+                            && this is OrSequence { RemainingTests: [Not { Negated: var negated }, ..] }
+                            && isRecognizedPartOfNegated(test, negated))
+                        {
+                            // We report a warning based on heuristic to catch a common mistake (`not ... or <redundant>` pattern)
+                            diagnostics.Add(ErrorCode.WRN_RedundantPattern, getSyntax(other));
+                        }
                     }
 
                     whenTrue = Update(trueBuilder);
                     whenFalse = Update(falseBuilder);
+
+                    return;
+
+                    // Check that the given test, which causes the `or` condition to become redundant,
+                    // is indeed the test being negated by the `not`
+                    static bool isRecognizedPartOfNegated(BoundDagTest test, Tests negated)
+                    {
+                        // If we have something like `not A or ... or B` where the A test being true implies that the B test is false,
+                        // so the B test could only be true when the `not A` test is true,
+                        // then the user probably made a mistake having the `or B` condition
+                        // Also handles `not "literal" or ...`
+                        if (negated is AndSequence { RemainingTests: [One { Test: BoundDagNonNullTest nonNullTest }, One { Test: BoundDagTypeTest typeTest }, ..] }
+                            && nonNullTest.Input == typeTest.Input
+                            && test == typeTest)
+                        {
+                            return true;
+                        }
+
+                        // If we have something like `not null or ... or C`,
+                        // the C test could only be true when the `not null` test is true,
+                        // then the user probably made a mistake having the `or C` condition
+                        if (negated is One { Test: BoundDagExplicitNullTest nonNullTest2 }
+                            && test == nonNullTest2)
+                        {
+                            return true;
+                        }
+
+                        // Handle `not 42 or ...`
+                        if (negated is One { Test: BoundDagValueTest valueTest }
+                            && test == valueTest)
+                        {
+                            return true;
+                        }
+
+                        return false;
+                    }
+
+                    static SyntaxNode getSyntax(Tests tests)
+                    {
+                        return tests switch
+                        {
+                            One { Test: var oneTest } => oneTest.Syntax,
+                            Not not => getSyntax(not.Negated),
+                            SequenceTests sequence => getSyntax(sequence.RemainingTests[0]),
+                            _ => throw ExceptionUtilities.UnexpectedValue(tests),
+                        };
+                    }
                 }
+
                 public sealed override Tests RemoveEvaluation(BoundDagEvaluation e)
                 {
                     var builder = ArrayBuilder<Tests>.GetInstance(RemainingTests.Length);

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -8017,4 +8017,10 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
   <data name="ERR_IteratorRefLikeElementType" xml:space="preserve">
     <value>Element type of an iterator may not be a ref struct or a type parameter allowing ref structs</value>
   </data>
+  <data name="WRN_RedundantPattern" xml:space="preserve">
+    <value>The pattern is redundant</value>
+  </data>
+  <data name="WRN_RedundantPattern_Title" xml:space="preserve">
+    <value>The pattern is redundant</value>
+  </data>
 </root>

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -2350,6 +2350,7 @@ internal enum ErrorCode
         WRN_AccessorDoesNotUseBackingField = 9266,
 
         ERR_IteratorRefLikeElementType = 9267,
+        WRN_RedundantPattern = 9268,
 
         // Note: you will need to do the following after adding errors:
         //  1) Update ErrorFacts.IsBuildOnlyDiagnostic (src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs)

src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs

@@ -565,6 +565,7 @@ internal static int GetWarningLevel(ErrorCode code)
                 case ErrorCode.WRN_FieldIsAmbiguous:
                 case ErrorCode.WRN_UninitializedNonNullableBackingField:
                 case ErrorCode.WRN_AccessorDoesNotUseBackingField:
+                case ErrorCode.WRN_RedundantPattern:
                     return 1;
                 default:
                     return 0;
@@ -2467,6 +2468,7 @@ or ErrorCode.WRN_UninitializedNonNullableBackingField
                 or ErrorCode.WRN_UnassignedInternalRefField
                 or ErrorCode.WRN_AccessorDoesNotUseBackingField
                 or ErrorCode.ERR_IteratorRefLikeElementType
+                or ErrorCode.WRN_RedundantPattern
                     => false,
             };
 #pragma warning restore CS8524 // The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value.