Look past comment placeholders when matching statement sequences - #4130
Merged
Conversation
A constructor store to an auto-property's backing field is expressible after the field declaration is gone in one of two ways: ReplaceBackingFieldUsage rewrites it to an assignment of a setter-less property, or TransformFieldAndConstructorInitializers lifts it into a property initializer. A deconstruction target assigns several members at once, so it can never take the second route, and a property that kept a setter would invoke that setter instead of storing the field. Without the restriction the declaration is removed while the store keeps referencing it. Assisted-by: Claude:claude-opus-5:Claude Code
The decompiler emits comments - //IL_ warnings, "Could not convert BlockContainer", "try-fault", a Nop's comment - as an EmptyStatement in the middle of a statement sequence. Every transform that walks such a sequence then stops recognizing its pattern the moment one of those lands in it: constructor initializers stay in the body, `using var` and `for` are not introduced, and a finalizer keeps its `override Finalize` shape, which does not compile at all. The destructor matcher moves the placeholders it skipped into the body that replaces the old one, so the warning that caused the problem is not dropped along with the statement carrying it. Assisted-by: Claude:claude-opus-5:Claude Code
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.
The decompiler emits its comments as an
EmptyStatementcarrying trivia://IL_warnings(
ExpressionBuilder.cs:2599,StatementBuilder.cs:1524), "Could not convert BlockContainer"(
ExpressionBuilder.cs:2870), "try-fault" (StatementBuilder.cs:510), and anyNopwith aComment(StatementBuilder.cs:151, fed byYieldReturnDecompiler). The last one can landanywhere in a statement sequence, not just at a block head.
Every C# AST transform that walks a statement sequence stops recognizing its pattern as soon as
one of those appears in it. Two of the consequences are output that does not compile:
TransformFieldAndConstructorInitializers.MoveConstructorInitializermatches the ctor callagainst
Body.Statements.FirstOrDefault(). A leading placeholder fails the match andthis(...)/base(...)stays in the body as an expression statement.PatternStatementTransform.destructorPatternrequired the body to be exactly oneTryCatchStatement. A placeholder anywhere in it leaves the method asprotected override void Finalize()— CS0249.The rest degrade quietly: constructor initializers are not moved, a constructor is misfiled as
not chained with
this(which changes initializer placement for the whole type), andusing var,forand switch-section flattening are not introduced.Changes
GetFirstNonEmptyStatementOrDefault()andGetNextNonEmptyStatement()inSyntaxExtensions,applied at the nine sequence-walking sites in
TransformFieldAndConstructorInitializers,PatternStatementTransformandFlattenSwitchBlocks.The destructor matcher is restructured into
MatchDestructorBody, shared byTransformDestructorand
TransformDestructorBodyso the two cannot drift. Strictness is unchanged — the try statementmust still be alone in the body, still have no catch clauses, and the finally block must still hold
nothing but
base.Finalize(). Placeholders skipped there are moved to the front of the body thatreplaces the old one, so the warning that caused the problem is not dropped with the statement
carrying it.
The first commit is separate: a deconstruction target of a backing-field store is accepted only for
a setter-less property. It assigns several members at once, so it can never become a property
initializer, and a property that kept a setter would invoke that setter instead of storing the
field — without the restriction the field declaration is removed while the store still references it.
Testing
ICSharpCode.Decompiler.Testsis green: 3606 total, 0 failed, 50 skipped (Windows-only and theILSpy-testsround-trip cases). Finalizers were checked by hand in Release and Debug builds,including one with a user-written
try/finallynested inside the compiler's.Neither the destructor case nor the deconstruction guard has a regression test. Plain
nopopcodes are removed before the C# AST is built, and a settable property's constructor store goes
through the setter, so neither state is reachable from compiled C# — both would need hand-written
IL fixtures.
🤖 Generated with Claude Code