fix: more visibility checks for @Override (#984)

Signed-off-by: Skylot <skylot@gmail.com>

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

@@ -31,14 +31,13 @@ public class OverrideMethodVisitor extends AbstractVisitor {
 
 	@Override
 	public boolean visit(ClassNode cls) throws JadxException {
-		RootNode root = cls.root();
 		List<ArgType> superTypes = collectSuperTypes(cls);
 		for (MethodNode mth : cls.getMethods()) {
 			if (mth.isConstructor() || mth.getAccessFlags().isStatic()) {
 				continue;
 			}
 			String signature = mth.getMethodInfo().makeSignature(false);
-			List<IMethodDetails> overrideList = collectOverrideMethods(root, superTypes, signature);
+			List<IMethodDetails> overrideList = collectOverrideMethods(cls, superTypes, signature);
 			if (!overrideList.isEmpty()) {
 				mth.addAttr(new MethodOverrideAttr(overrideList));
 				fixMethodReturnType(mth, overrideList, superTypes);
@@ -48,15 +47,14 @@ public class OverrideMethodVisitor extends AbstractVisitor {
 		return true;
 	}
 
-	private List<IMethodDetails> collectOverrideMethods(RootNode root, List<ArgType> superTypes, String signature) {
+	private List<IMethodDetails> collectOverrideMethods(ClassNode cls, List<ArgType> superTypes, String signature) {
 		List<IMethodDetails> overrideList = new ArrayList<>();
 		for (ArgType superType : superTypes) {
-			ClassNode classNode = root.resolveClass(superType);
+			ClassNode classNode = cls.root().resolveClass(superType);
 			if (classNode != null) {
 				for (MethodNode mth : classNode.getMethods()) {
-					AccessInfo accessFlags = mth.getAccessFlags();
-					if (!accessFlags.isPrivate()
-							&& !accessFlags.isStatic()) {
+					if (!mth.getAccessFlags().isStatic()
+							&& isMethodVisibleInCls(mth, cls)) {
 						String mthShortId = mth.getMethodInfo().getShortId();
 						if (mthShortId.startsWith(signature)) {
 							overrideList.add(mth);
@@ -64,7 +62,7 @@ public class OverrideMethodVisitor extends AbstractVisitor {
 					}
 				}
 			} else {
-				ClspClass clsDetails = root.getClsp().getClsDetails(superType);
+				ClspClass clsDetails = cls.root().getClsp().getClsDetails(superType);
 				if (clsDetails != null) {
 					Map<String, ClspMethod> methodsMap = clsDetails.getMethodsMap();
 					for (Map.Entry<String, ClspMethod> entry : methodsMap.entrySet()) {
@@ -79,6 +77,21 @@ public class OverrideMethodVisitor extends AbstractVisitor {
 		return overrideList;
 	}
 
+	/**
+	 * NOTE: Simplified version of method from {@link ModVisitor#isFieldVisibleInMethod}
+	 */
+	private boolean isMethodVisibleInCls(MethodNode superMth, ClassNode cls) {
+		AccessInfo accessFlags = superMth.getAccessFlags();
+		if (accessFlags.isPrivate()) {
+			return false;
+		}
+		if (accessFlags.isPublic() || accessFlags.isProtected()) {
+			return true;
+		}
+		// package-private
+		return Objects.equals(superMth.getParentClass().getPackage(), cls.getPackage());
+	}
+
 	private List<ArgType> collectSuperTypes(ClassNode cls) {
 		Map<String, ArgType> superTypes = new HashMap<>();
 		collectSuperTypes(cls, superTypes);

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

@@ -0,0 +1,67 @@
+package jadx.tests.integration.others;
+
+import java.util.List;
+
+import org.junit.jupiter.api.Test;
+
+import jadx.NotYetImplemented;
+import jadx.core.dex.nodes.ClassNode;
+import jadx.tests.api.SmaliTest;
+
+import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat;
+
+public class TestOverridePackagePrivateMethod extends SmaliTest {
+	// @formatter:off
+	/*
+		-----------------------------------------------------------
+		package test;
+
+		public class A {
+			void a() { // package-private
+			}
+		}
+		-----------------------------------------------------------
+		package test;
+
+		public class B extends A {
+			@Override // test.A
+			public void a() {
+			}
+		}
+		-----------------------------------------------------------
+		package other;
+
+		import test.A;
+
+		public class C extends A {
+			// No @Override here
+			public void a() {
+			}
+		}
+		-----------------------------------------------------------
+	*/
+	// @formatter:on
+
+	@NotYetImplemented("Don't change access modifiers if not needed")
+	@Test
+	public void test() {
+		commonChecks();
+	}
+
+	@Test
+	public void testDontChangeAccFlags() {
+		getArgs().setRespectBytecodeAccModifiers(true);
+		commonChecks();
+	}
+
+	private void commonChecks() {
+		List<ClassNode> classes = loadFromSmaliFiles();
+		assertThat(searchCls(classes, "test.A"))
+				.code()
+				.doesNotContain("/* access modifiers changed")
+				.containsLine(1, "void a() {");
+
+		assertThat(searchCls(classes, "test.B")).code().containsOne("@Override");
+		assertThat(searchCls(classes, "other.C")).code().doesNotContain("@Override");
+	}
+}

jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/A.smali

@@ -0,0 +1,7 @@
+.class public Ltest/A;
+.super Ljava/lang/Object;
+
+.method a()V # package-private
+    .locals 1
+    return-void
+.end method

jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/B.smali

@@ -0,0 +1,7 @@
+.class public Ltest/B;
+.super Ltest/A;
+
+.method public a()V
+    .locals 1
+    return-void
+.end method

jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/C.smali

@@ -0,0 +1,7 @@
+.class public Lother/C;
+.super Ltest/A;
+
+.method public a()V
+    .locals 1
+    return-void
+.end method