Skip to content

Commit 3975c8d

Browse files
fzhinkinfzhinkin
authored
Check if benchmark names are legal Java names on JVM (#304)
Closes #272
1 parent f6bffdc commit 3975c8d

File tree

5 files changed

+172
-2
lines changed

5 files changed

+172
-2
lines changed

‎integration/src/test/kotlin/kotlinx/benchmark/integration/BenchmarkFunctionNameValidationTest.kt

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package kotlinx.benchmark.integration
2+
3+
import kotlin.test.*
4+
5+
class BenchmarkFunctionNameValidationTest : GradleTest() {
6+
@Test
7+
fun jvmNamesValidations() {
8+
project("funny-names").apply {
9+
runAndFail("jvmBenchmarkCompile") {
10+
assertOutputContains("One or more benchmark functions are invalid and could not be processed by JMH. See logs for details.")
11+
assertOutputDoesNotContain("Group name should be the legal Java identifier")
12+
13+
assertOutputContains("""Benchmark function name is a reserved Java keyword and cannot be used: "test.CommonBenchmark.assert" (declared as "test.CommonBenchmark.assert")""")
14+
15+
assertOutputContains("""Benchmark function name is not a valid Java identifier: "test.BenchmarkBase.-illegal base name" (declared as "test.BenchmarkBase.base")""")
16+
val firstOccurrence = output.indexOf(""""test.BenchmarkBase.-illegal base name"""")
17+
assertEquals(-1, output.indexOf(""""test.BenchmarkBase.-illegal base name"""", firstOccurrence + 1),
18+
"\"test.BenchmarkBase.-illegal base name\" is expected to be reported only once")
19+
20+
assertOutputContains("""Benchmark function name is not a valid Java identifier: "test.CommonBenchmark.wrapString-gu_FwkY" (declared as "test.CommonBenchmark.wrapString")""")
21+
assertOutputContains("""Benchmark function name is not a valid Java identifier: "test.CommonBenchmark.-explicitlyRenamed" (declared as "test.CommonBenchmark.explicitlyRenamed")""")
22+
assertOutputContains("""Benchmark function name is not a valid Java identifier: "test.CommonBenchmark.backticked name" (declared as "test.CommonBenchmark.backticked name")""")
23+
}
24+
}
25+
}
26+
}

‎integration/src/test/resources/templates/funny-names/build.gradle

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
kotlin {
2+
jvm { }
3+
}
4+
5+
benchmark {
6+
targets {
7+
register("jvm")
8+
}
9+
}

‎integration/src/test/resources/templates/funny-names/gradle.properties

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
org.gradle.jvmargs=-Xmx2g -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8

‎integration/src/test/resources/templates/funny-names/src/commonMain/kotlin/CommonBenchmark.kt

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
package test
2+
3+
import kotlinx.benchmark.*
4+
import kotlin.jvm.*
5+
6+
@JvmInline
7+
value class StringWrapper(val value: String)
8+
9+
@State(Scope.Benchmark)
10+
@Measurement(iterations = 1, time = 1, timeUnit = BenchmarkTimeUnit.SECONDS)
11+
@Warmup(iterations = 1, time = 500, timeUnit = BenchmarkTimeUnit.MILLISECONDS)
12+
@OutputTimeUnit(BenchmarkTimeUnit.MILLISECONDS)
13+
@BenchmarkMode(Mode.Throughput)
14+
open class CommonBenchmark {
15+
@Benchmark // will be wrapString-something on JVM
16+
fun wrapString() = StringWrapper("Hello World!")
17+
18+
@Benchmark
19+
@JvmName("-explicitlyRenamed")
20+
fun explicitlyRenamed() = 0
21+
22+
@Benchmark
23+
fun `backticked name`() = 0
24+
25+
@Benchmark
26+
fun `assert`() = 0
27+
}
28+
29+
abstract class BenchmarkBase {
30+
@Benchmark
31+
@JvmName("-illegal base name")
32+
fun base() = 0
33+
}
34+
35+
@State(Scope.Benchmark)
36+
@Measurement(iterations = 1, time = 1, timeUnit = BenchmarkTimeUnit.SECONDS)
37+
@Warmup(iterations = 1, time = 500, timeUnit = BenchmarkTimeUnit.MILLISECONDS)
38+
@OutputTimeUnit(BenchmarkTimeUnit.MILLISECONDS)
39+
@BenchmarkMode(Mode.Throughput)
40+
open class ConcreteBenchmark : BenchmarkBase()

‎plugin/main/src/kotlinx/benchmark/gradle/JmhBytecodeGeneratorWorker.kt

Lines changed: 96 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,9 @@
1616
package kotlinx.benchmark.gradle
1717

1818
import kotlinx.benchmark.gradle.internal.KotlinxBenchmarkPluginInternalApi
19-
import org.gradle.api.file.*
20-
import org.gradle.api.logging.*
19+
import org.gradle.api.file.ConfigurableFileCollection
20+
import org.gradle.api.file.DirectoryProperty
21+
import org.gradle.api.logging.Logging
2122
import org.gradle.workers.WorkAction
2223
import org.gradle.workers.WorkParameters
2324
import org.openjdk.jmh.annotations.Benchmark
@@ -26,8 +27,10 @@ import org.openjdk.jmh.generators.core.FileSystemDestination
2627
import org.openjdk.jmh.generators.reflection.RFGeneratorSource
2728
import org.openjdk.jmh.util.FileUtils
2829
import java.io.File
30+
import java.lang.reflect.Method
2931
import java.net.URL
3032
import java.net.URLClassLoader
33+
import kotlin.reflect.jvm.kotlinFunction
3134

3235
@KotlinxBenchmarkPluginInternalApi
3336
// TODO https://github.com/Kotlin/kotlinx-benchmark/issues/211
@@ -96,6 +99,7 @@ abstract class JmhBytecodeGeneratorWorker : WorkAction<JmhBytecodeGeneratorWorkP
9699
}
97100

98101
val source = RFGeneratorSource()
102+
var noValidationErrors = true
99103
for ((directory, files) in allFiles) {
100104
println("Analyzing ${files.size} files from $directory")
101105
val directoryPath = directory.absolutePath
@@ -105,10 +109,15 @@ abstract class JmhBytecodeGeneratorWorker : WorkAction<JmhBytecodeGeneratorWorkP
105109
val className = resourceName.replace('\\', '.').replace('/', '.')
106110
val clazz = Class.forName(className.removeSuffix(classSuffix), false, introspectionClassLoader)
107111
source.processClasses(clazz)
112+
noValidationErrors = noValidationErrors.and(validateBenchmarkFunctions(clazz))
108113
}
109114
}
110115
}
111116

117+
check(noValidationErrors) {
118+
"One or more benchmark functions are invalid and could not be processed by JMH. See logs for details."
119+
}
120+
112121
logger.lifecycle("Writing out Java source to $outputSourceDirectory and resources to $outputResourceDirectory")
113122
val gen = BenchmarkGenerator()
114123
gen.generate(source, destination)
@@ -124,8 +133,93 @@ abstract class JmhBytecodeGeneratorWorker : WorkAction<JmhBytecodeGeneratorWorkP
124133
throw RuntimeException("Generation of JMH bytecode failed with $errCount errors:\n$sb")
125134
}
126135
}
136+
137+
/**
138+
* Validates functions annotated with `@Benchmark` and return `true` if no issues were found.
139+
* Otherwise, the function returns `false` and logs all detected errors.
140+
*/
141+
private fun validateBenchmarkFunctions(clazz: Class<*>): Boolean {
142+
// Using declaredMethods to abstain from reporting the same method multiple times in the case
143+
// of benchmark classes extending some other classes.
144+
return clazz.declaredMethods.filter { it.isAnnotationPresent(Benchmark::class.java) }
145+
.map(::validateBenchmarkFunction)
146+
.fold(true, Boolean::and)
147+
}
148+
149+
/**
150+
* Validates a benchmark function [function] and return `true` if no issues were found.
151+
* Otherwise, the function returns `false` and logs all detected errors.
152+
*/
153+
private fun validateBenchmarkFunction(function: Method): Boolean {
154+
isValidJavaFunctionName(function.name)?.let {
155+
logger.error(formatInvalidFunctionNameMessage(it, function))
156+
return false
157+
}
158+
return true
159+
}
160+
161+
/**
162+
* Validates if [identifier] is a valid Java function name and return a string describing an error if it's not.
163+
* If the [identifier] is a valid function name, the function returns `null`.
164+
*/
165+
private fun isValidJavaFunctionName(identifier: String): String? {
166+
// See Java Language Specification, §3.8 Identifiers
167+
if (reservedLiterals.contains(identifier)) {
168+
return "Benchmark function name is a boolean or null literal and cannot be used as a function name"
169+
}
170+
// See Java Language Specification, §3.9 Keywords
171+
if (reservedJavaIdentifierNames.contains(identifier)) {
172+
return "Benchmark function name is a reserved Java keyword and cannot be used"
173+
}
174+
// See Java Language Specification, §3.8 Identifiers
175+
if (!(Character.isJavaIdentifierStart(identifier.first())
176+
&& identifier.substring(1).all(Character::isJavaIdentifierPart))) {
177+
return "Benchmark function name is not a valid Java identifier"
178+
}
179+
180+
return null
181+
}
182+
183+
private fun formatInvalidFunctionNameMessage(errorDescription: String, function: Method): String {
184+
val holder = function.declaringClass
185+
val javaName = "${holder.canonicalName}.${function.name}"
186+
val kotlinName =
187+
"${holder.kotlin.qualifiedName ?: holder.name}.${function.kotlinFunction?.name ?: function.name}"
188+
return "$errorDescription: \"${javaName}\" (declared as \"$kotlinName\"). " +
189+
"This might happen if the function has a backticked (`) name, " +
190+
"illegal named specified in @JvmName annotation, or a function returns an inline value class. " +
191+
"Consider using @JvmName annotation to provide a valid runtime name."
192+
}
127193
}
128194

195+
/**
196+
* Words reserved for boolean and null-literals, that could not be used as Java identifier names.
197+
*
198+
* See Java Language specification, §3.8. Identifiers for details.
199+
*/
200+
private val reservedLiterals = setOf("true", "false", "null")
201+
202+
/**
203+
* Reserved keywords that could not be used as Java identifier names.
204+
*
205+
* See Java Language specification, §3.9. Keywords for details.
206+
*
207+
* Note some keywords (like module or exports) are contextual and require some specific conditions
208+
* to be met before they will be recognized as keywords and become a problem for us.
209+
* Thus, they are not included here.
210+
*/
211+
private val reservedJavaIdentifierNames = setOf(
212+
"abstract", "continue", "for", "new", "switch",
213+
"assert", "default", "if", "package", "synchronized",
214+
"boolean", "do", "goto", "private", "this",
215+
"break", "double", "implements", "protected", "throw", "byte", "else", "import", "public", "throws",
216+
"case", "enum", "instanceof", "return", "transient",
217+
"catch", "extends", "int", "short", "try",
218+
"char", "final", "interface", "static", "void",
219+
"class", "finally", "long", "strictfp", "volatile",
220+
"const", "float", "native", "super", "while", "_"
221+
)
222+
129223
@KotlinxBenchmarkPluginInternalApi
130224
// TODO https://github.com/Kotlin/kotlinx-benchmark/issues/211
131225
// Move to a nested interface inside of JmhBytecodeGeneratorWorker (like the other workers)

0 commit comments

Comments
 (0)