core: fix variable declaration in else-if chain (#273)

jadx-core/src/main/java/jadx/core/Jadx.java

@@ -94,10 +94,6 @@ public class Jadx {
 			passes.add(new SimplifyVisitor());
 			passes.add(new CheckRegions());
 
-			if (args.isCfgOutput()) {
-				passes.add(DotGraphVisitor.dumpRegions());
-			}
-
 			passes.add(new MethodInlineVisitor());
 			passes.add(new ExtractFieldInit());
 			passes.add(new ClassModifier());
@@ -106,6 +102,10 @@ public class Jadx {
 			passes.add(new LoopRegionVisitor());
 			passes.add(new ProcessVariables());
 
+			if (args.isCfgOutput()) {
+				passes.add(DotGraphVisitor.dumpRegions());
+			}
+
 			passes.add(new DependencyCollector());
 
 			passes.add(new RenameVisitor());

jadx-core/src/main/java/jadx/core/ProcessClass.java

@@ -56,8 +56,6 @@ public final class ProcessClass {
 	}
 
 	private static void processDependencies(ClassNode cls, List<IDexTreeVisitor> passes) {
-		for (ClassNode depCls : cls.getDependencies()) {
-			process(depCls, passes, null);
-		}
+		cls.getDependencies().forEach(depCls -> process(depCls, passes, null));
 	}
 }

jadx-core/src/main/java/jadx/core/codegen/RegionGen.java

@@ -134,17 +134,16 @@ public class RegionGen extends InsnGen {
 	 * Connect if-else-if block
 	 */
 	private boolean connectElseIf(CodeWriter code, IContainer els) throws CodegenException {
-		if (!els.contains(AFlag.ELSE_IF_CHAIN)) {
-			return false;
-		}
-		if (!(els instanceof Region)) {
-			return false;
-		}
-		List<IContainer> subBlocks = ((Region) els).getSubBlocks();
-		if (subBlocks.size() == 1
-				&& subBlocks.get(0) instanceof IfRegion) {
-			makeIf((IfRegion) subBlocks.get(0), code, false);
-			return true;
+		if (els.contains(AFlag.ELSE_IF_CHAIN) && els instanceof Region) {
+			List<IContainer> subBlocks = ((Region) els).getSubBlocks();
+			if (subBlocks.size() == 1) {
+				IContainer elseBlock = subBlocks.get(0);
+				if (elseBlock instanceof IfRegion) {
+					declareVars(code, elseBlock);
+					makeIf((IfRegion) elseBlock, code, false);
+					return true;
+				}
+			}
 		}
 		return false;
 	}

jadx-core/src/main/java/jadx/core/dex/nodes/IRegion.java

@@ -6,6 +6,8 @@ public interface IRegion extends IContainer {
 
 	IRegion getParent();
 
+	void setParent(IRegion parent);
+
 	List<IContainer> getSubBlocks();
 
 	boolean replaceSubBlock(IContainer oldBlock, IContainer newBlock);

jadx-core/src/main/java/jadx/core/dex/regions/AbstractRegion.java

@@ -21,6 +21,7 @@ public abstract class AbstractRegion extends AttrNode implements IRegion {
 		return parent;
 	}
 
+	@Override
 	public void setParent(IRegion parent) {
 		this.parent = parent;
 	}
@@ -30,4 +31,10 @@ public abstract class AbstractRegion extends AttrNode implements IRegion {
 		LOG.warn("Replace sub block not supported for class \"{}\"", this.getClass());
 		return false;
 	}
+
+	public void updateParent(IContainer container, IRegion newParent) {
+		if (container instanceof IRegion) {
+			((IRegion) container).setParent(newParent);
+		}
+	}
 }

jadx-core/src/main/java/jadx/core/dex/regions/Region.java

@@ -21,9 +21,7 @@ public final class Region extends AbstractRegion {
 	}
 
 	public void add(IContainer region) {
-		if (region instanceof AbstractRegion) {
-			((AbstractRegion) region).setParent(this);
-		}
+		updateParent(region, this);
 		blocks.add(region);
 	}
 
@@ -32,6 +30,7 @@ public final class Region extends AbstractRegion {
 		int i = blocks.indexOf(oldBlock);
 		if (i != -1) {
 			blocks.set(i, newBlock);
+			updateParent(newBlock, this);
 			return true;
 		}
 		return false;

jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfRegion.java

@@ -105,10 +105,12 @@ public final class IfRegion extends AbstractRegion implements IBranchRegion {
 	public boolean replaceSubBlock(IContainer oldBlock, IContainer newBlock) {
 		if (oldBlock == thenRegion) {
 			thenRegion = newBlock;
+			updateParent(thenRegion, this);
 			return true;
 		}
 		if (oldBlock == elseRegion) {
 			elseRegion = newBlock;
+			updateParent(elseRegion, this);
 			return true;
 		}
 		return false;
@@ -128,6 +130,6 @@ public final class IfRegion extends AbstractRegion implements IBranchRegion {
 
 	@Override
 	public String toString() {
-		return "IF " + header + " then " + thenRegion + " else " + elseRegion;
+		return "IF " + header + " then (" + thenRegion + ") else (" + elseRegion + ")";
 	}
 }

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

@@ -10,12 +10,8 @@ public class DepthTraversal {
 	public static void visit(IDexTreeVisitor visitor, ClassNode cls) {
 		try {
 			if (visitor.visit(cls)) {
-				for (ClassNode inCls : cls.getInnerClasses()) {
-					visit(visitor, inCls);
-				}
-				for (MethodNode mth : cls.getMethods()) {
-					visit(visitor, mth);
-				}
+				cls.getInnerClasses().forEach(inCls -> visit(visitor, inCls));
+				cls.getMethods().forEach(mth -> visit(visitor, mth));
 			}
 		} catch (Exception e) {
 			ErrorsCounter.classError(cls,

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

 package jadx.core.dex.visitors.regions;
 
-import java.util.Iterator;
-
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import jadx.core.dex.nodes.BlockNode;
-import jadx.core.dex.nodes.IContainer;
 import jadx.core.dex.nodes.IRegion;
 import jadx.core.dex.nodes.MethodNode;
 import jadx.core.dex.regions.Region;
 
 public class CleanRegions {
-	private static final Logger LOG = LoggerFactory.getLogger(CleanRegions.class);
-
-	private CleanRegions() {
-	}
 
 	public static void process(MethodNode mth) {
 		if (mth.isNoCode() || mth.getBasicBlocks().isEmpty()) {
@@ -24,27 +14,21 @@ public class CleanRegions {
 		IRegionVisitor removeEmptyBlocks = new AbstractRegionVisitor() {
 			@Override
 			public boolean enterRegion(MethodNode mth, IRegion region) {
-				if (!(region instanceof Region)) {
-					return true;
-				}
-
-				for (Iterator<IContainer> it = region.getSubBlocks().iterator(); it.hasNext(); ) {
-					IContainer container = it.next();
-					if (container instanceof BlockNode) {
-						BlockNode block = (BlockNode) container;
-						if (block.getInstructions().isEmpty()) {
-							try {
-								it.remove();
-							} catch (UnsupportedOperationException e) {
-								LOG.warn("Can't remove block: {} from: {}, mth: {}", block, region, mth);
-							}
+				if (region instanceof Region) {
+					region.getSubBlocks().removeIf(container -> {
+						if (container instanceof BlockNode) {
+							BlockNode block = (BlockNode) container;
+							return block.getInstructions().isEmpty();
 						}
-					}
-
+						return false;
+					});
 				}
 				return true;
 			}
 		};
 		DepthRegionTraversal.traverse(mth, removeEmptyBlocks);
 	}
+
+	private CleanRegions() {
+	}
 }

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

@@ -54,9 +54,7 @@ public class DepthRegionTraversal {
 		} else if (container instanceof IRegion) {
 			IRegion region = (IRegion) container;
 			if (visitor.enterRegion(mth, region)) {
-				for (IContainer subCont : region.getSubBlocks()) {
-					traverseInternal(mth, visitor, subCont);
-				}
+				region.getSubBlocks().forEach(subCont -> traverseInternal(mth, visitor, subCont));
 			}
 			visitor.leaveRegion(mth, region);
 		}

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

@@ -4,7 +4,6 @@ import java.util.List;
 
 import jadx.core.dex.attributes.AFlag;
 import jadx.core.dex.instructions.args.ArgType;
-import jadx.core.dex.nodes.IBlock;
 import jadx.core.dex.nodes.IContainer;
 import jadx.core.dex.nodes.IRegion;
 import jadx.core.dex.nodes.MethodNode;
@@ -17,18 +16,22 @@ import jadx.core.utils.RegionUtils;
 
 import static jadx.core.utils.RegionUtils.insnsCount;
 
-public class IfRegionVisitor extends AbstractVisitor implements IRegionVisitor, IRegionIterativeVisitor {
+public class IfRegionVisitor extends AbstractVisitor {
 
 	private static final TernaryVisitor TERNARY_VISITOR = new TernaryVisitor();
+	private static final ProcessIfRegionVisitor PROCESS_IF_REGION_VISITOR = new ProcessIfRegionVisitor();
+	private static final RemoveRedundantElseVisitor REMOVE_REDUNDANT_ELSE_VISITOR = new RemoveRedundantElseVisitor();
 
 	@Override
 	public void visit(MethodNode mth) {
-		// collapse ternary operators
 		DepthRegionTraversal.traverseIterative(mth, TERNARY_VISITOR);
-		DepthRegionTraversal.traverse(mth, this);
-		DepthRegionTraversal.traverseIterative(mth, this);
+		DepthRegionTraversal.traverse(mth, PROCESS_IF_REGION_VISITOR);
+		DepthRegionTraversal.traverseIterative(mth, REMOVE_REDUNDANT_ELSE_VISITOR);
 	}
 
+	/**
+	 * Collapse ternary operators
+	 */
 	private static class TernaryVisitor implements IRegionIterativeVisitor {
 		@Override
 		public boolean visitRegion(MethodNode mth, IRegion region) {
@@ -37,35 +40,28 @@ public class IfRegionVisitor extends AbstractVisitor implements IRegionVisitor,
 		}
 	}
 
-	@Override
-	public boolean enterRegion(MethodNode mth, IRegion region) {
-		if (region instanceof IfRegion) {
-			processIfRegion(mth, (IfRegion) region);
+	private static class ProcessIfRegionVisitor extends AbstractRegionVisitor {
+		@Override
+		public boolean enterRegion(MethodNode mth, IRegion region) {
+			if (region instanceof IfRegion) {
+				IfRegion ifRegion = (IfRegion) region;
+				simplifyIfCondition(ifRegion);
+				moveReturnToThenBlock(mth, ifRegion);
+				moveBreakToThenBlock(ifRegion);
+				markElseIfChains(ifRegion);
+			}
+			return true;
 		}
-		return true;
 	}
 
-	@Override
-	public boolean visitRegion(MethodNode mth, IRegion region) {
-		if (region instanceof IfRegion) {
-			return removeRedundantElseBlock(mth, (IfRegion) region);
+	private static class RemoveRedundantElseVisitor implements IRegionIterativeVisitor {
+		@Override
+		public boolean visitRegion(MethodNode mth, IRegion region) {
+			if (region instanceof IfRegion) {
+				return removeRedundantElseBlock(mth, (IfRegion) region);
+			}
+			return false;
 		}
-		return false;
-	}
-
-	@Override
-	public void processBlock(MethodNode mth, IBlock container) {
-	}
-
-	@Override
-	public void leaveRegion(MethodNode mth, IRegion region) {
-	}
-
-	private static void processIfRegion(MethodNode mth, IfRegion ifRegion) {
-		simplifyIfCondition(ifRegion);
-		moveReturnToThenBlock(mth, ifRegion);
-		moveBreakToThenBlock(ifRegion);
-		markElseIfChains(ifRegion);
 	}
 
 	private static void simplifyIfCondition(IfRegion ifRegion) {
@@ -90,7 +86,6 @@ public class IfRegionVisitor extends AbstractVisitor implements IRegionVisitor,
 					&& !isIfRegion(elseRegion)) {
 				invertIfRegion(ifRegion);
 			}
-
 		}
 	}
 

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

@@ -64,7 +64,7 @@ public class ProcessVariables extends AbstractVisitor {
 
 		@Override
 		public String toString() {
-			return regNum + " " + type;
+			return "r" + regNum + ":" + type;
 		}
 	}
 
@@ -119,7 +119,7 @@ public class ProcessVariables extends AbstractVisitor {
 
 		public CollectUsageRegionVisitor(Map<Variable, Usage> usageMap) {
 			this.usageMap = usageMap;
-			args = new ArrayList<>();
+			this.args = new ArrayList<>();
 		}
 
 		@Override
@@ -177,8 +177,10 @@ public class ProcessVariables extends AbstractVisitor {
 		if (mth.isNoCode()) {
 			return;
 		}
+		List<RegisterArg> mthArguments = mth.getArguments(true);
+
 		Map<Variable, Usage> usageMap = new LinkedHashMap<>();
-		for (RegisterArg arg : mth.getArguments(true)) {
+		for (RegisterArg arg : mthArguments) {
 			addToUsageMap(arg, usageMap);
 		}
 
@@ -187,8 +189,7 @@ public class ProcessVariables extends AbstractVisitor {
 		DepthRegionTraversal.traverse(mth, collect);
 
 		// reduce assigns map
-		List<RegisterArg> mthArgs = mth.getArguments(true);
-		for (RegisterArg arg : mthArgs) {
+		for (RegisterArg arg : mthArguments) {
 			usageMap.remove(new Variable(arg));
 		}
 
@@ -350,6 +351,10 @@ public class ProcessVariables extends AbstractVisitor {
 				}
 			}
 		}
+		// can't declare in else-if chain between 'else' and next 'if'
+		if (region.contains(AFlag.ELSE_IF_CHAIN)) {
+			return false;
+		}
 		return isAllRegionsAfter(region, pos, u.getAssigns(), regionsOrder)
 				&& isAllRegionsAfter(region, pos, u.getUseRegions(), regionsOrder);
 	}

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

@@ -76,7 +76,7 @@ public class DebugUtils {
 	}
 
 	private static void printRegion(MethodNode mth, IRegion region, String indent, boolean printInsns) {
-		LOG.debug("{}{}", indent, region);
+		LOG.debug("{}{} {}", indent, region, region.getAttributesString());
 		indent += "|  ";
 		for (IContainer container : region.getSubBlocks()) {
 			if (container instanceof IRegion) {
@@ -99,9 +99,9 @@ public class DebugUtils {
 				CodeWriter code = new CodeWriter();
 				ig.makeInsn(insn, code);
 				String insnStr = code.toString().substring(CodeWriter.NL.length());
-				LOG.debug("{} - {}", indent, insnStr);
+				LOG.debug("{}> {}", indent, insnStr);
 			} catch (CodegenException e) {
-				LOG.debug("{} - {}", indent, insn);
+				LOG.debug("{}>!! {}", indent, insn);
 			}
 		}
 	}

jadx-core/src/test/java/jadx/tests/integration/variables/TestVariablesIfElseChain.java

+package jadx.tests.integration.variables;
+
+import org.junit.Test;
+
+import jadx.core.dex.nodes.ClassNode;
+import jadx.tests.api.IntegrationTest;
+
+import static jadx.tests.api.utils.JadxMatchers.containsOne;
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertThat;
+
+public class TestVariablesIfElseChain extends IntegrationTest {
+
+	public static class TestCls {
+		String used;
+
+		public String test(int a) {
+			if (a == 0) {
+				use("zero");
+			} else if (a == 1) {
+				String r = m(a);
+				if (r != null) {
+					use(r);
+				}
+			} else if (a == 2) {
+				String r = m(a);
+				if (r != null) {
+					use(r);
+				}
+			} else {
+				return "miss";
+			}
+			return null;
+		}
+
+		public String m(int a) {
+			return "hit" + a;
+		}
+
+		public void use(String s) {
+			used = s;
+		}
+
+		public void check() {
+			test(0);
+			assertThat(used, is("zero"));
+			test(1);
+			assertThat(used, is("hit1"));
+			test(2);
+			assertThat(used, is("hit2"));
+			assertThat(test(5), is("miss"));
+		}
+	}
+
+	@Test
+	public void test() {
+		ClassNode cls = getClassNode(TestCls.class);
+		String code = cls.getCode().toString();
+
+		assertThat(code, containsOne("return \"miss\";"));
+		// and compilable
+	}
+}