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

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

Addresses #75506

Open questions for LDM:

confirm we prefer this compiler-based detection over an analyzer
confirm that a regular warning is preferrable to a warning wave warning or a LangVer-gated warning

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

@@ -87,3 +87,18 @@ class C
    }
 }
 ```
+
+## 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

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


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

@@ -87,3 +87,18 @@ class C
    }
 }
 ```
+
+## 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

RikkiGibson · 2024-11-07T04:21:03Z

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

+                        // 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 }, ..] }


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.

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)

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 · 2024-11-08T16:14:28Z

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

                        trueBuilder.Add(oneTrue);
                        falseBuilder.Add(oneFalse);
+
+                        if (i > 0
+                            && oneTrue is False


&& oneTrue is False

Could you explain and add a comment about what conclusion we draw from this fact?

AlekseyTs · 2024-11-08T16:57:46Z

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

+                        // 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 }, ..] }


if

I am not sure how to map this condition to the comment above. It doesn't look they are related

jcouv · 2024-11-11T21:39:38Z

From discussion with @AlekseyTs, going back to drawing board to explore some alternative design. Moving back to draft

jcouv added the Area-Compilers label Oct 22, 2024

jcouv self-assigned this Oct 22, 2024

dotnet-issue-labeler bot 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 jjonescz, 333fred and jaredpar 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

jcouv added 6 commits November 7, 2024 12:37

Keep looking for test affected by isRecognizedPartOfNegated

0043011

Show effect of filter logic

ca3d7a2

Address feedback

2b3ab87

Another test

15e6ac8

Revert logic

ae2834c

Flatten local functions

13e312e

AlekseyTs reviewed Nov 8, 2024

View reviewed changes

WIP

e20627a

jcouv marked this pull request as draft November 11, 2024 21:38

Add test

ceb26dc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jcouv commented Oct 22, 2024 •

edited

Loading

jcouv Oct 22, 2024 •

edited

Loading

RikkiGibson Nov 7, 2024

jcouv Nov 7, 2024 •

edited

Loading

RikkiGibson Nov 7, 2024

RikkiGibson Nov 7, 2024

jjonescz Oct 24, 2024 •

edited by jcouv

Loading

jjonescz Oct 24, 2024 •

edited by jcouv

Loading

RikkiGibson Nov 7, 2024

jcouv Nov 7, 2024

jjonescz Nov 8, 2024

jaredpar Oct 24, 2024 •

edited by jcouv

Loading

jcouv Oct 25, 2024

333fred Oct 24, 2024 •

edited by jcouv

Loading

jcouv commented Nov 5, 2024

jcouv commented Nov 6, 2024

RikkiGibson left a comment

RikkiGibson Nov 7, 2024

RikkiGibson Nov 7, 2024

jcouv Nov 7, 2024

RikkiGibson Nov 7, 2024

AlekseyTs Nov 8, 2024

AlekseyTs Nov 8, 2024

jcouv commented Nov 11, 2024

Warn for redundant pattern in not ... or <redundant> pattern #75581

Are you sure you want to change the base?

Warn for redundant pattern in not ... or <redundant> pattern #75581

Conversation

jcouv commented Oct 22, 2024 • edited Loading

jcouv Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Oct 24, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

jjonescz Oct 24, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Oct 24, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Oct 24, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Nov 5, 2024

jcouv commented Nov 6, 2024

RikkiGibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Nov 11, 2024

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

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

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