From 6023507e36acd1a706b03a8fd5a034bd01ca60c2 Mon Sep 17 00:00:00 2001 From: daviss Date: Thu, 28 Mar 2024 16:14:24 -0700 Subject: [PATCH 1/4] add JvmSuppressWildcards to intellij test harness This is quite a useful annotation for kotlin/java interop - and people do use it with motif - so we'd like to add tests with this annotation --- intellij/src/test/kotlin/motif/intellij/TestHarness.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/intellij/src/test/kotlin/motif/intellij/TestHarness.kt b/intellij/src/test/kotlin/motif/intellij/TestHarness.kt index 2e0ba1aa..59f0ddc3 100644 --- a/intellij/src/test/kotlin/motif/intellij/TestHarness.kt +++ b/intellij/src/test/kotlin/motif/intellij/TestHarness.kt @@ -48,6 +48,7 @@ class TestHarness : LightJavaCodeInsightFixtureTestCase() { addLibrary(Inject::class) addLibrary(Scope::class) addLibrary(Nullable::class) + addLibrary(JvmSuppressWildcards::class) } private fun addLibrary(clazz: KClass<*>) { From 64eb9671fb85f3e02c06a2c559601dde0786a173 Mon Sep 17 00:00:00 2001 From: daviss Date: Thu, 28 Mar 2024 16:16:13 -0700 Subject: [PATCH 2/4] update java/kotlin name comparison hack We no longer need to map the primitive names for tests - possibly due to a room compiler processing version bump. However, we do notice that there's still a space difference when generating the java/kotlin string representation of generic classes --- viewmodel/src/main/kotlin/motif/viewmodel/TestRenderer.kt | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/viewmodel/src/main/kotlin/motif/viewmodel/TestRenderer.kt b/viewmodel/src/main/kotlin/motif/viewmodel/TestRenderer.kt index da05ed22..b233239b 100644 --- a/viewmodel/src/main/kotlin/motif/viewmodel/TestRenderer.kt +++ b/viewmodel/src/main/kotlin/motif/viewmodel/TestRenderer.kt @@ -141,10 +141,5 @@ object TestRenderer { /** HACK: Map kotlin types to Java for graph validation (issue when KSP processes Java sources) */ private fun String.toJvmSimpleName(): String { - return when (this) { - "Int" -> "int" - "Boolean" -> "boolean" - "Byte" -> "byte" - else -> this - } + return this.replace(", ", ",") } From 34c49bea5470779054eadb3a7236765e28657c35 Mon Sep 17 00:00:00 2001 From: daviss Date: Thu, 28 Mar 2024 16:18:53 -0700 Subject: [PATCH 3/4] proposal to update name generation This commit updates the name generation to always add a trailing underscore for the following reasons: 1. There are some missing keywords - for example, a custom type named `Null` will crash motif since it will try to name the function `null()`, it seems that adding to the list is not as safe as always append 2. Motif append a `2` (or `3` and so on) to a function name to avoid collision, so a type whose name ends with a `2` e.g. `Function2` will have its name `Function22` (or `Function23` and so on) - it's not wrong, but reads a bit funny --- .../src/main/kotlin/motif/compiler/Names.kt | 64 ++----------------- .../src/test/java/motif/compiler/NamesTest.kt | 32 +++++----- .../testcases/T056_deferred_rounds/GRAPH.txt | 2 +- 3 files changed, 23 insertions(+), 75 deletions(-) diff --git a/compiler/src/main/kotlin/motif/compiler/Names.kt b/compiler/src/main/kotlin/motif/compiler/Names.kt index 70f3bcd6..9a882f22 100644 --- a/compiler/src/main/kotlin/motif/compiler/Names.kt +++ b/compiler/src/main/kotlin/motif/compiler/Names.kt @@ -17,6 +17,8 @@ package motif.compiler import androidx.room.compiler.processing.XAnnotation import androidx.room.compiler.processing.XType +import com.google.common.annotations.VisibleForTesting +import java.util.Locale import motif.ast.compiler.CompilerAnnotation import motif.ast.compiler.CompilerType import motif.models.Type @@ -49,14 +51,11 @@ private class UniqueNameSet(blacklist: Iterable) { object Names { @JvmStatic + @VisibleForTesting fun safeName(typeMirror: XType, annotation: XAnnotation?): String { - var name = XNameVisitor.visit(typeMirror) + val name = XNameVisitor.visit(typeMirror) val annotationString = annotationString(annotation) - name = "$annotationString$name".decapitalize() - if (name in KEYWORDS) { - name += "_" - } - return name + return "$annotationString${name}_".replaceFirstChar { it.lowercase(Locale.ENGLISH) } } private fun annotationString(annotation: XAnnotation?): String { @@ -67,56 +66,3 @@ object Names { } } } - -private val KEYWORDS = - setOf( - "abstract", - "continue", - "for", - "new", - "switch", - "assert", - "default", - "goto", - "package", - "synchronized", - "boolean", - "do", - "if", - "private", - "this", - "break", - "double", - "implements", - "protected", - "throw", - "byte", - "else", - "import", - "public", - "throws", - "case", - "enum", - "instanceof", - "return", - "transient", - "catch", - "extends", - "int", - "short", - "try", - "char", - "final", - "interface", - "static", - "void", - "class", - "finally", - "long", - "strictfp", - "volatile", - "const", - "float", - "native", - "super", - "while") diff --git a/compiler/src/test/java/motif/compiler/NamesTest.kt b/compiler/src/test/java/motif/compiler/NamesTest.kt index 5969a992..fd884943 100644 --- a/compiler/src/test/java/motif/compiler/NamesTest.kt +++ b/compiler/src/test/java/motif/compiler/NamesTest.kt @@ -31,7 +31,7 @@ import org.junit.runners.Parameterized @RunWith(Parameterized::class) @OptIn(ExperimentalProcessingApi::class) -class XNamesTest(private val processorType: ProcessorType, private val srcLang: SourceLanguage) { +class NamesTest(private val processorType: ProcessorType, private val srcLang: SourceLanguage) { companion object { @JvmStatic @@ -49,17 +49,17 @@ class XNamesTest(private val processorType: ProcessorType, private val srcLang: @Test fun primitive() { - assertSafeName("int", "kotlin.Int", "integer") + assertSafeName("int", "kotlin.Int", "integer_") } @Test fun boxed() { - assertSafeName("java.lang.Integer", "kotlin.Int?", "integer") + assertSafeName("java.lang.Integer", "kotlin.Int?", "integer_") } @Test fun raw() { - assertSafeName("java.util.HashMap", "java.util.HashMap<*, *>", "hashMap") + assertSafeName("java.util.HashMap", "java.util.HashMap<*, *>", "hashMap_") } @Test @@ -67,7 +67,7 @@ class XNamesTest(private val processorType: ProcessorType, private val srcLang: assertSafeName( "java.util.HashMap", "java.util.HashMap", - "stringIntegerHashMap") + "stringIntegerHashMap_") } @Test @@ -75,22 +75,24 @@ class XNamesTest(private val processorType: ProcessorType, private val srcLang: assertSafeName( "java.util.HashMap", "java.util.HashMap", - "stringIntegerHashMap") + "stringIntegerHashMap_") } @Test fun wildcardUnbounded() { - assertSafeName("java.util.HashMap", "java.util.HashMap<*, *>", "hashMap") + assertSafeName("java.util.HashMap", "java.util.HashMap<*, *>", "hashMap_") } @Test fun typeVariable() { - assertSafeName("java.util.HashMap", "java.util.HashMap", "stringAHashMap") + assertSafeName( + "java.util.HashMap", "java.util.HashMap", "stringAHashMap_") } @Test fun typeVariableUnbounded() { - assertSafeName("java.util.HashMap", "java.util.HashMap", "stringBHashMap") + assertSafeName( + "java.util.HashMap", "java.util.HashMap", "stringBHashMap_") } @Test @@ -98,7 +100,7 @@ class XNamesTest(private val processorType: ProcessorType, private val srcLang: assertSafeName( "java.util.HashMap, Integer>", "java.util.HashMap, Integer>", - "stringIntegerHashMapIntegerHashMap") + "stringIntegerHashMapIntegerHashMap_") } @Test @@ -106,7 +108,7 @@ class XNamesTest(private val processorType: ProcessorType, private val srcLang: assertSafeName( "java.util.Map.Entry", "java.util.Map.Entry", - "stringIntegerMapEntry") + "stringIntegerMapEntry_") } @Test @@ -116,22 +118,22 @@ class XNamesTest(private val processorType: ProcessorType, private val srcLang: @Test fun named() { - assertSafeName("String", "String", "fooString", "@javax.inject.Named(\"Foo\")") + assertSafeName("String", "String", "fooString_", "@javax.inject.Named(\"Foo\")") } @Test fun customQualifier() { - assertSafeName("String", "String", "fooString", qualifierString = "@Foo") + assertSafeName("String", "String", "fooString_", qualifierString = "@Foo") } @Test fun array() { - assertSafeName("String[]", "Array", "stringArray") + assertSafeName("String[]", "Array", "stringArray_") } @Test fun enum() { - assertSafeName("LogLevel", "LogLevel", "logLevel") + assertSafeName("LogLevel", "LogLevel", "logLevel_") } @Test diff --git a/tests/src/main/java/testcases/T056_deferred_rounds/GRAPH.txt b/tests/src/main/java/testcases/T056_deferred_rounds/GRAPH.txt index 1f2f1811..9bf4d3c2 100644 --- a/tests/src/main/java/testcases/T056_deferred_rounds/GRAPH.txt +++ b/tests/src/main/java/testcases/T056_deferred_rounds/GRAPH.txt @@ -23,7 +23,7 @@ ---- String | Objects.string ---- [ Required ] [ Consumed By ] - * Scope | Scope.string() + * Scope | Scope.string_() ---- Scope | implicit ---- [ Required ] From 41d546719cbf14bbe577fbde456d0f61fc1a2dd0 Mon Sep 17 00:00:00 2001 From: daviss Date: Thu, 28 Mar 2024 16:25:28 -0700 Subject: [PATCH 4/4] add test to make use of the recent adjustments This commit adds a test that would fail without the changes in this pull request, such as JvmSuppressWildcard, `Null` class name, and kotlin/java generic name difference --- .../testcases/KT008_funky_names/ChildScope.kt | 26 +++++ .../testcases/KT008_funky_names/GRAPH.txt | 102 ++++++++++++++++++ .../java/testcases/KT008_funky_names/Null.kt | 18 ++++ .../java/testcases/KT008_funky_names/Scope.kt | 68 ++++++++++++ .../testcases/KT008_funky_names/Test.java | 26 +++++ 5 files changed, 240 insertions(+) create mode 100644 tests/src/main/java/testcases/KT008_funky_names/ChildScope.kt create mode 100644 tests/src/main/java/testcases/KT008_funky_names/GRAPH.txt create mode 100644 tests/src/main/java/testcases/KT008_funky_names/Null.kt create mode 100644 tests/src/main/java/testcases/KT008_funky_names/Scope.kt create mode 100644 tests/src/main/java/testcases/KT008_funky_names/Test.java diff --git a/tests/src/main/java/testcases/KT008_funky_names/ChildScope.kt b/tests/src/main/java/testcases/KT008_funky_names/ChildScope.kt new file mode 100644 index 00000000..203445ab --- /dev/null +++ b/tests/src/main/java/testcases/KT008_funky_names/ChildScope.kt @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2023 Uber Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package testcases.KT008_funky_names + +@motif.Scope +interface ChildScope { + + @motif.Objects + abstract class Objects { + + abstract fun myNull(): Null + } +} \ No newline at end of file diff --git a/tests/src/main/java/testcases/KT008_funky_names/GRAPH.txt b/tests/src/main/java/testcases/KT008_funky_names/GRAPH.txt new file mode 100644 index 00000000..67485f7f --- /dev/null +++ b/tests/src/main/java/testcases/KT008_funky_names/GRAPH.txt @@ -0,0 +1,102 @@ +######################################################################## +# # +# This file is auto-generated by running the Motif compiler tests and # +# serves a as validation of graph correctness. IntelliJ plugin tests # +# also rely on this file to ensure that the plugin graph understanding # +# is equivalent to the compiler's. # +# # +# - Do not edit manually. # +# - Commit changes to source control. # +# - Since this file is autogenerated, code review changes carefully to # +# ensure correctness. # +# # +######################################################################## + + ------- +| Scope | + ------- + + ==== Required ==== + + ==== Provides ==== + + ---- boolean | Objects.myBoolean ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.myBoolean() + + ---- byte | Objects.myByte ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.myByte() + + ---- char | Objects.myChar ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.myChar() + + ---- double | Objects.myDouble ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.myDouble() + + ---- float | Objects.myFloat ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.myFloat() + + ---- int | Objects.myInt ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.myInt() + + ---- Object | Objects.myAny ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.myAny() + + ---- Function2 | Objects.function2 ---- + [ Required ] + [ Consumed By ] + * ChildScope | Objects.myNull(function2) + + ---- long | Objects.myLong ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.myLong() + + ---- short | Objects.myShort ---- + [ Required ] + [ Consumed By ] + * Scope | Scope.myShort() + + ---- Scope | implicit ---- + [ Required ] + [ Consumed By ] + + ------------ + | ChildScope | + ------------ + + ==== Required ==== + + ---- Function2 ---- + [ Provided By ] + * Scope | Objects.function2 + [ Consumed By ] + * ChildScope | Objects.myNull(function2) + + ==== Provides ==== + + ---- ChildScope | implicit ---- + [ Required ] + [ Consumed By ] + + ---- Null | Objects.myNull ---- + [ Required ] + Function2 + [ Provided By ] + * Scope | Objects.function2 + [ Consumed By ] + + diff --git a/tests/src/main/java/testcases/KT008_funky_names/Null.kt b/tests/src/main/java/testcases/KT008_funky_names/Null.kt new file mode 100644 index 00000000..a9926dec --- /dev/null +++ b/tests/src/main/java/testcases/KT008_funky_names/Null.kt @@ -0,0 +1,18 @@ +/* + * Copyright (c) 2023 Uber Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package testcases.KT008_funky_names + +class Null(function2: @JvmSuppressWildcards ((Long, Double) -> Short)) \ No newline at end of file diff --git a/tests/src/main/java/testcases/KT008_funky_names/Scope.kt b/tests/src/main/java/testcases/KT008_funky_names/Scope.kt new file mode 100644 index 00000000..8204c0c4 --- /dev/null +++ b/tests/src/main/java/testcases/KT008_funky_names/Scope.kt @@ -0,0 +1,68 @@ +/* + * Copyright (c) 2023 Uber Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package testcases.KT008_funky_names + +import motif.Expose + + +@motif.Scope +interface Scope { + + fun child(): ChildScope + + fun myByte(): Byte + + fun myShort(): Short + + fun myInt(): Int + + fun myLong(): Long + + fun myFloat(): Float + + fun myDouble(): Double + + fun myBoolean(): Boolean + + fun myChar(): Char + + fun myAny(): Any + + @motif.Objects + open class Objects { + + @Expose + fun function2(): ((Long, Double) -> Short) = { _, d -> d.toInt().toShort() } + + fun myByte(): Byte = 3 + + fun myShort(): Short = 3 + + fun myInt(): Int = 3 + + fun myLong(): Long = 3 + + fun myFloat(): Float = 0.2f + + fun myDouble(): Double = 0.2 + + fun myBoolean(): Boolean = true + + fun myChar(): Char = 'c' + + fun myAny(): Any = Any() + } +} \ No newline at end of file diff --git a/tests/src/main/java/testcases/KT008_funky_names/Test.java b/tests/src/main/java/testcases/KT008_funky_names/Test.java new file mode 100644 index 00000000..bf8c3735 --- /dev/null +++ b/tests/src/main/java/testcases/KT008_funky_names/Test.java @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2023 Uber Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package testcases.KT008_funky_names; + +import static com.google.common.truth.Truth.assertThat; + +public class Test { + + public static void run() { + Scope scope = new ScopeImpl(); + assertThat(scope.child()).isNotNull(); + } +}