pimhwang
/
ILSpy
mirror of https://github.com/icsharpcode/ILSpy.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

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
pull/3925/head
Siegfried Pammer 2 months ago committed by Siegfried Pammer
parent
132b33e530
commit
9bff4ad956
2 changed files with 166 additions and 37 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 80
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs
  2. 123
      ICSharpCode.Decompiler/CSharp/StatementBuilder.cs

80
ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs
Unescape View File

@ -20,6 +20,7 @@ using System; @@ -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 @@ -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<T>
{
public T Current {
@ -360,15 +385,34 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty @@ -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<T>(CustomClassEnumerator<T> e)
{
@ -378,15 +422,15 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty @@ -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<T>(CustomStructEnumerator<T> e)
//{
// foreach (T item in e) {
// Console.WriteLine(item);
// }
//}
// CustomStructEnumerator<T> does not implement IDisposable,
// so no try-finally-Dispose is generated around the loop.
public void ForEachOnGenericCustomStructEnumerator<T>(CustomStructEnumerator<T> e)
{
foreach (T item in e)
{
Console.WriteLine(item);
}
}
public void ForEachOnCustomClassEnumeratorWithIDisposable(CustomClassEnumeratorWithIDisposable e)
{

123
ICSharpCode.Decompiler/CSharp/StatementBuilder.cs
Unescape View File

@ -606,49 +606,128 @@ namespace ICSharpCode.Decompiler.CSharp @@ -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);
}
/// <summary>
/// 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, <paramref name="i"/> is advanced
/// past the loop container.
/// </summary>
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 @@ -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 @@ -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 @@ -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)

Write Preview
Loading…
Cancel
Save