BridgeJS: Fix issues exporting nested types#783
Conversation
There was a problem hiding this comment.
Thanks for tackling this, @wfltaylor, one thing to address, good otherwise
| @_cdecl("bjs_Account_Credentials_static_maxLength_get") | ||
| public func _bjs_Account_Credentials_static_maxLength_get() -> Int32 { | ||
| #if arch(wasm32) | ||
| let ret = Account_Credentials.maxLength |
There was a problem hiding this comment.
This generated line won't compile: the Swift type is Account.Credentials, but the thunk emits Account_Credentials (the flattened ABI name), which isn't a real identifier. The same shows up in StructWithNestedTypes.swift (Widget_Bounds.dimensions). The snapshot tests don't catch it because they only diff text - they never compile the generated Swift. It fails the moment the runtime target builds:
Generated/BridgeJS.swift: cannot find 'Account_Credentials' in scope
let ret = Account_Credentials.maxLength
Curiously, the nested init and static func on the same type emit the correct dotted path (Account.Credentials(...), Account.Credentials.empty()) — only the static property getter is wrong.
This isn't introduced by your PR - it's an existing bug in ExportSwift, and your new nested-type fixtures are simply the first to exercise it. I traced the history to confirm it's "never worked" rather than a regression:
ExportedProperty.callName(prefix:)short-circuits onstaticContextand ignores the passedprefix(in place since722fa703, before structs existed).PropertyRenderingContext.callName(for:)routes.structStatic/.enumStaticthrough that method, so theswiftCallNameprefix is dead-on-arrival;staticContextholds the flattened ABI name (Account_Credentials).- The identical bug for classes was fixed in
57c1fd83("Fix static property call expression for namespaced classes"), but only.classStaticwas patched — struct/enum were left behind because no fixture had ever exercised a nested static property. Top-level types are unaffected only because their ABI name coincidentally equals theirswiftCallName(no nesting → no_flattening).
Since your fixtures now commit the broken output as the expected snapshot, it'd be great to fix it here so the generated glue actually compiles. The fix mirrors the existing .classStatic one (in Plugins/BridgeJS/Sources/BridgeJSCore/ExportSwift.swift):
func callName(for property: ExportedProperty) -> String {
switch self {
case .enumStatic(let enumDef):
// property.callName() uses staticContext (the ABI name) as the prefix;
// build from swiftCallName directly so the emitted expression is valid
// Swift for nested types (`Outer.Inner`, not `Outer_Inner`).
return "\(enumDef.swiftCallName).\(property.name)"
case .classStatic(let klass):
// property.callName() would use staticContext (the ABI name) as prefix;
// use swiftCallName directly so the emitted expression is valid Swift.
return "\(klass.swiftCallName).\(property.name)"
case .classInstance:
return property.callName()
case .structStatic(let structDef):
return "\(structDef.swiftCallName).\(property.name)"
}
}After changing this, regenerate the snapshots so ClassWithNestedTypes.swift / StructWithNestedTypes.swift pick up the dotted paths.
Snapshot tests can't catch non-compiling glue, but the Wasm runtime target compiles and executes it - so a small e2e is the right guard rail (and it also proves your merge works at runtime). Here's the one I used. Swift side, Tests/BridgeJSRuntimeTests/NestedTypeAPIs.swift:
import JavaScriptKit
import XCTest
@JS class NtAccount {
@JS enum NtRole: String {
case admin
case guest
}
@JS struct NtCreds {
var token: String
@JS init(token: String) {
self.token = token
}
@JS static var maxLength: Int { 64 }
@JS static func empty() -> NtCreds {
NtCreds(token: "")
}
}
@JS var name: String
@JS init(name: String) {
self.name = name
}
@JS func describe() -> String {
name
}
}
@JS struct NtWidget {
@JS enum NtVariant: String {
case button
case slider
}
@JS struct NtBounds {
var width: Int
var height: Int
@JS init(width: Int, height: Int) {
self.width = width
self.height = height
}
@JS static func zero() -> NtBounds {
NtBounds(width: 0, height: 0)
}
}
var name: String
@JS init(name: String) {
self.name = name
}
}
@_extern(wasm, module: "BridgeJSRuntimeTests", name: "runJsNestedTypesWork")
@_extern(c)
func runJsNestedTypesWork() -> Void
final class SwiftNestedTypeTests: XCTestCase {
func testNestedTypeMerging() {
runJsNestedTypesWork()
}
}JS side, in Tests/prelude.mjs — register the handler alongside the other bridgeJSRuntimeTests[...] entries:
bridgeJSRuntimeTests["runJsNestedTypesWork"] = () => {
const exports = getExports();
if (!exports) {
throw new Error("No exports!?");
}
return BridgeJSRuntimeTests_runJsNestedTypesWork(exports);
};…and add the assertion function:
function BridgeJSRuntimeTests_runJsNestedTypesWork(exports) {
// Class whose name collides with its nested-type namespace: still constructible
// AND carrying its merged nested members.
const account = new exports.NtAccount("alice");
assert.equal(account.describe(), "alice");
account.release();
assert.equal(exports.NtAccount.NtRole.Admin, "admin");
assert.equal(exports.NtAccount.NtRole.Guest, "guest");
const creds = exports.NtAccount.NtCreds.init("tok");
assert.equal(creds.token, "tok");
assert.equal(exports.NtAccount.NtCreds.maxLength, 64); // exercises the static-property getter
assert.equal(exports.NtAccount.NtCreds.empty().token, "");
// Struct whose name collides with its nested-type namespace: the value-side
// object literal must merge `init`, the nested enum, and the nested struct.
const widget = exports.NtWidget.init("w");
assert.equal(widget.name, "w");
assert.equal(exports.NtWidget.NtVariant.Button, "button");
assert.equal(exports.NtWidget.NtVariant.Slider, "slider");
const bounds = exports.NtWidget.NtBounds.init(3, 4);
assert.equal(bounds.width, 3);
assert.equal(bounds.height, 4);
const zero = exports.NtWidget.NtBounds.zero();
assert.equal(zero.width, 0);
assert.equal(zero.height, 0);
}With the callName(for:) fix in place this passes
There are currently a few issues with the JS/TS code generation, due to a lack of merging. For example, the following Swift code:
generates a
.d.ts:This doesn’t type check correctly. Similar issues occur with the emitted
.js, except instead of failing to type check, these will cause runtime issues (since keys are overwritten instead of merged).This PR is a bit of a refactor of
BridgeJSLinkto address this. Instead of being a member of aNamespaceNode, each class and struct becomes its ownNamespaceNode. This makes it much easier to merge everything together so it can be emitted as one unit.