From 4a0918cbe5f6a34ef5babff4083a904d676fb614 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Thu, 10 Sep 2026 18:16:38 +0200 Subject: [PATCH] Look past comment placeholders when matching statement sequences 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 --- .../CSharp/Syntax/SyntaxExtensions.cs | 31 +++++++ .../CSharp/Transforms/FlattenSwitchBlocks.cs | 5 +- .../Transforms/PatternStatementTransform.cs | 82 +++++++++++++++---- ...ransformFieldAndConstructorInitializers.cs | 12 +-- 4 files changed, 104 insertions(+), 26 deletions(-) diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/SyntaxExtensions.cs b/ICSharpCode.Decompiler/CSharp/Syntax/SyntaxExtensions.cs index daae52821..c52969a05 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/SyntaxExtensions.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/SyntaxExtensions.cs @@ -61,6 +61,37 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax return (Statement?)next; } + /// + /// The first statement of that is not an + /// , or null if there is none. + /// + /// + /// An empty statement is either a stray ';' or a placeholder carrying a comment the + /// decompiler emitted (a warning, an unconvertible block, a "try-fault" marker, ...). + /// Neither is a statement in the sense a transform matching a statement sequence means, + /// so every such transform has to look past them or it silently stops recognizing its + /// pattern as soon as one of those comments lands in the middle of the sequence. + /// + public static Statement? GetFirstNonEmptyStatementOrDefault(this AstNodeCollection statements) + { + return statements.FirstOrNull(statement => statement is not EmptyStatement); + } + + /// + /// The next statement after that is not an + /// , or null if there is none. + /// + /// + /// See for why the skip is needed. + /// + public static Statement? GetNextNonEmptyStatement(this Statement statement) + { + var next = statement.GetNextStatement(); + while (next is EmptyStatement) + next = next.GetNextStatement(); + return next; + } + public static bool IsArgList(this AstType? type) { var simpleType = type as SimpleType; diff --git a/ICSharpCode.Decompiler/CSharp/Transforms/FlattenSwitchBlocks.cs b/ICSharpCode.Decompiler/CSharp/Transforms/FlattenSwitchBlocks.cs index 73d62258c..99e49bde0 100644 --- a/ICSharpCode.Decompiler/CSharp/Transforms/FlattenSwitchBlocks.cs +++ b/ICSharpCode.Decompiler/CSharp/Transforms/FlattenSwitchBlocks.cs @@ -33,10 +33,11 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms { foreach (var switchSection in rootNode.Descendants.OfType()) { - if (switchSection.Statements.Count != 1) + var onlyStatement = switchSection.Statements.GetFirstNonEmptyStatementOrDefault(); + if (onlyStatement == null || onlyStatement.GetNextNonEmptyStatement() != null) continue; - var blockStatement = switchSection.Statements.First() as BlockStatement; + var blockStatement = onlyStatement as BlockStatement; if (blockStatement == null || blockStatement.Statements.Any(ContainsLocalDeclaration)) continue; diff --git a/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs b/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs index b5a40d081..e82fb461c 100644 --- a/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs +++ b/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs @@ -197,7 +197,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms if (!m1.Success) return null; var variable = m1.Get("variable").Single().GetILVariable(); - AstNode? next = node.NextSibling; + AstNode? next = node.GetNextNonEmptyStatement(); if (next == null) return null; if (next is ForStatement forStatement && ForStatementUsesVariable(forStatement, variable)) @@ -598,7 +598,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms Match m = default(Match); while (i < upperBounds.Length && MatchLowerBound(i, out var indexVariable, collection, stmt)) { - m = forOnArrayMultiDimPattern.Match(stmt.GetNextStatement()); + m = forOnArrayMultiDimPattern.Match(stmt.GetNextNonEmptyStatement()); if (!m.Success) return false; var upperBound = m.Get("upperBoundVariable").Single().GetILVariable(); @@ -654,7 +654,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms if (!int.TryParse(m.Get("index").Single().Value?.ToString() ?? "", out int index) || index != i) break; upperBounds[i] = m.Get("variable").Single().GetILVariable()!; - stmt = stmt.GetNextStatement(); + stmt = stmt.GetNextNonEmptyStatement(); i++; } while (stmt != null && upperBounds != null && i < upperBounds.Length); @@ -664,7 +664,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms return null; statementsToDelete.Add(stmt); // The matched multi-dimensional foreach pattern guarantees a statement after stmt. - statementsToDelete.Add(stmt.GetNextStatement()!); + statementsToDelete.Add(stmt.GetNextNonEmptyStatement()!); var itemVariable = foreachVariable.GetILVariable(); if (itemVariable == null || !itemVariable.IsSingleDefinition || (itemVariable.Kind != IL.VariableKind.Local && itemVariable.Kind != IL.VariableKind.StackSlot) @@ -1261,34 +1261,79 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms #endregion #region Destructor - static readonly BlockStatement destructorBodyPattern = new BlockStatement { - new TryCatchStatement { - TryBlock = new AnyNode("body"), - FinallyBlock = new BlockStatement { - new InvocationExpression(new MemberReferenceExpression(new BaseReferenceExpression(), "Finalize")) - } - } + static readonly TryCatchStatement destructorTryFinallyPattern = new TryCatchStatement { + TryBlock = new AnyNode("body"), + FinallyBlock = new AnyNode("finallyBlock") }; + static readonly Statement baseFinalizeCallPattern = new ExpressionStatement( + new InvocationExpression(new MemberReferenceExpression(new BaseReferenceExpression(), "Finalize"))); + static readonly MethodDeclaration destructorPattern = new MethodDeclaration { Attributes = { new Repeat(new AnyNode()) }, Modifiers = Modifiers.Any, ReturnType = new PrimitiveType("void"), Name = "Finalize", - Body = destructorBodyPattern + Body = new AnyNode() }; + /// + /// Matches the body a compiler emits for a destructor - a single try statement whose + /// finally block does nothing but call base.Finalize() - and returns the try block + /// holding the user-written code, or null if has another + /// shape. Comment placeholders around the two statements are skipped: leaving the method + /// in its "override Finalize" shape over a decompiler warning produces output that does + /// not compile (CS0249). + /// + static BlockStatement? MatchDestructorBody(BlockStatement body) + { + var statement = body.Statements.GetFirstNonEmptyStatementOrDefault(); + if (statement is not TryCatchStatement || statement.GetNextNonEmptyStatement() != null) + return null; + Match m = destructorTryFinallyPattern.Match(statement); + if (!m.Success) + return null; + var finalizeCall = m.Get("finallyBlock").Single() + .Statements.GetFirstNonEmptyStatementOrDefault(); + if (finalizeCall == null || finalizeCall.GetNextNonEmptyStatement() != null + || !baseFinalizeCallPattern.IsMatch(finalizeCall)) + { + return null; + } + return m.Get("body").Single(); + } + + /// + /// Moves the comment placeholders of to the front of + /// , which replaces it. They describe the member, so dropping + /// them with the body they happen to sit in would lose a decompiler warning. + /// + static void MovePlaceholderComments(BlockStatement oldBody, BlockStatement newBody) + { + var anchor = newBody.Statements.FirstOrNull(); + foreach (var placeholder in oldBody.Statements.OfType().ToList()) + { + placeholder.Detach(); + if (anchor != null) + newBody.Statements.InsertBefore(anchor, placeholder); + else + newBody.Statements.Add(placeholder); + } + } + DestructorDeclaration? TransformDestructor(MethodDeclaration methodDef) { Match m = destructorPattern.Match(methodDef); - if (m.Success) + if (m.Success && methodDef.Body is BlockStatement oldBody + && MatchDestructorBody(oldBody) is BlockStatement tryBlock) { context.Step("Convert Finalize method to destructor", methodDef); DestructorDeclaration dd = new DestructorDeclaration(); methodDef.Attributes.MoveTo(dd.Attributes); dd.CopyAnnotationsFrom(methodDef); dd.Modifiers = methodDef.Modifiers & ~(Modifiers.Protected | Modifiers.Override); - dd.Body = m.Get("body").Single().Detach(); + MovePlaceholderComments(oldBody, tryBlock); + dd.Body = tryBlock.Detach(); // A destructor only appears inside a type declaration, so the context tracker // has an enclosing type at this point. dd.Name = currentTypeDefinition!.Name; @@ -1301,11 +1346,12 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms DestructorDeclaration? TransformDestructorBody(DestructorDeclaration dtorDef) { - Match m = destructorBodyPattern.Match(dtorDef.Body); - if (m.Success) + if (dtorDef.Body is BlockStatement oldBody + && MatchDestructorBody(oldBody) is BlockStatement tryBlock) { context.Step("Simplify destructor body", dtorDef); - dtorDef.Body = m.Get("body").Single().Detach(); + MovePlaceholderComments(oldBody, tryBlock); + dtorDef.Body = tryBlock.Detach(); return dtorDef; } return null; @@ -1455,7 +1501,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms if (!context.Settings.UseEnhancedUsing) return usingStatement; - if (usingStatement.GetNextStatement() != null || !(usingStatement.Parent is BlockStatement)) + if (usingStatement.GetNextNonEmptyStatement() != null || !(usingStatement.Parent is BlockStatement)) return usingStatement; if (!(usingStatement.ResourceAcquisition is VariableDeclarationStatement)) diff --git a/ICSharpCode.Decompiler/CSharp/Transforms/TransformFieldAndConstructorInitializers.cs b/ICSharpCode.Decompiler/CSharp/Transforms/TransformFieldAndConstructorInitializers.cs index b7d379594..b287b541c 100644 --- a/ICSharpCode.Decompiler/CSharp/Transforms/TransformFieldAndConstructorInitializers.cs +++ b/ICSharpCode.Decompiler/CSharp/Transforms/TransformFieldAndConstructorInitializers.cs @@ -130,7 +130,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms bool skippedStmts = false; Statement? stmt; - for (stmt = ctor.Body?.Statements.FirstOrDefault(); stmt != null; stmt = stmt.GetNextStatement()) + for (stmt = ctor.Body?.Statements.GetFirstNonEmptyStatementOrDefault(); stmt != null; stmt = stmt.GetNextNonEmptyStatement()) { var m = memberInitializerPattern.Match(stmt); if (!m.Success) @@ -178,7 +178,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms : ThisCallClassPattern.Match(stmt); if (m.Success) { - sequence.CoversFullBody = stmt.GetNextStatement() == null; + sequence.CoversFullBody = stmt.GetNextNonEmptyStatement() == null; } } } @@ -228,7 +228,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms if (ctor.Body is null) return false; var stmts = ctor.Body.Statements; - var otherStmt = stmts.FirstOrDefault(); + var otherStmt = stmts.GetFirstNonEmptyStatementOrDefault(); foreach (var (stmt, member, initializer, _) in Statements) { var m = memberInitializerPattern.Match(otherStmt); @@ -249,7 +249,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms StatementToOtherCtorsMap[stmt] = list; } list.Add((otherStmt, otherInitializer)); - otherStmt = otherStmt.GetNextStatement(); + otherStmt = otherStmt.GetNextNonEmptyStatement(); } return true; } @@ -369,7 +369,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms else { // find this-ctor call - var stmt = ctor.Body?.Statements.FirstOrDefault(); + var stmt = ctor.Body?.Statements.GetFirstNonEmptyStatementOrDefault(); var m = ctorMethod.DeclaringType.Kind == TypeKind.Struct ? ThisCallStructPattern.Match(stmt) : ThisCallClassPattern.Match(stmt); @@ -527,7 +527,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms { if (constructorDeclaration.Body is null) return false; - Statement stmt = constructorDeclaration.Body.Statements.FirstOrDefault()!; + Statement stmt = constructorDeclaration.Body.Statements.GetFirstNonEmptyStatementOrDefault()!; var isValueType = ctorMethod.DeclaringType.Kind == TypeKind.Struct; // value types may omit the constructor initializer completely