diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/FieldNode.java b/jadx-core/src/main/java/jadx/core/dex/nodes/FieldNode.java index 3bde140bf..bb877c760 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/FieldNode.java +++ b/jadx-core/src/main/java/jadx/core/dex/nodes/FieldNode.java @@ -57,6 +57,10 @@ public class FieldNode extends NotificationAttrNode implements ICodeNode { return accFlags.isStatic(); } + public boolean isInstance() { + return !accFlags.isStatic(); + } + public String getName() { return fieldInfo.getName(); } diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/InsnNode.java b/jadx-core/src/main/java/jadx/core/dex/nodes/InsnNode.java index 438be28a6..d7ae005c2 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/InsnNode.java +++ b/jadx-core/src/main/java/jadx/core/dex/nodes/InsnNode.java @@ -264,21 +264,6 @@ public class InsnNode extends LineAttrNode { } } - public boolean canReorderRecursive() { - if (!canReorder()) { - return false; - } - for (InsnArg arg : this.getArguments()) { - if (arg.isInsnWrap()) { - InsnNode wrapInsn = ((InsnWrapArg) arg).getWrapInsn(); - if (!wrapInsn.canReorderRecursive()) { - return false; - } - } - } - return true; - } - public boolean containsWrappedInsn() { for (InsnArg arg : this.getArguments()) { if (arg.isInsnWrap()) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/ExtractFieldInit.java b/jadx-core/src/main/java/jadx/core/dex/visitors/ExtractFieldInit.java index 2662d506f..e83fd4421 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/ExtractFieldInit.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/ExtractFieldInit.java @@ -29,6 +29,7 @@ import jadx.core.dex.nodes.MethodNode; import jadx.core.dex.visitors.shrink.CodeShrinkVisitor; import jadx.core.utils.BlockUtils; import jadx.core.utils.InsnRemover; +import jadx.core.utils.ListUtils; import jadx.core.utils.Utils; import jadx.core.utils.exceptions.JadxException; @@ -45,20 +46,22 @@ public class ExtractFieldInit extends AbstractVisitor { for (ClassNode inner : cls.getInnerClasses()) { visit(inner); } - moveStaticFieldsInit(cls); - moveCommonFieldsInit(cls); + if (!cls.getFields().isEmpty()) { + moveStaticFieldsInit(cls); + moveCommonFieldsInit(cls); + } return false; } private static final class FieldInitInfo { final FieldNode fieldNode; final IndexInsnNode putInsn; - final boolean singlePath; + final boolean canMove; - public FieldInitInfo(FieldNode fieldNode, IndexInsnNode putInsn, boolean singlePath) { + public FieldInitInfo(FieldNode fieldNode, IndexInsnNode putInsn, boolean canMove) { this.fieldNode = fieldNode; this.putInsn = putInsn; - this.singlePath = singlePath; + this.canMove = canMove; } } @@ -80,6 +83,9 @@ public class ExtractFieldInit extends AbstractVisitor { || classInitMth.getBasicBlocks() == null) { return; } + if (ListUtils.noneMatch(cls.getFields(), FieldNode::isStatic)) { + return; + } while (processStaticFields(cls, classInitMth)) { // sometimes instructions moved to field init prevent from vars inline -> inline and try again CodeShrinkVisitor.shrinkMethod(classInitMth); @@ -116,15 +122,15 @@ public class ExtractFieldInit extends AbstractVisitor { } private static void moveCommonFieldsInit(ClassNode cls) { + if (ListUtils.noneMatch(cls.getFields(), FieldNode::isInstance)) { + return; + } List constructors = getConstructorsList(cls); if (constructors.isEmpty()) { return; } List infoList = new ArrayList<>(constructors.size()); for (MethodNode constructorMth : constructors) { - if (constructorMth.isNoCode()) { - return; - } List inits = collectFieldsInit(cls, constructorMth, InsnType.IPUT); filterFieldsInit(inits); if (inits.isEmpty()) { @@ -168,19 +174,25 @@ public class ExtractFieldInit extends AbstractVisitor { Set singlePathBlocks = new HashSet<>(); BlockUtils.visitSinglePath(mth.getEnterBlock(), singlePathBlocks::add); + boolean canReorder = true; for (BlockNode block : mth.getBasicBlocks()) { for (InsnNode insn : block.getInstructions()) { + boolean fieldInsn = false; if (insn.getType() == putType) { IndexInsnNode putInsn = (IndexInsnNode) insn; FieldInfo field = (FieldInfo) putInsn.getIndex(); if (field.getDeclClass().equals(cls.getClassInfo())) { FieldNode fn = cls.searchField(field); if (fn != null) { - boolean singlePath = singlePathBlocks.contains(block); - fieldsInit.add(new FieldInitInfo(fn, putInsn, singlePath)); + boolean canMove = canReorder && singlePathBlocks.contains(block); + fieldsInit.add(new FieldInitInfo(fn, putInsn, canMove)); + fieldInsn = true; } } } + if (!fieldInsn && canReorder && !insn.canReorder()) { + canReorder = false; + } } } return fieldsInit; @@ -226,14 +238,14 @@ public class ExtractFieldInit extends AbstractVisitor { } private static boolean checkInsn(FieldInitInfo initInfo) { - if (!initInfo.singlePath) { + if (!initInfo.canMove) { return false; } IndexInsnNode insn = initInfo.putInsn; InsnArg arg = insn.getArg(0); if (arg.isInsnWrap()) { InsnNode wrapInsn = ((InsnWrapArg) arg).getWrapInsn(); - if (!wrapInsn.canReorderRecursive() && insn.contains(AType.EXC_CATCH)) { + if (!wrapInsn.canReorder() && insn.contains(AType.EXC_CATCH)) { return false; } } else { @@ -364,7 +376,7 @@ public class ExtractFieldInit extends AbstractVisitor { AccessInfo accFlags = mth.getAccessFlags(); if (!accFlags.isStatic() && accFlags.isConstructor()) { list.add(mth); - if (BlockUtils.isAllBlocksEmpty(mth.getBasicBlocks())) { + if (mth.isNoCode() || BlockUtils.isAllBlocksEmpty(mth.getBasicBlocks())) { return Collections.emptyList(); } } diff --git a/jadx-core/src/main/java/jadx/core/utils/ListUtils.java b/jadx-core/src/main/java/jadx/core/utils/ListUtils.java index 65ffb1f52..fce3fc92e 100644 --- a/jadx-core/src/main/java/jadx/core/utils/ListUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/ListUtils.java @@ -160,6 +160,10 @@ public class ListUtils { return true; } + public static boolean noneMatch(Collection list, Predicate test) { + return !anyMatch(list, test); + } + public static boolean anyMatch(Collection list, Predicate test) { if (list == null || list.isEmpty()) { return false; diff --git a/jadx-core/src/test/java/jadx/tests/api/compiler/TestCompiler.java b/jadx-core/src/test/java/jadx/tests/api/compiler/TestCompiler.java index 6c42ff9f2..15d9acfc6 100644 --- a/jadx-core/src/test/java/jadx/tests/api/compiler/TestCompiler.java +++ b/jadx-core/src/test/java/jadx/tests/api/compiler/TestCompiler.java @@ -113,7 +113,7 @@ public class TestCompiler implements Closeable { assertNotNull(mth, "Failed to get method " + methodName + '(' + Arrays.toString(types) + ')'); return mth.invoke(inst, args); } catch (Throwable e) { - IntegrationTest.rethrow("Invoke error", e); + IntegrationTest.rethrow("Invoke error for method: " + methodName, e); return null; } } diff --git a/jadx-core/src/test/java/jadx/tests/integration/others/TestFieldInitNegative.java b/jadx-core/src/test/java/jadx/tests/integration/others/TestFieldInitNegative.java new file mode 100644 index 000000000..fecc6ab61 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/others/TestFieldInitNegative.java @@ -0,0 +1,49 @@ +package jadx.tests.integration.others; + +import org.junit.jupiter.api.Test; + +import jadx.tests.api.IntegrationTest; + +import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat; + +/** + * Negative case for field initialization move (#1599). + * Can't reorder with other instance methods. + */ +public class TestFieldInitNegative extends IntegrationTest { + + public static class TestCls { + StringBuilder sb; + int field; + + public TestCls() { + initBuilder(new StringBuilder("sb")); + this.field = initField(); + this.sb.append(this.field); + } + + private void initBuilder(StringBuilder sb) { + this.sb = sb; + } + + private int initField() { + return sb.length(); + } + + public String getStr() { + return sb.toString(); + } + + public void check() { + assertThat(new TestCls().getStr()).isEqualTo("sb2"); // no NPE + } + } + + @Test + public void test() { + assertThat(getClassNode(TestCls.class)) + .code() + .doesNotContain("int field = initField();") + .containsOne("this.field = initField();"); + } +}