fix: support branched object construction (#1019)

jadx-core/src/main/java/jadx/core/dex/attributes/AFlag.java

@@ -72,6 +72,7 @@ public enum AFlag {
 	INCONSISTENT_CODE, // warning about incorrect decompilation
 
 	REQUEST_IF_REGION_OPTIMIZE, // run if region visitor again
+	RERUN_SSA_TRANSFORM,
 
 	// Class processing flags
 	RESTART_CODEGEN, // codegen must be executed again

jadx-core/src/main/java/jadx/core/dex/instructions/args/RegisterArg.java

@@ -83,6 +83,10 @@ public class RegisterArg extends InsnArg implements Named {
 		this.sVar = sVar;
 	}
 
+	public void resetSSAVar() {
+		this.sVar = null;
+	}
+
 	@Override
 	public String getName() {
 		if (isSuper()) {

jadx-core/src/main/java/jadx/core/dex/instructions/mods/ConstructorInsn.java

@@ -35,17 +35,17 @@ public final class ConstructorInsn extends BaseInvokeNode {
 	}
 
 	private CallType getCallType(MethodNode mth, ClassInfo classType, InsnArg instanceArg) {
-		if (instanceArg.isThis()) {
-			if (classType.equals(mth.getParentClass().getClassInfo())) {
-				if (callMth.getShortId().equals(mth.getMethodInfo().getShortId())) {
-					// self constructor
-					return CallType.SELF;
-				}
-				return CallType.THIS;
-			}
+		if (!instanceArg.isThis()) {
+			return CallType.CONSTRUCTOR;
+		}
+		if (!classType.equals(mth.getParentClass().getClassInfo())) {
 			return CallType.SUPER;
 		}
-		return CallType.CONSTRUCTOR;
+		if (callMth.getShortId().equals(mth.getMethodInfo().getShortId())) {
+			// self constructor
+			return CallType.SELF;
+		}
+		return CallType.THIS;
 	}
 
 	public ConstructorInsn(MethodInfo callMth, CallType callType) {

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

@@ -5,7 +5,7 @@ import java.util.ArrayList;
 import org.jetbrains.annotations.Nullable;
 
 import jadx.core.codegen.TypeGen;
-import jadx.core.dex.info.MethodInfo;
+import jadx.core.dex.attributes.AFlag;
 import jadx.core.dex.instructions.InsnType;
 import jadx.core.dex.instructions.InvokeNode;
 import jadx.core.dex.instructions.args.InsnArg;
@@ -33,8 +33,10 @@ public class ConstructorVisitor extends AbstractVisitor {
 		if (mth.isNoCode()) {
 			return;
 		}
-
 		replaceInvoke(mth);
+		if (mth.contains(AFlag.RERUN_SSA_TRANSFORM)) {
+			SSATransform.rerun(mth);
+		}
 	}
 
 	private static void replaceInvoke(MethodNode mth) {
@@ -53,55 +55,46 @@ public class ConstructorVisitor extends AbstractVisitor {
 	}
 
 	private static void processInvoke(MethodNode mth, BlockNode block, int indexInBlock, InsnRemover remover) {
-		ClassNode parentClass = mth.getParentClass();
-		InsnNode insn = block.getInstructions().get(indexInBlock);
-		InvokeNode inv = (InvokeNode) insn;
-		MethodInfo callMth = inv.getCallMth();
-		if (!callMth.isConstructor()) {
+		InvokeNode inv = (InvokeNode) block.getInstructions().get(indexInBlock);
+		if (!inv.getCallMth().isConstructor()) {
 			return;
 		}
-		RegisterArg instanceArg = ((RegisterArg) inv.getArg(0));
-		InsnNode instArgAssignInsn = instanceArg.getAssignInsn();
 		ConstructorInsn co = new ConstructorInsn(mth, inv);
+		if (canRemoveConstructor(mth, co)) {
+			remover.addAndUnbind(inv);
+			return;
+		}
+		RegisterArg instanceArg = ((RegisterArg) inv.getArg(0));
+		InsnNode newInstInsn = null;
 		if (co.isNewInstance()) {
-			co.setResult(instanceArg);
-			// convert from 'use' to 'assign'
-			instanceArg.getSVar().setAssign(instanceArg);
+			InsnNode assignInsn = instanceArg.getAssignInsn();
+			if (assignInsn != null) {
+				if (assignInsn.getType() == InsnType.CONSTRUCTOR) {
+					// arg already used in another constructor instruction
+					mth.add(AFlag.RERUN_SSA_TRANSFORM);
+				} else {
+					newInstInsn = removeAssignChain(mth, assignInsn, remover, InsnType.NEW_INSTANCE);
+					if (newInstInsn != null) {
+						newInstInsn.add(AFlag.REMOVE);
+						remover.addWithoutUnbind(newInstInsn);
+					}
+				}
+			}
+			// convert instance arg from 'use' to 'assign'
+			co.setResult(instanceArg.duplicate());
 		}
 		instanceArg.getSVar().removeUse(instanceArg);
 
 		co.rebindArgs();
-		boolean remove = false;
-		if (co.isSuper() && (co.getArgsCount() == 0 || parentClass.isEnum())) {
-			remove = true;
-		} else if (co.isThis() && co.getArgsCount() == 0) {
-			MethodNode defCo = parentClass.searchMethodByShortId(callMth.getShortId());
-			if (defCo == null || defCo.isNoCode()) {
-				// default constructor not implemented
-				remove = true;
-			}
-		}
-		// remove super() call in instance initializer
-		if (parentClass.isAnonymous() && mth.isDefaultConstructor() && co.isSuper()) {
-			remove = true;
-		}
-		if (remove) {
-			remover.addAndUnbind(insn);
-			return;
-		}
-		if (co.isNewInstance()) {
-			InsnNode newInstInsn = removeAssignChain(mth, instArgAssignInsn, remover, InsnType.NEW_INSTANCE);
-			if (newInstInsn != null) {
-				remover.addWithoutUnbind(newInstInsn);
-				RegisterArg instArg = newInstInsn.getResult();
-				RegisterArg resultArg = co.getResult();
-				if (!resultArg.equals(instArg)) {
-					// replace all usages of 'instArg' with result of this constructor instruction
-					for (RegisterArg useArg : new ArrayList<>(instArg.getSVar().getUseList())) {
-						InsnNode parentInsn = useArg.getParentInsn();
-						if (parentInsn != null) {
-							parentInsn.replaceArg(useArg, resultArg.duplicate());
-						}
+		if (co.isNewInstance() && newInstInsn != null) {
+			RegisterArg instArg = newInstInsn.getResult();
+			RegisterArg resultArg = co.getResult();
+			if (!resultArg.equals(instArg)) {
+				// replace all usages of 'instArg' with result of this constructor instruction
+				for (RegisterArg useArg : new ArrayList<>(instArg.getSVar().getUseList())) {
+					InsnNode parentInsn = useArg.getParentInsn();
+					if (parentInsn != null) {
+						parentInsn.replaceArg(useArg, resultArg.duplicate());
 					}
 				}
 			}
@@ -109,9 +102,26 @@ public class ConstructorVisitor extends AbstractVisitor {
 		ConstructorInsn replace = processConstructor(mth, co);
 		if (replace != null) {
 			remover.addAndUnbind(co);
-			co = replace;
+			BlockUtils.replaceInsn(mth, block, indexInBlock, replace);
+		} else {
+			BlockUtils.replaceInsn(mth, block, indexInBlock, co);
 		}
-		BlockUtils.replaceInsn(mth, block, indexInBlock, co);
+	}
+
+	private static boolean canRemoveConstructor(MethodNode mth, ConstructorInsn co) {
+		ClassNode parentClass = mth.getParentClass();
+		if (co.isSuper() && (co.getArgsCount() == 0 || parentClass.isEnum())) {
+			return true;
+		}
+		if (co.isThis() && co.getArgsCount() == 0) {
+			MethodNode defCo = parentClass.searchMethodByShortId(co.getCallMth().getShortId());
+			if (defCo == null || defCo.isNoCode()) {
+				// default constructor not implemented
+				return true;
+			}
+		}
+		// remove super() call in instance initializer
+		return parentClass.isAnonymous() && mth.isDefaultConstructor() && co.isSuper();
 	}
 
 	/**

jadx-core/src/main/java/jadx/core/dex/visitors/ssa/SSATransform.java

@@ -41,11 +41,16 @@ public class SSATransform extends AbstractVisitor {
 		process(mth);
 	}
 
+	public static void rerun(MethodNode mth) {
+		mth.remove(AFlag.RERUN_SSA_TRANSFORM);
+		resetSSAVars(mth);
+		process(mth);
+	}
+
 	private static void process(MethodNode mth) {
 		if (!mth.getSVars().isEmpty()) {
 			return;
 		}
-
 		LiveVarAnalysis la = new LiveVarAnalysis(mth);
 		la.runAnalysis();
 		int regsCount = mth.getRegsCount();
@@ -433,4 +438,15 @@ public class SSATransform extends AbstractVisitor {
 			block.getInstructions().removeIf(insn -> insn.getType() == InsnType.PHI);
 		}
 	}
+
+	private static void resetSSAVars(MethodNode mth) {
+		for (SSAVar ssaVar : mth.getSVars()) {
+			ssaVar.getAssign().resetSSAVar();
+			ssaVar.getUseList().forEach(RegisterArg::resetSSAVar);
+		}
+		for (BlockNode block : mth.getBasicBlocks()) {
+			block.remove(AType.PHI_LIST);
+		}
+		mth.getSVars().clear();
+	}
 }

jadx-core/src/test/java/jadx/tests/integration/others/TestConstructorBranched.java

+package jadx.tests.integration.others;
+
+import org.junit.jupiter.api.Test;
+
+import jadx.tests.api.SmaliTest;
+
+import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat;
+
+@SuppressWarnings("CommentedOutCode")
+public class TestConstructorBranched extends SmaliTest {
+	// @formatter:off
+	/*
+		public Set<String> test(Collection<String> collection) {
+			Set<String> set;
+			if (collection == null) {
+				set = new HashSet<>();
+			} else {
+				set = new HashSet<>(collection);
+			}
+			set.add("end");
+			return set;
+		}
+	 */
+	// @formatter:on
+
+	@Test
+	public void test() {
+		assertThat(getClassNodeFromSmali())
+				.code()
+				.containsOne("new HashSet()")
+				.containsOne("new HashSet(collection)");
+	}
+}

jadx-core/src/test/smali/others/TestConstructorBranched.smali

+.class public Lothers/TestConstructorBranched;
+.super Ljava/lang/Object;
+
+.method public test(Ljava/util/Collection;)Ljava/util/Set;
+    .registers 4
+    .annotation system Ldalvik/annotation/Signature;
+        value = {
+            "(",
+            "Ljava/util/Collection",
+            "<",
+            "Ljava/lang/String;",
+            ">;)",
+            "Ljava/util/Set",
+            "<",
+            "Ljava/lang/String;",
+            ">;"
+        }
+    .end annotation
+
+    new-instance v0, Ljava/util/HashSet;
+
+    if-nez p1, :cond_d
+    invoke-direct {v0}, Ljava/util/HashSet;-><init>()V
+    goto :goto_7
+
+    :cond_d
+    invoke-direct {v0, p1}, Ljava/util/HashSet;-><init>(Ljava/util/Collection;)V
+
+    :goto_7
+    const-string v1, "end"
+    invoke-interface {v0, v1}, Ljava/util/Set;->add(Ljava/lang/Object;)Z
+    return-object v0
+.end method