Warn for redundant pattern in not ... or <redundant> pattern
#75581
Conversation
dotnet-issue-labeler
bot
added
the
untriaged
| if (i > 0 | ||
| && oneTrue is False | ||
| && this is OrSequence { RemainingTests: [Not { Negated: var negated }, ..] } | ||
| && isRecognizedPartOfNegated(test, negated)) |
There was a problem hiding this comment.
📝 Surprisingly, removing this isRecognizedPartOfNegated check doesn't seem to impact any tests or bootstrapping. #Resolved
There was a problem hiding this comment.
It feels like if we can't come up with any tests which depend on this filtering (e.g. scenarios where an unwanted warning is reported in absence of this check), then it bears further investigation. We should try to understand this area well enough to either identify such a scenario and test it, or to be confident that this check is not needed and remove it.
There was a problem hiding this comment.
I don't think it's possible. We would need a test that appears inside a condition inside Tests.Not but is not a requirement for that condition to succeed.
For example, Or(Not(Not(Test)), RedundantTest or Or(Not(Or(Test, OtherTest)), RedundantTest).
But neither of these structures are possible, as not not Test gets represented as Test and not (Test or OtherTest) gets represented as And(Not(Test), Not(OtherTest)).
I'll add more tests to illustrate.
So all the tests I've added to show the impact of this filtering logic only show that it limits useful warnings. I was not able to show the impact on false warnings.
Still, I feel safer keeping this filtering logic, in case I'm missing something or the above situation changed.
There was a problem hiding this comment.
Perhaps we could, without changing the behavior in this PR, add an assertion if the preceding logic holds, the isRecognizedPartOfNegated must also hold. So, if we ever find a situation where the implication is not met, we will at least know about it.
There was a problem hiding this comment.
Followed up offline. It seems this predicate does have effects, in terms of which cases produce warnings or not. Our concern was about does this predicate actually prevent certain subjectively annoying warnings. This can't be enforced with an assertion.
jcouv
force-pushed
the
not-or
branch
2 times, most recently
from
59d9dd7 to
c4a5bdf
Compare
| 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: | ||
| ``` |
There was a problem hiding this comment.
| ``` | |
| ```c# | |
| ```` #Resolved |
| @@ -87,3 +87,18 @@ class C | |||
| } | |||
| } | |||
| ``` | |||
|
|
|||
| ## Warn for redundant pattern in `not ... or <redundant>` pattern | |||
There was a problem hiding this comment.
This warning affects existing code and can break builds when users update VS / SDK. I don't see a reason why it shouldn't be a warning-wave warning. #Resolved
There was a problem hiding this comment.
It is thought that code which compiler reports this warning on, is likely to be so broken and against developer expectations, that we want to break people's builds on upgrade to make them think about what to do (parenthesize or delete the redundant subpattern).
There was a problem hiding this comment.
From discussion in LDM, we're okay with taking this break (ie. report a regular warning). Users seem to be making this mistake frequently and the impact can be meaningful, so we're not helping users by letting them keep this redundant pattern.
But we do limit the warning to cases that are definitely redundant (no false alarm) to minimize the break, even if we don't catch all the cases of redundancy.
There was a problem hiding this comment.
Aren't most (all?) warnings we add to the compiler are on likely-broken code or redundant things? Other warnings are added to analyzers. So I'm not sure I see the line between "break the build" and "warning wave" warnings. I would expect users that want useful warnings and don't mind broken builds would set their warning level accordingly. Anyway, it's fine, especially if LDM discussed this. Thanks.
| _ = 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. |
There was a problem hiding this comment.
Does this catch all such patterns or just most of them? As we discussed I think even catching most of them, particularly the most common case that motivated this issue, would be a huge win. But I want to make sure that the documentation is clear on this point. #Resolved
There was a problem hiding this comment.
I handled specific patterns, to be safer. But the PR handles all known patterns that were found in github searches.
I could refine to "The compiler provides a warning in common cases of this mistake"
| @@ -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> | |||
There was a problem hiding this comment.
Can we include something about "did you mean to parenthesize this"? #Resolved
|
@dotnet/roslyn-compiler for review. Thanks |
|
@dotnet/roslyn-compiler for review. I'm happy to offer a walkthrough for more context on DAG construction. Let me know |
There was a problem hiding this comment.
I don't have any fundamental problem with the approach, but I am still interested in taking a little more time on my own to investigate. In particular, the question about 'isRecognizedPartOfNegated' is concerning to me. Apologies for the long turnaround on my review here since we did speak offline about it a few weeks ago.
src/Compilers/CSharp/Test/Emit3/Semantics/PatternMatchingTests4.cs
Outdated
Show resolved
Hide resolved
| @@ -87,3 +87,18 @@ class C | |||
| } | |||
| } | |||
| ``` | |||
|
|
|||
| ## Warn for redundant pattern in `not ... or <redundant>` pattern | |||
There was a problem hiding this comment.
It is thought that code which compiler reports this warning on, is likely to be so broken and against developer expectations, that we want to break people's builds on upgrade to make them think about what to do (parenthesize or delete the redundant subpattern).
| // 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 }, ..] } |
There was a problem hiding this comment.
It feels unfortunate that we do a "normalization" of the tests at this phase, as it feels like we are already having to work backwards to infer what the user pattern actually looked like. #Resolved
There was a problem hiding this comment.
Yes, that's been the biggest challenge for this PR: CheckConsistentDecision operates on lowered nodes (BoundDagTest) and is invoked during the incremental construction of the DAG. There may be a way to implement a similar method to operate on BoundPattern from initial binding instead. It could be simplified somewhat (we don't need to return 4 flags, just 1)
| if (i > 0 | ||
| && oneTrue is False | ||
| && this is OrSequence { RemainingTests: [Not { Negated: var negated }, ..] } | ||
| && isRecognizedPartOfNegated(test, negated)) |
There was a problem hiding this comment.
It feels like if we can't come up with any tests which depend on this filtering (e.g. scenarios where an unwanted warning is reported in absence of this check), then it bears further investigation. We should try to understand this area well enough to either identify such a scenario and test it, or to be confident that this check is not needed and remove it.
| // This type provides all the context needed to perform a reachability analysis | ||
| // on a binary OR pattern we're visiting inside a normalized pattern, | ||
| // and collect the redundant nodes that are identified. | ||
| private struct ReachabilityAnalysisContext |
| return; | ||
|
|
||
| static void populateStateForCases(ArrayBuilder<BoundPattern> set, PooledHashSet<LabelSymbol> labelsToIgnore, | ||
| ref TemporaryArray<StateForCase> casesBuilder, ReachabilityAnalysisContext context) |
| // Given a normalized pattern (so there are only `and` and `or` patterns at the root of the tree) | ||
| // we traverse the binary patterns building a set of cases and reporting reachability issues | ||
| // on that set of cases when applicable. | ||
| static void analyze(BoundPattern pattern, ReachabilityAnalysisContext context) |
| return; | ||
| } | ||
|
|
||
| static void analyzePattern(ArrayBuilder<BoundPattern> currentCases, BoundPattern pattern, Func<BoundPattern, BoundPattern>? wrapIntoParentAndPattern, ReachabilityAnalysisContext context) |
| } | ||
| } | ||
|
|
||
| static void analyzeBinary(ArrayBuilder<BoundPattern> currentCases, BoundBinaryPattern binaryPattern, Func<BoundPattern, BoundPattern>? wrapIntoParentAndPattern, ReachabilityAnalysisContext context) |
| // we'll be able to check reachability on: `case A1:`, ... `case Ai:`, `case B1:`, ... `case Bn:` | ||
| for (int i = 0; i < patterns.Count; i++) | ||
| { | ||
| analyzePattern(currentCases, patterns[i], wrapIntoParentAndPattern, context); |
| // So we'll check reachability on `case A and B1:`, ..., `case A and Bn:` | ||
|
|
||
| // Given `newPattern`, produce `A and newPattern` | ||
| Func<BoundPattern, BoundPattern> newWrapIntoParentAndPattern = (BoundPattern newPattern) => |
| } | ||
| } | ||
|
|
||
| static void checkReachability(ArrayBuilder<BoundPattern> orCases, ReachabilityAnalysisContext context) |
| { | ||
| // We construct a set of cases using the previous cases from context and the current/given cases. | ||
| // We then construct a DAG and analyze reachability of branches. | ||
| // Unreachable cases for patterns marked as compiler-generated will not reported. |
| // It also erases/simplifies some patterns (variable declarations). | ||
| // | ||
| // For example, given `not { Prop: 42 or 43 }` | ||
| // it produces `not null or ({ Prop: not 42 } and { Prop: not 43 })`. |
| if (_operand is null) | ||
| { | ||
| Debug.Assert(_disjunction.HasValue); | ||
| disjunction = _disjunction.Value; |
| return patternNormalizer.GetResult(inputType); | ||
| } | ||
|
|
||
| // Reconstitutes a normalized pattern from the operands (ie. patterns) and operations (`and` or `or`) |
| return base.Visit(node); | ||
| } | ||
|
|
||
| // Updates the eval sequence from [...eval sequence...] |
| ImmutableArray<BoundPattern> newSubpatterns = equivalentDefaultPatterns.SetItem(i, newPattern); | ||
|
|
||
| BoundPattern newList = new BoundListPattern( | ||
| newPattern.Syntax, newSubpatterns, hasSlice: newSubpatterns.Any(p => p is BoundSlicePattern), listPattern.LengthAccess, listPattern.IndexerAccess, |
| }; | ||
|
|
||
| // Given `newPattern`, produce `[..., _, ..newPattern, _, ...]` | ||
| Func<BoundPattern, BoundPattern> makeListPatternWithSlice = (BoundPattern newPattern) => |
| _makeEvaluationSequenceOperand = saveMakeEvaluationSequenceOperand; | ||
| } | ||
|
|
||
| if (!node.Properties.IsDefault) |
|
Done with review pass (commit 50) |
src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.CheckOrReachability.cs
Show resolved
Hide resolved
|
@RikkiGibson for a second review. Thanks |
| @@ -158,6 +158,21 @@ class C | |||
| } | |||
| ``` | |||
|
|
|||
| ## Warn for redundant pattern in simple `or` patterns | |||
|
|
|||
| ***Introduced in Visual Studio 2022 version 17.13*** | |||
There was a problem hiding this comment.
this will likely ship in 17.14 at earliest now.
| ReportRedundant(redundantNodes, diagnostics); | ||
|
|
||
| redundantNodes.Free(); | ||
| noPreviousCases.Free(); |
There was a problem hiding this comment.
Should we assert that noPreviousCases was empty here?
| ImmutableArray<BoundSwitchExpressionArm> switchArms, | ||
| BindingDiagnosticBag diagnostics) | ||
| { | ||
| LabelSymbol defaultLabel = new GeneratedLabelSymbol("isPatternFailure"); |
There was a problem hiding this comment.
nit: perhaps label name should be adjusted to reflect that this is a switch expression.
| ImmutableArray<BoundSwitchSection> switchSections, | ||
| BindingDiagnosticBag diagnostics) | ||
| { | ||
| LabelSymbol defaultLabel = new GeneratedLabelSymbol("isPatternFailure"); |
There was a problem hiding this comment.
similar comment about the label name.
|
|
||
| // Detect a `not` at top-level or inside a tree of binary patterns | ||
| // Note: we don't dig into parenthesized patterns as the meaning of `not` is not problematic then | ||
| static bool findNotInBinary(SyntaxNode syntax) |
There was a problem hiding this comment.
It looks like I accidentally deleted a comment from Aleksey here, which was marked "pending". Sorry about that. Here is the comment text from the original email notification:
In src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.CheckOrReachability.cs:
> + syntax = listPattern;
+ goto start;
+ }
+
+ if (syntax.Parent is SlicePatternSyntax slicePattern)
+ {
+ syntax = slicePattern;
+ goto start;
+ }
+
+ return false;
+ }
+
+ // Detect a `not` at top-level or inside a tree of binary patterns
+ // Note: we don't dig into parenthesized patterns as the meaning of `not` is not problematic then
+ static bool findNotInBinary(SyntaxNode syntax)
findNotInBinaryIt looks like implementation in this function can be simplified as follows:
static bool findNotInBinary(SyntaxNode syntax)
{
while (syntax is BinaryPatternSyntax binarySyntax)
{
if (findNotInBinary(binarySyntax.Right))
{
return true;
}
binarySyntax= left;
}
return syntax.Kind() == SyntaxKind.NotPattern;
}
|
Sorry the review is not done yet. I have not forgotten about this. Very soon I will set aside the necessary time to get the rest of the way through this. Thanks. |
| <value>The pattern is redundant.</value> | ||
| </data> | ||
| <data name="HDN_RedundantPatternStackGuard" xml:space="preserve"> | ||
| <value>The pattern is too deep to analyze for redundancy.</value> |
There was a problem hiding this comment.
nit: I think 'complex' is more suitable term than 'deep' a la "An expression is too long or complex to compile".
| comp.VerifyDiagnostics( | ||
| // (10,40): hidden CS9271: The pattern is redundant. | ||
| // (message: null, isColInit: false) => 43, | ||
| Diagnostic(ErrorCode.HDN_RedundantPattern, "false").WithLocation(10, 40), |
There was a problem hiding this comment.
I thought it was a goal to not diagnose cases like these. That is, it seems reasonable for user to put the false pattern here for purpose of readability. So, if an analyzer or IDE feature came around which "greyed out" a pattern like this, and suggested using _ instead, it seems like it would be an undesirable experience.
edit: saw the thinking around this in the implementation comments. This is fine, if IDE wants to key off those hidden diagnostics at some point we may need to be more discerning and have a more concrete design for what patterns user is likely to find it helpful for us to mark redundant.
| { | ||
| var source = """ | ||
| System.Runtime.CompilerServices.ITuple s = null; | ||
| _ = s is (not 42 or 43, not 44 or 45); // 1, 2 |
There was a problem hiding this comment.
Are there tests for 43 or not 42? I am guessing there is no diagnostic in this case.
| _ = s is (_, _) and (_, _); // 1 | ||
| _ = s is (_, _) and (_, var s5); | ||
| _ = s is (_, _) and (_, _) s6; // 2, 3 | ||
| _ = s is { Length: 2 } and (_, _); |
There was a problem hiding this comment.
I found it a little surprising that this does not warn on (_, _) as a redundant pattern. But I think i'm ok with that.
| // _ = o is not S (42 and not 43, int x2); // 1 | ||
| Diagnostic(ErrorCode.HDN_RedundantPattern, "43").WithLocation(4, 28), | ||
| // (5,36): hidden CS9274: The pattern is redundant. | ||
| // _ = o is not S { Prop1: 42 and not 43, Prop2: _ and var x3 }; // 2, 3 |
There was a problem hiding this comment.
It looks like // 3 is missing.
| Diagnostic(ErrorCode.WRN_RedundantPattern, "42").WithLocation(2, 31), | ||
| // (3,22): warning CS9275: The pattern is redundant. | ||
| // _ = s is not null or { Prop1: 44 or 45 }; // 3, 4, 5, 6 | ||
| Diagnostic(ErrorCode.WRN_RedundantPattern, "{ Prop1: 44 or 45 }").WithLocation(3, 22), |
There was a problem hiding this comment.
I'm wondering if warnings on a parent pattern and subpatterns at the same time is something we should try to avoid.
Hidden diagnostics seems fine. But warnings may be a little more disruptive, as users may be scrolling through the error list to figure out what the most relevant ones are.
| // We're okay reporting the following as just hidden diagnostics | ||
| C s = null; | ||
| _ = s is { Prop1: not 42 } or { Prop1: 43 }; // 1 | ||
| _ = s is (not 42, _) or (not 43, _); // 2 |
There was a problem hiding this comment.
I find it confusing that this line does not 43 while the previous line does just 43. Maybe this avoids "multiplying out" the combinations?
|
|
||
| if (s is { P0: 0 } or (({ P2: 2 } or { P1: > 1 }) and { P3: 3 }) or { P2: > -1 }) { } | ||
|
|
||
| if (s is { P0: 0 } or (({ P1: > 1 } or { P2: 2 }) and { P3: 3 }) ) { } // 3 |
There was a problem hiding this comment.
// 3 looks unnecessary.
| var source = """ | ||
| var c = new C(); | ||
|
|
||
| _ = c is ({F: 1 and not 1} and {O: not A or B}) or {F: 2}; // 1 |
There was a problem hiding this comment.
Could you please explain a bit why the part we blame here is the entire not A or B? B itself seems redundant, but not A seems meaningful. It's just that it's preceded by a "contradiction" pattern.
| // | ||
| // When visiting a pattern, the caller indicates: | ||
| // - whether the pattern should be negated, | ||
| // - whether the evaluations yieled by the visit will be combined in `or` or `and`, |
There was a problem hiding this comment.
| // - whether the evaluations yieled by the visit will be combined in `or` or `and`, | |
| // - whether the evaluations yielded by the visit will be combined in `or` or `and`, |
Addresses #75506
Filled language issue/discussion for asymmetries in types flowing in patterns: dotnet/csharplang#8888
Validated performance with Jared and Cyrus by replaying a build of roslyn and looking at the trace. With blanked GroupPats and default FoldPats,
CheckOrAndAndReachabilityshows as 0.1 for Inc%. We have an option to skip the analysis entirely based on global nowarn settings, but feel we don't need to implement that yet.