From e60e80f19abbfb2fb078549331b0f8cb7c729688 Mon Sep 17 00:00:00 2001 From: Ilya Chernikov Date: Tue, 17 Aug 2021 17:25:47 +0200 Subject: [PATCH] Scripting: improve reporting on cyclic dependency #KT-48177 fixed the issue is in fact fixed by the previous commit (report error on duplicated import) by normalizing import path before processing, but here we're making error reporting nicer for the case. --- .../script/experimental/api/errorHandling.kt | 11 ++++ .../kotlin/mainKts/test/mainKtsTest.kt | 6 ++ .../testData/import-cycle-1.main.kts | 4 ++ .../testData/import-cycle-2.main.kts | 4 ++ .../plugin/impl/KJvmReplCompilerBase.kt | 34 +++++------ .../plugin/impl/ScriptJvmCompilerImpls.kt | 19 +++--- .../plugin/impl/jvmCompilationUtil.kt | 59 +++++++++++-------- 7 files changed, 86 insertions(+), 51 deletions(-) create mode 100644 libraries/tools/kotlin-main-kts-test/testData/import-cycle-1.main.kts create mode 100644 libraries/tools/kotlin-main-kts-test/testData/import-cycle-2.main.kts diff --git a/libraries/scripting/common/src/kotlin/script/experimental/api/errorHandling.kt b/libraries/scripting/common/src/kotlin/script/experimental/api/errorHandling.kt index b87f583c4f0..651997c1b21 100644 --- a/libraries/scripting/common/src/kotlin/script/experimental/api/errorHandling.kt +++ b/libraries/scripting/common/src/kotlin/script/experimental/api/errorHandling.kt @@ -147,6 +147,17 @@ inline fun Iterable.mapSuccess(body: (T) -> ResultWithDiagnostics): results.add(r) } +/** + * maps transformation ([body]) over iterable merging diagnostics + * return failure with merged diagnostics after first failed transformation + * and success with merged diagnostics and list of not null results if all transformations succeeded + */ +inline fun Iterable.mapNotNullSuccess(body: (T) -> ResultWithDiagnostics): ResultWithDiagnostics> = + mapSuccessImpl(body) { results, r -> + if (r != null) + results.add(r) + } + /** * maps transformation ([body]) over iterable merging diagnostics and flatten the results * return failure with merged diagnostics after first failed transformation diff --git a/libraries/tools/kotlin-main-kts-test/test/org/jetbrains/kotlin/mainKts/test/mainKtsTest.kt b/libraries/tools/kotlin-main-kts-test/test/org/jetbrains/kotlin/mainKts/test/mainKtsTest.kt index 3a690f94aaa..29bb1988782 100644 --- a/libraries/tools/kotlin-main-kts-test/test/org/jetbrains/kotlin/mainKts/test/mainKtsTest.kt +++ b/libraries/tools/kotlin-main-kts-test/test/org/jetbrains/kotlin/mainKts/test/mainKtsTest.kt @@ -127,6 +127,12 @@ class MainKtsTest { assertFailed("Duplicate imports:", res) } + @Test + fun testCyclicImportError() { + val res = evalFile(File("$TEST_DATA_ROOT/import-cycle-1.main.kts")) + assertFailed("Unable to handle recursive script dependencies", res) + } + @Test fun testCompilerOptions() { diff --git a/libraries/tools/kotlin-main-kts-test/testData/import-cycle-1.main.kts b/libraries/tools/kotlin-main-kts-test/testData/import-cycle-1.main.kts new file mode 100644 index 00000000000..efe129b3e7e --- /dev/null +++ b/libraries/tools/kotlin-main-kts-test/testData/import-cycle-1.main.kts @@ -0,0 +1,4 @@ + +@file:Import("import-cycle-2.main.kts") + +1 diff --git a/libraries/tools/kotlin-main-kts-test/testData/import-cycle-2.main.kts b/libraries/tools/kotlin-main-kts-test/testData/import-cycle-2.main.kts new file mode 100644 index 00000000000..720bf9aab10 --- /dev/null +++ b/libraries/tools/kotlin-main-kts-test/testData/import-cycle-2.main.kts @@ -0,0 +1,4 @@ + +@file:Import("import-cycle-1.main.kts") + +2 diff --git a/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/KJvmReplCompilerBase.kt b/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/KJvmReplCompilerBase.kt index d330e75966b..a19a9886f92 100644 --- a/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/KJvmReplCompilerBase.kt +++ b/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/KJvmReplCompilerBase.kt @@ -150,25 +150,25 @@ open class KJvmReplCompilerBase protected cons history.push(LineId(snippetNo, 0, snippet.hashCode()), scriptDescriptor) val dependenciesProvider = ScriptDependenciesProvider.getInstance(context.environment.project) - val compiledScript = - makeCompiledScript( - generationState, - snippet, - sourceFiles.first(), - sourceDependencies - ) { ktFile -> - dependenciesProvider?.getScriptConfiguration(ktFile)?.configuration - ?: context.baseScriptCompilationConfiguration - } + makeCompiledScript( + generationState, + snippet, + sourceFiles.first(), + sourceDependencies + ) { ktFile -> + dependenciesProvider?.getScriptConfiguration(ktFile)?.configuration + ?: context.baseScriptCompilationConfiguration + }.onSuccess { compiledScript -> - lastCompiledSnippet = lastCompiledSnippet.add(compiledScript) + lastCompiledSnippet = lastCompiledSnippet.add(compiledScript) - lastCompiledSnippet?.asSuccess(messageCollector.diagnostics) - ?: failure( - snippet, - messageCollector, - "last compiled snippet should not be null" - ) + lastCompiledSnippet?.asSuccess(messageCollector.diagnostics) + ?: failure( + snippet, + messageCollector, + "last compiled snippet should not be null" + ) + } } }.last() diff --git a/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/ScriptJvmCompilerImpls.kt b/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/ScriptJvmCompilerImpls.kt index 5a6c4a3bf34..b7be171f5d0 100644 --- a/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/ScriptJvmCompilerImpls.kt +++ b/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/ScriptJvmCompilerImpls.kt @@ -196,16 +196,15 @@ private fun doCompile( val generationState = generate(analysisResult, sourceFiles, context.environment.configuration) - val compiledScript = - makeCompiledScript( - generationState, - script, - sourceFiles.first(), - sourceDependencies, - getScriptConfiguration - ) - - return ResultWithDiagnostics.Success(compiledScript, messageCollector.diagnostics) + return makeCompiledScript( + generationState, + script, + sourceFiles.first(), + sourceDependencies, + getScriptConfiguration + ).onSuccess { compiledScript -> + ResultWithDiagnostics.Success(compiledScript, messageCollector.diagnostics) + } } private fun analyze(sourceFiles: Collection, environment: KotlinCoreEnvironment): AnalysisResult { diff --git a/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/jvmCompilationUtil.kt b/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/jvmCompilationUtil.kt index 30e6950791f..318f4eb9c8a 100644 --- a/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/jvmCompilationUtil.kt +++ b/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/jvmCompilationUtil.kt @@ -116,32 +116,41 @@ internal fun makeCompiledScript( ktFile: KtFile, sourceDependencies: List, getScriptConfiguration: (KtFile) -> ScriptCompilationConfiguration -): KJvmCompiledScript { +): ResultWithDiagnostics { val scriptDependenciesStack = ArrayDeque() val ktScript = ktFile.declarations.firstIsInstanceOrNull() ?: throw IllegalStateException("Expecting script file: KtScript is not found in ${ktFile.name}") - fun makeOtherScripts(script: KtScript): List { + fun makeOtherScripts(script: KtScript): ResultWithDiagnostics> { // TODO: ensure that it is caught earlier (as well) since it would be more economical if (scriptDependenciesStack.contains(script)) - throw IllegalArgumentException("Unable to handle recursive script dependencies") + return ResultWithDiagnostics.Failure( + ScriptDiagnostic( + ScriptDiagnostic.unspecifiedError, + "Unable to handle recursive script dependencies", + sourcePath = script.containingFile.virtualFile?.path + ) + ) scriptDependenciesStack.push(script) val containingKtFile = script.containingKtFile - val otherScripts: List = - sourceDependencies.find { it.scriptFile == containingKtFile }?.sourceDependencies?.valueOrThrow()?.mapNotNull { sourceFile -> - sourceFile.declarations.firstIsInstanceOrNull()?.let { - KJvmCompiledScript( - containingKtFile.virtualFilePath, - getScriptConfiguration(sourceFile), - it.fqName.asString(), - null, - makeOtherScripts(it), - null - ) - } - } ?: emptyList() + val otherScripts = + sourceDependencies.find { it.scriptFile == containingKtFile }?.sourceDependencies?.valueOrThrow() + ?.mapNotNullSuccess { sourceFile -> + sourceFile.declarations.firstIsInstanceOrNull()?.let { ktScript -> + makeOtherScripts(ktScript).onSuccess { otherScripts -> + KJvmCompiledScript( + containingKtFile.virtualFilePath, + getScriptConfiguration(sourceFile), + ktScript.fqName.asString(), + null, + otherScripts, + null + ).asSuccess() + } + } ?: null.asSuccess() + } ?: emptyList().asSuccess() scriptDependenciesStack.pop() return otherScripts @@ -154,13 +163,15 @@ internal fun makeCompiledScript( else resultFieldName!! to KotlinType(resultTypeString ?: DescriptorRenderer.FQ_NAMES_IN_TYPES.renderType(resultType!!)) } - return KJvmCompiledScript( - script.locationId, - getScriptConfiguration(ktScript.containingKtFile), - ktScript.fqName.asString(), - resultField, - makeOtherScripts(ktScript), - module - ) + return makeOtherScripts(ktScript).onSuccess { otherScripts -> + KJvmCompiledScript( + script.locationId, + getScriptConfiguration(ktScript.containingKtFile), + ktScript.fqName.asString(), + resultField, + otherScripts, + module + ).asSuccess() + } }