◐ Shell
clean mode source ↗

BridgeJS: Add Dictionary support by kateinoigakukun · Pull Request #581 · swiftwasm/JavaScriptKit

kateinoigakukun

Hey @kateinoigakukun, found and fixed a couple bugs in the dictionary PR for optional dictionary handling on exports.

Bug 1: Missing Optional dictionary support in BridgeJSIntrinsics.swift

The Optional where Wrapped: _BridgedSwiftDictionaryStackType extension was incomplete. When you have @JS func foo(_ values: [String: String]?) -> [String: String]?, both the parameter lifting and return lowering paths need proper handling.

Added complete Optional support:

extension Optional where Wrapped: _BridgedSwiftDictionaryStackType {
    @_spi(BridgeJS) public consuming func bridgeJSLowerParameter() -> Int32 {
        switch consume self {
        case .none:
            return 0
        case .some(let dict):
            dict.bridgeJSLowerReturn()
            return 1
        }
    }

    @_spi(BridgeJS) public consuming func bridgeJSLowerReturn() {
        switch consume self {
        case .none:
            _swift_js_push_i32(0)
        case .some(let dict):
            dict.bridgeJSLowerReturn()
            _swift_js_push_i32(1)
        }
    }

    @_spi(BridgeJS) public static func bridgeJSLiftParameter(_ isSome: Int32) -> [String: Wrapped.DictionaryValue]? {
        if isSome == 0 {
            return nil
        }
        return Dictionary<String, Wrapped.DictionaryValue>.bridgeJSLiftParameter()
    }

    // ... plus bridgeJSLiftParameter() and bridgeJSLiftReturn()
}

Bug 2: Missing .dictionary case in JSGlueGen.swift

The optionalLiftReturn function in JSGlueGen.swift was missing the .dictionary case for lifting optional dictionary return values on the JS side. Added following the same pattern as .array:

case .dictionary(let valueType):
    let isSomeVar = scope.variable("isSome")
    printer.write("const \(isSomeVar) = \(JSGlueVariableScope.reservedTmpRetInts).pop();")
    printer.write("let \(resultVar);")
    printer.write("if (\(isSomeVar)) {")
    printer.indent {
        let dictLiftFragment = try! dictionaryLift(valueType: valueType)
        let liftResults = dictLiftFragment.printCode([], scope, printer, cleanupCode)
        if let liftResult = liftResults.first {
            printer.write("\(resultVar) = \(liftResult);")
        }
    }
    printer.write("} else {")
    printer.indent {
        printer.write("\(resultVar) = \(absenceLiteral);")
    }
    printer.write("}")

So result was falling back to default code path resulting in invalid behavior, now removed:

-                    const optResult = tmpRetString;
-                    tmpRetString = undefined;
+                    const isSome1 = tmpRetInts.pop();
+                    let optResult;
+                    if (isSome1) {
+                        const dictLen = tmpRetInts.pop();
+                        const dictResult = {};
+                        for (let i = 0; i < dictLen; i++) {
+                            const string = tmpRetStrings.pop();
+                            const string1 = tmpRetStrings.pop();
+                            dictResult[string1] = string;
+                        }
+                        optResult = dictResult;
+                    } else {
+                        optResult = null;
+                    }

Also: e2e export tests were missing

Added e2e tests for both non-optional and optional dictionary exports.

Changes are on krodak/dict-fix, feel free to integrate them; I'm quite sure we need bug 2 fix with glue code, for bug 1 maybe there is more elegant solution, but couldn't find one

You can validate the issue, by including e2e export tests and running the build 🙏🏻