Merge pull request #4130 from icsharpcode/fix/comment-placeholder-statements

Look past comment placeholders when matching statement sequences

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)
@@ -1019,6 +1019,12 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 			return td.HasFlag(System.Reflection.TypeAttributes.BeforeFieldInit);
 		}
 
+		/// <summary>
+		/// Maps each backing field to whether its references outside the owning property's
+		/// accessors can still be expressed once the field declaration is gone: <c>true</c> for
+		/// rescuable constructor stores only, <c>false</c> for anything else. A field absent from
+		/// the map has no outside references at all and is therefore also expressible.
+		/// </summary>
 		Dictionary<IField, bool> BuildOutsideReferenceIndex(AstNode root)
 		{
 			var verdicts = new Dictionary<IField, bool>();
@@ -1190,10 +1196,12 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 		}
 
 		/// <summary>
-		/// True when <paramref name="node"/> is the left-hand side of a plain assignment to
+		/// True when <paramref name="node"/> is a target of a plain assignment to
 		/// <paramref name="field"/> inside a constructor of the field's declaring type - the
 		/// only outside reference the "field" keyword can still express (as a property
-		/// initializer, or an assignment to a setter-less property).
+		/// initializer, or an assignment to a setter-less property). A deconstruction target
+		/// counts only in the second form: it assigns several members at once, so it can never
+		/// move into an initializer, and a property that kept a setter would invoke it.
 		/// </summary>
 		/// <remarks>
 		/// Shared by <see cref="OutsideReferencesAreExpressible"/>, which decides whether the
@@ -1205,8 +1213,25 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
 		/// </remarks>
 		static bool IsConstructorStore(AstNode node, IField field, IMethod? enclosingMethod)
 		{
-			if (node.Parent is not AssignmentExpression { Operator: AssignmentOperatorType.Assign } assignment
-				|| assignment.Left != node)
+			AstNode currentNode = node;
+			bool viaDeconstruction = false;
+			while (true)
+			{
+				if (currentNode.Parent is AssignmentExpression { Operator: AssignmentOperatorType.Assign }
+					&& currentNode.Slot == AssignmentExpression.LeftSlot)
+				{
+					break;
+				}
+				if (currentNode.Parent is TupleExpression)
+				{
+					viaDeconstruction = true;
+					currentNode = currentNode.Parent;
+					continue;
+				}
+				return false;
+			}
+			if (viaDeconstruction
+				&& (!IsBackingFieldOfAutomaticProperty(field, out var property) || property.CanSet))
 			{
 				return false;
 			}
@@ -1236,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()
 		};
 
+		/// <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;
@@ -1276,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<BlockStatement>("body").Single().Detach();
+				MovePlaceholderComments(oldBody, tryBlock);
+				dtorDef.Body = tryBlock.Detach();
 				return dtorDef;
 			}
 			return null;
@@ -1430,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