From b0cb97635730ec543386cdc05f570e0cacd14480 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Sun, 17 Mar 2019 19:13:57 +0100 Subject: [PATCH] Fix #1454: Name collision after converting for-over-array to foreach. --- .../ICSharpCode.Decompiler.Tests.csproj | 1 + .../ILPrettyTestRunner.cs | 6 +++ .../CSharp/Transforms/DeclareVariables.cs | 11 +++++ .../Transforms/PatternStatementTransform.cs | 49 ++++++++++++++++++- 4 files changed, 66 insertions(+), 1 deletion(-) diff --git a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj index 2ec6fcc24..a6677613d 100644 --- a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj +++ b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj @@ -72,6 +72,7 @@ + diff --git a/ICSharpCode.Decompiler.Tests/ILPrettyTestRunner.cs b/ICSharpCode.Decompiler.Tests/ILPrettyTestRunner.cs index 6bc796546..9629c8a0d 100644 --- a/ICSharpCode.Decompiler.Tests/ILPrettyTestRunner.cs +++ b/ICSharpCode.Decompiler.Tests/ILPrettyTestRunner.cs @@ -142,6 +142,12 @@ namespace ICSharpCode.Decompiler.Tests Run(); } + [Test] + public void Issue1454() + { + Run(); + } + [Test] public void SequenceOfNestedIfs() { diff --git a/ICSharpCode.Decompiler/CSharp/Transforms/DeclareVariables.cs b/ICSharpCode.Decompiler/CSharp/Transforms/DeclareVariables.cs index 6d0a8c3d9..4b2051cc9 100644 --- a/ICSharpCode.Decompiler/CSharp/Transforms/DeclareVariables.cs +++ b/ICSharpCode.Decompiler/CSharp/Transforms/DeclareVariables.cs @@ -101,6 +101,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms public IdentifierExpression FirstUse; public VariableToDeclare ReplacementDueToCollision; + public bool InvolvedInCollision; public bool RemovedDueToCollision => ReplacementDueToCollision != null; public VariableToDeclare(ILVariable variable, bool defaultInitialization, InsertionPoint insertionPoint, IdentifierExpression firstUse, int sourceOrder) @@ -158,6 +159,15 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms return v.InsertionPoint.nextNode; } + /// + /// Determines whether a variable was merged with other variables. + /// + public bool WasMerged(ILVariable variable) + { + VariableToDeclare v = variableDict[variable]; + return v.InvolvedInCollision || v.RemovedDueToCollision; + } + public void ClearAnalysisResults() { variableDict.Clear(); @@ -374,6 +384,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms Debug.Assert(point1.level == point2.level); if (point1.nextNode.Parent == point2.nextNode.Parent) { // We found a collision! + v.InvolvedInCollision = true; prev.ReplacementDueToCollision = v; // Continue checking other entries in multiDict against the new position of `v`. if (prev.SourceOrder < v.SourceOrder) { diff --git a/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs b/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs index 19eafb673..7d23edfa8 100644 --- a/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs +++ b/ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs @@ -251,6 +251,53 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms } }; + bool VariableCanBeUsedAsForeachLocal(IL.ILVariable itemVar, Statement loop) + { + if (itemVar == null || !(itemVar.Kind == IL.VariableKind.Local || itemVar.Kind == IL.VariableKind.StackSlot)) { + // only locals/temporaries can be converted into foreach loop variable + return false; + } + + var blockContainer = loop.Annotation(); + + if (!itemVar.IsSingleDefinition) { + // foreach variable cannot be assigned to. + // As a special case, we accept taking the address for a method call, + // but only if the call is the only use, so that any mutation by the call + // cannot be observed. + if (!AddressUsedForSingleCall(itemVar, blockContainer)) { + return false; + } + } + + if (itemVar.CaptureScope != null && itemVar.CaptureScope != blockContainer) { + // captured variables cannot be declared in the loop unless the loop is their capture scope + return false; + } + + AstNode declPoint = declareVariables.GetDeclarationPoint(itemVar); + return declPoint.Ancestors.Contains(loop) && !declareVariables.WasMerged(itemVar); + } + + static bool AddressUsedForSingleCall(IL.ILVariable v, IL.BlockContainer loop) + { + if (v.StoreCount == 1 && v.AddressCount == 1 && v.LoadCount == 0 && v.Type.IsReferenceType == false) { + if (v.AddressInstructions[0].Parent is IL.Call call + && v.AddressInstructions[0].ChildIndex == 0 + && !call.Method.IsStatic) { + // used as this pointer for a method call + // this is OK iff the call is not within a nested loop + for (var node = call.Parent; node != null; node = node.Parent) { + if (node == loop) + return true; + else if (node is IL.BlockContainer) + break; + } + } + } + return false; + } + Statement TransformForeachOnArray(ForStatement forStatement) { if (!context.Settings.ForEachStatement) return null; @@ -262,7 +309,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms var loopContainer = forStatement.Annotation(); if (itemVariable == null || indexVariable == null || arrayVariable == null) return null; - if (!itemVariable.IsSingleDefinition || (itemVariable.CaptureScope != null && itemVariable.CaptureScope != loopContainer)) + if (!VariableCanBeUsedAsForeachLocal(itemVariable, forStatement)) return null; if (indexVariable.StoreCount != 2 || indexVariable.LoadCount != 3 || indexVariable.AddressCount != 0) return null;