From 358136a97feff6fd0586fe04a39adbc7ced381c1 Mon Sep 17 00:00:00 2001 From: Jonas Herzig Date: Fri, 12 Nov 2021 14:19:18 +0100 Subject: 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. --- .../kotlin/com/replaymod/gradle/remap/PsiMapper.kt | 25 +++++++++++++++++----- .../com/replaymod/gradle/remap/Transformer.kt | 2 +- .../mapper/kotlin/TestKotlinSyntheticProperties.kt | 16 ++++++++++++++ src/test/resources/mappings.srg | 1 + src/testA/java/a/pkg/A.java | 5 +++++ src/testB/java/b/pkg/B.java | 5 +++++ 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>() @@ -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()?.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>? { for (pair in annotation.parameterList.attributes) { @@ -510,7 +525,7 @@ internal class PsiMapper( } } - fun remapFile(bindingContext: BindingContext): Pair>> { + fun remapFile(): Pair>> { 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() { } -- cgit