C#: Avoid recursive operator simplification - #22345
Conversation
There was a problem hiding this comment.
Pull request overview
Prevents unsafe Boolean comparison simplifications that could introduce recursion in user-defined C# operators.
Changes:
- Detects potentially recursive enclosing operators.
- Adds standalone coverage for comparison pairs and nested callables.
- Documents the analysis improvement.
Show a summary per file
| File | Description |
|---|---|
SimplifyBoolExpr.ql |
Adds the recursion guard. |
SimplifyBoolExpr.cs |
Adds standalone test cases. |
SimplifyBoolExpr.expected |
Records expected alerts. |
SimplifyBoolExpr.qlref |
Configures the test query. |
options |
Enables standalone extraction. |
2026-08-13-simplifiable-operator-recursion.md |
Adds the change note. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| binary | ||
| .getLeftOperand() | ||
| .getType() | ||
| .isImplicitlyConvertibleTo(enclosingOperator.getParameter(0).getType()) and |
There was a problem hiding this comment.
Thank you very much for the contribution!
The change specifically caters for the case where we don't want to suggest simplifications that would involve recursive calls. However, possible recursion is only detected, if it is "direct" as enclosing callable is a syntactic property of an expression/callable. This probably handles many of the of real world false positives and is fine for most practical purposes.
Just some food for thought and not anything that needs to be addressed: Another (more conservative) approach could be that we simply don't make any simplification suggestions for operator replacements for expressions found in the declaring type of the operators being defined/replaced. That is, if we have an expression !(c1 != c2) somewhere in type C, where c1 and c2 are of type C then we don't make any rewrite suggestions. Furthermore, it could be argued that we generally should be cautious about suggesting simplications, if any user defined operator is involved as it can't be guaranteed that the replacement is logically equivalent.
I think the change looks good as it is, but will run DCA before approving.
|
DCA looks good; Some false positives are removed. |
Fixes #22190.
In
build-mode: nonedatabases, an overloaded comparison may be extracted as a binary operation. Avoid suggesting a rewrite when the replacement could recursively call an enclosing user-defined operator.Tests cover all comparison pairs, nested callables, and safe built-in comparisons.
Validated with focused query tests, query formatting, and strict compilation.