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

Improve ReduceNestingTransform by considering nested containers (Try/Using/Lock/Pinned/etc)

pull/1880/head
Chicken-Bones 6 years ago
parent
60490a4369
commit
22243de7b0
3 changed files with 296 additions and 42 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Unified View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 14
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExpressionTrees.cs
  2. 135
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs
  3. 189
      ICSharpCode.Decompiler/IL/Transforms/ReduceNestingTransform.cs

14
ICSharpCode.Decompiler.Tests/TestCases/Pretty/ExpressionTrees.cs
Unescape View File

@ -1037,17 +1037,15 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
public async Task Issue1524(string str) public async Task Issue1524(string str)
{ {
await Task.Delay(100); await Task.Delay(100);
if (string.IsNullOrEmpty(str)) {
#if CS70 #if CS70
if (int.TryParse(str, out int id)) { if (string.IsNullOrEmpty(str) && int.TryParse(str, out int id)) {
#else #else
int id; int id;
if (int.TryParse(str, out id)) { if (string.IsNullOrEmpty(str) && int.TryParse(str, out id)) {
#endif #endif
(from a in new List<int>().AsQueryable() (from a in new List<int>().AsQueryable()
where a == id where a == id
select a).FirstOrDefault(); select a).FirstOrDefault();
}
} }
} }

135
ICSharpCode.Decompiler.Tests/TestCases/Pretty/ReduceNesting.cs
Unescape View File

@ -194,7 +194,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
} }
} }
// nesting should not be reduced as maximum nesting level is 2 // nesting should be reduced as maximum nesting level is 2
public void EarlyExit2() public void EarlyExit2()
{ {
if (B(0)) { if (B(0)) {
@ -266,5 +266,138 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
} }
return s; return s;
} }
public void EarlyExitBeforeTry()
{
if (B(0)) {
return;
}
try {
if (B(1)) {
Console.WriteLine();
}
} catch {
}
}
public void EarlyExitInTry()
{
try {
if (B(0)) {
return;
}
Console.WriteLine();
if (B(1)) {
for (int i = 0; i < 10; i++) {
Console.WriteLine(i);
}
}
} catch {
}
}
public void ContinueLockInLoop()
{
while (B(0)) {
lock (Console.Out) {
if (B(1)) {
continue;
}
Console.WriteLine();
if (B(2)) {
for (int i = 0; i < 10; i++) {
Console.WriteLine(i);
}
}
}
}
}
public void BreakLockInLoop()
{
while (B(0)) {
lock (Console.Out) {
// Before ReduceNestingTransform, the rest of the lock body is nested in if(!B(1)) with a continue;
// the B(1) case falls through to a break outside the lock
if (B(1)) {
break;
}
Console.WriteLine();
if (B(2)) {
for (int i = 0; i < 10; i++) {
Console.WriteLine(i);
}
}
// the break gets duplicated into the lock (replacing the leave) making the lock 'endpoint unreachable' and the break outside the lock is removed
// After the condition is inverted, ReduceNestingTransform isn't smart enough to then move the continue out of the lock
// Thus the redundant continue;
continue;
}
}
Console.WriteLine();
}
public unsafe void BreakPinnedInLoop(int[] arr)
{
while (B(0)) {
fixed (int* ptr = arr) {
if (B(1)) {
break;
}
Console.WriteLine();
if (B(2)) {
for (int i = 0; i < 10; i++) {
Console.WriteLine(ptr[i]);
}
}
// Same reason as BreakLockInLoop
continue;
}
}
Console.WriteLine();
}
public void CannotEarlyExitInTry()
{
try {
if (B(0)) {
Console.WriteLine();
if (B(1)) {
for (int i = 0; i < 10; i++) {
Console.WriteLine(i);
}
}
}
} catch {
}
Console.WriteLine();
}
public void EndpointUnreachableDueToEarlyExit()
{
using (Console.Out) {
if (B(0)) {
return;
}
do {
if (B(1)) {
return;
}
} while (B(2));
throw new Exception();
}
}
} }
} }

189
ICSharpCode.Decompiler/IL/Transforms/ReduceNestingTransform.cs
Unescape View File

@ -33,7 +33,7 @@ namespace ICSharpCode.Decompiler.IL
/// This can lead to excessive indentation when the entire rest of the method/loop is included in the else block/default case. /// This can lead to excessive indentation when the entire rest of the method/loop is included in the else block/default case.
/// When an If/SwitchInstruction is followed immediately by a keyword exit, the exit can be moved into the child blocks /// When an If/SwitchInstruction is followed immediately by a keyword exit, the exit can be moved into the child blocks
/// allowing the else block or default case to be moved after the if/switch as all prior cases exit. /// allowing the else block or default case to be moved after the if/switch as all prior cases exit.
/// Most importantly, this transformatino does not change the IL order of any code. /// Most importantly, this transformation does not change the IL order of any code.
/// ///
/// ConditionDetection also has a block exit priority system to assist exit point reduction which in some cases ignores IL order. /// ConditionDetection also has a block exit priority system to assist exit point reduction which in some cases ignores IL order.
/// After HighLevelLoopTransform has run, all structures have been detected and preference can be returned to maintaining IL ordering. /// After HighLevelLoopTransform has run, all structures have been detected and preference can be returned to maintaining IL ordering.
@ -99,16 +99,16 @@ namespace ICSharpCode.Decompiler.IL
// reduce nesting in switch blocks // reduce nesting in switch blocks
if (container.Kind == ContainerKind.Switch && if (container.Kind == ContainerKind.Switch &&
CanDuplicateExit(NextInsn(), continueTarget) && CanDuplicateExit(NextInsn(), continueTarget, out var keywordExit1) &&
ReduceSwitchNesting(block, container, NextInsn())) { ReduceSwitchNesting(block, container, keywordExit1)) {
RemoveRedundantExit(block, nextInstruction); RemoveRedundantExit(block, nextInstruction);
} }
break; break;
case IfInstruction ifInst: case IfInstruction ifInst:
ImproveILOrdering(block, ifInst); ImproveILOrdering(block, ifInst, continueTarget);
// reduce nesting in if/else blocks // reduce nesting in if/else blocks
if (CanDuplicateExit(NextInsn(), continueTarget) && ReduceNesting(block, ifInst, NextInsn())) if (CanDuplicateExit(NextInsn(), continueTarget, out var keywordExit2) && ReduceNesting(block, ifInst, keywordExit2))
RemoveRedundantExit(block, nextInstruction); RemoveRedundantExit(block, nextInstruction);
// visit content blocks // visit content blocks
@ -124,27 +124,69 @@ namespace ICSharpCode.Decompiler.IL
Visit(falseBlock, continueTarget, NextInsn()); Visit(falseBlock, continueTarget, NextInsn());
} }
break; break;
default:
// blocks can only exit containers via leave instructions, not fallthrough, so the only relevant context is `continueTarget`
VisitContainers(inst, continueTarget);
// reducing nesting inside Try/Using/Lock etc, may make the endpoint unreachable.
// This should only happen by replacing a Leave with the exit instruction we're about to delete, but I can't see a good way to assert this
// This would be better placed in ReduceNesting, but it's more difficult to find the affected instructions/blocks there than here
if (i == block.Instructions.Count - 2 && inst.HasFlag(InstructionFlags.EndPointUnreachable)) {
context.Step("Remove unreachable exit", block.Instructions.Last());
block.Instructions.RemoveLast();
// This would be the right place to check and fix the redundant continue; in TestCases.Pretty.ReduceNesting.BreakLockInLoop
// but doing so would require knowledge of what `inst` is, and how it works. (eg. to target the try block and not catch or finally blocks)
}
break;
} }
} }
} }
// search for child containers to reduce nesting in
private void VisitContainers(ILInstruction inst, Block continueTarget)
{
switch (inst) {
case ILFunction _:
break; // assume inline ILFunctions are already transformed
case BlockContainer cont:
Visit(cont, continueTarget);
break;
default:
foreach (var child in inst.Children)
VisitContainers(child, continueTarget);
break;
}
}
/// <summary> /// <summary>
/// For an if statement with an unreachable end point and no else block, /// For an if statement with an unreachable end point and no else block,
/// inverts to match IL order of the first statement of each branch /// inverts to match IL order of the first statement of each branch
/// </summary> /// </summary>
private void ImproveILOrdering(Block block, IfInstruction ifInst) private void ImproveILOrdering(Block block, IfInstruction ifInst, Block continueTarget)
{ {
if (!block.HasFlag(InstructionFlags.EndPointUnreachable) if (!block.HasFlag(InstructionFlags.EndPointUnreachable)
|| !ifInst.TrueInst.HasFlag(InstructionFlags.EndPointUnreachable) || !ifInst.TrueInst.HasFlag(InstructionFlags.EndPointUnreachable)
|| !ifInst.FalseInst.MatchNop()) || !ifInst.FalseInst.MatchNop())
return; return;
Debug.Assert(ifInst != block.Instructions.Last()); Debug.Assert(ifInst != block.Instructions.Last());
var trueRangeStart = ConditionDetection.GetStartILOffset(ifInst.TrueInst, out bool trueRangeIsEmpty); var trueRangeStart = ConditionDetection.GetStartILOffset(ifInst.TrueInst, out bool trueRangeIsEmpty);
var falseRangeStart = ConditionDetection.GetStartILOffset(block.Instructions[block.Instructions.IndexOf(ifInst)+1], out bool falseRangeIsEmpty); var falseRangeStart = ConditionDetection.GetStartILOffset(block.Instructions[block.Instructions.IndexOf(ifInst) + 1], out bool falseRangeIsEmpty);
if (!trueRangeIsEmpty && !falseRangeIsEmpty && falseRangeStart < trueRangeStart) if (trueRangeIsEmpty || falseRangeIsEmpty || falseRangeStart >= trueRangeStart)
ConditionDetection.InvertIf(block, ifInst, context); return;
if (block.Instructions.Last() is Leave leave && !leave.IsLeavingFunction && leave.TargetContainer.Kind == ContainerKind.Normal) {
// non-keyword leave. Can't move out of the last position in the block (fall-through) without introducing goto, unless it can be replaced with a keyword (return/continue)
if (!CanDuplicateExit(block.Instructions.Last(), continueTarget, out var keywordExit))
return;
context.Step("Replace leave with keyword exit", ifInst.TrueInst);
block.Instructions.Last().ReplaceWith(keywordExit.Clone());
}
ConditionDetection.InvertIf(block, ifInst, context);
} }
/// <summary> /// <summary>
@ -159,16 +201,27 @@ namespace ICSharpCode.Decompiler.IL
// if (cond) { ... } exit; // if (cond) { ... } exit;
if (ifInst.FalseInst.MatchNop()) { if (ifInst.FalseInst.MatchNop()) {
// a separate heuristic tp ShouldReduceNesting as there is visual balancing to be performed based on number of statments // a separate heuristic to ShouldReduceNesting as there is visual balancing to be performed based on number of statments
if (maxDepth < 2) if (maxDepth < 2)
return false; return false;
// -> // ->
// if (!cond) exit; // if (!cond) exit;
// ...; exit; // ...; exit;
EnsureEndPointUnreachable(ifInst.TrueInst, exitInst);
EnsureEndPointUnreachable(block, exitInst); EnsureEndPointUnreachable(block, exitInst);
Debug.Assert(ifInst == block.Instructions.SecondToLastOrDefault());
// use the same exit the block has. If the block already has one (such as a leave from a try), keep it in place
EnsureEndPointUnreachable(ifInst.TrueInst, block.Instructions.Last());
ConditionDetection.InvertIf(block, ifInst, context); ConditionDetection.InvertIf(block, ifInst, context);
// ensure the exit inst of the if instruction is a keyword
Debug.Assert(!(ifInst.TrueInst is Block));
if (!ifInst.TrueInst.Match(exitInst).Success) {
Debug.Assert(ifInst.TrueInst is Leave);
context.Step("Replace leave with keyword exit", ifInst.TrueInst);
ifInst.TrueInst.ReplaceWith(exitInst.Clone());
}
return true; return true;
} }
@ -292,8 +345,37 @@ namespace ICSharpCode.Decompiler.IL
/// <summary> /// <summary>
/// Checks if an exit is a duplicable keyword exit (return; break; continue;) /// Checks if an exit is a duplicable keyword exit (return; break; continue;)
/// </summary> /// </summary>
private bool CanDuplicateExit(ILInstruction exit, Block continueTarget) => private bool CanDuplicateExit(ILInstruction exit, Block continueTarget, out ILInstruction keywordExit)
exit != null && (exit is Leave leave && leave.Value.MatchNop() || exit.MatchBranch(continueTarget)); {
keywordExit = exit;
if (exit != null && exit.MatchBranch(continueTarget))
return true; // keyword is continue
if (!(exit is Leave leave && leave.Value.MatchNop()))
return false; // don't duplicate valued returns
if (leave.IsLeavingFunction || leave.TargetContainer.Kind != ContainerKind.Normal)
return true; // keyword is return || break
// leave from a try/pinned/lock etc, check if the target (the instruction following the target container) is duplicable, if so, set keywordExit to that
ILInstruction leavingInst = leave.TargetContainer;
Debug.Assert(!leavingInst.HasFlag(InstructionFlags.EndPointUnreachable));
while (!(leavingInst.Parent is Block b) || leavingInst == b.Instructions.Last()) {
// cannot duplicate leaves from finally containers
if (leavingInst.Parent is TryFinally tryFinally && leavingInst.SlotInfo == TryFinally.FinallyBlockSlot) {
Debug.Assert(leave.TargetContainer == tryFinally.FinallyBlock); //finally cannot have control flow
return false;
}
leavingInst = leavingInst.Parent;
Debug.Assert(!leavingInst.HasFlag(InstructionFlags.EndPointUnreachable));
Debug.Assert(!(leavingInst is ILFunction));
}
var block = (Block)leavingInst.Parent;
var targetInst = block.Instructions[block.Instructions.IndexOf(leavingInst)+1];
return CanDuplicateExit(targetInst, continueTarget, out keywordExit);
}
/// <summary> /// <summary>
/// Ensures the end point of a block is unreachable by duplicating and appending the [exit] instruction following the end point /// Ensures the end point of a block is unreachable by duplicating and appending the [exit] instruction following the end point
@ -353,28 +435,54 @@ namespace ICSharpCode.Decompiler.IL
/// <summary> /// <summary>
/// Recursively computes the number of statements and maximum nested depth of an instruction /// Recursively computes the number of statements and maximum nested depth of an instruction
/// </summary> /// </summary>
private void ComputeStats(ILInstruction inst, ref int numStatements, ref int maxDepth, int currentDepth) private void ComputeStats(ILInstruction inst, ref int numStatements, ref int maxDepth, int currentDepth, bool isStatement = true)
{ {
if (isStatement)
numStatements++;
if (currentDepth > maxDepth) {
Debug.Assert(isStatement);
maxDepth = currentDepth;
}
// enumerate children statements and containers
switch (inst) { switch (inst) {
case Block block: case Block block:
foreach (var i in block.Instructions) if (isStatement)
ComputeStats(i, ref numStatements, ref maxDepth, currentDepth); numStatements--; // don't count blocks as statements
// add each child as a statement (unless we're a named block)
foreach (var child in block.Instructions)
ComputeStats(child, ref numStatements, ref maxDepth, currentDepth, block.Kind != BlockKind.CallWithNamedArgs && block.Kind != BlockKind.CallInlineAssign);
// final instruction as an expression
ComputeStats(block.FinalInstruction, ref numStatements, ref maxDepth, currentDepth, false);
break; break;
case BlockContainer container: case BlockContainer container:
numStatements++; // one statement for the container head (switch/loop) if (!isStatement)
numStatements++; //always add a statement for a container in an expression
var containerBody = container.EntryPoint; var containerBody = container.EntryPoint;
if (container.Kind == ContainerKind.For || container.Kind == ContainerKind.While) { if (container.Kind == ContainerKind.For || container.Kind == ContainerKind.While) {
Debug.Assert(isStatement);
if (!container.MatchConditionBlock(container.EntryPoint, out _, out containerBody)) if (!container.MatchConditionBlock(container.EntryPoint, out _, out containerBody))
throw new NotSupportedException("Invalid condition block in loop."); throw new NotSupportedException("Invalid condition block in loop.");
} }
// don't count implicit leave. Can't avoid counting for loop initializers but close enough, for loops can have an extra statement of visual weight
var lastInst = containerBody.Instructions.Last();
if ((container.Kind == ContainerKind.For || container.Kind == ContainerKind.DoWhile) && lastInst.MatchBranch(container.Blocks.Last()) ||
(container.Kind == ContainerKind.Loop || container.Kind == ContainerKind.While) && lastInst.MatchBranch(container.Blocks[0]) ||
container.Kind == ContainerKind.Normal && lastInst.MatchLeave(container) ||
container.Kind == ContainerKind.Switch) // SwitchInstructyion always counts as a statement anyway, so no need to count the container as well
numStatements--;
// add the nested body // add the nested body
ComputeStats(containerBody, ref numStatements, ref maxDepth, currentDepth + 1); ComputeStats(containerBody, ref numStatements, ref maxDepth, currentDepth + 1);
break; break;
case IfInstruction ifInst: case IfInstruction ifInst when ifInst.ResultType == StackType.Void:
numStatements++; // one statement for the if/condition itself Debug.Assert(isStatement);
// nested then instruction // nested then instruction
ComputeStats(ifInst.TrueInst, ref numStatements, ref maxDepth, currentDepth + 1); ComputeStats(ifInst.TrueInst, ref numStatements, ref maxDepth, currentDepth + 1);
@ -389,21 +497,36 @@ namespace ICSharpCode.Decompiler.IL
// include all nested else instruction // include all nested else instruction
ComputeStats(elseInst, ref numStatements, ref maxDepth, currentDepth + 1); ComputeStats(elseInst, ref numStatements, ref maxDepth, currentDepth + 1);
break; break;
case SwitchInstruction switchInst: case SwitchSection section:
// one statement per case label Debug.Assert(!isStatement); // labels are just children of the SwitchInstruction
numStatements += switchInst.Sections.Count + 1; numStatements++; // add a statement for each case label
// add all the case blocks at the current depth // add all the case blocks at the current depth
// most formatters indent switch blocks twice, but we don't want this heuristic to be based on formatting // most formatters indent switch blocks twice, but we don't want this heuristic to be based on formatting
// so we remain conservative and only include the increase in depth from the container and not the labels // so we remain conservative and only include the increase in depth from the container and not the labels
foreach (var section in switchInst.Sections) if (section.Body.MatchBranch(out var caseBlock) && caseBlock.Parent == section.Parent.Parent.Parent)
if (section.Body.MatchBranch(out var caseBlock) && caseBlock.Parent == switchInst.Parent.Parent) ComputeStats(caseBlock, ref numStatements, ref maxDepth, currentDepth);
ComputeStats(caseBlock, ref numStatements, ref maxDepth, currentDepth); break;
case ILFunction func:
Debug.Assert(!isStatement);
int bodyStatements = 0;
int bodyMaxDepth = maxDepth;
ComputeStats(func.Body, ref bodyStatements, ref bodyMaxDepth, currentDepth);
if (bodyStatements >= 2) { // don't count inline functions
numStatements += bodyStatements;
maxDepth = bodyMaxDepth;
}
break; break;
default: default:
// just a regular statement // search each child instruction. Containers will contain statements and contribute to stats
numStatements++; int subStatements = 0;
if (currentDepth > maxDepth) foreach (var child in inst.Children)
maxDepth = currentDepth; ComputeStats(child, ref subStatements, ref maxDepth, currentDepth, false);
numStatements += subStatements;
if (isStatement && subStatements > 0)
numStatements--; // don't count the first container, only its contents, because this statement is already counted
break; break;
} }
} }

Write Preview
Loading…
Cancel
Save