Consider location of expression when determining field accessibility

A protected field is not accessible unless we are referencing it from within a
class which extends its owner. Therefore, we may use a synthetic property with
the same name as long as we do not have access to the field.

6 files changed, 48 insertions, 6 deletions

diff --git a/src/main/kotlin/com/replaymod/gradle/remap/PsiMapper.kt b/src/main/kotlin/com/replaymod/gradle/remap/PsiMapper.kt
index 0ad498b..58224a5 100644
--- a/src/main/kotlin/com/replaymod/gradle/remap/PsiMapper.kt
+++ b/src/main/kotlin/com/replaymod/gradle/remap/PsiMapper.kt
@@ -20,7 +20,11 @@ import org.jetbrains.kotlin.js.resolve.diagnostics.findPsi
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.name.Name
 import org.jetbrains.kotlin.psi.*
+import org.jetbrains.kotlin.psi.psiUtil.getNonStrictParentOfType
+import org.jetbrains.kotlin.psi.synthetics.findClassDescriptor
 import org.jetbrains.kotlin.resolve.BindingContext
+import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameOrNull
+import org.jetbrains.kotlin.resolve.descriptorUtil.getAllSuperclassesWithoutAny
 import org.jetbrains.kotlin.resolve.descriptorUtil.overriddenTreeAsSequence
 import org.jetbrains.kotlin.synthetic.SyntheticJavaPropertyDescriptor
 import org.jetbrains.kotlin.synthetic.SyntheticJavaPropertyDescriptor.Companion.propertyNameByGetMethodName
@@ -30,6 +34,7 @@ internal class PsiMapper(
         private val map: MappingSet,
         private val remappedProject: Project?,
         private val file: PsiFile,
+        private val bindingContext: BindingContext,
         private val patterns: PsiPatterns?
 ) {
     private val mixinMappings = mutableMapOf<String, ClassMapping<*, *>>()
@@ -128,7 +133,7 @@ internal class PsiMapper(
                     // `super.getDebugInfo()` is a special case which cannot be replaced with a synthetic property
                     && expr.parent.parent.let { it !is KtDotQualifiedExpression || it.firstChild !is KtSuperExpression }
                     // cannot use synthetic properties if there is a field of the same name
-                    && !isSyntheticPropertyShadowedByField(maybeGetter.identifier, mapping)
+                    && !isSyntheticPropertyShadowedByField(maybeGetter.identifier, mapping, expr)
                     // cannot use synthetic properties outside of kotlin files (cause they're a kotlin thing)
                     && expr.containingFile  is KtFile) {
                 // E.g. `entity.canUsePortal()` maps to `entity.isNonBoss()` but when we're using kotlin and the target
@@ -159,7 +164,7 @@ internal class PsiMapper(
         val mappedGetter = mapping?.deobfuscatedName ?: return
         if (mappedGetter != getter.name) {
             val maybeMapped = propertyNameByGetMethodName(Name.identifier(mappedGetter))
-            if (maybeMapped == null || isSyntheticPropertyShadowedByField(maybeMapped.identifier, mapping)) {
+            if (maybeMapped == null || isSyntheticPropertyShadowedByField(maybeMapped.identifier, mapping, expr)) {
                 // Can happen if a method is a synthetic property in the current mapping (e.g. `isNonBoss`) but not
                 // in the target mapping (e.g. `canUsePortal()`)
                 // This is the reverse to the operation in [map(PsiElement, PsiMethod)].
@@ -263,12 +268,22 @@ internal class PsiMapper(
         }
     }
 
-    private fun isSyntheticPropertyShadowedByField(propertyName: String, mapping: MethodMapping): Boolean {
+    private fun isSyntheticPropertyShadowedByField(propertyName: String, mapping: MethodMapping, expr: PsiElement): Boolean {
         val cls = findPsiClass(mapping.parent.fullDeobfuscatedName, remappedProject ?: file.project) ?: return false
         val field = cls.findFieldByName(propertyName, true) ?: return false
-        return field.hasModifier(JvmModifier.PUBLIC) || field.hasModifier(JvmModifier.PROTECTED)
+
+        val canAccessProtected = expr.getNonStrictParentOfType<KtClassOrObject>()?.extends(
+            mapping.parent.fullObfuscatedName.replace('/', '.')
+        ) == true
+
+        return field.hasModifier(JvmModifier.PUBLIC) || (canAccessProtected && field.hasModifier(JvmModifier.PROTECTED))
     }
 
+    private fun KtClassOrObject.extends(className: String): Boolean =
+        findClassDescriptor(bindingContext)
+            .getAllSuperclassesWithoutAny()
+            .any { it.fqNameOrNull()?.asString() == className }
+
     // Note: Supports only Mixins with a single target (ignores others)
     private fun getMixinTarget(annotation: PsiAnnotation): Pair<PsiClass, ClassMapping<*, *>>? {
         for (pair in annotation.parameterList.attributes) {
@@ -510,7 +525,7 @@ internal class PsiMapper(
         }
     }
 
-    fun remapFile(bindingContext: BindingContext): Pair<String, List<Pair<Int, String>>> {
+    fun remapFile(): Pair<String, List<Pair<Int, String>>> {
         if (patterns != null) {
             file.accept(object : JavaRecursiveElementVisitor() {
                 override fun visitCodeBlock(block: PsiCodeBlock) {
diff --git a/src/main/kotlin/com/replaymod/gradle/remap/Transformer.kt b/src/main/kotlin/com/replaymod/gradle/remap/Transformer.kt
index 0fd8093..24f41ce 100644
--- a/src/main/kotlin/com/replaymod/gradle/remap/Transformer.kt
+++ b/src/main/kotlin/com/replaymod/gradle/remap/Transformer.kt
@@ -122,7 +122,7 @@ class Transformer(private val map: MappingSet) {
                 val psiFile = psiManager.findFile(file)!!
 
                 val mapped = try {
-                    PsiMapper(map, remappedProject, psiFile, patterns).remapFile(analysis.bindingContext)
+                    PsiMapper(map, remappedProject, psiFile, analysis.bindingContext, patterns).remapFile()
                 } catch (e: Exception) {
                     throw RuntimeException("Failed to map file \"$name\".", e)
                 }
diff --git a/src/test/kotlin/com/replaymod/gradle/remap/mapper/kotlin/TestKotlinSyntheticProperties.kt b/src/test/kotlin/com/replaymod/gradle/remap/mapper/kotlin/TestKotlinSyntheticProperties.kt
index 20b65b9..65459f2 100644
--- a/src/test/kotlin/com/replaymod/gradle/remap/mapper/kotlin/TestKotlinSyntheticProperties.kt
+++ b/src/test/kotlin/com/replaymod/gradle/remap/mapper/kotlin/TestKotlinSyntheticProperties.kt
@@ -103,9 +103,15 @@ class TestKotlinSyntheticProperties {
         TestData.remapKt("""
             import a.pkg.A
             val v = A().getConflictingFieldWithoutConflict()
+            class C : A() {
+                val v = getProtectedFieldWithoutConflict()
+            }
         """.trimIndent()) shouldBe """
             import b.pkg.B
             val v = B().getConflictingField()
+            class C : B() {
+                val v = getProtectedField()
+            }
         """.trimIndent()
     }
 
@@ -114,9 +120,11 @@ class TestKotlinSyntheticProperties {
         TestData.remapKt("""
             import a.pkg.A
             val v = A().getA()
+            val v = A().getProtectedFieldWithoutConflict()
         """.trimIndent()) shouldBe """
             import b.pkg.B
             val v = B().b
+            val v = B().protectedField
         """.trimIndent()
     }
 
@@ -125,9 +133,15 @@ class TestKotlinSyntheticProperties {
         TestData.remapKt("""
             import a.pkg.A
             val v = A().conflictingFieldWithoutConflict
+            class C : A() {
+                val v = protectedFieldWithoutConflict
+            }
         """.trimIndent()) shouldBe """
             import b.pkg.B
             val v = B().getConflictingField()
+            class C : B() {
+                val v = getProtectedField()
+            }
         """.trimIndent()
     }
 
@@ -136,9 +150,11 @@ class TestKotlinSyntheticProperties {
         TestData.remapKt("""
             import a.pkg.A
             val v = A().a
+            val v = A().protectedFieldWithoutConflict
         """.trimIndent()) shouldBe """
             import b.pkg.B
             val v = B().b
+            val v = B().protectedField
         """.trimIndent()
     }
 
diff --git a/src/test/resources/mappings.srg b/src/test/resources/mappings.srg
index f8255aa..65cad17 100644
--- a/src/test/resources/mappings.srg
+++ b/src/test/resources/mappings.srg
@@ -17,6 +17,7 @@ MD: a/pkg/A/getterBooleanA ()Z; b/pkg/B/isNonSyntheticBooleanB ()Z;
 MD: a/pkg/A/setterBooleanA (Z)V; b/pkg/B/setNonSyntheticBooleanB (Z)V;
 FD: a/pkg/A/conflictingField b/pkg/B/conflictingField
 MD: a/pkg/A/getConflictingFieldWithoutConflict ()I; b/pkg/B/getConflictingField ()I;
+MD: a/pkg/A/getProtectedFieldWithoutConflict ()I b/pkg/B/getProtectedField ()I
 MD: a/pkg/A/aOverloaded ()V b/pkg/B/bOverloaded ()V
 MD: a/pkg/A/aOverloaded (I)V b/pkg/B/bOverloaded (I)V
 MD: a/pkg/A/aOverloaded (Z)V b/pkg/B/bOverloaded (Z)V
diff --git a/src/testA/java/a/pkg/A.java b/src/testA/java/a/pkg/A.java
index f4a30ef..1a0dcac 100644
--- a/src/testA/java/a/pkg/A.java
+++ b/src/testA/java/a/pkg/A.java
@@ -67,6 +67,11 @@ public class A extends AParent implements AInterface {
         return conflictingField;
     }
 
+    protected int protectedField;
+    public int getProtectedFieldWithoutConflict() {
+        return protectedField;
+    }
+
     public void aOverloaded() {
     }
 
diff --git a/src/testB/java/b/pkg/B.java b/src/testB/java/b/pkg/B.java
index 150ab0b..67d2ea8 100644
--- a/src/testB/java/b/pkg/B.java
+++ b/src/testB/java/b/pkg/B.java
@@ -67,6 +67,11 @@ public class B extends BParent implements BInterface {
         return conflictingField;
     }
 
+    protected int protectedField;
+    public int getProtectedFieldWithoutConflict() {
+        return protectedField;
+    }
+
     public void bOverloaded() {
     }