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

ICSharpCode.Decompiler/CSharp/Syntax/SyntaxExtensions.cs

@@ -61,6 +61,37 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax
 			return (Statement?)next;
 		}
 
+		/// <summary>
+		/// The first statement of <paramref name="statements"/> that is not an
+		/// <see cref="EmptyStatement"/>, or <c>null</c> if there is none.
+		/// </summary>
+		/// <remarks>
+		/// 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.
+		/// </remarks>
+		public static Statement? GetFirstNonEmptyStatementOrDefault(this AstNodeCollection<Statement> statements)
+		{
+			return statements.FirstOrNull(statement => statement is not EmptyStatement);
+		}
+
+		/// <summary>
+		/// The next statement after <paramref name="statement"/> that is not an
+		/// <see cref="EmptyStatement"/>, or <c>null</c> if there is none.
+		/// </summary>
+		/// <remarks>
+		/// See <see cref="GetFirstNonEmptyStatementOrDefault"/> for why the skip is needed.
+		/// </remarks>
+		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;

ICSharpCode.Decompiler/CSharp/Transforms/FlattenSwitchBlocks.cs

@@ -33,10 +33,11 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 		{
 			foreach (var switchSection in rootNode.Descendants.OfType<SwitchSection>())
 			{
-				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;
 

ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs

@@ -197,7 +197,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 			if (!m1.Success)
 				return null;
 			var variable = m1.Get<IdentifierExpression>("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<IdentifierExpression>("upperBoundVariable").Single().GetILVariable();
@@ -654,7 +654,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 				if (!int.TryParse(m.Get<PrimitiveExpression>("index").Single().Value?.ToString() ?? "", out int index) || index != i)
 					break;
 				upperBounds[i] = m.Get<IdentifierExpression>("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 {
+		static readonly TryCatchStatement destructorTryFinallyPattern = new TryCatchStatement {
-			new TryCatchStatement {
+			TryBlock = new AnyNode("body"),
-				TryBlock = new AnyNode("body"),
+			FinallyBlock = new AnyNode("finallyBlock")
-				FinallyBlock = new BlockStatement {
-					new InvocationExpression(new MemberReferenceExpression(new BaseReferenceExpression(), "Finalize"))
-				}
-			}
 		};
 
+		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()
 		};
 
+		/// <summary>
+		/// Matches the body a compiler emits for a destructor - a single try statement whose
+		/// finally block does nothing but call <c>base.Finalize()</c> - and returns the try block
+		/// holding the user-written code, or <c>null</c> if <paramref name="body"/> 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).
+		/// </summary>
+		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<BlockStatement>("finallyBlock").Single()
+				.Statements.GetFirstNonEmptyStatementOrDefault();
+			if (finalizeCall == null || finalizeCall.GetNextNonEmptyStatement() != null
+				|| !baseFinalizeCallPattern.IsMatch(finalizeCall))
+			{
+				return null;
+			}
+			return m.Get<BlockStatement>("body").Single();
+		}
+
+		/// <summary>
+		/// Moves the comment placeholders of <paramref name="oldBody"/> to the front of
+		/// <paramref name="newBody"/>, which replaces it. They describe the member, so dropping
+		/// them with the body they happen to sit in would lose a decompiler warning.
+		/// </summary>
+		static void MovePlaceholderComments(BlockStatement oldBody, BlockStatement newBody)
+		{
+			var anchor = newBody.Statements.FirstOrNull();
+			foreach (var placeholder in oldBody.Statements.OfType<EmptyStatement>().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<BlockStatement>("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 (dtorDef.Body is BlockStatement oldBody
-			if (m.Success)
+				&& MatchDestructorBody(oldBody) is BlockStatement tryBlock)
 			{
 				context.Step("Simplify destructor body", dtorDef);
-				dtorDef.Body = m.Get<BlockStatement>("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))

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