BridgeJS: fix named-import regressions found in review

kateinoigakukun · kateinoigakukun · commit 02b8bce33b45 · 2026-07-30T13:37:09.000+01:00
Do not require a module export for a wrapper-only `@JSClass`, since a named import is a
link-time requirement and nothing looks that name up; accept `jsName: nil` and the explicit
`.name(...)` spelling; and validate the tagged `from` form like the plain-string form.
diff --git a/Plugins/BridgeJS/Sources/BridgeJSCore/SwiftToSkeleton.swift b/Plugins/BridgeJS/Sources/BridgeJSCore/SwiftToSkeleton.swift
@@ -2570,6 +2570,30 @@ private final class ImportSwiftMacrosAPICollector: SyntaxAnyVisitor {
             return ExtractedJSName(memberName: value, isDefaultExportSpelling: false)
         }
 
+        // An explicit `jsName: nil` means the same as omitting the argument.
+        if argument.expression.is(NilLiteralExprSyntax.self) {
+            return nil
+        }
+
+        // Accept the explicit `.name("...")` spelling of a plain member name.
+        if let call = argument.expression.as(FunctionCallExprSyntax.self),
+            call.calledExpression.trimmedDescription.split(separator: ".").last == "name"
+        {
+            guard call.arguments.count == 1,
+                let literal = call.arguments.first?.expression.as(StringLiteralExprSyntax.self),
+                let value = literal.representedLiteralValue
+            else {
+                errors.append(
+                    DiagnosticError(
+                        node: call.arguments.first?.expression ?? argument.expression,
+                        message: "jsName must be a string literal or '.default'."
+                    )
+                )
+                return nil
+            }
+            return ExtractedJSName(memberName: value, isDefaultExportSpelling: false)
+        }
+
         // Accept `.default`, `JSName.default`, and the backticked spellings.
         let description = argument.expression.trimmedDescription
         let caseName = description.split(separator: ".").last.map(String.init) ?? description
diff --git a/Plugins/BridgeJS/Sources/BridgeJSLink/ImportedJSModuleRegistry.swift b/Plugins/BridgeJS/Sources/BridgeJSLink/ImportedJSModuleRegistry.swift
@@ -61,18 +61,21 @@ final class ImportedJSModuleRegistry {
 
         for (index, reference) in references.enumerated() {
             let members = (membersByReference[reference] ?? []).sorted()
+            // A reference with no member lookups keeps the namespace form: a named import
+            // is a hard link-time requirement, so importing a name nothing references
+            // would fail the whole module load if the module does not export it.
             bindings[reference] = Binding(
                 index: index,
                 members: members,
-                usesNamedImports: members.allSatisfy(Self.isValidJSIdentifier)
+                usesNamedImports: !members.isEmpty && members.allSatisfy(Self.isValidJSIdentifier)
             )
         }
     }
 
     static func collectReferences(skeletons: [BridgeJSSkeleton]) -> [Reference] {
         var references = Set<Reference>()
         for skeleton in skeletons {
-            forEachMemberLookup(skeleton: skeleton) { reference, _ in
+            forEachOrigin(skeleton: skeleton) { reference in
                 references.insert(reference)
             }
         }
@@ -86,12 +89,40 @@ final class ImportedJSModuleRegistry {
         }
     }
 
+    /// Visits every module origin mentioned by the skeleton, whether or not code
+    /// generation looks a member up on it.
+    ///
+    /// This is what decides which modules are imported at all, and for local paths
+    /// which files packaging copies. It stays broader than `forEachMemberLookup` so
+    /// that a module mentioned only by a wrapper-only `@JSClass` is still imported,
+    /// preserving its side effects.
+    private static func forEachOrigin(
+        skeleton: BridgeJSSkeleton,
+        _ body: (Reference) -> Void
+    ) {
+        func visit(from: JSImportFrom?) {
+            guard let reference = Self.reference(swiftModuleName: skeleton.moduleName, from: from) else { return }
+            body(reference)
+        }
+        for file in skeleton.imported?.children ?? [] {
+            for function in file.functions { visit(from: function.from) }
+            for getter in file.globalGetters { visit(from: getter.from) }
+            for type in file.types { visit(from: type.from) }
+        }
+    }
+
     /// Visits every module member lookup that code generation will emit.
     ///
     /// The member name taken here must match what the corresponding emitter in
-    /// `BridgeJSLink` looks up: a class contributes a single binding that serves both
-    /// its constructor and its static methods, and instance methods contribute nothing
-    /// because they call through an already-constructed instance.
+    /// `BridgeJSLink` looks up, and must not include names it never emits: a named
+    /// import is a hard link-time requirement, so recording a member that no
+    /// generated code references would make the module fail to load whenever the
+    /// module does not happen to export that name.
+    ///
+    /// A class contributes a single binding that serves both its constructor and its
+    /// static methods, and only when it has one of those. Instance methods, getters,
+    /// and setters contribute nothing because they go through an already-constructed
+    /// instance, so a wrapper-only `@JSClass` needs no export from the module at all.
     private static func forEachMemberLookup(
         skeleton: BridgeJSSkeleton,
         _ body: (Reference, String) -> Void
@@ -108,6 +139,7 @@ final class ImportedJSModuleRegistry {
                 visit(from: getter.from, memberName: getter.jsName ?? getter.name)
             }
             for type in file.types {
+                guard type.constructor != nil || !type.staticMethods.isEmpty else { continue }
                 visit(from: type.from, memberName: type.jsName ?? type.name)
             }
         }
diff --git a/Plugins/BridgeJS/Sources/BridgeJSSkeleton/BridgeJSSkeleton.swift b/Plugins/BridgeJS/Sources/BridgeJSSkeleton/BridgeJSSkeleton.swift
@@ -1161,7 +1161,34 @@ public enum JSImportFrom: Codable, Equatable, Sendable {
                 debugDescription: "Unknown import origin kind '\(kind)'. Expected \"module\"."
             )
         }
-        self = .module(try container.decode(String.self, forKey: .specifier))
+        let specifier = try container.decode(String.self, forKey: .specifier)
+        // Apply the same rules as the single-value form above, so a specifier cannot
+        // reach code generation through this path that the string path would reject.
+        guard !specifier.isEmpty else {
+            throw DecodingError.dataCorruptedError(
+                forKey: .specifier,
+                in: container,
+                debugDescription: "Module specifier must not be empty."
+            )
+        }
+        guard !specifier.hasPrefix(".") else {
+            throw DecodingError.dataCorruptedError(
+                forKey: .specifier,
+                in: container,
+                debugDescription: "Module specifier '\(specifier)' must not be relative."
+            )
+        }
+        // A rooted path names a file we resolve inside the Swift target, so it must not
+        // traverse out of it. A bare specifier is resolved by the JavaScript host, where
+        // path segments carry no such meaning, so it is left alone.
+        guard !specifier.hasPrefix("/") || !specifier.split(separator: "/").contains("..") else {
+            throw DecodingError.dataCorruptedError(
+                forKey: .specifier,
+                in: container,
+                debugDescription: "Local module path '\(specifier)' must not contain '..'."
+            )
+        }
+        self = .module(specifier)
     }
 
     public func encode(to encoder: any Encoder) throws {
diff --git a/Plugins/BridgeJS/Tests/BridgeJSToolTests/BridgeJSCodegenTests.swift b/Plugins/BridgeJS/Tests/BridgeJSToolTests/BridgeJSCodegenTests.swift
@@ -88,6 +88,19 @@ import Testing
         }
     }
 
+    /// The keyed form must reject what the string form rejects, so a specifier cannot reach
+    /// code generation through the tagged object that the plain-string path would refuse.
+    @Test(arguments: [
+        #"{"kind": "module", "specifier": ""}"#,
+        #"{"kind": "module", "specifier": "./relative.mjs"}"#,
+        #"{"kind": "module", "specifier": "/../../escape.mjs"}"#,
+    ])
+    func invalidKeyedJSImportFromFailsToDecode(json: String) {
+        #expect(throws: DecodingError.self) {
+            try JSONDecoder().decode(JSImportFrom.self, from: Data(json.utf8))
+        }
+    }
+
     private func snapshotCodegen(
         skeleton: BridgeJSSkeleton,
         name: String,
diff --git a/Plugins/BridgeJS/Tests/BridgeJSToolTests/DiagnosticsTests.swift b/Plugins/BridgeJS/Tests/BridgeJSToolTests/DiagnosticsTests.swift
@@ -109,6 +109,26 @@ import Testing
         #expect(diagnostics.description.contains("is not supported on a class member"))
     }
 
+    /// `jsName: nil` is valid Swift and means the same as omitting the argument.
+    @Test
+    func explicitNilJSNameIsAccepted() throws {
+        let source = """
+            let unrelated = 0
+            @JSFunction(jsName: nil, from: .global) func imported() throws(JSException)
+            """
+        #expect(moduleDiagnostics(source: source) == nil)
+    }
+
+    /// `JSName.name(_:)` is public and documented, so its explicit spelling must work.
+    @Test
+    func explicitNameCaseSpellingIsAccepted() throws {
+        let source = """
+            let unrelated = 0
+            @JSFunction(jsName: .name("basename"), from: .module("node:path")) func imported() throws(JSException)
+            """
+        #expect(moduleDiagnostics(source: source) == nil)
+    }
+
     @Test
     func jsNameMustBeStringLiteralOrDefault() throws {
         let source = """
diff --git a/Plugins/BridgeJS/Tests/BridgeJSToolTests/ImportedJSModuleRegistryTests.swift b/Plugins/BridgeJS/Tests/BridgeJSToolTests/ImportedJSModuleRegistryTests.swift
@@ -69,6 +69,66 @@ import Testing
         #expect(lines.contains { $0.contains("bridge-js-modules/Beta/utils.mjs") })
     }
 
+    /// A named import is a hard link-time requirement, so a class whose module export is
+    /// never looked up must not produce one. A wrapper-only `@JSClass` has no constructor
+    /// and no static methods, so nothing references the module's export of that name and
+    /// the module need not export it at all; requiring it would fail the whole module load.
+    @Test func wrapperOnlyClassDoesNotRequireANamedExport() throws {
+        let lines = try importLines([
+            skeleton(
+                moduleName: "Alpha",
+                types: [
+                    ImportedTypeSkeleton(
+                        name: "Wrapper",
+                        from: .module("some-pkg"),
+                        methods: [
+                            ImportedFunctionSkeleton(name: "read", parameters: [], returnType: .void)
+                        ]
+                    )
+                ]
+            )
+        ])
+        #expect(lines.count == 1)
+        #expect(lines[0].hasPrefix("import * as "))
+        #expect(!lines[0].contains("Wrapper as "))
+    }
+
+    /// A class with a constructor does have its export looked up, so it keeps a named import.
+    @Test func classWithConstructorUsesANamedImport() throws {
+        let lines = try importLines([
+            skeleton(
+                moduleName: "Alpha",
+                types: [
+                    ImportedTypeSkeleton(
+                        name: "Wrapper",
+                        from: .module("some-pkg"),
+                        constructor: ImportedConstructorSkeleton(parameters: [])
+                    )
+                ]
+            )
+        ])
+        #expect(lines == [#"import { Wrapper as __bjs_import_0_Wrapper } from "some-pkg";"#])
+    }
+
+    /// A class with only static methods also looks its export up.
+    @Test func classWithOnlyStaticMethodsUsesANamedImport() throws {
+        let lines = try importLines([
+            skeleton(
+                moduleName: "Alpha",
+                types: [
+                    ImportedTypeSkeleton(
+                        name: "Wrapper",
+                        from: .module("some-pkg"),
+                        staticMethods: [
+                            ImportedFunctionSkeleton(name: "create", parameters: [], returnType: .void)
+                        ]
+                    )
+                ]
+            )
+        ])
+        #expect(lines == [#"import { Wrapper as __bjs_import_0_Wrapper } from "some-pkg";"#])
+    }
+
     /// Local references are emitted before bare ones, each group sorted, so that the numbered
     /// aliases in generated JavaScript are stable across runs.
     @Test func referencesAreOrderedDeterministically() throws {
diff --git a/Sources/JavaScriptKit/Documentation.docc/Articles/BridgeJS/Unsupported-Features.md b/Sources/JavaScriptKit/Documentation.docc/Articles/BridgeJS/Unsupported-Features.md
@@ -21,6 +21,8 @@ An **external module** is any other value, passed to the JavaScript module resol
 
 Generated packages use static ECMAScript module imports. This works with the existing PackageToJS browser and Node ESM entry points. CommonJS and classic non-module script output are not generated or translated. Note that named exports of a CommonJS package are only importable when Node can statically detect them; when in doubt, use `jsName: .default` and reach members through the default export.
 
+A module export is called through a named import, so `this` is `undefined` inside the called function rather than the module namespace object. A function that reaches sibling exports through `this` — which happens in CommonJS packages consumed through Node's ESM interop — will fail. Import the default export and call the member through it when a package needs that receiver.
+
 Module origins apply to top-level `@JSFunction`, top-level `@JSGetter`, and an entire `@JSClass`. Per-member origins, top-level setters, inline JavaScript source, package-root-relative paths, and per-member module overrides are not supported. `jsName: .default` is likewise only valid on those three declaration forms and only together with `from: .module(...)`; it cannot be used on `@JSSetter`, because ECMAScript module bindings are read-only.
 
 The TypeScript-definition workflow (`bridge-js.d.ts`) always imports from `globalThis` and cannot yet target a module origin. To import from a module, declare the API with the macros instead.
