Skip to content

Commit 4097e13

Browse files
krodakkrodak
committed
BridgeJS: Code review polish — descriptor-driven liftExpression/emitOptionalPlaceholders, trailing comma fix, throw on unknown types
1 parent 7ca5dac commit 4097e13

6 files changed

Lines changed: 39 additions & 51 deletions

File tree

‎Plugins/BridgeJS/Sources/BridgeJSCore/ClosureCodegen.swift‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,6 @@ public struct ClosureCodegen {
154154
abiReturnWasmType = nil
155155
}
156156

157-
let throwReturn = abiReturnWasmType?.swiftReturnPlaceholderStmt ?? "return"
158-
159157
// Build signature using SwiftSignatureBuilder
160158
let funcSignature = SwiftSignatureBuilder.buildABIFunctionSignature(
161159
abiParameters: abiParams,

‎Plugins/BridgeJS/Sources/BridgeJSLink/BridgeJSLink.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1535,15 +1535,15 @@ public struct BridgeJSLink {
15351535

15361536
switch enumDefinition.enumType {
15371537
case .simple:
1538-
let fragment = IntrinsicJSFragment.simpleEnumHelper(enumDefinition: enumDefinition)
1538+
let fragment = IntrinsicJSFragment.caseEnumHelper(enumDefinition: enumDefinition)
15391539
_ = try fragment.printCode([enumValuesName], context)
15401540
jsTopLevelLines.append(contentsOf: printer.lines)
15411541
case .rawValue:
15421542
guard enumDefinition.rawType != nil else {
15431543
throw BridgeJSLinkError(message: "Raw value enum \(enumDefinition.name) is missing rawType")
15441544
}
15451545

1546-
let fragment = IntrinsicJSFragment.simpleEnumHelper(enumDefinition: enumDefinition)
1546+
let fragment = IntrinsicJSFragment.caseEnumHelper(enumDefinition: enumDefinition)
15471547
_ = try fragment.printCode([enumValuesName], context)
15481548
jsTopLevelLines.append(contentsOf: printer.lines)
15491549
case .associatedValue:

‎Plugins/BridgeJS/Sources/BridgeJSLink/JSGlueGen.swift‎

Lines changed: 28 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -847,7 +847,9 @@ struct IntrinsicJSFragment: Sendable {
847847
printer.write("}")
848848
resultExpr = dictVar
849849
default:
850-
resultExpr = "\(isSome) ? \(wrappedValue) : \(absenceLiteral)"
850+
throw BridgeJSLinkError(
851+
message: "Unsupported wrapped type for optionalLiftParameter: \(wrappedType)"
852+
)
851853
}
852854

853855
return [resultExpr]
@@ -999,7 +1001,9 @@ struct IntrinsicJSFragment: Sendable {
9991001
cleanupCode.write("}")
10001002
return ["+\(isSomeVar)", "\(isSomeVar) ? \(idVar) : 0"]
10011003
default:
1002-
return ["+\(isSomeVar)", "\(isSomeVar) ? \(value) : 0"]
1004+
throw BridgeJSLinkError(
1005+
message: "Unsupported wrapped type for optionalLowerParameter: \(wrappedType)"
1006+
)
10031007
}
10041008
}
10051009
)
@@ -1365,26 +1369,15 @@ struct IntrinsicJSFragment: Sendable {
13651369
scope: JSGlueVariableScope,
13661370
printer: CodeFragmentPrinter
13671371
) -> Bool {
1368-
switch wrappedType {
1369-
case .float:
1370-
scope.emitPushF32Parameter("0.0", printer: printer)
1371-
return true
1372-
case .double:
1373-
scope.emitPushF64Parameter("0.0", printer: printer)
1374-
return true
1375-
case .swiftStruct, .array, .dictionary:
1372+
let desc = wrappedType.descriptor
1373+
let params = desc.wasmParams
1374+
if params.isEmpty {
13761375
return false
1377-
case .string, .rawValueEnum(_, .string):
1378-
scope.emitPushI32Parameter("0", printer: printer)
1379-
scope.emitPushI32Parameter("0", printer: printer)
1380-
return true
1381-
case .swiftHeapObject:
1382-
scope.emitPushPointerParameter("0", printer: printer)
1383-
return true
1384-
default:
1385-
scope.emitPushI32Parameter("0", printer: printer)
1386-
return true
13871376
}
1377+
for param in params {
1378+
emitPush(for: param.type, value: param.type.jsZeroLiteral, scope: scope, printer: printer)
1379+
}
1380+
return true
13881381
}
13891382

13901383
/// Builds the four-direction glue fragments from a descriptor's jsGlue info.
@@ -1796,7 +1789,7 @@ struct IntrinsicJSFragment: Sendable {
17961789
)
17971790
}
17981791

1799-
static func simpleEnumHelper(enumDefinition: ExportedEnum) -> IntrinsicJSFragment {
1792+
static func caseEnumHelper(enumDefinition: ExportedEnum) -> IntrinsicJSFragment {
18001793
return IntrinsicJSFragment(
18011794
parameters: ["enumName"],
18021795
printCode: { arguments, context in
@@ -2346,7 +2339,7 @@ struct IntrinsicJSFragment: Sendable {
23462339
let (scope, printer) = (context.scope, context.printer)
23472340
let resultVar = scope.variable("enumValue")
23482341
printer.write(
2349-
"const \(resultVar) = \(JSGlueVariableScope.reservedEnumHelpers).\(base).lift(\(scope.popI32()), );"
2342+
"const \(resultVar) = \(JSGlueVariableScope.reservedEnumHelpers).\(base).lift(\(scope.popI32()));"
23502343
)
23512344
return [resultVar]
23522345
}
@@ -2658,7 +2651,11 @@ struct IntrinsicJSFragment: Sendable {
26582651

26592652
let instanceProps = structDef.properties.filter { !$0.isStatic }
26602653
for property in instanceProps {
2661-
let fragment = try structFieldLowerFragment(field: property, allStructs: allStructs)
2654+
let fragment = try structFieldLowerFragment(
2655+
type: property.type,
2656+
fieldName: property.name,
2657+
allStructs: allStructs
2658+
)
26622659
let fieldValue = "value.\(property.name)"
26632660
_ = try fragment.printCode(
26642661
[fieldValue],
@@ -2766,10 +2763,11 @@ struct IntrinsicJSFragment: Sendable {
27662763
}
27672764

27682765
private static func structFieldLowerFragment(
2769-
field: ExportedProperty,
2766+
type: BridgeType,
2767+
fieldName: String,
27702768
allStructs: [ExportedStruct]
27712769
) throws -> IntrinsicJSFragment {
2772-
switch field.type {
2770+
switch type {
27732771
case .jsValue:
27742772
preconditionFailure("Struct field of JSValue is not supported yet")
27752773
case .jsObject:
@@ -2954,12 +2952,8 @@ struct IntrinsicJSFragment: Sendable {
29542952
printer.write("if (\(isSomeVar)) {")
29552953
try printer.indent {
29562954
let innerFragment = try structFieldLowerFragment(
2957-
field: ExportedProperty(
2958-
name: field.name,
2959-
type: wrappedType,
2960-
isReadonly: true,
2961-
isStatic: false
2962-
),
2955+
type: wrappedType,
2956+
fieldName: fieldName,
29632957
allStructs: allStructs
29642958
)
29652959
let _ = try innerFragment.printCode(
@@ -3022,13 +3016,13 @@ struct IntrinsicJSFragment: Sendable {
30223016
parameters: ["value"],
30233017
printCode: { arguments, context in
30243018
context.printer.write(
3025-
"throw new Error(\"Unsupported struct field type for lowering: \(field.type)\");"
3019+
"throw new Error(\"Unsupported struct field type for lowering: \(type)\");"
30263020
)
30273021
return []
30283022
}
30293023
)
30303024
default:
3031-
return try stackLowerFragment(elementType: field.type)
3025+
return try stackLowerFragment(elementType: type)
30323026
}
30333027
}
30343028

@@ -3057,7 +3051,7 @@ struct IntrinsicJSFragment: Sendable {
30573051
let caseIdVar = scope.variable("enumCaseId")
30583052
printer.write("const \(caseIdVar) = \(scope.popI32());")
30593053
printer.write(
3060-
"\(optVar) = \(JSGlueVariableScope.reservedEnumHelpers).\(base).lift(\(caseIdVar), );"
3054+
"\(optVar) = \(JSGlueVariableScope.reservedEnumHelpers).\(base).lift(\(caseIdVar));"
30613055
)
30623056
} else {
30633057
let wrappedFragment = try structFieldLiftFragment(

‎Plugins/BridgeJS/Sources/BridgeJSSkeleton/BridgeJSSkeleton.swift‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ public enum WasmCoreType: String, Codable, Sendable {
172172
case .i32, .i64, .pointer: return "0"
173173
}
174174
}
175+
175176
}
176177

177178
// MARK: - ABI Descriptor
@@ -1436,19 +1437,18 @@ extension BridgeType {
14361437
guard case .nullable(let wrappedType, _) = self else {
14371438
return false
14381439
}
1439-
14401440
switch wrappedType {
1441-
case .string, .int, .float, .double, .jsObject, .swiftProtocol:
1441+
case .string, .rawValueEnum(_, .string):
1442+
return true
1443+
case .jsObject, .swiftProtocol:
1444+
return true
1445+
case .int, .uint, .float, .double:
14421446
return true
14431447
case .rawValueEnum(_, let rawType):
14441448
switch rawType {
1445-
case .string, .int, .float, .double:
1446-
return true
1447-
default:
1448-
return false
1449+
case .bool: return false
1450+
default: return true
14491451
}
1450-
case .bool, .caseEnum, .swiftHeapObject, .associatedValueEnum:
1451-
return false
14521452
default:
14531453
return false
14541454
}

‎Plugins/BridgeJS/Tests/BridgeJSToolTests/__Snapshots__/BridgeJSLinkTests/EnumAssociatedValue.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -699,7 +699,7 @@ export async function createInstantiator(options, swift) {
699699
return { tag: AllTypesResultValues.Tag.JsObjectPayload, param0: obj };
700700
}
701701
case AllTypesResultValues.Tag.NestedEnum: {
702-
const enumValue = enumHelpers.APIResult.lift(i32Stack.pop(), );
702+
const enumValue = enumHelpers.APIResult.lift(i32Stack.pop());
703703
return { tag: AllTypesResultValues.Tag.NestedEnum, param0: enumValue };
704704
}
705705
case AllTypesResultValues.Tag.ArrayPayload: {

‎Tests/BridgeJSRuntimeTests/JavaScript/OptionalSupportTests.mjs‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -210,10 +210,6 @@ export function runJsOptionalSupportTests(exports) {
210210
const oatr_empty = { tag: OptionalAllTypesResultValues.Tag.Empty };
211211
assert.deepEqual(exports.roundTripOptionalPayloadResult(oatr_empty), oatr_empty);
212212

213-
// Stack-corruption regression: lowering a None optional array must not leak
214-
// stale values onto the i32Stack. By passing two enums in one call, both are
215-
// lowered onto the same stacks before Swift reads them. A stale placeholder
216-
// from the first value would misalign the second value's lift.
217213
const twoResult = exports.roundTripTwoOptionalPayloadResults(oatr_arrayNone, oatr_nestedEnumSome);
218214
assert.deepEqual(twoResult, oatr_nestedEnumSome);
219215
const twoResult2 = exports.roundTripTwoOptionalPayloadResults(oatr_arrayNone, oatr_arraySome);

0 commit comments

Comments
 (0)