fix: remove shadowed catch handlers (#1377)

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

 package jadx.core.dex.visitors;
 
 import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.List;
 
 import org.jetbrains.annotations.Nullable;
@@ -15,12 +16,16 @@ import jadx.core.dex.attributes.AFlag;
 import jadx.core.dex.attributes.AType;
 import jadx.core.dex.info.ClassInfo;
 import jadx.core.dex.instructions.InsnType;
+import jadx.core.dex.instructions.args.ArgType;
 import jadx.core.dex.nodes.InsnNode;
 import jadx.core.dex.nodes.MethodNode;
 import jadx.core.dex.trycatch.CatchAttr;
 import jadx.core.dex.trycatch.ExcHandlerAttr;
 import jadx.core.dex.trycatch.ExceptionHandler;
+import jadx.core.dex.visitors.typeinference.TypeCompare;
+import jadx.core.dex.visitors.typeinference.TypeCompareEnum;
 import jadx.core.utils.exceptions.JadxException;
+import jadx.core.utils.exceptions.JadxRuntimeException;
 
 import static jadx.core.dex.visitors.ProcessInstructionsVisitor.getNextInsnOffset;
 
@@ -51,7 +56,7 @@ public class AttachTryCatchVisitor extends AbstractVisitor {
 			tries.forEach(tryData -> LOG.debug(" - {}", tryData));
 		}
 		for (ITry tryData : tries) {
-			List<ExceptionHandler> handlers = attachHandlers(mth, tryData.getCatch(), insnByOffset);
+			List<ExceptionHandler> handlers = convertToHandlers(mth, tryData.getCatch(), insnByOffset);
 			if (handlers.isEmpty()) {
 				continue;
 			}
@@ -102,7 +107,7 @@ public class AttachTryCatchVisitor extends AbstractVisitor {
 		}
 	}
 
-	private static List<ExceptionHandler> attachHandlers(MethodNode mth, ICatch catchBlock, InsnNode[] insnByOffset) {
+	private static List<ExceptionHandler> convertToHandlers(MethodNode mth, ICatch catchBlock, InsnNode[] insnByOffset) {
 		int[] handlerOffsetArr = catchBlock.getHandlers();
 		String[] handlerTypes = catchBlock.getTypes();
 
@@ -117,6 +122,7 @@ public class AttachTryCatchVisitor extends AbstractVisitor {
 		if (allHandlerOffset >= 0) {
 			Utils.addToList(list, createHandler(mth, insnByOffset, allHandlerOffset, null));
 		}
+		checkAndFilterHandlers(mth, list);
 		return list;
 	}
 
@@ -143,6 +149,45 @@ public class AttachTryCatchVisitor extends AbstractVisitor {
 		return handler;
 	}
 
+	private static void checkAndFilterHandlers(MethodNode mth, List<ExceptionHandler> list) {
+		if (list.size() <= 1) {
+			return;
+		}
+		// Remove shadowed handlers (with same or narrow type compared to previous)
+		TypeCompare typeCompare = mth.root().getTypeCompare();
+		Iterator<ExceptionHandler> it = list.iterator();
+		ArgType maxType = null;
+		while (it.hasNext()) {
+			ExceptionHandler handler = it.next();
+			ArgType maxCatch = maxCatchFromHandler(handler, typeCompare);
+			if (maxType == null) {
+				maxType = maxCatch;
+			} else {
+				TypeCompareEnum result = typeCompare.compareObjects(maxType, maxCatch);
+				if (result.isWiderOrEqual()) {
+					if (Consts.DEBUG_EXC_HANDLERS) {
+						LOG.debug("Removed shadowed catch handler: {}, from list: {}", handler, list);
+					}
+					it.remove();
+				}
+			}
+		}
+	}
+
+	private static ArgType maxCatchFromHandler(ExceptionHandler handler, TypeCompare typeCompare) {
+		List<ClassInfo> catchTypes = handler.getCatchTypes();
+		if (catchTypes.isEmpty()) {
+			return ArgType.THROWABLE;
+		}
+		if (catchTypes.size() == 1) {
+			return catchTypes.get(0).getType();
+		}
+		return catchTypes.stream()
+				.map(ClassInfo::getType)
+				.max(typeCompare.getComparator())
+				.orElseThrow(() -> new JadxRuntimeException("Failed to get max type from catch list: " + catchTypes));
+	}
+
 	private static InsnNode insertNOP(InsnNode[] insnByOffset, int offset) {
 		InsnNode nop = new InsnNode(InsnType.NOP, 0);
 		nop.setOffset(offset);

jadx-core/src/main/java/jadx/core/dex/visitors/typeinference/TypeCompare.java

@@ -9,6 +9,7 @@ import java.util.Objects;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import jadx.core.dex.info.ClassInfo;
 import jadx.core.dex.instructions.args.ArgType;
 import jadx.core.dex.instructions.args.ArgType.WildcardBound;
 import jadx.core.dex.instructions.args.PrimitiveType;
@@ -42,6 +43,17 @@ public class TypeCompare {
 		return compareObjects(first.getType(), second.getType());
 	}
 
+	public TypeCompareEnum compareTypes(ClassInfo first, ClassInfo second) {
+		return compareObjects(first.getType(), second.getType());
+	}
+
+	public TypeCompareEnum compareObjects(ArgType first, ArgType second) {
+		if (first == second || Objects.equals(first, second)) {
+			return TypeCompareEnum.EQUAL;
+		}
+		return compareObjectsNoPreCheck(first, second);
+	}
+
 	/**
 	 * Compare two type and return result for first argument (narrow, wider or conflict)
 	 */
@@ -81,7 +93,7 @@ public class TypeCompare {
 		boolean firstObj = first.isObject();
 		boolean secondObj = second.isObject();
 		if (firstObj && secondObj) {
-			return compareObjects(first, second);
+			return compareObjectsNoPreCheck(first, second);
 		} else {
 			// primitive types conflicts with objects
 			if (firstObj && secondPrimitive) {
@@ -159,7 +171,7 @@ public class TypeCompare {
 		return CONFLICT;
 	}
 
-	private TypeCompareEnum compareObjects(ArgType first, ArgType second) {
+	private TypeCompareEnum compareObjectsNoPreCheck(ArgType first, ArgType second) {
 		boolean objectsEquals = first.getObject().equals(second.getObject());
 		boolean firstGenericType = first.isGenericType();
 		boolean secondGenericType = second.isGenericType();
@@ -262,7 +274,7 @@ public class TypeCompare {
 			return NARROW;
 		}
 		for (ArgType extendType : extendTypes) {
-			TypeCompareEnum res = compareObjects(extendType, objType);
+			TypeCompareEnum res = compareObjectsNoPreCheck(extendType, objType);
 			if (!res.isNarrow()) {
 				return res;
 			}

jadx-core/src/main/java/jadx/core/dex/visitors/typeinference/TypeCompareEnum.java

@@ -39,6 +39,10 @@ public enum TypeCompareEnum {
 		return this == WIDER || this == WIDER_BY_GENERIC;
 	}
 
+	public boolean isWiderOrEqual() {
+		return isEqual() || isWider();
+	}
+
 	public boolean isNarrow() {
 		return this == NARROW || this == NARROW_BY_GENERIC;
 	}

jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java

@@ -5,6 +5,7 @@ import java.io.IOException;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Collections;
 import java.util.List;
@@ -16,8 +17,9 @@ import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import java.util.jar.JarEntry;
+import java.util.jar.JarOutputStream;
 
-import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assumptions;
@@ -95,6 +97,8 @@ public abstract class IntegrationTest extends TestUtils {
 	protected boolean useEclipseCompiler;
 	private int targetJavaVersion = 8;
 
+	private boolean saveTestJar = false;
+
 	protected Map<Integer, String> resMap = Collections.emptyMap();
 
 	private boolean allowWarnInCode;
@@ -457,17 +461,28 @@ public abstract class IntegrationTest extends TestUtils {
 		Path outTmp = FileUtils.createTempDir("jadx-tmp-classes");
 		List<File> files = StaticCompiler.compile(compileFileList, outTmp.toFile(), withDebugInfo, useEclipseCompiler, targetJavaVersion);
 		files.forEach(File::deleteOnExit);
+		if (saveTestJar) {
+			saveToJar(files, outTmp);
+		}
 		// remove classes which are parents for test class
 		String clsName = clsFullName.substring(clsFullName.lastIndexOf('.') + 1);
 		files.removeIf(next -> !next.getName().contains(clsName));
 		return files;
 	}
 
-	@NotNull
-	protected static String removeLineComments(ClassNode cls) {
-		String code = cls.getCode().getCodeStr().replaceAll("\\W*//.*", "");
-		System.out.println(code);
-		return code;
+	private void saveToJar(List<File> files, Path baseDir) throws IOException {
+		Path jarFile = Files.createTempFile("tests-" + getTestName() + '-', ".jar");
+		try (JarOutputStream jar = new JarOutputStream(Files.newOutputStream(jarFile))) {
+			for (File file : files) {
+				Path fullPath = file.toPath();
+				Path relativePath = baseDir.relativize(fullPath);
+				JarEntry entry = new JarEntry(relativePath.toString());
+				jar.putNextEntry(entry);
+				jar.write(Files.readAllBytes(fullPath));
+				jar.closeEntry();
+			}
+		}
+		LOG.info("Test jar saved to: {}", jarFile.toAbsolutePath());
 	}
 
 	public JadxArgs getArgs() {
@@ -540,4 +555,9 @@ public abstract class IntegrationTest extends TestUtils {
 	protected void printDisassemble() {
 		this.printDisassemble = true;
 	}
+
+	// Use only for debug purpose
+	protected void saveTestJar() {
+		this.saveTestJar = true;
+	}
 }

jadx-core/src/test/java/jadx/tests/integration/trycatch/TestNestedTryCatch3.java

+package jadx.tests.integration.trycatch;
+
+import jadx.tests.api.IntegrationTest;
+import jadx.tests.api.extensions.profiles.TestProfile;
+import jadx.tests.api.extensions.profiles.TestWithProfiles;
+
+import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat;
+
+public class TestNestedTryCatch3 extends IntegrationTest {
+
+	public static class TestCls {
+		public I test() {
+			try {
+				try {
+					return new A();
+				} catch (Throwable e) {
+					return new B();
+				}
+			} catch (Throwable e) {
+				return new C();
+			}
+		}
+
+		private interface I {
+		}
+
+		private static class A implements I {
+		}
+
+		private static class B implements I {
+		}
+
+		private static class C implements I {
+		}
+	}
+
+	@TestWithProfiles({ TestProfile.JAVA8, TestProfile.DX_J8 })
+	public void test() {
+		assertThat(getClassNode(TestCls.class))
+				.code()
+				.containsOne("return new A();")
+				.containsOne("return new B();")
+				.containsOne("return new C();")
+				.countString(2, "} catch (Throwable ");
+	}
+}