From ec3ccf1ba8883da1746a4b515901568f407b4dc0 Mon Sep 17 00:00:00 2001 From: Ilya Chernikov Date: Thu, 25 Jul 2019 15:23:58 +0200 Subject: [PATCH] Properly handle REPL snippets with exceptions ... so the REPL remain operational after exception in one of the snippets: - separately return script class and script instance on evaluation ( because in case of an exception the class is valid, while the instance is not). - store both the class and the instance in the history - handle this data accordingly --- .../experimental/api/scriptEvaluation.kt | 15 ++++++++----- .../jsr223/test/KotlinJsr223ScriptEngineIT.kt | 17 ++++++++++++++ .../experimental/jvmhost/test/ReplTest.kt | 15 ++++++++----- .../jsr223/KotlinJsr223ScriptEngineImpl.kt | 3 +-- .../jvmhost/repl/legacyReplEvaluation.kt | 22 ++++++++++--------- .../jvm/BasicJvmScriptEvaluator.kt | 6 ++--- 6 files changed, 52 insertions(+), 26 deletions(-) diff --git a/libraries/scripting/common/src/kotlin/script/experimental/api/scriptEvaluation.kt b/libraries/scripting/common/src/kotlin/script/experimental/api/scriptEvaluation.kt index 07e101886bd..c1449428c13 100644 --- a/libraries/scripting/common/src/kotlin/script/experimental/api/scriptEvaluation.kt +++ b/libraries/scripting/common/src/kotlin/script/experimental/api/scriptEvaluation.kt @@ -69,7 +69,7 @@ val ScriptEvaluationConfigurationKeys.constructorArgs by PropertiesCollection.ke * For the first snippet in a REPL an empty list should be passed explicitly * An array of the previous snippets will be passed to the current snippet constructor */ -val ScriptEvaluationConfigurationKeys.previousSnippets by PropertiesCollection.key>(isTransient = true) +val ScriptEvaluationConfigurationKeys.previousSnippets by PropertiesCollection.key>(isTransient = true) @Deprecated("use scriptsInstancesSharing flag instead", level = DeprecationLevel.ERROR) val ScriptEvaluationConfigurationKeys.scriptsInstancesSharingMap by PropertiesCollection.key, EvaluationResult>>(isTransient = true) @@ -134,24 +134,28 @@ fun ScriptEvaluationConfiguration.refineBeforeEvaluation( /** * The script evaluation result value */ -sealed class ResultValue(val scriptInstance: Any? = null) { +sealed class ResultValue(val scriptClass: KClass<*>? = null, val scriptInstance: Any? = null) { /** * The result value representing a script return value - the value of the last expression in the script * @param name assigned name of the result field - used e.g. in REPL * @param value actual result value * @param type name of the result type + * @param scriptClass the loaded class of the script * @param scriptInstance instance of the script class */ - class Value(val name: String, val value: Any?, val type: String, scriptInstance: Any) : ResultValue(scriptInstance) { + class Value(val name: String, val value: Any?, val type: String, scriptClass: KClass<*>, scriptInstance: Any) : + ResultValue(scriptClass, scriptInstance) { + override fun toString(): String = "$name: $type = $value" } /** * The result value representing unit result, e.g. when the script ends with a statement + * @param scriptClass the loaded class of the script * @param scriptInstance instance of the script class */ - class Unit(scriptInstance: Any) : ResultValue(scriptInstance) { + class Unit(scriptClass: KClass<*>, scriptInstance: Any) : ResultValue(scriptClass, scriptInstance) { override fun toString(): String = "Unit" } @@ -159,8 +163,9 @@ sealed class ResultValue(val scriptInstance: Any? = null) { * The result value representing an exception from script itself * @param error the actual exception thrown on script evaluation * @param wrappingException the wrapping exception e.g. InvocationTargetException, sometimes useful for calculating the relevant stacktrace + * @param scriptClass the loaded class of the script, if any */ - class Error(val error: Throwable, val wrappingException: Throwable? = null) : ResultValue() { + class Error(val error: Throwable, val wrappingException: Throwable? = null, scriptClass: KClass<*>? = null) : ResultValue(scriptClass) { override fun toString(): String = error.toString() } diff --git a/libraries/scripting/jsr223-test/test/kotlin/script/experimental/jsr223/test/KotlinJsr223ScriptEngineIT.kt b/libraries/scripting/jsr223-test/test/kotlin/script/experimental/jsr223/test/KotlinJsr223ScriptEngineIT.kt index ab1ea04fdfe..2e6f34b9077 100644 --- a/libraries/scripting/jsr223-test/test/kotlin/script/experimental/jsr223/test/KotlinJsr223ScriptEngineIT.kt +++ b/libraries/scripting/jsr223-test/test/kotlin/script/experimental/jsr223/test/KotlinJsr223ScriptEngineIT.kt @@ -85,6 +85,23 @@ class KotlinJsr223ScriptEngineIT { Assert.assertEquals(5, res3) } + @Test + fun testEvalWithException() { + val engine = ScriptEngineManager().getEngineByExtension("kts")!! + try { + engine.eval("throw Exception(\"!!\")") + Assert.fail("Expecting exception to propagate") + } catch (e: ScriptException) { + Assert.assertEquals("!!", e.cause?.message) + } + // engine should remain operational + val res1 = engine.eval("val x = 3") + Assert.assertNull(res1) + val res2 = engine.eval("x + 4") + Assert.assertEquals(7, res2) + } + + @Test fun testEngineRepeatWithReset() { val code = "open class A {}\n" + diff --git a/libraries/scripting/jvm-host-test/test/kotlin/script/experimental/jvmhost/test/ReplTest.kt b/libraries/scripting/jvm-host-test/test/kotlin/script/experimental/jvmhost/test/ReplTest.kt index fd2061067e6..039aa5194be 100644 --- a/libraries/scripting/jvm-host-test/test/kotlin/script/experimental/jvmhost/test/ReplTest.kt +++ b/libraries/scripting/jvm-host-test/test/kotlin/script/experimental/jvmhost/test/ReplTest.kt @@ -80,9 +80,11 @@ class ReplTest : TestCase() { simpleScriptompilationConfiguration, simpleScriptEvaluationConfiguration, sequenceOf( - "throw RuntimeException(\"abc\")" + "throw RuntimeException(\"abc\")", + "val x = 3", + "x + 1" ), - sequenceOf(RuntimeException("abc")) + sequenceOf(RuntimeException("abc"), null, 4) ) } @@ -107,11 +109,12 @@ class ReplTest : TestCase() { } } .onSuccess { - it.returnValue.scriptInstance?.let { snippetInstance -> - currentEvalConfig = ScriptEvaluationConfiguration(currentEvalConfig) { - previousSnippets.append(snippetInstance) + val snippetClass = it.returnValue.scriptClass + currentEvalConfig = ScriptEvaluationConfiguration(currentEvalConfig) { + previousSnippets.append(it.returnValue.scriptInstance) + if (snippetClass != null) { jvm { - baseClassLoader(snippetInstance::class.java.classLoader) + baseClassLoader(snippetClass.java.classLoader) } } } diff --git a/libraries/scripting/jvm-host/src/kotlin/script/experimental/jvmhost/jsr223/KotlinJsr223ScriptEngineImpl.kt b/libraries/scripting/jvm-host/src/kotlin/script/experimental/jvmhost/jsr223/KotlinJsr223ScriptEngineImpl.kt index 41e00860f03..e1807156876 100644 --- a/libraries/scripting/jvm-host/src/kotlin/script/experimental/jvmhost/jsr223/KotlinJsr223ScriptEngineImpl.kt +++ b/libraries/scripting/jvm-host/src/kotlin/script/experimental/jvmhost/jsr223/KotlinJsr223ScriptEngineImpl.kt @@ -7,7 +7,6 @@ package kotlin.script.experimental.jvmhost.jsr223 import org.jetbrains.kotlin.cli.common.repl.* import java.util.concurrent.locks.ReentrantReadWriteLock -import javax.script.Bindings import javax.script.ScriptContext import javax.script.ScriptEngineFactory import kotlin.script.experimental.api.ScriptCompilationConfiguration @@ -71,7 +70,7 @@ class KotlinJsr223ScriptEngineImpl( get() = null override val backwardInstancesHistory: Sequence - get() = getCurrentState(getContext()).asState(JvmReplEvaluatorState::class.java).history.asReversed().asSequence().map { it.item } + get() = getCurrentState(getContext()).asState(JvmReplEvaluatorState::class.java).history.asReversed().asSequence().map { it.item.second }.filterNotNull() override val baseClassLoader: ClassLoader get() = evaluationConfiguration[ScriptEvaluationConfiguration.jvm.baseClassLoader]!! diff --git a/libraries/scripting/jvm-host/src/kotlin/script/experimental/jvmhost/repl/legacyReplEvaluation.kt b/libraries/scripting/jvm-host/src/kotlin/script/experimental/jvmhost/repl/legacyReplEvaluation.kt index d29ef3c55fb..bdacce3793f 100644 --- a/libraries/scripting/jvm-host/src/kotlin/script/experimental/jvmhost/repl/legacyReplEvaluation.kt +++ b/libraries/scripting/jvm-host/src/kotlin/script/experimental/jvmhost/repl/legacyReplEvaluation.kt @@ -9,6 +9,7 @@ import kotlinx.coroutines.runBlocking import org.jetbrains.kotlin.cli.common.repl.* import java.util.concurrent.locks.ReentrantReadWriteLock import kotlin.concurrent.write +import kotlin.reflect.KClass import kotlin.script.experimental.api.* import kotlin.script.experimental.jvm.BasicJvmScriptEvaluator import kotlin.script.experimental.jvm.baseClassLoader @@ -37,15 +38,15 @@ class JvmReplEvaluator( val compiledScript = (compileResult.data as? KJvmCompiledScript<*>) ?: return ReplEvalResult.Error.CompileTime("Unable to access compiled script: ${compileResult.data}") - val lastSnippetInstance = history.peek()?.item - val historyBeforeSnippet = history.previousItems(compileResult.lineId) + val lastSnippetClass = history.peek()?.item?.first + val historyBeforeSnippet = history.previousItems(compileResult.lineId).map { it.second }.toList() val currentConfiguration = ScriptEvaluationConfiguration(baseScriptEvaluationConfiguration) { - if (historyBeforeSnippet.any()) { - previousSnippets.put(historyBeforeSnippet.toList()) + if (historyBeforeSnippet.isNotEmpty()) { + previousSnippets.put(historyBeforeSnippet) } - if (lastSnippetInstance != null) { + if (lastSnippetClass != null) { jvm { - baseClassLoader(lastSnippetInstance::class.java.classLoader) + baseClassLoader(lastSnippetClass.java.classLoader) } } if (scriptArgs != null) { @@ -59,17 +60,18 @@ class JvmReplEvaluator( is ResultWithDiagnostics.Success -> { when (val retVal = res.value.returnValue) { is ResultValue.Error -> { + history.replaceOrPush(compileResult.lineId, retVal.scriptClass to null) ReplEvalResult.Error.Runtime( retVal.error.message ?: "unknown error", (retVal.error as? Exception) ?: (retVal.wrappingException as? Exception) ) } is ResultValue.Value -> { - history.replaceOrPush(compileResult.lineId, retVal.scriptInstance!!) + history.replaceOrPush(compileResult.lineId, retVal.scriptClass to retVal.scriptInstance) ReplEvalResult.ValueResult(retVal.name, retVal.value, retVal.type) } is ResultValue.Unit -> { - history.replaceOrPush(compileResult.lineId, retVal.scriptInstance!!) + history.replaceOrPush(compileResult.lineId, retVal.scriptClass to retVal.scriptInstance) ReplEvalResult.UnitResult() } else -> throw IllegalStateException("Unexpected snippet result value $retVal") @@ -87,8 +89,8 @@ class JvmReplEvaluator( open class JvmReplEvaluatorState( scriptEvaluationConfiguration: ScriptEvaluationConfiguration, override val lock: ReentrantReadWriteLock = ReentrantReadWriteLock() -) : IReplStageState { - override val history: IReplStageHistory = ReplStageHistoryWithReplace(lock) +) : IReplStageState?, Any?>> { + override val history: IReplStageHistory?, Any?>> = ReplStageHistoryWithReplace(lock) override val currentGeneration: Int get() = (history as BasicReplStageHistory<*>).currentGeneration.get() } diff --git a/libraries/scripting/jvm/src/kotlin/script/experimental/jvm/BasicJvmScriptEvaluator.kt b/libraries/scripting/jvm/src/kotlin/script/experimental/jvm/BasicJvmScriptEvaluator.kt index 7fb3936d3af..49035923b1b 100644 --- a/libraries/scripting/jvm/src/kotlin/script/experimental/jvm/BasicJvmScriptEvaluator.kt +++ b/libraries/scripting/jvm/src/kotlin/script/experimental/jvm/BasicJvmScriptEvaluator.kt @@ -41,11 +41,11 @@ open class BasicJvmScriptEvaluator : ScriptEvaluator { compiledScript.resultField?.let { (resultFieldName, resultType) -> val resultField = scriptClass.java.getDeclaredField(resultFieldName).apply { isAccessible = true } - ResultValue.Value(resultFieldName, resultField.get(instance), resultType.typeName, instance) - } ?: ResultValue.Unit(instance) + ResultValue.Value(resultFieldName, resultField.get(instance), resultType.typeName, scriptClass, instance) + } ?: ResultValue.Unit(scriptClass, instance) } catch (e: InvocationTargetException) { - ResultValue.Error(e.targetException ?: e, e) + ResultValue.Error(e.targetException ?: e, e, scriptClass) } EvaluationResult(resultValue, refinedEvalConfiguration).let {