Fix synthetic property becoming shadowed by field of same name

7 files changed, 103 insertions, 4 deletions

diff --git a/src/main/kotlin/com/replaymod/gradle/remap/PsiMapper.kt b/src/main/kotlin/com/replaymod/gradle/remap/PsiMapper.kt
index 29ee7cf..13bcc14 100644
--- a/src/main/kotlin/com/replaymod/gradle/remap/PsiMapper.kt
+++ b/src/main/kotlin/com/replaymod/gradle/remap/PsiMapper.kt
@@ -8,6 +8,7 @@ import org.cadixdev.lorenz.model.MethodMapping
 import org.jetbrains.kotlin.asJava.getRepresentativeLightMethod
 import org.jetbrains.kotlin.com.intellij.lang.ASTNode
 import org.jetbrains.kotlin.com.intellij.lang.jvm.JvmModifier
+import org.jetbrains.kotlin.com.intellij.openapi.project.Project
 import org.jetbrains.kotlin.com.intellij.openapi.util.TextRange
 import org.jetbrains.kotlin.com.intellij.openapi.util.text.StringUtil
 import org.jetbrains.kotlin.com.intellij.psi.*
@@ -26,6 +27,7 @@ import java.util.*
 
 internal class PsiMapper(
         private val map: MappingSet,
+        private val remappedProject: Project?,
         private val file: PsiFile,
         private val patterns: PsiPatterns?
 ) {
@@ -114,7 +116,8 @@ internal class PsiMapper(
             return
         }
 
-        val mapped = findMapping(method)?.deobfuscatedName
+        val mapping = findMapping(method)
+        val mapped = mapping?.deobfuscatedName
         if (mapped != null && mapped != method.name) {
             val maybeGetter = propertyNameByGetMethodName(Name.identifier(mapped))
             if (maybeGetter != null // must have getter-style name
@@ -123,6 +126,8 @@ internal class PsiMapper(
                     && !method.hasModifier(JvmModifier.STATIC) // synthetic properties cannot be static
                     // `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)
                     // 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
@@ -146,10 +151,11 @@ internal class PsiMapper(
 
     private fun map(expr: PsiElement, property: SyntheticJavaPropertyDescriptor) {
         val getter = property.getMethod.findPsi() as? PsiMethod ?: return
-        val mappedGetter = findMapping(getter)?.deobfuscatedName ?: return
+        val mapping = findMapping(getter)
+        val mappedGetter = mapping?.deobfuscatedName ?: return
         if (mappedGetter != getter.name) {
             val maybeMapped = propertyNameByGetMethodName(Name.identifier(mappedGetter))
-            if (maybeMapped == null) {
+            if (maybeMapped == null || isSyntheticPropertyShadowedByField(maybeMapped.identifier, mapping)) {
                 // 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)].
@@ -253,6 +259,12 @@ internal class PsiMapper(
         }
     }
 
+    private fun isSyntheticPropertyShadowedByField(propertyName: String, mapping: MethodMapping): 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)
+    }
+
     // Note: Supports only Mixins with a single target (ignores others)
     private fun getMixinTarget(annotation: PsiAnnotation): Pair<PsiClass, ClassMapping<*, *>>? {
         for (pair in annotation.parameterList.attributes) {
diff --git a/src/main/kotlin/com/replaymod/gradle/remap/Transformer.kt b/src/main/kotlin/com/replaymod/gradle/remap/Transformer.kt
index dd4215f..0fd8093 100644
--- a/src/main/kotlin/com/replaymod/gradle/remap/Transformer.kt
+++ b/src/main/kotlin/com/replaymod/gradle/remap/Transformer.kt
@@ -16,8 +16,10 @@ import org.jetbrains.kotlin.cli.jvm.config.JavaSourceRoot
 import org.jetbrains.kotlin.cli.jvm.config.JvmClasspathRoot
 import org.jetbrains.kotlin.com.intellij.codeInsight.CustomExceptionHandler
 import org.jetbrains.kotlin.com.intellij.mock.MockProject
+import org.jetbrains.kotlin.com.intellij.openapi.Disposable
 import org.jetbrains.kotlin.com.intellij.openapi.extensions.ExtensionPoint
 import org.jetbrains.kotlin.com.intellij.openapi.extensions.Extensions
+import org.jetbrains.kotlin.com.intellij.openapi.project.Project
 import org.jetbrains.kotlin.com.intellij.openapi.util.Disposer
 import org.jetbrains.kotlin.com.intellij.openapi.vfs.StandardFileSystems
 import org.jetbrains.kotlin.com.intellij.openapi.vfs.VirtualFileManager
@@ -41,6 +43,7 @@ import kotlin.system.exitProcess
 
 class Transformer(private val map: MappingSet) {
     var classpath: Array<String>? = null
+    var remappedClasspath: Array<String>? = null
     var patternAnnotation: String? = null
 
     @Throws(IOException::class)
@@ -95,6 +98,8 @@ class Transformer(private val map: MappingSet) {
                     { scope: GlobalSearchScope -> environment.createPackagePartProvider(scope) }
             )
 
+            val remappedProject = remappedClasspath?.let { setupRemappedProject(disposable, it) }
+
             val patterns = patternAnnotation?.let { annotationFQN ->
                 val patterns = PsiPatterns(annotationFQN)
                 val annotationName = annotationFQN.substring(annotationFQN.lastIndexOf('.') + 1)
@@ -117,7 +122,7 @@ class Transformer(private val map: MappingSet) {
                 val psiFile = psiManager.findFile(file)!!
 
                 val mapped = try {
-                    PsiMapper(map, psiFile, patterns).remapFile(analysis.bindingContext)
+                    PsiMapper(map, remappedProject, psiFile, patterns).remapFile(analysis.bindingContext)
                 } catch (e: Exception) {
                     throw RuntimeException("Failed to map file \"$name\".", e)
                 }
@@ -130,6 +135,28 @@ class Transformer(private val map: MappingSet) {
         }
     }
 
+    private fun setupRemappedProject(disposable: Disposable, classpath: Array<String>): Project {
+        val config = CompilerConfiguration()
+        config.put(CommonConfigurationKeys.MODULE_NAME, "main")
+        config.addAll(CLIConfigurationKeys.CONTENT_ROOTS, classpath.map { JvmClasspathRoot(File(it)) })
+        config.put(CLIConfigurationKeys.MESSAGE_COLLECTOR_KEY, PrintingMessageCollector(System.err, MessageRenderer.GRADLE_STYLE, true))
+
+        val environment = KotlinCoreEnvironment.createForProduction(
+            disposable,
+            config,
+            EnvironmentConfigFiles.JVM_CONFIG_FILES
+        )
+        val project = environment.project
+        TopDownAnalyzerFacadeForJVM.analyzeFilesWithJavaIntegration(
+            project,
+            emptyList(),
+            NoScopeRecordCliBindingTrace(),
+            environment.configuration,
+            { scope: GlobalSearchScope -> environment.createPackagePartProvider(scope) }
+        )
+        return environment.project
+    }
+
     companion object {
 
         @Throws(IOException::class)
diff --git a/src/test/java/a/pkg/A.java b/src/test/java/a/pkg/A.java
index fd177e5..f4a30ef 100644
--- a/src/test/java/a/pkg/A.java
+++ b/src/test/java/a/pkg/A.java
@@ -62,6 +62,11 @@ public class A extends AParent implements AInterface {
     public void setterBooleanA(boolean arg) {
     }
 
+    public int conflictingField;
+    public int getConflictingFieldWithoutConflict() {
+        return conflictingField;
+    }
+
     public void aOverloaded() {
     }
 
diff --git a/src/test/java/b/pkg/B.java b/src/test/java/b/pkg/B.java
index b153be4..150ab0b 100644
--- a/src/test/java/b/pkg/B.java
+++ b/src/test/java/b/pkg/B.java
@@ -62,6 +62,11 @@ public class B extends BParent implements BInterface {
     public void setterBooleanB(boolean arg) {
     }
 
+    public int conflictingField;
+    public int getConflictingField() {
+        return conflictingField;
+    }
+
     public void bOverloaded() {
     }
 
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 d0f6fc2..b77978a 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
@@ -97,4 +97,48 @@ class TestKotlinSyntheticProperties {
             }
         """.trimIndent()
     }
+
+    @Test
+    fun `does not convert getter to synthetic property if it would be shadowed by a field with the same name`() {
+        TestData.remapKt("""
+            import a.pkg.A
+            val v = A().getConflictingFieldWithoutConflict()
+        """.trimIndent()) shouldBe """
+            import b.pkg.B
+            val v = B().getConflictingField()
+        """.trimIndent()
+    }
+
+    @Test
+    fun `does convert getter to synthetic property if the field which it would be shadowed by is inaccessible`() {
+        TestData.remapKt("""
+            import a.pkg.A
+            val v = A().getA()
+        """.trimIndent()) shouldBe """
+            import b.pkg.B
+            val v = B().b
+        """.trimIndent()
+    }
+
+    @Test
+    fun `convert synthetic property to getter if it would be shadowed by a field with the same name`() {
+        TestData.remapKt("""
+            import a.pkg.A
+            val v = A().conflictingFieldWithoutConflict
+        """.trimIndent()) shouldBe """
+            import b.pkg.B
+            val v = B().getConflictingField()
+        """.trimIndent()
+    }
+
+    @Test
+    fun `does not convert synthetic property to getter if the field which it would be shadowed by is inaccessible`() {
+        TestData.remapKt("""
+            import a.pkg.A
+            val v = A().a
+        """.trimIndent()) shouldBe """
+            import b.pkg.B
+            val v = B().b
+        """.trimIndent()
+    }
 }
diff --git a/src/test/kotlin/com/replaymod/gradle/remap/util/TestData.kt b/src/test/kotlin/com/replaymod/gradle/remap/util/TestData.kt
index adb6f52..c40635c 100644
--- a/src/test/kotlin/com/replaymod/gradle/remap/util/TestData.kt
+++ b/src/test/kotlin/com/replaymod/gradle/remap/util/TestData.kt
@@ -41,6 +41,10 @@ object TestData {
             findClasspathEntry("org.spongepowered.asm.mixin.Mixin"),
             findClasspathEntry("a.pkg.A"),
         )
+        remappedClasspath = arrayOf(
+            findClasspathEntry("org.spongepowered.asm.mixin.Mixin"),
+            findClasspathEntry("b.pkg.B"),
+        )
         patternAnnotation = "remap.Pattern"
     }
 
diff --git a/src/test/resources/mappings.srg b/src/test/resources/mappings.srg
index df3d7ef..f8255aa 100644
--- a/src/test/resources/mappings.srg
+++ b/src/test/resources/mappings.srg
@@ -15,6 +15,8 @@ MD: a/pkg/A/getterA ()La/pkg/A; b/pkg/B/getNonSyntheticB ()Lb/pkg/B;
 MD: a/pkg/A/setterA (La/pkg/A;)V; b/pkg/B/setNonSyntheticB (Lb/pkg/B;)V;
 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/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