Don't undo the compiler's &&-to-& optimization#3899
Open
siegfriedpammer wants to merge 1 commit into
Open
Conversation
ExpressionBuilder printed bitwise & / | on booleans as && / || whenever the right-hand side was pure, but Roslyn lowers && / || to & / | only when the right operand is a bare local or parameter read (LocalRewriter.MakeBinaryOperator, unchanged since 2014). Shapes like (c == 'a') | (c == 'b') can therefore only originate from a bitwise source operator, yet were shown as short-circuiting. Per the discussion in #1545, show the operator the IL actually uses instead of guessing the source form: the reversal is dropped entirely, so Roslyn-compiled "a && b" now decompiles to "a & b", which recompiles to the same IL. Assisted-by: Claude:claude-fable-5:Claude Code
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the C# decompiler to preserve boolean bitwise operators (& / |) as they appear in IL, instead of rewriting them back to short-circuiting logical operators (&& / ||) when the right-hand side is considered pure. This aligns ILSpy’s output more closely with the actual IL and fixes the incorrect |-to-|| decompilation reported in #1924.
Changes:
- Removed the logic in
ExpressionBuilder.HandleBinaryNumericthat converted boolean bitwise&/|into&&/||based on RHS purity. - Updated pretty test expectations where Roslyn-produced IL previously decompiled to
&&but now intentionally decompiles to&. - Reworked the short-circuit test coverage to explicitly validate bitwise boolean operator output and added the #1924 repro case.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs | Removes the boolean bitwise-to-logical operator rewrite so decompilation reflects the IL operator. |
| ICSharpCode.Decompiler.Tests/TestCases/Pretty/Switch.cs | Adjusts expected pretty output to match the new & rendering in affected conditions. |
| ICSharpCode.Decompiler.Tests/TestCases/Pretty/ShortCircuit.cs | Replaces the prior test with explicit bitwise-operator cases and adds the #1924 repro `(c == 'a') |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1924; implements option (c) from the discussion in #1545 (remove the de-optimization, stay close to the IL).
ExpressionBuilder.HandleBinaryNumericprinted bitwise&/|on booleans as&&/||whenever the right-hand side was side-effect-free. Roslyn's forward optimization is much narrower:LocalRewriter.MakeBinaryOperatorlowers&&/||to&/|only when the lowered right operand isBoundKind.LocalorBoundKind.Parameter(unchanged since 2014, applies in Debug and Release alike, expression-tree lambdas exempt). So for shapes like(c == 'a') | (c == 'b')the IL can only originate from a bitwise source operator, yet ILSpy showed it as short-circuiting -- the #1924 report.Per the maintainer position in both threads, the reversal is removed entirely rather than narrowed: the decompiled code now shows the operator the IL actually uses.
Consequences:
(c == 'a') | (c == 'b')(the Incorrectly decompiling a bitwise or to a logical or #1924 repro) round-trips as|.&&/||(e.g.a && i == 1, which Roslyn never optimizes toand).a && b(bare local/parameter RHS) now decompiles toa & b-- the deliberate trade-off of option (c); both forms compile back to identical IL.Test changes:
ShortCircuit.PreferLogicalToBitwiseis reworked intoBitwiseBooleanOperatorsplus aBitwiseOrWithComparisonsmethod with the #1924 repro (shown failing before the code change); four&& localconditions inSwitch.cs"Overagressive Switch Use" flip to the&form (for Roslyn the IL is identical either way). Bitwise&/|on bools compiles toand/orunder every supported compiler, so the fixture expectations stay uniform across the matrix without#ifsplits. Full XPlat suite green on Linux (2404 tests, 32 pre-existing skips); the legacy csc/mcs configs only run on Windows CI.This PR was authored by an AI agent (Claude Code) operated by @siegfriedpammer.
🤖 Generated with Claude Code