Load Java parameter names correctly in BinaryJavaMethod

PSI-based implementation (accessible via
`-Xuse-old-class-files-reading`) loads parameter names from the
"MethodParameters" attribute if it's present, so our own implementation
should as well.

This metadata doesn't seem supported in the java.lang.model.element API
though, so SymbolBasedValueParameter (which is used in `-Xuse-javac`)
will continue to have incorrect behavior for now

 #KT-25193 Fixed

compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Annotations.kt

@@ -25,18 +25,34 @@ import org.jetbrains.kotlin.utils.addToStdlib.safeAs
 import org.jetbrains.org.objectweb.asm.*
 import java.lang.reflect.Array
 
-internal class AnnotationsCollectorMethodVisitor(
+internal class AnnotationsAndParameterCollectorMethodVisitor(
         private val member: BinaryJavaMethodBase,
         private val context: ClassifierResolutionContext,
         private val signatureParser: BinaryClassSignatureParser,
         private val parametersToSkipNumber: Int
 ) : MethodVisitor(ASM_API_VERSION_FOR_CLASS_READING) {
+    private var parameterIndex = 0
+
     override fun visitAnnotationDefault(): AnnotationVisitor? {
         member.safeAs<BinaryJavaMethod>()?.hasAnnotationParameterDefaultValue = true
         // We don't store default value in Java model
         return null
     }
 
+    override fun visitParameter(name: String?, access: Int) {
+        if (name != null) {
+            val index = parameterIndex - parametersToSkipNumber
+            if (index >= 0) {
+                val parameter = member.valueParameters.getOrNull(index) ?: error(
+                    "No parameter with index $parameterIndex-$parametersToSkipNumber (name=$name access=$access) " +
+                            "in method ${member.containingClass.fqName}.${member.name}"
+                )
+                parameter.updateName(Name.identifier(name))
+            }
+        }
+        parameterIndex++
+    }
+
     override fun visitAnnotation(desc: String, visible: Boolean) =
             BinaryJavaAnnotation.addAnnotation(
                     member.annotations as MutableCollection<JavaAnnotation>,

compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Methods.kt

@@ -31,7 +31,7 @@ import java.text.StringCharacterIterator
 abstract class BinaryJavaMethodBase(
         override val access: Int,
         override val containingClass: JavaClass,
-        val valueParameters: List<JavaValueParameter>,
+        val valueParameters: List<BinaryJavaValueParameter>,
         val typeParameters: List<JavaTypeParameter>,
         override val name: Name
 ) : JavaMember, MapBasedJavaAnnotationOwner, BinaryJavaModifierListOwner {
@@ -80,13 +80,13 @@ abstract class BinaryJavaMethodBase(
                         }
 
             val parameterTypes = info.valueParameterTypes
-            val parameterList = ContainerUtil.newArrayList<JavaValueParameter>()
+            val parameterList = ContainerUtil.newArrayList<BinaryJavaValueParameter>()
             val paramCount = parameterTypes.size
             for (i in 0..paramCount - 1) {
                 val type = parameterTypes[i]
                 val isEllipsisParam = isVarargs && i == paramCount - 1
 
-                parameterList.add(BinaryJavaValueParameter(null, type, isEllipsisParam))
+                parameterList.add(BinaryJavaValueParameter(type, isEllipsisParam))
             }
 
             val member: BinaryJavaMethodBase =
@@ -106,7 +106,7 @@ abstract class BinaryJavaMethodBase(
                 else -> 0
             }
 
-            return member to AnnotationsCollectorMethodVisitor(member, parentContext, signatureParser, paramIgnoreCount)
+            return member to AnnotationsAndParameterCollectorMethodVisitor(member, parentContext, signatureParser, paramIgnoreCount)
         }
 
         private fun parseMethodDescription(
@@ -155,7 +155,7 @@ abstract class BinaryJavaMethodBase(
 class BinaryJavaMethod(
         flags: Int,
         containingClass: JavaClass,
-        valueParameters: List<JavaValueParameter>,
+        valueParameters: List<BinaryJavaValueParameter>,
         typeParameters: List<JavaTypeParameter>,
         name: Name,
         override val returnType: JavaType
@@ -168,7 +168,7 @@ class BinaryJavaMethod(
 class BinaryJavaConstructor(
         flags: Int,
         containingClass: JavaClass,
-        valueParameters: List<JavaValueParameter>,
+        valueParameters: List<BinaryJavaValueParameter>,
         typeParameters: List<JavaTypeParameter>
 ) : BinaryJavaMethodBase(
         flags, containingClass, valueParameters, typeParameters,

compiler/frontend.java/src/org/jetbrains/kotlin/load/java/structure/impl/classFiles/Other.kt

@@ -50,12 +50,18 @@ class BinaryJavaTypeParameter(
 }
 
 class BinaryJavaValueParameter(
-        override val name: Name?,
         override val type: JavaType,
         override val isVararg: Boolean
 ) : JavaValueParameter, MapBasedJavaAnnotationOwner {
     override val annotations: MutableCollection<JavaAnnotation> = ContainerUtil.newSmartList()
     override val annotationsByFqName by buildLazyValueForMap()
+
+    override var name: Name? = null
+
+    internal fun updateName(newName: Name) {
+        assert(name == null) { "Parameter can't have two names: $name and $newName" }
+        name = newName
+    }
 }
 
 fun isNotTopLevelClass(classContent: ByteArray): Boolean {

compiler/testData/codegen/java8/box/parametersMetadata/delegation.kt

@@ -0,0 +1,35 @@
+// WITH_RUNTIME
+// FULL_JDK
+// JAVAC_OPTIONS: -parameters
+// KOTLIN_CONFIGURATION_FLAGS: +JVM.PARAMETERS_METADATA
+
+// FILE: JavaInterface.java
+
+public interface JavaInterface {
+    void plugin(String id);
+}
+
+// FILE: test.kt
+
+import kotlin.test.assertEquals
+
+interface KotlinInterface {
+    fun plugin(id: String)
+}
+
+class KotlinDelegate(impl: KotlinInterface) : KotlinInterface by impl
+
+class JavaDelegate(impl: JavaInterface) : JavaInterface by impl
+
+private fun check(javaClass: Class<*>) {
+    val pluginMethod = javaClass.getDeclaredMethod("plugin", String::class.java)
+    assertEquals(listOf("id"), pluginMethod.parameters.map { it.name }, "Incorrect parameters for $javaClass")
+}
+
+fun box(): String {
+    check(JavaInterface::class.java)
+    check(KotlinInterface::class.java)
+    check(KotlinDelegate::class.java)
+    check(JavaDelegate::class.java)
+    return "OK"
+}

compiler/testData/loadJava8/compiledJava/ParameterNames.java

@@ -0,0 +1,21 @@
+// JAVAC_OPTIONS: -parameters
+
+package test;
+
+public class ParameterNames {
+    public ParameterNames(long longConstructorParam, String stringConstructorParam) {}
+
+    public void foo(long longMethodParam, int intMethodParam) {}
+
+    static void bar(ParameterNames staticMethodParam) {}
+
+    class Inner {
+        public Inner(double doubleInnerParam, Object objectInnerParam) {}
+    }
+
+    enum Enum {
+        E(0.0, "");
+
+        Enum(double doubleEnumParam, String stringEnumParam) {}
+    }
+}

compiler/testData/loadJava8/compiledJava/ParameterNames.javac.txt

@@ -0,0 +1,29 @@
+package test
+
+public open class ParameterNames {
+    public constructor ParameterNames(/*0*/ p0: kotlin.Long, /*1*/ p1: kotlin.String!)
+    public open fun foo(/*0*/ p0: kotlin.Long, /*1*/ p1: kotlin.Int): kotlin.Unit
+
+    public/*package*/ final enum class Enum : kotlin.Enum<test.ParameterNames.Enum!> {
+        enum entry E
+
+        private constructor Enum(/*0*/ p0: kotlin.Double, /*1*/ p1: kotlin.String!)
+        public final override /*1*/ /*fake_override*/ val name: kotlin.String
+        public final override /*1*/ /*fake_override*/ val ordinal: kotlin.Int
+        protected final override /*1*/ /*fake_override*/ fun clone(): kotlin.Any
+        public final override /*1*/ /*fake_override*/ fun compareTo(/*0*/ other: test.ParameterNames.Enum!): kotlin.Int
+        protected/*protected and package*/ final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun finalize(): kotlin.Unit
+        public final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun getDeclaringClass(): java.lang.Class<test.ParameterNames.Enum!>!
+
+        // Static members
+        public final /*synthesized*/ fun valueOf(/*0*/ value: kotlin.String): test.ParameterNames.Enum
+        public final /*synthesized*/ fun values(): kotlin.Array<test.ParameterNames.Enum>
+    }
+
+    public/*package*/ open inner class Inner {
+        public constructor Inner(/*0*/ p0: kotlin.Double, /*1*/ p1: kotlin.Any!)
+    }
+
+    // Static members
+    public/*package*/ open fun bar(/*0*/ p0: test.ParameterNames!): kotlin.Unit
+}

compiler/testData/loadJava8/compiledJava/ParameterNames.txt

@@ -0,0 +1,29 @@
+package test
+
+public open class ParameterNames {
+    public constructor ParameterNames(/*0*/ longConstructorParam: kotlin.Long, /*1*/ stringConstructorParam: kotlin.String!)
+    public open fun foo(/*0*/ longMethodParam: kotlin.Long, /*1*/ intMethodParam: kotlin.Int): kotlin.Unit
+
+    public/*package*/ final enum class Enum : kotlin.Enum<test.ParameterNames.Enum!> {
+        enum entry E
+
+        private constructor Enum(/*0*/ doubleEnumParam: kotlin.Double, /*1*/ stringEnumParam: kotlin.String!)
+        public final override /*1*/ /*fake_override*/ val name: kotlin.String
+        public final override /*1*/ /*fake_override*/ val ordinal: kotlin.Int
+        protected final override /*1*/ /*fake_override*/ fun clone(): kotlin.Any
+        public final override /*1*/ /*fake_override*/ fun compareTo(/*0*/ other: test.ParameterNames.Enum!): kotlin.Int
+        protected/*protected and package*/ final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun finalize(): kotlin.Unit
+        public final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun getDeclaringClass(): java.lang.Class<test.ParameterNames.Enum!>!
+
+        // Static members
+        public final /*synthesized*/ fun valueOf(/*0*/ value: kotlin.String): test.ParameterNames.Enum
+        public final /*synthesized*/ fun values(): kotlin.Array<test.ParameterNames.Enum>
+    }
+
+    public/*package*/ open inner class Inner {
+        public constructor Inner(/*0*/ doubleInnerParam: kotlin.Double, /*1*/ objectInnerParam: kotlin.Any!)
+    }
+
+    // Static members
+    public/*package*/ open fun bar(/*0*/ staticMethodParam: test.ParameterNames!): kotlin.Unit
+}

compiler/tests-common/tests/org/jetbrains/kotlin/jvm/compiler/LoadDescriptorUtil.java

@@ -19,10 +19,8 @@ package org.jetbrains.kotlin.jvm.compiler;
 import com.intellij.openapi.Disposable;
 import com.intellij.openapi.util.Pair;
 import com.intellij.openapi.util.io.FileUtil;
-import com.intellij.util.text.StringKt;
 import kotlin.collections.CollectionsKt;
 import kotlin.io.FilesKt;
-import kotlin.sequences.SequencesKt;
 import kotlin.text.Charsets;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -118,14 +116,21 @@ public class LoadDescriptorUtil {
     }
 
     public static void compileJavaWithAnnotationsJar(@NotNull Collection<File> javaFiles, @NotNull File outDir) throws IOException {
+        List<String> args = new ArrayList<>(Arrays.asList(
+                "-sourcepath", "compiler/testData/loadJava/include",
+                "-d", outDir.getPath())
+        );
+
         List<File> classpath = new ArrayList<>();
 
         classpath.add(ForTestCompileRuntime.runtimeJarForTests());
         classpath.add(KotlinTestUtils.getAnnotationsJar());
 
-        for (File test: javaFiles) {
+        for (File test : javaFiles) {
             String content = FilesKt.readText(test, Charsets.UTF_8);
 
+            args.addAll(InTextDirectivesUtils.findListWithPrefixes(content, "JAVAC_OPTIONS:"));
+
             if (InTextDirectivesUtils.isDirectiveDefined(content, "ANDROID_ANNOTATIONS")) {
                 classpath.add(ForTestCompileRuntime.androidAnnotationsForTests());
             }
@@ -135,11 +140,10 @@ public class LoadDescriptorUtil {
             }
         }
 
-        KotlinTestUtils.compileJavaFiles(javaFiles, Arrays.asList(
-                "-classpath", classpath.stream().map(File::getPath).collect(Collectors.joining(File.pathSeparator)),
-                "-sourcepath", "compiler/testData/loadJava/include",
-                "-d", outDir.getPath()
-        ));
+        args.add("-classpath");
+        args.add(classpath.stream().map(File::getPath).collect(Collectors.joining(File.pathSeparator)));
+
+        KotlinTestUtils.compileJavaFiles(javaFiles, args);
     }
 
     @NotNull

compiler/tests-java8/tests/org/jetbrains/kotlin/codegen/BlackBoxWithJava8CodegenTestGenerated.java

@@ -711,6 +711,11 @@ public class BlackBoxWithJava8CodegenTestGenerated extends AbstractBlackBoxCodeg
             runTest("compiler/testData/codegen/java8/box/parametersMetadata/defaultImpls.kt");
         }
 
+        @TestMetadata("delegation.kt")
+        public void testDelegation() throws Exception {
+            runTest("compiler/testData/codegen/java8/box/parametersMetadata/delegation.kt");
+        }
+
         @TestMetadata("enum.kt")
         public void testEnum() throws Exception {
             runTest("compiler/testData/codegen/java8/box/parametersMetadata/enum.kt");

compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/LoadJava8TestGenerated.java

@@ -41,6 +41,11 @@ public class LoadJava8TestGenerated extends AbstractLoadJava8Test {
             runTest("compiler/testData/loadJava8/compiledJava/MapRemove.java");
         }
 
+        @TestMetadata("ParameterNames.java")
+        public void testParameterNames() throws Exception {
+            runTest("compiler/testData/loadJava8/compiledJava/ParameterNames.java");
+        }
+
         @TestMetadata("TypeAnnotations.java")
         public void testTypeAnnotations() throws Exception {
             runTest("compiler/testData/loadJava8/compiledJava/TypeAnnotations.java");

compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/LoadJava8WithFastClassReadingTestGenerated.java

@@ -39,6 +39,11 @@ public class LoadJava8WithFastClassReadingTestGenerated extends AbstractLoadJava
         runTest("compiler/testData/loadJava8/compiledJava/MapRemove.java");
     }
 
+    @TestMetadata("ParameterNames.java")
+    public void testParameterNames() throws Exception {
+        runTest("compiler/testData/loadJava8/compiledJava/ParameterNames.java");
+    }
+
     @TestMetadata("TypeAnnotations.java")
     public void testTypeAnnotations() throws Exception {
         runTest("compiler/testData/loadJava8/compiledJava/TypeAnnotations.java");

compiler/tests-java8/tests/org/jetbrains/kotlin/jvm/compiler/javac/LoadJava8UsingJavacTestGenerated.java

@@ -41,6 +41,11 @@ public class LoadJava8UsingJavacTestGenerated extends AbstractLoadJava8UsingJava
             runTest("compiler/testData/loadJava8/compiledJava/MapRemove.java");
         }
 
+        @TestMetadata("ParameterNames.java")
+        public void testParameterNames() throws Exception {
+            runTest("compiler/testData/loadJava8/compiledJava/ParameterNames.java");
+        }
+
         @TestMetadata("TypeAnnotations.java")
         public void testTypeAnnotations() throws Exception {
             runTest("compiler/testData/loadJava8/compiledJava/TypeAnnotations.java");

core/descriptors.runtime/tests/org/jetbrains/kotlin/jvm/runtime/Jvm8RuntimeDescriptorLoaderTestGenerated.java

@@ -39,6 +39,11 @@ public class Jvm8RuntimeDescriptorLoaderTestGenerated extends AbstractJvm8Runtim
         runTest("compiler/testData/loadJava8/compiledJava/MapRemove.java");
     }
 
+    @TestMetadata("ParameterNames.java")
+    public void testParameterNames() throws Exception {
+        runTest("compiler/testData/loadJava8/compiledJava/ParameterNames.java");
+    }
+
     @TestMetadata("TypeAnnotations.java")
     public void testTypeAnnotations() throws Exception {
         runTest("compiler/testData/loadJava8/compiledJava/TypeAnnotations.java");