Add explicit script import prep APIs and VM regression tests

This commit is contained in:
Sergey Chernov 2026-03-27 15:51:17 +03:00
parent 3d8bfe8863
commit ecc4d8b9ed
8 changed files with 271 additions and 1 deletions

View File

@ -72,6 +72,14 @@ For module-level APIs, the default workflow is:
1. declare globals in Lyng using `extern fun` / `extern val` / `extern var`;
2. bind Kotlin implementation via `ModuleScope.globalBinder()`.
This is also the recommended way to expose a Kotlin-backed value that should behave like a true
Lyng global variable/property. If you need `x` to read/write through Kotlin on every access, use
`extern var` / `extern val` plus `bindGlobalVar(...)`.
Do not use `addConst(...)` for this case: `addConst(...)` installs a value, not a Kotlin-backed
property accessor. It is appropriate for fixed values and objects, but not for a global that should
delegate reads/writes back into Kotlin state.
```kotlin
import net.sergeych.lyng.bridge.*
import net.sergeych.lyng.obj.ObjInt
@ -117,6 +125,12 @@ assertEquals("changed", globalProp)
assertEquals("1.0.0", globalVersion)
```
Minimal rule of thumb:
- use `bindGlobalFun(...)` for global functions
- use `bindGlobalVar(...)` for Kotlin-backed global variables/properties
- use `addConst(...)` only for fixed values/objects that do not need getter/setter behavior
For custom argument handling and full runtime access:
```kotlin
@ -462,14 +476,21 @@ im.addPackage("my.tools") { module: ModuleScope ->
module.eval(
"""
extern val version: String
extern var status: String
extern fun triple(x: Int): Int
""".trimIndent()
)
val binder = module.globalBinder()
var status = "ready"
binder.bindGlobalVar(
name = "version",
get = { "1.0" }
)
binder.bindGlobalVar(
name = "status",
get = { status },
set = { status = it }
)
binder.bindGlobalFun1<Int>("triple") { x ->
ObjInt.of((x * 3).toLong())
}
@ -479,6 +500,7 @@ im.addPackage("my.tools") { module: ModuleScope ->
scope.eval("""
import my.tools.*
val v = triple(14)
status = "busy"
""")
val v = scope.eval("v").toKotlin(scope) // -> 42
```

View File

@ -46,6 +46,28 @@ class Script(
// private val catchReturn: Boolean = false,
) : Statement() {
fun statements(): List<Statement> = statements
/**
* Explicitly apply this script's import/module bindings to [scope] without executing the script.
* This is intended for embedding scenarios where the host owns scope lifecycle and wants
* script-specific imports to be a separate, opt-in preparation step.
*/
suspend fun importInto(scope: Scope, seedScope: Scope = scope): Scope {
prepareScriptScope(scope, seedScope)
return scope
}
/**
* Create a fresh raw module scope and apply this script's import/module bindings to it.
* If [seedScope] is provided, its import provider is reused and seed-bound imports resolve from it.
*/
suspend fun instantiateModule(seedScope: Scope? = null, pos: Pos = this.pos): ModuleScope {
val provider = seedScope?.currentImportProvider ?: defaultImportManager
val module = provider.newModuleAt(pos)
prepareScriptScope(module, seedScope ?: module)
return module
}
override suspend fun execute(scope: Scope): Obj {
scope.pos = pos
val execScope = resolveModuleScope(scope) ?: scope
@ -75,6 +97,19 @@ class Script(
return ObjVoid
}
private suspend fun prepareScriptScope(scope: Scope, seedScope: Scope) {
if (importBindings.isNotEmpty() || importedModules.isNotEmpty()) {
seedImportBindings(scope, seedScope)
}
if (moduleSlotPlan.isNotEmpty()) {
scope.applySlotPlan(moduleSlotPlan)
for (name in moduleSlotPlan.keys) {
val record = scope.objects[name] ?: scope.localBindings[name] ?: continue
scope.updateSlotFor(name, record)
}
}
}
private suspend fun seedModuleSlots(scope: Scope, seedScope: Scope) {
if (importBindings.isEmpty() && importedModules.isEmpty()) return
seedImportBindings(scope, seedScope)

View File

@ -36,7 +36,7 @@ internal suspend fun seedFrameLocalsFromScope(frame: CmdFrame, scope: Scope) {
} else {
record.value
}
if (value is net.sergeych.lyng.FrameSlotRef && value.refersTo(frame.frame, base + i)) {
if (value is net.sergeych.lyng.FrameSlotRef && value.refersTo(frame.frame, i)) {
continue
}
frame.setObjUnchecked(base + i, value)

View File

@ -0,0 +1,87 @@
/*
* Copyright 2026 Sergey S. Chernov
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import kotlinx.coroutines.test.runTest
import net.sergeych.lyng.Compiler
import net.sergeych.lyng.Script
import net.sergeych.lyng.Source
import net.sergeych.lyng.obj.toInt
import net.sergeych.lyng.pacman.ImportManager
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertNotNull
import kotlin.test.assertNull
class ScriptImportPreparationTest {
@Test
fun scriptImportIntoExplicitlyPreparesExistingScope() = runTest {
val manager = ImportManager()
manager.addTextPackages(
"""
package foo
val answer = 42
""".trimIndent()
)
val script = Compiler.compile(
Source(
"<prepare-scope>",
"""
import foo
answer
""".trimIndent()
),
manager
)
val scope = manager.newModule()
assertNull(scope["answer"])
script.importInto(scope)
val record = assertNotNull(scope["answer"])
assertEquals(42, scope.resolve(record, "answer").toInt())
}
@Test
fun scriptInstantiateModuleUsesSeedScopeImportProvider() = runTest {
val manager = ImportManager()
manager.addTextPackages(
"""
package foo
val answer = 42
""".trimIndent()
)
val script = Compiler.compile(
Source(
"<instantiate-module>",
"""
import foo
answer
""".trimIndent()
),
manager
)
val seedScope = manager.newModule()
val module = script.instantiateModule(seedScope)
val record = assertNotNull(module["answer"])
assertEquals(42, module.resolve(record, "answer").toInt())
}
}

View File

@ -0,0 +1,60 @@
/*
* Copyright 2026 Sergey S. Chernov
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import kotlinx.coroutines.test.runTest
import net.sergeych.lyng.FrameSlotRef
import net.sergeych.lyng.Pos
import net.sergeych.lyng.Scope
import net.sergeych.lyng.bytecode.BytecodeConst
import net.sergeych.lyng.bytecode.CmdFrame
import net.sergeych.lyng.bytecode.CmdFunction
import net.sergeych.lyng.bytecode.CmdVm
import net.sergeych.lyng.bytecode.seedFrameLocalsFromScope
import kotlin.test.Test
import kotlin.test.assertNull
class SeedLocalsRegressionTest {
@Test
fun seedFrameLocalsSkipsSelfReferentialFrameSlotRef() = runTest {
val fn = CmdFunction(
name = "seed-self-ref",
localCount = 1,
addrCount = 0,
returnLabels = emptySet(),
scopeSlotCount = 1,
scopeSlotIndices = intArrayOf(0),
scopeSlotNames = arrayOf(null),
scopeSlotIsModule = booleanArrayOf(false),
localSlotNames = arrayOf("x"),
localSlotMutables = booleanArrayOf(true),
localSlotDelegated = booleanArrayOf(false),
localSlotCaptures = booleanArrayOf(false),
constants = listOf(BytecodeConst.IntVal(0)),
cmds = emptyArray(),
posByIp = emptyArray()
)
val scope = Scope(null, pos = Pos.builtIn)
val record = scope.addConst("x", net.sergeych.lyng.obj.ObjInt.of(1))
val frame = CmdFrame(CmdVm(), fn, scope, emptyList())
record.value = FrameSlotRef(frame.frame, 0)
scope.updateSlotFor("x", record)
seedFrameLocalsFromScope(frame, scope)
assertNull(frame.frame.getRawObj(0))
}
}

View File

@ -0,0 +1,66 @@
# :lynglib Compiler and VM Review (2026-03-26)
## Scope
- Reviewed `:lynglib` compiler and bytecode VM paths, focusing on compile/execution correctness.
- Read core files around `Compiler`, `Script`, `BytecodeCompiler`, `CmdRuntime`, `Scope`, `ModuleScope`, and capture/slot plumbing.
- Ran `./gradlew :lynglib:jvmTest` on 2026-03-26: PASS.
## Findings
### 1. High: local seeding uses the wrong slot index when checking for self-referential `FrameSlotRef`
- Status: fixed in worktree on 2026-03-26; covered by `SeedLocalsRegressionTest`.
- File: `lynglib/src/commonMain/kotlin/net/sergeych/lyng/bytecode/SeedLocals.kt:39`
- The self-reference guard compares `FrameSlotRef` against `base + i`, but `FrameSlotRef` stores the underlying `BytecodeFrame` local slot index, not the VM absolute slot id.
- The same slot is then written using `setObjUnchecked(base + i, value)` at `SeedLocals.kt:42`, which means a self-reference is not filtered out before being reinserted.
- Impact: this can seed a local with a reference to itself, creating recursive `FrameSlotRef` chains. Reads then recurse through `BytecodeFrame.getObj()` / `FrameSlotRef.read()` instead of resolving to a value, which is a realistic path to stack overflow or non-terminating execution in bytecode-only helper execution (`executeBytecodeWithSeed`).
- Why this looks real: the equivalent check in `CmdFrame.applyCaptureRecords()` uses the local index directly (`CmdRuntime.kt:3867`), and `Script.seedModuleLocals()` also compares with the local index, not `scopeSlotCount + i` (`Script.kt:109`).
- Suggested fix: compare with `i`, not `base + i`, and add a regression test around `executeBytecodeWithSeed` seeding a scope that already contains a `FrameSlotRef` to the same local.
### 2. Design note: script-specific import/module preparation should stay explicit, not hidden in `execute(scope)`
- Status: resolved by API addition, without changing `Script.execute(scope)` semantics.
- File: `lynglib/src/commonMain/kotlin/net/sergeych/lyng/Script.kt:51-57`
- `shouldSeedModule` is only true when `execScope` is a `ModuleScope` or `thisObj === ObjVoid`.
- If a compiled script is executed against a non-module scope with a real receiver object, `seedModuleSlots()` is skipped entirely, so script imports and explicit module bindings are never installed into `moduleTarget`.
- Original concern: top-level bytecode that expects imported names or module globals can see different behavior depending on whether the host prepared the target scope explicitly.
- Resolution taken: do **not** change `Script.execute(scope)`, because in this codebase it is expected to run on exactly the provided scope and many embedding flows already rely on explicit scope setup via `Script.newScope()`, `Scope.eval(...)`, `addFn(...)`, `addConst(...)`, and direct import-manager configuration.
- New explicit APIs added instead:
- `Script.importInto(scope, seedScope = scope)`
- `Script.instantiateModule(seedScope = null, pos = script.pos)`
- This keeps existing execution behavior stable while giving hosts an opt-in way to apply a script's import/module bindings when they actually want that preparation step.
- Coverage added: `ScriptImportPreparationTest`.
### 3. Medium: missing module captures are silently converted into fresh `Unset` slots
- File: `lynglib/src/commonMain/kotlin/net/sergeych/lyng/bytecode/CmdRuntime.kt:3950-3970`
- In `CmdFrame.buildCaptureRecords()`, the module-capture path first tries several lookups. If the requested `slotId` is missing but the capture has a `name`, it calls `target.applySlotPlan(mapOf(name to slotId))` and immediately returns `target.getSlotRecord(slotId)`.
- That record is a newly created placeholder from `Scope.applySlotPlan()` (`Scope.kt:460-471`) and defaults to `ObjUnset`.
- Impact: a compiler/runtime disagreement in capture resolution is masked as a normal capture of `Unset`, so the failure moves far away from closure creation and becomes data corruption or an unrelated later exception. This will be difficult to debug when it happens.
- Suggested fix: if the named module capture cannot be resolved to an existing record, fail immediately with a Lyng error instead of manufacturing a placeholder slot. Add a regression test around missing imported/module captures.
### 4. Medium: subject-less `when { ... }` still crashes through a raw Kotlin `TODO`
- File: `lynglib/src/commonMain/kotlin/net/sergeych/lyng/Compiler.kt:6447-6449`
- The unsupported branch uses `TODO("when without object is not yet implemented")`.
- Current docs explicitly say subject-less `when` is not implemented, so the language limitation itself is documented. The problem is the failure mode: the compiler throws a raw Kotlin `NotImplementedError` instead of a normal `ScriptError` or a feature diagnostic.
- Impact: IDE/embedding callers get an implementation exception rather than a source-positioned language error, which is especially bad across non-JVM targets and for editor tooling.
- Suggested fix: replace the `TODO(...)` with a `ScriptError` at the current source position, or gate it earlier in parsing with a normal diagnostic.
## Risks Worth Checking Next
### 5. Medium risk: module frame growth only retargets records stored in `objects`/`localBindings`
- File: `lynglib/src/commonMain/kotlin/net/sergeych/lyng/ModuleScope.kt:39-67`
- When `ensureModuleFrame()` replaces the old `BytecodeFrame` with a larger one, it retargets `FrameSlotRef`s only in `objects` and `localBindings`.
- Existing closures/capture records that already hold `FrameSlotRef(oldFrame, slot)` are not retargeted here.
- Impact: if a module scope survives across recompilation/re-execution with a larger local frame, previously exported closures can keep reading stale values from the old frame instance.
- Confidence is lower because this depends on module reuse across differing compiled shapes, but the reference-retargeting logic is clearly incomplete for that scenario.
- Suggested check: add a regression covering module re-execution with increased local count and an exported closure captured before the resize.
## Test Status
- `./gradlew :lynglib:jvmTest` passed during this review.
- `./gradlew :lynglib:jvmTest --tests ScriptImportPreparationTest --tests SeedLocalsRegressionTest` passed after the fixes/API additions.
- Finding 1 is covered directly; finding 2 is covered by explicit preparation API tests.
## Suggested Fix Order
1. Fix finding 1 first: it is a concrete slot-index bug with likely recursive failure modes. Done.
2. Keep `Script.execute(scope)` semantics stable and use explicit preparation APIs where script-owned import/module setup is needed. Done.
3. Tighten finding 3 next: fail fast on capture mismatches.
4. Replace the raw `TODO` in finding 4 so unsupported syntax produces normal diagnostics.
5. Decide whether finding 5 matters for current module-reload workflows; add a regression before changing behavior.

View File

View File