Skip to content

NFC: BridgeJS: Refactor JSGlueGen with compositional optional handling and helper consolidation#656

Open
krodak wants to merge 1 commit intoswiftwasm:mainfrom
PassiveLogic:kr/jsglue-refactor
Open

NFC: BridgeJS: Refactor JSGlueGen with compositional optional handling and helper consolidation#656
krodak wants to merge 1 commit intoswiftwasm:mainfrom
PassiveLogic:kr/jsglue-refactor

Conversation

@krodak
Copy link
Member

@krodak krodak commented Feb 18, 2026

Mix of concrete simplifications and structural refactoring in JSGlueGen.swift. Happy to split this into a smaller PR with just the simplification items if the refactoring part is too much for one review.

Simplifications

  • Factory collapse: Struct and enum helper factories collapsed from () => { return () => ({...}); } to () => ({...}) - the double-invocation wrapper was left over from PR NFC: BridgeJS: Remove dead cleanup infrastructure #655
  • simpleEnumHelper/rawValueEnumHelper unified into caseEnumHelper - they had identical logic
  • Dead code removed: emitPushI32Return, emitPushF64Return, emitPushPointerReturn were copies of their parameter counterparts; several fragment aliases that just forwarded to another fragment
  • emitRetainCase helper extracted to deduplicate 5 identical JSValue retain-and-break cases
  • jsZeroLiteral scoped down from public WasmCoreType API in BridgeJSSkeleton to fileprivate in JSGlueGen - nobody else uses it
  • Redundant (ret | 0) coercion dropped from optional case enum returns - the non-optional path never applied it, and the value is already an integer from the enum helper so | 0 was a no-op

Structural refactoring

  • Compositional optional handling: Instead of separate IntrinsicJSFragment definitions for every optional(X) combination, generic fragments wrap the inner type's fragments. Adding a new BridgeType no longer requires writing dedicated optional fragment variants - you set a few properties and optional support composes from the base fragments. The stack ABI optional path is fully generic, which should help with planned external class support.
  • Type-knowledge properties on BridgeType: wasmParams, optionalConvention, nilSentinel, isSingleParamScalar, lowerCoerce, etc. moved from free functions and static methods into a private extension BridgeType in JSGlueGen.swift. Reads as type.wasmParams instead of wasmParams(for: type).
  • reserveNames scope change to fileprivate, moved to bottom of JSGlueVariableScope

Trade-offs

The composite optional wrapper produces slightly more verbose JS than the old per-type fragments (result intermediary vars and if/else vs inline ternaries). The codegen is easier to extend and engines optimize both patterns the same way.

@krodak krodak self-assigned this Feb 18, 2026
@krodak krodak marked this pull request as draft February 18, 2026 14:14
@krodak krodak force-pushed the kr/jsglue-refactor branch 5 times, most recently from 0126fea to 9cd388d Compare February 18, 2026 14:59
@krodak krodak force-pushed the kr/jsglue-refactor branch from 9cd388d to 1ad419b Compare February 18, 2026 15:06
@krodak krodak marked this pull request as ready for review February 18, 2026 15:39
Copy link
Member

@kateinoigakukun kateinoigakukun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

return IntrinsicJSFragment(
parameters: ["value"],
printCode: { arguments, context in
let (scope, printer) = (context.scope, context.printer)
let value = arguments[0]
scope.reserveNames(arguments)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arguments given to IntrinsicJSFragment are not guaranteed to be a variable name but it can be an arbitrary expression, so we can't register them into the scope.

Instead, what we really need to do here seems like registering parameter names into the scope?

class ExportedThunkBuilder {
    func lowerParameter(param: Parameter) throws {
        let loweringFragment = try IntrinsicJSFragment.lowerParameter(type: param.type)
        assert(
            loweringFragment.parameters.count == 1,
            "Lowering fragment should have exactly one parameter to lower"
        )
        let paramName = scope.variable(param.name)
        self.parameterNames.append(paramName)
        let loweredValues = try loweringFragment.printCode([paramName], context)
        parameterForwardings.append(contentsOf: loweredValues)
    }
}

)
}

static func optionalLowerParameter(wrappedType: BridgeType) throws -> IntrinsicJSFragment {
private static func compositeOptionalLowerParameter(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments