Comments for review

core/script.runtime/src/kotlin/script/dependencies/dependencies.kt

@@ -20,6 +20,15 @@ package kotlin.script.dependencies
 
 import java.io.File
 
+// discuss
+
+// Currently File is used to disambiguate between plain strings and paths
+// One of the ideas is to use java.nio.Path but this requires JDK 7
+// that means that script templates would require higher JDK (but since script are run by calling koltinc it seems ok to me after consideration)
+// Andrey expressed the idea that File (or Path) does not support other protocols, should we use URL/URI? (is it viable to support non-file protocols in compiler and IDE?)
+//
+// Explicitly references javaHome, what if it's not a jvm targeted script?
+// Currently it's convenient for IDE code and for the user because including jdk classes in classpath is a mess
 data class ScriptDependencies(
         val javaHome: File? = null,
         val classpath: List<File> = emptyList(),

core/script.runtime/src/kotlin/script/dependencies/dependencies_deprecated.kt

@@ -20,6 +20,7 @@ package kotlin.script.dependencies
 
 import java.io.File
 
+// nothing to discuss, for compatibility with previous version
 @Deprecated("Deprecated API. Use ScriptDependencies class.")
 interface KotlinScriptExternalDependencies : Comparable<KotlinScriptExternalDependencies> {
     val javaHome: String? get() = null

core/script.runtime/src/kotlin/script/dependencies/experimental/AsyncDependenciesResolver.kt

@@ -23,6 +23,9 @@ import kotlin.script.dependencies.DependenciesResolver.ResolveResult
 import kotlin.script.dependencies.Environment
 import kotlin.script.dependencies.ScriptContents
 
+// discuss
+
+// provides updates is async manner (IDE takes advantage of it being asynchronous, compiler just calls `runBlocking { resolveAsync (...) }`)
 interface AsyncDependenciesResolver : DependenciesResolver {
     suspend fun resolveAsync(
             scriptContents: ScriptContents, environment: Environment

core/script.runtime/src/kotlin/script/dependencies/resolvers.kt

@@ -21,6 +21,24 @@ package kotlin.script.dependencies
 import java.io.File
 import kotlin.script.dependencies.DependenciesResolver.ResolveResult
 
+//discuss
+
+// Provides api to discover dependencies of scripts
+// Dependencies can be content-dependent
+//
+// Some concerns on naming:
+// Environment -> ScriptEnvironment (top level class with too common a name)
+// ResolveResult -> ResolutionResult
+//
+// Admittedly DependenciesResolver is too generic a name, but ScriptDependenciesResolver is already taken by deprecated interface
+// My guess is script.runtime.jar is not gonna be used as application wide dependency in users project (like stdlib)
+// but rather as a dependency for a module that contains DependenciesResolver implementation so maybe this is a non-problem
+//
+// ResolveResult contains resulting dependencies and reports (diagnostics?)
+// reports may contain errors (regardless of result being Success or Failure) which is gonna lead to compiler error and script not run/error highlighting in IDE.
+// Is this semantic reasonable? Are Success and Failure misleading names?
+// Main idea behind Failure is to be able to distinguish between scenarios where resolver could or could not return meaningful ScriptDependencies object.
+// For example, IDE can avoid repainting all external references as errors when Resolver threw an exception or is in inconsistent state.
 typealias Environment = Map<String, Any?>
 
 interface DependenciesResolver : @Suppress("DEPRECATION") ScriptDependenciesResolver {
@@ -32,6 +50,7 @@ interface DependenciesResolver : @Suppress("DEPRECATION") ScriptDependenciesReso
 
     sealed class ResolveResult {
         abstract val dependencies: ScriptDependencies?
+        // reports -> diagnostics
         abstract val reports: List<ScriptReport>
 
         data class Success(
@@ -47,18 +66,23 @@ interface DependenciesResolver : @Suppress("DEPRECATION") ScriptDependenciesReso
     }
 }
 
+// is File a could type to use here?
+// No way to get script name if file is not present, should add another property (val fileName: String)
 interface ScriptContents {
     val file: File?
     val annotations: Iterable<Annotation>
     val text: CharSequence?
 
+    // nothing to discuss, for compatibility with previous version
     @Deprecated("Use DependenciesResolver interface")
     data class Position(val line: Int, val col: Int)
 }
 
+// ScriptReport -> ScriptDiagnostic
 data class ScriptReport(val message: String, val severity: Severity = ScriptReport.Severity.ERROR, val position: Position? = null) {
     data class Position(val startLine: Int, val startColumn: Int, val endLine: Int? = null, val endColumn: Int? = null)
     enum class Severity { ERROR, WARNING, INFO, DEBUG }
 }
 
-fun ScriptDependencies.asSuccess(): ResolveResult.Success = ResolveResult.Success(this)
+// should we expose this helper?
+fun ScriptDependencies.asSuccess(): ResolutionResult.Success = ResolutionResult.Success(this)

core/script.runtime/src/kotlin/script/dependencies/resolvers_deprecated.kt

@@ -21,6 +21,7 @@ package kotlin.script.dependencies
 import java.util.concurrent.Future
 import java.util.concurrent.TimeUnit
 
+// nothing to discuss, for compatibility with previous version
 @Deprecated("Use DependenciesResolver interface")
 interface ScriptDependenciesResolver {
 

core/script.runtime/src/kotlin/script/extensions/samWithReceiver.kt

@@ -16,6 +16,11 @@
 
 package kotlin.script.extensions
 
+// discuss
+
+// Is this an appropriate place to put this class?
+// Used like this: https://github.com/gradle/kotlin-dsl/blob/cb44112374e36b41732ab390531b8bc29e8de327/provider/src/main/kotlin/org/gradle/kotlin/dsl/KotlinBuildScript.kt#L35
+// Provides access to some specific compiler extension in scripts.
 @Target(AnnotationTarget.CLASS)
 @Retention(AnnotationRetention.RUNTIME)
 annotation class SamWithReceiverAnnotations(vararg val annotations: String)

core/script.runtime/src/kotlin/script/templates/annotations.kt

@@ -22,13 +22,28 @@ import kotlin.reflect.KClass
 import kotlin.script.dependencies.DependenciesResolver.NoDependencies
 import kotlin.script.dependencies.ScriptDependenciesResolver
 
+// discuss
+
+// One of the requests is to allow to share compiler args via script templates: https://youtrack.jetbrains.com/issue/KT-19120
+// Making it a property of ScriptTemplateDefinition is a viable options but it doesn't allow to changes compiler arguments based on script content.
+// For example, it could be possible to allow file level annotation and use it like:
+// @file: ExtraCompilerArgs("-enableWhatever")
+
 const val DEFAULT_SCRIPT_FILE_PATTERN = ".*\\.kts"
 
+// classes annotated with ScriptTemplateDefinition become script templates.
+// examples: https://github.com/JetBrains/kotlin/blob/5faad493b4cf7bf33bf82475e966a99a8e835720/compiler/tests/org/jetbrains/kotlin/scripts/ScriptTemplateTest.kt#L564-L601
 @Target(AnnotationTarget.CLASS)
 @Retention(AnnotationRetention.RUNTIME)
 annotation class ScriptTemplateDefinition(val resolver: KClass<out ScriptDependenciesResolver> = NoDependencies::class,
                                           val scriptFilePattern: String = DEFAULT_SCRIPT_FILE_PATTERN)
 
+// usage example: https://github.com/JetBrains/kotlin/blob/88652154c96402475d42ae0496ab0b423cc0a2b2/libraries/tools/kotlin-script-util/src/main/kotlin/org/jetbrains/kotlin/script/util/resolve.kt#L42-L42
+//
+// ScriptContents::annotations allows access to annotations that are specified on script file via '@file: []'
+// AcceptedAnnotations is intended to be put on DependenciesResolver::resolve method and limits what annotations are accessible (none if AcceptedAnnotations is not specified)
+// Do we have to limit annotations that are accessible via ScriptContents?
+// Seems a good idea since we have to load annotation instances via reflection, which maybe and overhead if resolver doesn't really care
 @Target(AnnotationTarget.FUNCTION, AnnotationTarget.CLASS)
 @Retention(AnnotationRetention.RUNTIME)
 annotation class AcceptedAnnotations(vararg val supportedAnnotationClasses: KClass<out Annotation>)

core/script.runtime/src/kotlin/script/templates/standard/templates.kt

@@ -16,18 +16,25 @@
 
 package kotlin.script.templates.standard
 
+// discuss
+//
+// These are some 'basic' script templates
+// Should we keep them here?
 /**
  * Basic script definition template without parameters
  */
+// didn't find any usages of this class.
 public abstract class SimpleScriptTemplate()
 
 /**
  * Script definition template with standard argv-like parameter; default for regular kotlin scripts
  */
+// Used here: https://github.com/JetBrains/kotlin/blob/65dba3615c2699e35e8da65850efc97afd674ad5/compiler/frontend/src/org/jetbrains/kotlin/script/KotlinScriptDefinition.kt#L50-L50
 public abstract class ScriptTemplateWithArgs(val args: Array<String>)
 
 /**
  * Script definition template with generic bindings parameter (String to Object)
  */
+// Used here: https://github.com/JetBrains/kotlin/blob/f152af6385b2a5df8e598f2a604b1e66114f860e/idea/idea-repl/src/org/jetbrains/kotlin/jsr223/KotlinJsr223StandardScriptEngineFactory4Idea.kt#L38-L38
 public abstract class ScriptTemplateWithBindings(val bindings: Map<String, Any?>)