fix: stop Kotlin internal members leaking into metadata and typings

Kotlin compiles internal declarations down to public bytecode, so the
@kotlin.Metadata annotation is the only way to tell them apart.

The metadata generator consulted it for functions only. Property accessors are
described under `properties` rather than `functions`, so an internal property's
getter and setter were treated as public and stayed callable from JS under
their mangled names - names that also differ between debug and release builds,
which left nothing able to depend on them safely.

Typings gain the same filtering through the android-dts-generator submodule,
which now reads the annotation too and can therefore also drop internal classes.

Three build fixes come along with it:

- buildMetadata and generateTypescriptDefinitions declare their generator jars
  as inputs, so editing a generator regenerates its output instead of leaving
  the task up to date.
- generateTypescriptDefinitions passes an absolute path to dts-generator.jar
  the way the other two build tools already do. It previously only resolved
  from a packaged framework and failed from a source checkout.
- copyTypings no longer reports success after a failed generation, and copies
  only when there is a project to copy into. USER_PROJECT_ROOT sits two levels
  above the gradle root, which outside an app lands beyond the repository.

Typings generated from android.jar are unchanged.

Author: edusperoni

test-app/app/build.gradle

@@ -742,6 +742,9 @@ task buildMetadata(type: BuildToolTask) {
     if (!findProject(':android-metadata-generator').is(null)) {
         rootPath = Paths.get(project(':android-metadata-generator').projectDir.path, "build/libs").toString()
         dependsOn ':android-metadata-generator:jar'
+        // Without this the task stays up-to-date after the generator changes, so edits to it
+        // silently produce no new metadata.
+        inputs.files(tasks.getByPath(':android-metadata-generator:jar'))
     }
 
 
@@ -1031,8 +1034,11 @@ task buildMetadata(type: BuildToolTask) {
 }
 
 task generateTypescriptDefinitions(type: BuildToolTask) {
+    def rootPath = ""
     if (!findProject(':dts-generator').is(null)) {
+        rootPath = Paths.get(project(':dts-generator').projectDir.path, "build/libs").toString()
         dependsOn ':dts-generator:jar'
+        inputs.files(tasks.getByPath(':dts-generator:jar'))
     }
 
     def paramz = new ArrayList<String>()
@@ -1044,7 +1050,9 @@ task generateTypescriptDefinitions(type: BuildToolTask) {
     doFirst {
         delete "$TYPINGS_PATH"
 
-        paramz.add("dts-generator.jar")
+        // Resolved against the project when building from source; an empty rootPath leaves it
+        // relative to workingDir, which is where the packaged framework keeps the jar.
+        paramz.add(Paths.get(rootPath, "dts-generator.jar").toString())
         paramz.add("-input")
 
         for (String jarPath : allJarLibraries) {
@@ -1095,7 +1103,21 @@ task 'copyTypings' {
     }
 }
 
-copyTypings.onlyIf { generateTypescriptDefinitions.didWork }
+// USER_PROJECT_ROOT is two levels above the gradle root, which only lands on an app when that
+// root is <app>/platforms/android. In a runtime checkout it points outside the repository, so
+// require some evidence of a real project before copying anything there.
+def hasUserProject = { ->
+    file("$USER_PROJECT_ROOT/$PACKAGE_JSON").exists() ||
+            rootDir.toString().replace(File.separator, "/").endsWith(PLATFORMS_ANDROID)
+}
+
+// didWork only means the task ran, so on a failed generation this would still report success
+// and copy an empty typings directory over the user's project.
+copyTypings.onlyIf {
+    generateTypescriptDefinitions.didWork &&
+            generateTypescriptDefinitions.state.failure == null &&
+            hasUserProject()
+}
 generateTypescriptDefinitions.finalizedBy(copyTypings)
 
 task 'validateAppIdMatch' {

test-app/build-tools/android-dts-generator

@@ -4,7 +4,10 @@ import com.telerik.metadata.parsing.bytecode.methods.NativeMethodBytecodeDescrip
 import com.telerik.metadata.parsing.kotlin.classes.KotlinClassDescriptor
 import kotlin.metadata.KmDeclarationContainer
 import kotlin.metadata.Visibility
+import kotlin.metadata.jvm.JvmMethodSignature
 import kotlin.metadata.jvm.KotlinClassMetadata
+import kotlin.metadata.jvm.getterSignature
+import kotlin.metadata.jvm.setterSignature
 import kotlin.metadata.jvm.signature
 import kotlin.metadata.visibility
 import org.apache.bcel.classfile.Method
@@ -44,9 +47,28 @@ class KotlinMethodDescriptor(private val method: Method, private val originClass
     private fun checkIfMethodIsInternal(method: Method, kotlinDeclarationContainer: KmDeclarationContainer): Boolean {
         val function = kotlinDeclarationContainer
                 .functions
-                .firstOrNull {
-                    it.signature != null && it.signature!!.name == method.name && it.signature!!.descriptor == method.signature
-                }
-        return if (function != null) function.visibility == Visibility.INTERNAL else false
+                .firstOrNull { it.signature.matches(method) }
+        if (function != null) {
+            return function.visibility == Visibility.INTERNAL
+        }
+
+        // Property accessors are absent from `functions`, so without this an internal property's
+        // getter and setter are treated as public and stay reachable from JS under their mangled
+        // names. An accessor can also narrow the property's visibility on its own.
+        for (property in kotlinDeclarationContainer.properties) {
+            if (property.getterSignature.matches(method)) {
+                return property.visibility == Visibility.INTERNAL || property.getter.visibility == Visibility.INTERNAL
+            }
+
+            if (property.setterSignature.matches(method)) {
+                return property.visibility == Visibility.INTERNAL || property.setter?.visibility == Visibility.INTERNAL
+            }
+        }
+
+        return false
+    }
+
+    private fun JvmMethodSignature?.matches(method: Method): Boolean {
+        return this != null && name == method.name && descriptor == method.signature
     }
 }

test-app/build-tools/android-metadata-generator/src/src/com/telerik/metadata/parsing/kotlin/methods/KotlinMethodDescriptor.kt

@@ -0,0 +1,16 @@
+package com.telerik.metadata.parsing.kotlin.methods
+
+@Suppress("unused")
+class KotlinInternalMembersFixture {
+
+    var publicProperty: String = ""
+
+    internal var internalProperty: String = ""
+
+    var internalSetterProperty: String = ""
+        internal set
+
+    fun publicFunction(): String = publicProperty
+
+    internal fun internalFunction(): String = internalProperty
+}

test-app/build-tools/android-metadata-generator/src/test/java/com/telerik/metadata/parsing/kotlin/methods/KotlinInternalMembersFixture.kt

@@ -0,0 +1,54 @@
+package com.telerik.metadata.parsing.kotlin.methods
+
+import com.telerik.metadata.parsing.kotlin.classes.KotlinClassDescriptor
+import com.telerik.metadata.parsing.kotlin.metadata.bytecode.BytecodeMetadataAnnotation
+import org.apache.bcel.classfile.ClassParser
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+class KotlinMethodDescriptorInternalTest {
+
+    private val descriptor = descriptorFor(KotlinInternalMembersFixture::class.java)
+
+    @Test
+    fun `internal property accessors are internal`() {
+        assertTrue(isInternal("getInternalProperty"))
+        assertTrue(isInternal("setInternalProperty"))
+    }
+
+    @Test
+    fun `public property accessors are not internal`() {
+        assertFalse(isInternal("getPublicProperty"))
+        assertFalse(isInternal("setPublicProperty"))
+    }
+
+    @Test
+    fun `an internal setter on a public property only hides the setter`() {
+        assertFalse(isInternal("getInternalSetterProperty"))
+        assertTrue(isInternal("setInternalSetterProperty"))
+    }
+
+    @Test
+    fun `internal functions are still detected`() {
+        assertTrue(isInternal("internalFunction"))
+        assertFalse(isInternal("publicFunction"))
+    }
+
+    // Kotlin mangles internal members as name$module, and the module segment depends on the
+    // compiling project, so tests match on the declared name instead of the full JVM name.
+    private fun isInternal(declaredName: String): Boolean {
+        val matches = descriptor.methods.filter { it.name == declaredName || it.name.startsWith("$declaredName$") }
+        assertTrue("No method found for '$declaredName'", matches.isNotEmpty())
+        return matches.single().isInternal
+    }
+
+    private fun descriptorFor(clazz: Class<*>): KotlinClassDescriptor {
+        val resource = clazz.name.replace('.', '/') + ".class"
+        val javaClass = clazz.classLoader.getResourceAsStream(resource).use {
+            ClassParser(it, resource).parse()
+        }
+        val metadataEntry = javaClass.annotationEntries.single { it.annotationType == "Lkotlin/Metadata;" }
+        return KotlinClassDescriptor(javaClass, BytecodeMetadataAnnotation(metadataEntry), false)
+    }
+}