core: refactor 'if' regions processing

jadx-core/src/main/java/jadx/core/dex/visitors/ConstInlinerVisitor.java

@@ -11,9 +11,9 @@ import jadx.core.dex.nodes.BlockNode;
 import jadx.core.dex.nodes.InsnNode;
 import jadx.core.dex.nodes.MethodNode;
 import jadx.core.utils.BlockUtils;
+import jadx.core.utils.InstructionRemover;
 import jadx.core.utils.exceptions.JadxException;
 
-import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
 
@@ -25,12 +25,13 @@ public class ConstInlinerVisitor extends AbstractVisitor {
 			return;
 		}
 		for (BlockNode block : mth.getBasicBlocks()) {
-			for (Iterator<InsnNode> it = block.getInstructions().iterator(); it.hasNext(); ) {
-				InsnNode insn = it.next();
+			InstructionRemover remover = new InstructionRemover(block.getInstructions());
+			for (InsnNode insn : block.getInstructions()) {
 				if (checkInsn(mth, block, insn)) {
-					it.remove();
+					remover.add(insn);
 				}
 			}
+			remover.perform();
 		}
 	}
 
@@ -53,9 +54,11 @@ public class ConstInlinerVisitor extends AbstractVisitor {
 	private static boolean replaceConst(MethodNode mth, BlockNode block, InsnNode insn, long literal) {
 		List<InsnArg> use = insn.getResult().getTypedVar().getUseList();
 		int replaceCount = 0;
+		int assignCount = 0;
 		for (InsnArg arg : use) {
 			InsnNode useInsn = arg.getParentInsn();
 			if (arg == insn.getResult() || useInsn == null) {
+				assignCount++;
 				continue;
 			}
 			BlockNode useBlock = BlockUtils.getBlockByInsn(mth, useInsn);
@@ -76,7 +79,7 @@ public class ConstInlinerVisitor extends AbstractVisitor {
 				}
 			}
 		}
-		return replaceCount == use.size() - 1;
+		return replaceCount == use.size() - assignCount;
 	}
 
 	private static boolean registerReassignOnPath(BlockNode block, BlockNode useBlock, InsnNode assignInsn) {

jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessReturnInsns.java

@@ -7,7 +7,9 @@ import jadx.core.dex.nodes.IContainer;
 import jadx.core.dex.nodes.IRegion;
 import jadx.core.dex.nodes.InsnNode;
 import jadx.core.dex.nodes.MethodNode;
+import jadx.core.dex.regions.IfRegion;
 import jadx.core.dex.regions.LoopRegion;
+import jadx.core.dex.regions.SwitchRegion;
 import jadx.core.utils.RegionUtils;
 
 import java.util.List;
@@ -52,6 +54,12 @@ public class ProcessReturnInsns extends TracedRegionVisitor {
 	private boolean noTrailInstructions(BlockNode block) {
 		IContainer curContainer = block;
 		for (IRegion region : regionStack) {
+			// ignore paths on other branches
+			if (region instanceof IfRegion
+					|| region instanceof SwitchRegion) {
+				curContainer = region;
+				continue;
+			}
 			List<IContainer> subBlocks = region.getSubBlocks();
 			if (!subBlocks.isEmpty()) {
 				ListIterator<IContainer> itSubBlock = subBlocks.listIterator(subBlocks.size());

jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java

@@ -461,13 +461,8 @@ public class RegionMaker {
 			}
 		}
 
-		if (elseBlock != null) {
-			if (stack.containsExit(elseBlock)) {
-				elseBlock = null;
-			} else if (elseBlock.getAttributes().contains(AttributeFlag.RETURN)) {
-				out = elseBlock;
-				elseBlock = null;
-			}
+		if (elseBlock != null && stack.containsExit(elseBlock)) {
+			elseBlock = null;
 		}
 
 		stack.push(ifRegion);
@@ -481,73 +476,88 @@ public class RegionMaker {
 	}
 
 	private IfInfo mergeNestedIfNodes(BlockNode block, BlockNode bThen, BlockNode bElse, List<BlockNode> merged) {
+		IfInfo info = new IfInfo();
+		info.setIfnode(block);
+		info.setCondition(IfCondition.fromIfBlock(block));
+		info.setThenBlock(bThen);
+		info.setElseBlock(bElse);
+		return mergeNestedIfNodes(info, merged);
+	}
+
+	private IfInfo mergeNestedIfNodes(IfInfo info, List<BlockNode> merged) {
+		BlockNode bThen = info.getThenBlock();
+		BlockNode bElse = info.getElseBlock();
 		if (bThen == bElse) {
 			return null;
 		}
 
-		boolean found;
-		IfCondition condition = IfCondition.fromIfBlock(block);
-		IfInfo result = null;
-		do {
-			found = false;
-			for (BlockNode succ : block.getSuccessors()) {
-				BlockNode nestedIfBlock = getIfNode(succ);
-				if (nestedIfBlock != null && nestedIfBlock != block) {
-					IfNode nestedIfInsn = (IfNode) nestedIfBlock.getInstructions().get(0);
-					BlockNode nbThen = nestedIfInsn.getThenBlock();
-					BlockNode nbElse = nestedIfInsn.getElseBlock();
-
-					boolean inverted = false;
-					IfCondition nestedCondition = IfCondition.fromIfNode(nestedIfInsn);
-					if (isPathExists(bElse, nestedIfBlock)) {
-						// else branch
-						if (!isEqualPaths(bThen, nbThen)) {
-							if (!isEqualPaths(bThen, nbElse)) {
-								// not connected conditions
-								break;
-							}
-							nestedIfInsn.invertCondition();
-							inverted = true;
-						}
-						condition = IfCondition.merge(Mode.OR, condition, nestedCondition);
-					} else {
-						// then branch
-						if (!isEqualPaths(bElse, nbElse)) {
-							if (!isEqualPaths(bElse, nbThen)) {
-								// not connected conditions
-								break;
-							}
-							nestedIfInsn.invertCondition();
-							inverted = true;
-						}
-						condition = IfCondition.merge(Mode.AND, condition, nestedCondition);
-					}
-					result = new IfInfo();
-					result.setCondition(condition);
-					nestedIfBlock.getAttributes().add(AttributeFlag.SKIP);
-					skipSimplePath(bThen);
-
-					if (merged != null) {
-						merged.add(nestedIfBlock);
-					}
+		BlockNode ifBlock = info.getIfnode();
+		BlockNode nestedIfBlock = getNextIfBlock(ifBlock);
+		if (nestedIfBlock == null) {
+			return null;
+		}
 
-					// set new blocks
-					result.setIfnode(nestedIfBlock);
-					result.setThenBlock(inverted ? nbElse : nbThen);
-					result.setElseBlock(inverted ? nbThen : nbElse);
+		IfNode nestedIfInsn = (IfNode) nestedIfBlock.getInstructions().get(0);
+		IfCondition nestedCondition = IfCondition.fromIfNode(nestedIfInsn);
+		BlockNode nbThen = nestedIfInsn.getThenBlock();
+		BlockNode nbElse = nestedIfInsn.getElseBlock();
 
-					found = true;
-					block = nestedIfBlock;
-					bThen = result.getThenBlock();
-					bElse = result.getElseBlock();
-					break;
+		IfCondition condition = info.getCondition();
+		boolean inverted = false;
+		if (isPathExists(bElse, nestedIfBlock)) {
+			// else branch
+			if (!isEqualPaths(bThen, nbThen)) {
+				if (!isEqualPaths(bThen, nbElse)) {
+					// not connected conditions
+					return null;
+				}
+				nestedIfInsn.invertCondition();
+				inverted = true;
+			}
+			condition = IfCondition.merge(Mode.OR, condition, nestedCondition);
+		} else {
+			// then branch
+			if (!isEqualPaths(bElse, nbElse)) {
+				if (!isEqualPaths(bElse, nbThen)) {
+					// not connected conditions
+					return null;
 				}
+				nestedIfInsn.invertCondition();
+				inverted = true;
 			}
-		} while (found);
+			condition = IfCondition.merge(Mode.AND, condition, nestedCondition);
+		}
+		if (merged != null) {
+			merged.add(nestedIfBlock);
+		}
+		nestedIfBlock.getAttributes().add(AttributeFlag.SKIP);
+		BlockNode blockToNestedIfBlock = BlockUtils.getNextBlockToPath(ifBlock, nestedIfBlock);
+		skipSimplePath(BlockUtils.selectOther(blockToNestedIfBlock, ifBlock.getCleanSuccessors()));
+
+		IfInfo result = new IfInfo();
+		result.setIfnode(nestedIfBlock);
+		result.setCondition(condition);
+		result.setThenBlock(inverted ? nbElse : nbThen);
+		result.setElseBlock(inverted ? nbThen : nbElse);
 
+		// search next nested if block
+		IfInfo next = mergeNestedIfNodes(result, merged);
+		if (next != null) {
+			return next;
+		}
 		return result;
 	}
 
+	private BlockNode getNextIfBlock(BlockNode block) {
+		for (BlockNode succ : block.getSuccessors()) {
+			BlockNode nestedIfBlock = getIfNode(succ);
+			if (nestedIfBlock != null && nestedIfBlock != block) {
+				return nestedIfBlock;
+			}
+		}
+		return null;
+	}
+
 	private static BlockNode getIfNode(BlockNode block) {
 		if (block != null && !block.getAttributes().contains(AttributeType.LOOP)) {
 			List<InsnNode> insns = block.getInstructions();

jadx-core/src/main/java/jadx/core/utils/BlockUtils.java

@@ -131,6 +131,23 @@ public class BlockUtils {
 		return s.isEmpty() ? null : s.get(0);
 	}
 
+	/**
+	 * Return successor on path to 'pathEnd' block
+	 */
+	public static BlockNode getNextBlockToPath(BlockNode block, BlockNode pathEnd) {
+		List<BlockNode> successors = block.getCleanSuccessors();
+		if (successors.contains(pathEnd)) {
+			return pathEnd;
+		}
+		Set<BlockNode> path = getAllPathsBlocks(block, pathEnd);
+		for (BlockNode s : successors) {
+			if (path.contains(s)) {
+				return s;
+			}
+		}
+		return null;
+	}
+
 	/**
 	 * Collect blocks from all possible execution paths from 'start' to 'end'
 	 */

jadx-core/src/main/java/jadx/core/utils/Utils.java

@@ -35,18 +35,18 @@ public class Utils {
 				case '/':
 				case ';':
 				case '$':
+				case ' ':
+				case ',':
 				case '<':
-				case '[':
 					sb.append('_');
 					break;
 
-				case ']':
+				case '[':
 					sb.append('A');
 					break;
 
+				case ']':
 				case '>':
-				case ',':
-				case ' ':
 				case '?':
 				case '*':
 					break;

jadx-core/src/test/java/jadx/api/InternalJadxTest.java

 package jadx.api;
 
 import jadx.core.Jadx;
-import jadx.core.dex.attributes.AttributeFlag;
 import jadx.core.dex.nodes.ClassNode;
 import jadx.core.dex.nodes.MethodNode;
 import jadx.core.dex.visitors.DepthTraverser;
@@ -20,9 +19,11 @@ import java.util.jar.JarEntry;
 import java.util.jar.JarOutputStream;
 
 import static junit.framework.Assert.assertEquals;
-import static junit.framework.Assert.assertFalse;
 import static junit.framework.Assert.assertNotNull;
 import static junit.framework.Assert.fail;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertThat;
 
 public abstract class InternalJadxTest extends TestUtils {
 
@@ -67,7 +68,7 @@ public abstract class InternalJadxTest extends TestUtils {
 			for (IDexTreeVisitor visitor : passes) {
 				DepthTraverser.visit(visitor, cls);
 			}
-			assertFalse(cls.getAttributes().contains(AttributeFlag.INCONSISTENT_CODE));
+			assertThat(cls.getCode().toString(), not(containsString("inconsistent")));
 			return cls;
 		} catch (Exception e) {
 			e.printStackTrace();