From 9bff4ad9564ccbdbc665a2079da38427f3783f27 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Mon, 27 Jul 2026 06:13:57 +0200 Subject: [PATCH] Fix #1603: recognize foreach over enumerators that cannot be disposable The foreach pattern was only matched against using instructions, but the compiler emits no using/try-finally at all when the enumerator's static type can never require disposal: a struct or a sealed class that does not implement IDisposable (SerializationInfoEnumerator in the issue's example). Such loops stayed while loops. Recognize the bare 'enumerator = x.GetEnumerator(); while (enumerator.MoveNext())' shape during statement building and reuse the existing foreach transformation core for it. The transformation is restricted to exactly the cases where recompilation would produce the same IL: the enumerator type rules above (ref structs are excluded because of pattern-based disposal), a single-store enumerator variable unused outside the loop, and synchronous enumeration only, since async enumerators are always IAsyncDisposable. Assisted-by: Claude:claude-fable-5:Claude Code --- .../TestCases/Pretty/Loops.cs | 80 +++++++++--- .../CSharp/StatementBuilder.cs | 123 +++++++++++++++--- 2 files changed, 166 insertions(+), 37 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs index 290a5f598..a0038f90f 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs @@ -20,6 +20,7 @@ using System; using System.Collections; using System.Collections.Generic; using System.Runtime.InteropServices; +using System.Runtime.Serialization; using System.Text; namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty @@ -76,6 +77,30 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty } } + public sealed class CustomSealedClassEnumerator + { + public object Current { + get { + throw new NotImplementedException(); + } + } + + public bool MoveNext() + { + throw new NotImplementedException(); + } + + public void Reset() + { + throw new NotImplementedException(); + } + + public CustomSealedClassEnumerator GetEnumerator() + { + return this; + } + } + public class CustomClassEnumerator { public T Current { @@ -360,15 +385,34 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty } } - // TODO : Needs additional pattern detection - // CustomStructEnumerator does not implement IDisposable - // No try-finally-Dispose is generated. - //public void ForEachOnCustomStructEnumerator(CustomStructEnumerator e) - //{ - // foreach (object item in e) { - // Console.WriteLine(item); - // } - //} + // CustomStructEnumerator does not implement IDisposable, + // so no try-finally-Dispose is generated around the loop. + public void ForEachOnCustomStructEnumerator(CustomStructEnumerator e) + { + foreach (object item in e) + { + Console.WriteLine(item); + } + } + + // CustomSealedClassEnumerator is sealed and does not implement IDisposable, + // so no try-finally-Dispose is generated around the loop. + public void ForEachOnCustomSealedClassEnumerator(CustomSealedClassEnumerator e) + { + foreach (object item in e) + { + Console.WriteLine(item); + } + } + + // SerializationInfoEnumerator is a sealed class and does not implement IDisposable. + public void Issue1603(SerializationInfo info) + { + foreach (SerializationEntry item in info) + { + Console.WriteLine(item.Name); + } + } public void ForEachOnGenericCustomClassEnumerator(CustomClassEnumerator e) { @@ -378,15 +422,15 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty } } - // TODO : Needs additional pattern detection - // CustomStructEnumerator does not implement IDisposable - // No try-finally-Dispose is generated. - //public void ForEachOnGenericCustomStructEnumerator(CustomStructEnumerator e) - //{ - // foreach (T item in e) { - // Console.WriteLine(item); - // } - //} + // CustomStructEnumerator does not implement IDisposable, + // so no try-finally-Dispose is generated around the loop. + public void ForEachOnGenericCustomStructEnumerator(CustomStructEnumerator e) + { + foreach (T item in e) + { + Console.WriteLine(item); + } + } public void ForEachOnCustomClassEnumeratorWithIDisposable(CustomClassEnumeratorWithIDisposable e) { diff --git a/ICSharpCode.Decompiler/CSharp/StatementBuilder.cs b/ICSharpCode.Decompiler/CSharp/StatementBuilder.cs index 5686389b3..d7cb0c3a5 100644 --- a/ICSharpCode.Decompiler/CSharp/StatementBuilder.cs +++ b/ICSharpCode.Decompiler/CSharp/StatementBuilder.cs @@ -606,49 +606,128 @@ namespace ICSharpCode.Decompiler.CSharp } } - Statement? TransformToForeach(UsingInstruction inst, Expression resource) + bool MatchGetEnumeratorPattern(Expression resource, out Match m, out bool isAsync) { - if (!settings.ForEachStatement) - { - return null; - } - Match m; + isAsync = false; if (settings.ExtensionMethods && settings.ForEachWithGetEnumeratorExtension) { - // Check if the using resource matches the GetEnumerator pattern ... + // Check if the resource expression matches the GetEnumerator pattern ... m = getEnumeratorPattern.Match(resource); if (!m.Success) { // ... or the extension GetEnumeratorPattern. m = extensionGetEnumeratorPattern.Match(resource); if (!m.Success) - return null; + return false; // Validate that the invocation is an extension method invocation. if (!(resource.GetSymbol() is IMethod method && exprBuilder.resolver.CanTransformToExtensionMethodCall(method, true))) { - return null; + return false; } } } else { - // Check if the using resource matches the GetEnumerator pattern. + // Check if the resource expression matches the GetEnumerator pattern. m = getEnumeratorPattern.Match(resource); if (!m.Success) - return null; + return false; } + isAsync = ((MemberReferenceExpression)((InvocationExpression)resource).Target).MemberName == "GetAsyncEnumerator"; + return true; + } + + Statement? TransformToForeach(UsingInstruction inst, Expression resource) + { + if (!settings.ForEachStatement) + { + return null; + } + if (!MatchGetEnumeratorPattern(resource, out Match m, out bool isAsync)) + return null; // The using body must be a BlockContainer. if (!(inst.Body is BlockContainer container)) return null; - bool isAsync = ((MemberReferenceExpression)((InvocationExpression)resource).Target).MemberName == "GetAsyncEnumerator"; if (isAsync != inst.IsAsync) return null; - // The using-variable is the enumerator. - var enumeratorVar = inst.Variable; // If there's another BlockContainer nested in this container and it only has one child block, unwrap it. // If there's an extra leave inside the block, extract it into optionalReturnAfterLoop. var loopContainer = UnwrapNestedContainerIfPossible(container, out var optionalLeaveAfterLoop); + // The using-variable is the enumerator. + return TransformToForeach(container, loopContainer, optionalLeaveAfterLoop, inst.Variable, isAsync, m, inst.ResourceExpression); + } + + /// + /// Transforms the bare pattern 'enumerator = collection.GetEnumerator(); while (enumerator.MoveNext()) { ... }' + /// (without any using/try-finally) into a foreach statement. The compiler emits this shape when the + /// enumerator type can never require disposal: a struct or a sealed class that does not implement + /// IDisposable. + /// The pattern spans two consecutive instructions; on success, is advanced + /// past the loop container. + /// + Statement? TransformToForeachWithoutDispose(Block block, ref int i) + { + if (!(block.Instructions[i] is StLoc storeInst + && i + 1 < block.Instructions.Count + && block.Instructions[i + 1] is BlockContainer { Kind: ContainerKind.While } loopContainer)) + { + return null; + } + var transformed = TransformToForeachWithoutDispose(storeInst, loopContainer); + if (transformed == null) + return null; + i++; + return transformed.WithILInstruction(loopContainer); + } + + Statement? TransformToForeachWithoutDispose(StLoc storeInst, BlockContainer loopContainer) + { + if (!settings.ForEachStatement) + return null; + var enumeratorVar = storeInst.Variable; + if (!(enumeratorVar.Kind == VariableKind.Local || enumeratorVar.Kind == VariableKind.StackSlot)) + return null; + if (!EnumeratorTypeCanNeverBeDisposable(enumeratorVar.Type)) + return null; + // The enumerator variable must not be used outside of the loop. + if (!VariableIsOnlyUsedInBlock(storeInst, loopContainer, loopContainer)) + return null; + var resource = exprBuilder.Translate(storeInst.Value).Expression; + if (!MatchGetEnumeratorPattern(resource, out Match m, out bool isAsync)) + return null; + // Async enumerators always implement IAsyncDisposable, so they cannot occur in this pattern. + if (isAsync) + return null; + return TransformToForeach(loopContainer, loopContainer, null, enumeratorVar, isAsync: false, m, storeInst.Value); + } + + bool EnumeratorTypeCanNeverBeDisposable(IType type) + { + ITypeDefinition? typeDef = type.GetDefinition(); + if (typeDef == null) + return false; + switch (typeDef.Kind) + { + case TypeKind.Struct: + // A ref struct may use pattern-based disposal via a Dispose method. + if (typeDef.IsByRefLike) + return false; + break; + case TypeKind.Class: + // A non-sealed class would be enumerated with a + // 'finally { (enumerator as IDisposable)?.Dispose(); }' block instead. + if (!typeDef.IsSealed) + return false; + break; + default: + return false; + } + return !type.GetAllBaseTypes().Any(t => t.IsKnownType(KnownTypeCode.IDisposable)); + } + + Statement? TransformToForeach(BlockContainer container, BlockContainer loopContainer, Leave? optionalLeaveAfterLoop, ILVariable enumeratorVar, bool isAsync, Match m, ILInstruction resourceExpression) + { // Detect whether we're dealing with a while loop with multiple embedded statements. if (loopContainer.Kind != ContainerKind.While) return null; @@ -782,7 +861,7 @@ namespace ICSharpCode.Decompiler.CSharp InExpression = collectionExpr.Detach(), EmbeddedStatement = foreachBody }; - foreachStmt.AddAnnotation(new ForeachAnnotation(inst.ResourceExpression, conditionInst, singleGetter)); + foreachStmt.AddAnnotation(new ForeachAnnotation(resourceExpression, conditionInst, singleGetter)); foreachStmt.CopyAnnotationsFrom(whileLoop); // If there was an optional return statement, return it as well. // If there were labels or any other statements in the whileLoopBlock, move them after the foreach @@ -1204,9 +1283,14 @@ namespace ICSharpCode.Decompiler.CSharp return Default(block); // Block without container BlockStatement blockStatement = new BlockStatement(); - foreach (var inst in block.Instructions) + for (int i = 0; i < block.Instructions.Count; i++) { - blockStatement.Add(Convert(inst)); + if (TransformToForeachWithoutDispose(block, ref i) is Statement foreachStmt) + { + blockStatement.Add(foreachStmt); + continue; + } + blockStatement.Add(Convert(block.Instructions[i])); } if (block.FinalInstruction.OpCode != OpCode.Nop) blockStatement.Add(Convert(block.FinalInstruction)); @@ -1451,15 +1535,16 @@ namespace ICSharpCode.Decompiler.CSharp // If there are any incoming branches to this block, add a label: blockStatement.Add(new LabelStatement { Label = EnsureUniqueLabel(block) }); } - foreach (var inst in block.Instructions) + for (int i = 0; i < block.Instructions.Count; i++) { + var inst = block.Instructions[i]; if (!isLoop && inst is Leave leave && IsFinalLeave(leave)) { // skip the final 'leave' instruction and just fall out of the BlockStatement blockStatement.AddAnnotation(new ImplicitReturnAnnotation(leave)); continue; } - var stmt = Convert(inst); + Statement stmt = TransformToForeachWithoutDispose(block, ref i) ?? Convert(inst); if (stmt is BlockStatement b) { foreach (var nested in b.Statements)