Warn for redundant pattern in `not ... or <redundant>` pattern by jcouv · Pull Request #75581 · dotnet/roslyn

jcouv · 2024-10-22T17:18:12Z

This PR is going to wait for an early preview of .NET 10.0.200 since it adds a new warning

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, CheckOrAndAndReachability shows 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.

Tested with VS test insertion
Tested building the runtime repo (2025-10-09)

jcouv · 2024-10-22T23:09:06Z

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

+                        if (i > 0
+                            && oneTrue is False
+                            && this is OrSequence { RemainingTests: [Not { Negated: var negated }, ..] }
+                            && isRecognizedPartOfNegated(test, negated))


📝 Surprisingly, removing this isRecognizedPartOfNegated check doesn't seem to impact any tests or bootstrapping. #Resolved

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.

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.

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.

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.

jjonescz · 2024-10-24T08:16:20Z

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

+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:
+```


Suggested change

```

```c#

```` #Resolved

jjonescz · 2024-10-24T08:19:17Z

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

 }
 ```
+
+## Warn for redundant pattern in `not ... or <redundant>` pattern


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

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).

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.

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.

jaredpar · 2024-10-24T12:28:23Z

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

+_ = 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.


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

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"

333fred · 2024-10-24T18:38:01Z

src/Compilers/CSharp/Portable/CSharpResources.resx

    <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>


Can we include something about "did you mean to parenthesize this"? #Resolved

jcouv · 2024-11-05T18:12:49Z

@dotnet/roslyn-compiler for review. Thanks

jcouv · 2024-11-06T17:52:12Z

@dotnet/roslyn-compiler for review. I'm happy to offer a walkthrough for more context on DAG construction. Let me know

RikkiGibson

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

RikkiGibson · 2024-11-07T04:15:15Z

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

 }
 ```
+
+## Warn for redundant pattern in `not ... or <redundant>` pattern


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).

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

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

RikkiGibson · 2024-11-07T04:23:11Z

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

+                        if (i > 0
+                            && oneTrue is False
+                            && this is OrSequence { RemainingTests: [Not { Negated: var negated }, ..] }
+                            && isRecognizedPartOfNegated(test, negated))


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.

AlekseyTs · 2025-10-01T19:22:05Z

Done with review pass (commit 57) #Closed

CyrusNajmabadi · 2025-10-02T10:17:26Z

src/Features/CSharp/Portable/InlineHints/CSharpInlineTypeHintsService.cs

    private static bool IsValidType([NotNullWhen(true)] ITypeSymbol? type)
    {
-        return type is not null or IErrorTypeSymbol && type.Name != "var";
+        return type is not null && type.Name != "var";


Suggested change

return type is not null && type.Name != "var";

return type is not null and not IErrorTypeSymbol && type.Name != "var";

``` #Pending

AlekseyTs

LGTM (commit 60)

RikkiGibson · 2025-10-02T19:04:54Z

src/Features/CSharp/Portable/InlineHints/CSharpInlineTypeHintsService.cs

@@ -154,6 +154,6 @@ private static TextSpan GetSpan(

    private static bool IsValidType([NotNullWhen(true)] ITypeSymbol? type)
    {
-        return type is not (null or IErrorTypeSymbol) && type.Name != "var";
+        return type is not null and not IErrorTypeSymbol && type.Name != "var";


I am confused, was (null or IErrorTypeSymbol) identified as being a redundant pattern? #Resolved

No, only not null or IErrorTypeSymbol was. Both not (null or IErrorTypeSymbol) and not null and not IErrorTypeSymbol are fine.

RikkiGibson

LGTM (incremental review) aside from the one question

jcouv · 2025-10-09T22:57:48Z

Completed validation with VS test insertion and building the runtime repo. I'll go ahead and resolve the conflicts and merge.

jcouv added the Area-Compilers label Oct 22, 2024

jcouv self-assigned this Oct 22, 2024

ghost added the untriaged Issues and PRs which have not yet been triaged by a lead label Oct 22, 2024

jcouv force-pushed the not-or branch from 7c59fd2 to 9992998 Compare October 22, 2024 17:33

dotnet-policy-service bot added the VSCode label Oct 22, 2024

jcouv commented Oct 22, 2024

View reviewed changes

jcouv force-pushed the not-or branch 2 times, most recently from 59d9dd7 to c4a5bdf Compare October 22, 2024 23:54

jcouv removed the VSCode label Oct 23, 2024

jcouv force-pushed the not-or branch from c4a5bdf to b7cda30 Compare October 23, 2024 17:21

dotnet-policy-service bot added the VSCode label Oct 23, 2024

Report not ... or <redundant> patterns

0c1d4ad

jcouv force-pushed the not-or branch from b7cda30 to 0c1d4ad Compare October 23, 2024 17:58

jcouv marked this pull request as ready for review October 23, 2024 20:40

jcouv requested review from a team as code owners October 23, 2024 20:40

jjonescz reviewed Oct 24, 2024

View reviewed changes

jaredpar reviewed Oct 24, 2024

View reviewed changes

333fred reviewed Oct 24, 2024

View reviewed changes

RikkiGibson self-assigned this Oct 25, 2024

Address feedback

d6d42ba

jcouv requested review from 333fred, jaredpar and jjonescz October 25, 2024 16:19

jcouv added 3 commits October 25, 2024 10:49

Add more examples of legitimate 'not ... or ...' patterns

c196e15

Handle one more scenario

b191bab

Add one more case

31adeb2

RikkiGibson reviewed Nov 7, 2024

View reviewed changes

Julien Couvreur added 3 commits October 1, 2025 12:48

Update error codes

ed09537

Missing in

852f425

Adjust CSharpInlineTypeHintsService

6b41b8e

CyrusNajmabadi reviewed Oct 2, 2025

View reviewed changes

CyrusNajmabadi approved these changes Oct 2, 2025

View reviewed changes

AlekseyTs approved these changes Oct 2, 2025

View reviewed changes

Julien Couvreur added 2 commits October 2, 2025 06:46

IErrorTypeSymbol

979256e

Fix test accidentally hitting IErrorTypeSymbol

9bb3871

RikkiGibson reviewed Oct 2, 2025

View reviewed changes

RikkiGibson approved these changes Oct 2, 2025

View reviewed changes

Merge remote-tracking branch 'dotnet/main' into not-or

31dd73c

jcouv marked this pull request as ready for review October 9, 2025 23:31

jcouv enabled auto-merge (squash) October 9, 2025 23:34

jcouv merged commit 579f106 into dotnet:main Oct 10, 2025
27 of 28 checks passed

dotnet-policy-service bot added this to the Next milestone Oct 10, 2025

jcouv deleted the not-or branch October 10, 2025 12:25

dotnet-bot mentioned this pull request Oct 14, 2025

[Automated] PRs inserted in VS build main-11113.80 #80688

Closed

jcouv mentioned this pull request Oct 14, 2025

Suggestion: warn on use of "not A or B" pattern matching syntax #75506

Closed

dotnet-maestro bot mentioned this pull request Oct 17, 2025

[main] Source code updates from dotnet/roslyn dotnet/dotnet#2965

Closed

This was referenced Oct 17, 2025

[Automated] PRs inserted in VS build feature.polytype-11117.05 #80797

Closed

[Automated] PRs inserted in VS build feature.debugger.main-11116.247 #80798

Closed

[Automated] PRs inserted in VS build feature.polytype-11116.236 #80799

Closed

This was referenced Oct 23, 2025

[main] Source code updates from dotnet/roslyn dotnet/dotnet#3061

Closed

[main] Source code updates from dotnet/roslyn dotnet/dotnet#3147

Closed

dotnet-maestro bot mentioned this pull request Dec 18, 2025

[main] Source code updates from dotnet/roslyn dotnet/dotnet#3957

Merged

davidwengier modified the milestones: Next, 18.3 Jan 6, 2026

ViktorHofer mentioned this pull request Jan 12, 2026

Update the stack to net11.0 dotnet/dotnet#4130

Merged

	return type is not null && type.Name != "var";
	return type is not null and not IErrorTypeSymbol && type.Name != "var";
	``` #Pending

Conversation

jcouv commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Oct 24, 2024 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Oct 24, 2024 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Oct 24, 2024 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Oct 24, 2024 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 5, 2024

Uh oh!

jcouv commented Nov 6, 2024

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi Oct 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Oct 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Oct 9, 2025

Uh oh!

Uh oh!

Reviewers

jcouv commented Oct 22, 2024 •

edited

Loading

jcouv Oct 22, 2024 •

edited

Loading

jcouv Nov 7, 2024 •

edited

Loading

jjonescz Oct 24, 2024 •

edited by jcouv

Loading

jjonescz Oct 24, 2024 •

edited by jcouv

Loading

jaredpar Oct 24, 2024 •

edited by jcouv

Loading

333fred Oct 24, 2024 •

edited by jcouv

Loading

AlekseyTs commented Oct 1, 2025 •

edited

Loading

CyrusNajmabadi Oct 2, 2025 •

edited by jcouv

Loading

RikkiGibson Oct 2, 2025 •

edited by jcouv

Loading