Skip to content

[BridgeJS] Synthesize typed-closure init access from declaration surface (#709)#727

Open
matthewa26 wants to merge 1 commit intoswiftwasm:mainfrom
matthewa26:fix/issue-709-typed-closure-init-access
Open

[BridgeJS] Synthesize typed-closure init access from declaration surface (#709)#727
matthewa26 wants to merge 1 commit intoswiftwasm:mainfrom
matthewa26:fix/issue-709-typed-closure-init-access

Conversation

@matthewa26
Copy link
Copy Markdown
Contributor

Summary

Fixes #709. The extension JSTypedClosure where Signature == ... { init(...) } synthesized by BridgeJS is always emitted as internal, so a public @JSClass exposing a JSTypedClosure<...> parameter cannot be consumed from another target — downstream callers have no way to construct the closure value without hand-rolling a public wrapper.

This change derives the synthesized init's access level from the originating Swift declaration:

  • Imported skeleton entries (ImportedFunctionSkeleton, ImportedTypeSkeleton, ImportedConstructorSkeleton, ImportedGetterSkeleton, ImportedSetterSkeleton) record the source access level (new BridgeJSAccessLevel enum: internal < package < public, default internal).
  • BridgeSkeletonWalker threads the enclosing decl's access level into BridgeSkeletonVisitor.visitClosure(...). Exported decls reuse the existing explicitAccessControl: String? field ("public" / "package" / "internal" / nil → inherit).
  • ClosureSignatureCollectorVisitor now stores [ClosureSignature: BridgeJSAccessLevel] and takes the max access level across every surface that references a given signature, so a closure shape used by both a public and an internal method becomes public (one extension is generated per signature). signatures: Set<ClosureSignature> is preserved as a computed view for BridgeJSLink.
  • ClosureCodegen.renderClosureHelpers prefixes the synthesized init with public / package (or leaves it bare for internal). Mirrors the pattern JSClassMacro already uses for init(unsafelyWrapping:).

The user's example from #709 now generates public init(...):

@JSClass(jsName: "Document") public struct JSDocument {
    @JSFunction public func addEventListener(_ type: String, _ listener: JSTypedClosure<(JSEvent) -> Void>) throws(JSException)
}
// →
extension JSTypedClosure where Signature == (JSEvent) -> Void {
    public init(fileID: StaticString = #fileID, line: UInt32 = #line, _ body: @escaping (JSEvent) -> Void) { ... }
}

Test plan

  • New input fixture SwiftTypedClosureAccess.swift covering: a public @JSClass/@JSFunction (→ public init), a package surface (→ package init), an internal-only surface (→ bare init), and a closure shape shared between a public and an internal method (→ merges to public init).
  • swift test --package-path Plugins/BridgeJS — all 107 tests in 9 suites pass.
  • Codegen JSON snapshots re-recorded to include the new accessLevel field on imported decls (defaults to "internal" for unchanged fixtures, so no semantic drift).
  • swift build --package-path Examples/Basic builds clean end-to-end.

Resolves swiftwasm#709: a public `@JSClass` exposing a
`JSTypedClosure<...>` parameter could not be consumed from another target
because the synthesized `extension JSTypedClosure { init(...) }` was always
internal, leaving downstream callers no way to construct the closure value
without hand-rolling a public wrapper.

Imported skeleton entries now record the source access level
(`public`/`package`/`internal`); the closure-signature collector takes the
maximum across every surface that references a given signature, and
`ClosureCodegen` prefixes the synthesized init with the resulting modifier
(internal stays bare). This matches the pattern `JSClassMacro` already uses
for `init(unsafelyWrapping:)`.
Copy link
Copy Markdown
Member

@krodak krodak left a comment

Choose a reason for hiding this comment

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

Hey Matthew, appreciate clean fix for a real usability problem 👌🏻
The approach of threading access levels through the skeleton/walker and merging via max fits the existing architecture well. Test fixture covers the key scenarios. A few suggestions below, nothing blocking.

accessLevel: BridgeJSAccessLevel
) {
if let existing = signatureAccessLevels[signature] {
signatureAccessLevels[signature] = max(existing, accessLevel)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The "check existing, take max, else insert" pattern here is duplicated in recordInjectedSignature below. If the merge logic ever needs to change (e.g. adding a diagnostic for conflicting levels), you'd need to update both spots.

Small extract:

private mutating func recordSignature(
    _ signature: ClosureSignature,
    accessLevel: BridgeJSAccessLevel
) {
    if let existing = signatureAccessLevels[signature] {
        signatureAccessLevels[signature] = max(existing, accessLevel)
    } else {
        signatureAccessLevels[signature] = accessLevel
    }
}

Then both visitClosure and recordInjectedSignature call through to it.

_ body: (inout BridgeSkeletonWalker) -> Void
) {
withAccessLevel(rawLevel.flatMap(BridgeJSAccessLevel.init(rawValue:)), body)
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

rawLevel.flatMap(BridgeJSAccessLevel.init(rawValue:)) silently drops unknown strings (e.g. "open", "private") and falls back to inheriting the outer level. That's fine today since the macros reject those, but it's a quiet invariant. An assertion for unexpected values would save debugging time if the exported side ever gains new access strings:

private mutating func withAccessLevel(
    _ rawLevel: String?,
    _ body: (inout BridgeSkeletonWalker) -> Void
) {
    let level: BridgeJSAccessLevel?
    if let rawLevel {
        level = BridgeJSAccessLevel(rawValue: rawLevel)
        assert(level != nil, "Unexpected access level string: \(rawLevel)")
    } else {
        level = nil
    }
    withAccessLevel(level, body)
}

self.signatures = signatures
for signature in signatures {
signatureAccessLevels[signature] = .internal
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seeds every pre-existing signature as .internal. That's correct for the only current caller (BridgeJSLink, exported side), but the API doesn't communicate the assumption. If someone later pre-seeds signatures that should be public, they'd silently get capped.

Two options (both low-effort):

  1. Add a doc comment on this init noting the assumption:
/// Convenience for callers that only need to seed signatures without
/// access metadata (e.g. exported-side walking where closure init
/// access is irrelevant). All seeded signatures default to `.internal`.
public init(moduleName: String, signatures: Set<ClosureSignature>) {
  1. Or offer a dictionary-based init alongside it:
public init(moduleName: String, signatureAccessLevels: [ClosureSignature: BridgeJSAccessLevel] = [:]) {
    self.moduleName = moduleName
    self.signatureAccessLevels = signatureAccessLevels
}

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.

[BridgeJS] Generated JSTypedClosure initializer is always internal

2 participants