Normalization without intermediate binary patterns for sub-patterns

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

@@ -13,7 +13,7 @@ might be raised because a new overload is applicable but there is no single best
 The following example shows some ambiguities and possible workarounds.
 Note that another workaround is for API authors to use the `OverloadResolutionPriorityAttribute`.
 
-```cs
+```csharp
 var x = new long[] { 1 };
 Assert.Equal([2], x); // previously Assert.Equal<T>(T[], T[]), now ambiguous with Assert.Equal<T>(ReadOnlySpan<T>, Span<T>)
 Assert.Equal([2], x.AsSpan()); // workaround
@@ -27,7 +27,7 @@ Assert.Equal(y.AsSpan(), s); // workaround
 A `Span<T>` overload might be chosen in C# 14 where an overload taking an interface implemented by `T[]` (such as `IEnumerable<T>`) was chosen in C# 13,
 and that can lead to an `ArrayTypeMismatchException` at runtime if used with a covariant array:
 
-```cs
+```csharp
 string[] s = new[] { "a" };
 object[] o = s; // array variance
 
@@ -48,7 +48,7 @@ When using C# 14 or newer and targeting a .NET older than `net10.0`
 or .NET Framework with `System.Memory` reference,
 there is a breaking change with `Enumerable.Reverse` and arrays:
 
-```cs
+```csharp
 int[] x = new[] { 1, 2, 3 };
 var y = x.Reverse(); // previously Enumerable.Reverse, now MemoryExtensions.Reverse
 ```
@@ -59,7 +59,7 @@ than `Enumerable.Reverse(this IEnumerable<T>)` (which used to be resolved in C#
 Specifically, the `Span` extension does the reversal in place and returns `void`.
 As a workaround, one can define their own `Enumerable.Reverse(this T[])` or use `Enumerable.Reverse` explicitly:
 
-```cs
+```csharp
 int[] x = new[] { 1, 2, 3 };
 var y = Enumerable.Reverse(x); // instead of 'x.Reverse();'
 ```
@@ -118,3 +118,21 @@ class C
 }
 ```
 
+
+## Warn for redundant pattern in simple `or` patterns
+
+***Introduced in Visual Studio 2022 version 17.13***
+
+TODO2 update description
+
+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 provides a warning in common cases of this mistake:
+
+```csharp
+_ = o is not null or 42; // warning: pattern "42" is redundant
+_ = o is not 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/Binder_Patterns.cs

@@ -58,13 +58,15 @@ private BoundExpression MakeIsPatternExpression(
             BoundDecisionDag decisionDag = DecisionDagBuilder.CreateDecisionDagForIsPattern(
                 this.Compilation, pattern.Syntax, expression, innerPattern, whenTrueLabel: whenTrueLabel, whenFalseLabel: whenFalseLabel, diagnostics);
 
+            bool wasReported = false;
             if (!hasErrors && getConstantResult(decisionDag, negated, whenTrueLabel, whenFalseLabel) is { } constantResult)
             {
                 if (!constantResult)
                 {
                     Debug.Assert(expression.Type is object);
                     diagnostics.Add(ErrorCode.ERR_IsPatternImpossible, node.Location, expression.Type);
                     hasErrors = true;
+                    wasReported = true;
                 }
                 else
                 {
@@ -81,6 +83,7 @@ private BoundExpression MakeIsPatternExpression(
                         case BoundListPattern:
                             Debug.Assert(expression.Type is object);
                             diagnostics.Add(ErrorCode.WRN_IsPatternAlways, node.Location, expression.Type);
+                            wasReported = true;
                             break;
                         case BoundDiscardPattern _:
                             // we do not give a warning on this because it is an existing scenario, and it should
@@ -101,26 +104,34 @@ private BoundExpression MakeIsPatternExpression(
                     if (!simplifiedResult)
                     {
                         diagnostics.Add(ErrorCode.WRN_GivenExpressionNeverMatchesPattern, node.Location);
+                        wasReported = true;
                     }
                     else
                     {
                         switch (pattern)
                         {
                             case BoundConstantPattern _:
                                 diagnostics.Add(ErrorCode.WRN_GivenExpressionAlwaysMatchesConstant, node.Location);
+                                wasReported = true;
                                 break;
                             case BoundRelationalPattern _:
                             case BoundTypePattern _:
                             case BoundNegatedPattern _:
                             case BoundBinaryPattern _:
                             case BoundDiscardPattern _:
                                 diagnostics.Add(ErrorCode.WRN_GivenExpressionAlwaysMatchesPattern, node.Location);
+                                wasReported = true;
                                 break;
                         }
                     }
                 }
             }
 
+            if (!wasReported && diagnostics.AccumulatesDiagnostics)
+            {
+                DecisionDagBuilder.CheckRedundantPatternsForIsPattern(this.Compilation, pattern.Syntax, expression, pattern, diagnostics);
+            }
+
             // decisionDag, whenTrueLabel, and whenFalseLabel represent the decision DAG for the inner pattern,
             // after removing any outer 'not's, so consumers will need to compensate for negated patterns.
             return new BoundIsPatternExpression(
@@ -1796,58 +1807,11 @@ static BoundPattern bindBinaryPattern(
 
                     TypeSymbol? leastSpecificType(SyntaxNode node, ArrayBuilder<TypeSymbol> candidates, BindingDiagnosticBag diagnostics)
                     {
-                        Debug.Assert(candidates.Count >= 2);
                         CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = binder.GetNewCompoundUseSiteInfo(diagnostics);
-                        TypeSymbol? bestSoFar = candidates[0];
-                        // first pass: select a candidate for which no other has been shown to be an improvement.
-                        for (int i = 1, n = candidates.Count; i < n; i++)
-                        {
-                            TypeSymbol candidate = candidates[i];
-                            bestSoFar = lessSpecificCandidate(bestSoFar, candidate, ref useSiteInfo) ?? bestSoFar;
-                        }
-                        // second pass: check that it is no more specific than any candidate.
-                        for (int i = 0, n = candidates.Count; i < n; i++)
-                        {
-                            TypeSymbol candidate = candidates[i];
-                            TypeSymbol? spoiler = lessSpecificCandidate(candidate, bestSoFar, ref useSiteInfo);
-                            if (spoiler is null)
-                            {
-                                bestSoFar = null;
-                                break;
-                            }
-
-                            // Our specificity criteria are transitive
-                            Debug.Assert(spoiler.Equals(bestSoFar, TypeCompareKind.ConsiderEverything));
-                        }
-
+                        TypeSymbol? bestSoFar = LeastSpecificType(binder.Conversions, candidates, ref useSiteInfo);
                         diagnostics.Add(node, useSiteInfo);
                         return bestSoFar;
                     }
-
-                    // Given a candidate least specific type so far, attempt to refine it with a possibly less specific candidate.
-                    TypeSymbol? lessSpecificCandidate(TypeSymbol bestSoFar, TypeSymbol possiblyLessSpecificCandidate, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
-                    {
-                        if (bestSoFar.Equals(possiblyLessSpecificCandidate, TypeCompareKind.AllIgnoreOptions))
-                        {
-                            // When the types are equivalent, merge them.
-                            return bestSoFar.MergeEquivalentTypes(possiblyLessSpecificCandidate, VarianceKind.Out);
-                        }
-                        else if (binder.Conversions.HasImplicitReferenceConversion(bestSoFar, possiblyLessSpecificCandidate, ref useSiteInfo))
-                        {
-                            // When there is an implicit reference conversion from T to U, U is less specific
-                            return possiblyLessSpecificCandidate;
-                        }
-                        else if (binder.Conversions.HasBoxingConversion(bestSoFar, possiblyLessSpecificCandidate, ref useSiteInfo))
-                        {
-                            // when there is a boxing conversion from T to U, U is less specific.
-                            return possiblyLessSpecificCandidate;
-                        }
-                        else
-                        {
-                            // We have no improved candidate to offer.
-                            return null;
-                        }
-                    }
                 }
                 else
                 {
@@ -1872,7 +1836,58 @@ static void collectCandidates(BoundPattern pat, ArrayBuilder<TypeSymbol> candida
                     candidates.Add(pat.NarrowedType);
                 }
             }
+        }
+
+        internal static TypeSymbol? LeastSpecificType(Conversions conversions, ArrayBuilder<TypeSymbol> candidates, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
+        {
+            Debug.Assert(candidates.Count >= 2);
+            TypeSymbol? bestSoFar = candidates[0];
+            // first pass: select a candidate for which no other has been shown to be an improvement.
+            for (int i = 1, n = candidates.Count; i < n; i++)
+            {
+                TypeSymbol candidate = candidates[i];
+                bestSoFar = lessSpecificCandidate(bestSoFar, candidate, ref useSiteInfo, conversions) ?? bestSoFar;
+            }
+            // second pass: check that it is no more specific than any candidate.
+            for (int i = 0, n = candidates.Count; i < n; i++)
+            {
+                TypeSymbol candidate = candidates[i];
+                TypeSymbol? spoiler = lessSpecificCandidate(candidate, bestSoFar, ref useSiteInfo, conversions);
+                if (spoiler is null)
+                {
+                    bestSoFar = null;
+                    break;
+                }
+
+                // Our specificity criteria are transitive
+                Debug.Assert(spoiler.Equals(bestSoFar, TypeCompareKind.ConsiderEverything));
+            }
+
+            return bestSoFar;
 
+            static TypeSymbol? lessSpecificCandidate(TypeSymbol bestSoFar, TypeSymbol possiblyLessSpecificCandidate, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo, Conversions conversions)
+            {
+                if (bestSoFar.Equals(possiblyLessSpecificCandidate, TypeCompareKind.AllIgnoreOptions))
+                {
+                    // When the types are equivalent, merge them.
+                    return bestSoFar.MergeEquivalentTypes(possiblyLessSpecificCandidate, VarianceKind.Out);
+                }
+                else if (conversions.HasImplicitReferenceConversion(bestSoFar, possiblyLessSpecificCandidate, ref useSiteInfo))
+                {
+                    // When there is an implicit reference conversion from T to U, U is less specific
+                    return possiblyLessSpecificCandidate;
+                }
+                else if (conversions.HasBoxingConversion(bestSoFar, possiblyLessSpecificCandidate, ref useSiteInfo))
+                {
+                    // when there is a boxing conversion from T to U, U is less specific.
+                    return possiblyLessSpecificCandidate;
+                }
+                else
+                {
+                    // We have no improved candidate to offer.
+                    return null;
+                }
+            }
         }
     }
 }