Hi, this is a follow up on <https://kotlinlang.sla...

# scripting

[Alexey Subach]

Hi, this is a follow up on https://kotlinlang.slack.com/archives/C0BUHC9HD/p1630176850004300
(and https://youtrack.jetbrains.com/issue/KT-48414 - support for obtaining script file location)

It wasn't super clear to me how to implement this initially, but after clarification in https://github.com/Kotlin/KEEP/issues/75#issuecomment-908723404 I was able to get something that works.

The approach I was trying to implement was to support the following declarations in the script:

@file:ScriptFileLocation("scriptFileLocation")

scriptFileLocation.absolutePath

The commit with the changes is available here: https://github.com/Snipx/kotlin/commit/339500282347811f269e8d67d053af5991ba813b
Would it be possible to give me some feedback around that initial approach? Or should I submit a PR and continue the discussion there?

[ilya.chernikov]

About the procedure - I hope we can have a fruitful discussion on the feature UI here and then continue on implementation in the PR.

I personally see no benefit in exposing the customization of the file location via annotation. I guess that it would be enough to agree on a fixed variable name instead, or (if there is a demand for a custom variable name) provide a default name that doesn't require the annotation, and give possibility to customize it if needed,

[Alexey Subach]

Hi Ilya, thanks for the feedback!
Initially I introduced this customization to avoid any possible collisions with user variable names (was not sure about backwards compatibility with the scripting code for versions of Kotlin prior to this feature) and also to make it an opt-in feature rather that something that's available by default.

You obviously know everything inside out here so if you think that having this feature available by default is fine, then I would go into the second approach you described, to still allow that customization.
Do you have any suggestions regarding the default name of the variable? I'm thinking of something like KOTLIN_SCRIPT_SOURCE_FILE_LOCATION but I haven't found any similar variables defined from a brief glance so not 100% sure it fits into the convention.

[ilya.chernikov]

The hardest problem in programming. 🙂
I would go for something like __file__ or __FILE__, as it is commonly used for this purpose in many languages. But I hope that we can collect more that two opinions here.

[Alexey Subach]

Hi Ilya,

I was working on the second iteration of my implementation and I am a little bit stuck so I could lend a hand if you don't mind.

The case I'm currently stuck with is the following:

`a.main.kts`:

@file:Import("b.main.kts")

arrayOf(__FILE__.absolutePath, getDependentScriptFile().absolutePath)

`b.main.kts`:

import java.io.File

fun getDependentScriptFile(): File {
    return __FILE__
}

basically I expect something like [a.main.kts, b.main.kts]
as the output.

It's not currently clear, though, how to achieve that.

What I have tried is adding beforeCompiling and refineConfigurationBeforeEvaluate "callbacks".

So from ScriptConfigurationRefinementContext I can fetch the file ((context.script as? FileBasedScriptSource)?.file) and script ID (context.script.locationId ) - both the files and the script IDs are different for the above sample, as expected.

The ScriptEvaluationConfigurationRefinementContext , however, is problematic. context.compiledScript.sourceLocationId is the same for both invocations (and is equal to the entrypoint script ID). I can't seem to find a good way to differentiate between the two contexts I am getting, to provide proper providedProperties. There is something potentially interesting in e.g. KtFileScriptSource but it would require a dependency on an implementation module.

So the questions I have are:
1. Is kotlin-main-kts module and scriptDef.kt the right place/level to implement this feature in?
2. How can I properly differentiate the ScriptEvaluationConfigurationRefinementContext contexts of the two scripts in the example above, so that I can add the file locations I stored from ScriptConfigurationRefinementContext in beforeCompiling, or alternatively how I can get the real location of the script from ScriptEvaluationConfigurationRefinementContext?
My current code (second approach) is available here: https://github.com/Snipx/kotlin/commit/a493ef1f1b3a30af0a71b77146dca3ee5a2aa5db

Thanks a lot in advance.

[ilya.chernikov]

Hi @Alexey Subach
Sorry for the delay, I needed to find the time to reproduce it.
And this is not the behaviour I see, to be honest. Maybe something was fixed already.
If you have a possibility to debug tests in the kotlin repo, please try the following:
• apply the patch to the repo:

Index: libraries/scripting/jvm-host-test/test/kotlin/script/experimental/jvmhost/test/ScriptingHostTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/libraries/scripting/jvm-host-test/test/kotlin/script/experimental/jvmhost/test/ScriptingHostTest.kt b/libraries/scripting/jvm-host-test/test/kotlin/script/experimental/jvmhost/test/ScriptingHostTest.kt
--- a/libraries/scripting/jvm-host-test/test/kotlin/script/experimental/jvmhost/test/ScriptingHostTest.kt	(revision 14e0e2820b3c109c904cf1de2263f890135cabb4)
+++ b/libraries/scripting/jvm-host-test/test/kotlin/script/experimental/jvmhost/test/ScriptingHostTest.kt	(date 1632666125437)
@@ -236,6 +236,10 @@
         val output = doDiamondImportTest(
             ScriptEvaluationConfiguration {
                 enableScriptsInstancesSharing()
+                refineConfigurationBeforeEvaluate {
+                    @Suppress("UNUSED_VARIABLE") val id = it.compiledScript.sourceLocationId
+                    it.evaluationConfiguration.asSuccess()
+                }
             }
         )
         Assert.assertEquals(greeting, output)

• set the break point on the it.evaluationConfiguration.asSuccess() line and run the test under debugger
I'm getting 3 calls to this callback, all with it's own context.compiledScript.sourceLocationId value. Do you see something different?

[Alexey Subach]

Hi Ilya and thanks for getting back to me 🙂

I indeed get the behavior you describe in testDiamondImportWithSharing test you mention - I am getting (...)/kotlin/libraries/scripting/jvm-host-test/testData/importTest/diamondImportMiddle.kts, main.kts and null as values.

However, result in the context of MainKtsTest is different for me and all the values are the same - I am using commit e483b9b52b231167b04287be73bbf98e8e328cf4 from master from Sept 24th to reproduce.

So my steps to reproduce in the MainKtsTest :

1.) Apply the following patch:

Index: libraries/tools/kotlin-main-kts-test/test/org/jetbrains/kotlin/mainKts/test/mainKtsTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
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
--- a/libraries/tools/kotlin-main-kts-test/test/org/jetbrains/kotlin/mainKts/test/mainKtsTest.kt	(revision e483b9b52b231167b04287be73bbf98e8e328cf4)
+++ b/libraries/tools/kotlin-main-kts-test/test/org/jetbrains/kotlin/mainKts/test/mainKtsTest.kt	(date 1632689757592)
@@ -33,7 +33,12 @@
         )
 
         BasicJvmScriptingHost().eval(
-            scriptFile.toScriptSource(), scriptDefinition.compilationConfiguration, scriptDefinition.evaluationConfiguration
+            scriptFile.toScriptSource(), scriptDefinition.compilationConfiguration, ScriptEvaluationConfiguration(scriptDefinition.evaluationConfiguration) {
+                refineConfigurationBeforeEvaluate {
+                    @Suppress("UNUSED_VARIABLE") val id = it.compiledScript.sourceLocationId
+                    it.evaluationConfiguration.asSuccess()
+                }
+            }
         )
     }

2.) Set the break point on the it.evaluationConfiguration.asSuccess() line and run testImport test under debugger

Actual result: getting libraries\tools\kotlin-main-kts-test\testData\import-test.main.kts 3 times.

Of course I am running the tests in different module/context with slightly different code, so maybe I am missing something (sorry if that's the case). Or maybe there is a bug after all..

[ilya.chernikov]

Oh, most definitely a bug. Thank you for catching.
Here is a trivial fix:

Index: plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/jvmCompilationUtil.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
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
--- a/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/jvmCompilationUtil.kt	(revision 4cca5658342c572d659be027965068861aabeb95)
+++ b/plugins/scripting/scripting-compiler/src/org/jetbrains/kotlin/scripting/compiler/plugin/impl/jvmCompilationUtil.kt	(date 1632731678423)
@@ -141,7 +141,7 @@
                     sourceFile.declarations.firstIsInstanceOrNull<KtScript>()?.let { ktScript ->
                         makeOtherScripts(ktScript).onSuccess { otherScripts ->
                             KJvmCompiledScript(
-                                containingKtFile.virtualFilePath,
+                                sourceFile.virtualFilePath,
                                 getScriptConfiguration(sourceFile),
                                 ktScript.fqName.asString(),
                                 null,

Could you, please, just include it in your PR, in order to avoid another round?

[ilya.chernikov]

BTW, seems you are already deep enough in this issue, so you can try to solve the https://youtrack.jetbrains.com/issue/KT-48758 instead of implementing a workaround.
For this you'd need to add something like:

Index: libraries/scripting/jvm/src/kotlin/script/experimental/jvm/BasicJvmScriptEvaluator.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/libraries/scripting/jvm/src/kotlin/script/experimental/jvm/BasicJvmScriptEvaluator.kt b/libraries/scripting/jvm/src/kotlin/script/experimental/jvm/BasicJvmScriptEvaluator.kt
--- a/libraries/scripting/jvm/src/kotlin/script/experimental/jvm/BasicJvmScriptEvaluator.kt	(revision 4cca5658342c572d659be027965068861aabeb95)
+++ b/libraries/scripting/jvm/src/kotlin/script/experimental/jvm/BasicJvmScriptEvaluator.kt	(date 1632732570324)
@@ -28,7 +28,9 @@
                 }.onSuccess { importedScriptsEvalResults ->
 
                     val refinedEvalConfiguration =
-                        sharedConfiguration.refineBeforeEvaluation(compiledScript).valueOr {
+                        sharedConfiguration.with {
+                            compilationConfiguration(compiledScript.compilationConfiguration)
+                        }.refineBeforeEvaluation(compiledScript).valueOr {
                             return@invoke ResultWithDiagnostics.Failure(it.reports)
                         }

where compilationConfiguration should be defined in libraries/scripting/common/src/kotlin/script/experimental/api/scriptEvaluation.kt similarly to e.g. providedProperties with the appropriate type and isTransient = true.
The potential problem though that it may not work with caching, so some more code is needed to restore these properties when loaded from cache. (setting isTransient to false avoids this problem, but will cause data duplication.)

[Alexey Subach]

Thanks for your hints so far, Ilya!

I've been able to get the build green and now the tests in kotlin-main-kts-test pass for both compilers. For some reason, I did not have to restore the link between the evaluation configuration and compilation configuration when running from cache - I added a test for this use case. Could have missed something of course.

I took the liberty to already create a PR: https://github.com/JetBrains/kotlin/pull/4597 and hope that's okay 🙂

As always, thanks a lot for the feedback in advance!

[ilya.chernikov]

Thank you very much for the contribution! I'll try to review it this week.

[ilya.chernikov]

@Alexey Subach, I've reviewed the PR, please have a look at the comments. The important one is about documenting the new keys and it also would be nice if you can check if the passing data to the eval configurator could be simplified.

[Alexey Subach]

Thanks Ilya!
I will take a look, probably the week after the next one as I am going to be away for some time.
I will add the docs for sure, and I think I tried the simpler approach of storing just a single property in the compilation configuration but it seemed to me that the compilation configuration was shared among different scripts and in the end and it wasn't possible. I will double check anyway, maybe I am mistaken.

[Alexey Subach]

Hi Ilya, just a gentle reminder that I've addressed your remarks (to the best of my understanding) and updated the PR

[ilya.chernikov]

Hi Alexey!
Sorry, I missed the notifications for some reason.
I've reviewed it again and merged into Kotlin repo.
Thank you very much for the contribution!

[Alexey Subach]

Thank you for your support Ilya!