Merge PR icsharpcode#2425: Keep return statements around in original …

…form for ConditionDetection

ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExceptionHandling.cs

@@ -196,6 +196,28 @@ public void ThrowInFinally()
 			}
 		}
 
+		internal void EarlyReturnInTryBlock(bool a, bool b)
+		{
+			try
+			{
+				if (a)
+				{
+					Console.WriteLine("a");
+				}
+				else if (b)
+				{
+					// #2379: The only goto-free way of representing this code is to use a return statement
+					return;
+				}
+
+				Console.WriteLine("a || !b");
+			}
+			finally
+			{
+				Console.WriteLine("finally");
+			}
+		}
+
 #if ROSLYN || !OPT
 		// TODO Non-Roslyn compilers create a second while loop inside the try, by inverting the if
 		// This is fixed in the non-optimised version by the enabling the RemoveDeadCode flag

ICSharpCode.Decompiler.Tests/TestCases/Pretty/Loops.cs

@@ -906,5 +906,26 @@ public void MergeAroundContinue()
 			}
 			Console.WriteLine("end");
 		}
+
+		public void ForEachInSwitch(int i, IEnumerable<string> args)
+		{
+			switch (i)
+			{
+
+				case 1:
+					Console.WriteLine("one");
+					break;
+				case 2:
+				{
+					foreach (string arg in args)
+					{
+						Console.WriteLine(arg);
+					}
+					break;
+				}
+				default:
+					throw new NotImplementedException();
+			}
+		}
 	}
 }

ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs

@@ -174,7 +174,6 @@ public void NestedSwitchIf()
 						Console.WriteLine("case 1");
 						return;
 				}
-
 				if (B(1)) {
 					Console.WriteLine(1);
 				}

ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs

@@ -86,7 +86,7 @@ public static List<IILTransform> GetILTransforms()
 				new YieldReturnDecompiler(), // must run after inlining but before loop detection
 				new AsyncAwaitDecompiler(),  // must run after inlining but before loop detection
 				new DetectCatchWhenConditionBlocks(), // must run after inlining but before loop detection
-				new DetectExitPoints(canIntroduceExitForReturn: false),
+				new DetectExitPoints(),
 				new LdLocaDupInitObjTransform(),
 				new EarlyExpressionTransforms(),
 				// RemoveDeadVariableInit must run after EarlyExpressionTransforms so that stobj(ldloca V, ...)
@@ -111,7 +111,7 @@ public static List<IILTransform> GetILTransforms()
 					}
 				},
 				// re-run DetectExitPoints after loop detection
-				new DetectExitPoints(canIntroduceExitForReturn: true),
+				new DetectExitPoints(),
 				new BlockILTransform { // per-block transforms
 					PostOrderTransforms = {
 						new ConditionDetection(),
@@ -153,6 +153,7 @@ public static List<IILTransform> GetILTransforms()
 				new TransformDisplayClassUsage(),
 				new HighLevelLoopTransform(),
 				new ReduceNestingTransform(),
+				new RemoveRedundantReturn(),
 				new IntroduceDynamicTypeOnLocals(),
 				new IntroduceNativeIntTypeOnLocals(),
 				new AssignVariableNames(),

ICSharpCode.Decompiler/CSharp/StatementBuilder.cs

@@ -583,7 +583,7 @@ Statement TransformToForeach(UsingInstruction inst, Expression resource)
 			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 optionalReturnAfterLoop);
+			var loopContainer = UnwrapNestedContainerIfPossible(container, out var optionalLeaveAfterLoop);
 			// Detect whether we're dealing with a while loop with multiple embedded statements.
 			if (loopContainer.Kind != ContainerKind.While)
 				return null;
@@ -713,22 +713,22 @@ Statement TransformToForeach(UsingInstruction inst, Expression resource)
 			// 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
 			// loop.
-			if (optionalReturnAfterLoop != null || whileLoopBlock.Statements.Count > 1)
+			if (optionalLeaveAfterLoop != null || whileLoopBlock.Statements.Count > 1)
 			{
 				var block = new BlockStatement {
 					Statements = {
 						foreachStmt
 					}
 				};
-				if (optionalReturnAfterLoop != null)
+				if (optionalLeaveAfterLoop != null)
 				{
-					block.Statements.Add(optionalReturnAfterLoop.AcceptVisitor(this));
+					block.Statements.Add(optionalLeaveAfterLoop.AcceptVisitor(this));
 				}
 				if (whileLoopBlock.Statements.Count > 1)
 				{
 					block.Statements.AddRange(whileLoopBlock.Statements
 						.Skip(1)
-						.SkipWhile(s => s.Annotations.Any(a => a == optionalReturnAfterLoop))
+						.SkipWhile(s => s.Annotations.Any(a => a == optionalLeaveAfterLoop))
 						.Select(SyntaxExtensions.Detach));
 				}
 				return block;
@@ -794,10 +794,10 @@ private bool IsDynamicCastToIEnumerable(Expression expr, out Expression dynamicE
 		/// if the value has no side-effects.
 		/// Otherwise returns the unmodified container.
 		/// </summary>
-		/// <param name="optionalReturnInst">If the leave is a return and has no side-effects, we can move the return out of the using-block and put it after the loop, otherwise returns null.</param>
-		BlockContainer UnwrapNestedContainerIfPossible(BlockContainer container, out Leave optionalReturnInst)
+		/// <param name="optionalLeaveInst">If the leave is a return/break and has no side-effects, we can move the return out of the using-block and put it after the loop, otherwise returns null.</param>
+		BlockContainer UnwrapNestedContainerIfPossible(BlockContainer container, out Leave optionalLeaveInst)
 		{
-			optionalReturnInst = null;
+			optionalLeaveInst = null;
 			// Check block structure:
 			if (container.Blocks.Count != 1)
 				return container;
@@ -809,10 +809,11 @@ BlockContainer UnwrapNestedContainerIfPossible(BlockContainer container, out Lea
 			// If the leave has no value, just unwrap the BlockContainer.
 			if (leave.MatchLeave(container))
 				return nestedContainer;
-			// If the leave is a return, we can move the return out of the using-block and put it after the loop
+			// If the leave is a return/break, we can move it out of the using-block and put it after the loop
 			// (but only if the value doesn't have side-effects)
-			if (leave.IsLeavingFunction && SemanticHelper.IsPure(leave.Value.Flags)) {
-				optionalReturnInst = leave;
+			if (SemanticHelper.IsPure(leave.Value.Flags))
+			{
+				optionalLeaveInst = leave;
 				return nestedContainer;
 			}
 			return container;

ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj

@@ -64,6 +64,7 @@
     <Compile Include="CSharp\Syntax\VariableDesignation.cs" />
     <Compile Include="Humanizer\Vocabularies.cs" />
     <Compile Include="Humanizer\Vocabulary.cs" />
+    <Compile Include="IL\ControlFlow\RemoveRedundantReturn.cs" />
     <Compile Include="IL\Transforms\DeconstructionTransform.cs" />
     <Compile Include="IL\Transforms\FixLoneIsInst.cs" />
     <Compile Include="IL\Instructions\DeconstructInstruction.cs" />

ICSharpCode.Decompiler/IL/ControlFlow/ConditionDetection.cs

@@ -16,7 +16,6 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
 // DEALINGS IN THE SOFTWARE.
 
-using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
@@ -352,7 +351,6 @@ internal static void InvertIf(Block block, IfInstruction ifInst, ILTransformCont
 			Debug.Assert(ifInst.Parent == block);
 
 			//assert then block terminates
-			var trueExitInst = GetExit(ifInst.TrueInst);
 			var exitInst = GetExit(block);
 			context.Step($"InvertIf at IL_{ifInst.StartILOffset:x4}", ifInst);