From 5723e6481d2f4e07b6820201e74924643b5687bc Mon Sep 17 00:00:00 2001 From: Jonas Herzig Date: Fri, 12 Nov 2021 10:58:10 +0100 Subject: Fix synthetic property becoming shadowed by field of same name --- .../kotlin/com/replaymod/gradle/remap/PsiMapper.kt | 18 +++++++-- .../com/replaymod/gradle/remap/Transformer.kt | 29 +++++++++++++- src/test/java/a/pkg/A.java | 5 +++ src/test/java/b/pkg/B.java | 5 +++ .../mapper/kotlin/TestKotlinSyntheticProperties.kt | 44 ++++++++++++++++++++++ .../com/replaymod/gradle/remap/util/TestData.kt | 4 ++ src/test/resources/mappings.srg | 2 + 7 files changed, 103 insertions(+), 4 deletions(-) (limited to 'src') 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>? { 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? = null + var remappedClasspath: Array? = 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): 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 -- cgit