From 163ebf8181f2b668dc0ed9604ecdab68a465a46a Mon Sep 17 00:00:00 2001 From: Jake Teton-Landis Date: Mon, 8 Jul 2024 14:43:09 -0400 Subject: [PATCH 1/8] Add @NodeName macro to rename properties/methods This is most-useful to use NodeSymbol names for methods to implement well-known interfaces, but it could also be useful anywhere you want Swift names and Node names for APIs to diverge. Alternatively, we could add name parameters to the existing `@NodeProperty` and `@NodeMethod` macros. This was more straight-forward to implement for a macro beginner. --- Sources/NodeAPI/Sugar.swift | 4 ++ Sources/NodeAPIMacros/Diagnostics.swift | 4 ++ Sources/NodeAPIMacros/NodeClassMacro.swift | 4 +- Sources/NodeAPIMacros/NodeNameMacro.swift | 18 +++++++++ Sources/NodeAPIMacros/Plugin.swift | 1 + Sources/NodeAPIMacros/SyntaxUtils.swift | 29 +++++++++++--- .../NodeClassMacroTests.swift | 39 ++++++++++++++++++- 7 files changed, 91 insertions(+), 8 deletions(-) create mode 100644 Sources/NodeAPIMacros/NodeNameMacro.swift diff --git a/Sources/NodeAPI/Sugar.swift b/Sources/NodeAPI/Sugar.swift index b3398f8..04ca6f9 100644 --- a/Sources/NodeAPI/Sugar.swift +++ b/Sources/NodeAPI/Sugar.swift @@ -199,3 +199,7 @@ public macro NodeMethod(_: NodePropertyAttributes = .defaultMethod) @attached(peer, names: prefixed(`$`)) public macro NodeProperty(_: NodePropertyAttributes = .defaultProperty) = #externalMacro(module: "NodeAPIMacros", type: "NodePropertyMacro") + +@attached(peer, names: prefixed(`$`)) +public macro NodeName(_: NodeName) + = #externalMacro(module: "NodeAPIMacros", type: "NodeNameMacro") diff --git a/Sources/NodeAPIMacros/Diagnostics.swift b/Sources/NodeAPIMacros/Diagnostics.swift index 5568cd1..e92937b 100644 --- a/Sources/NodeAPIMacros/Diagnostics.swift +++ b/Sources/NodeAPIMacros/Diagnostics.swift @@ -37,4 +37,8 @@ extension DiagnosticMessage where Self == NodeDiagnosticMessage { static var expectedInit: Self { .init("@NodeConstructor can only be applied to an initializer") } + + static var expectedName: Self { + .init("@NodeName must have a name provided") + } } diff --git a/Sources/NodeAPIMacros/NodeClassMacro.swift b/Sources/NodeAPIMacros/NodeClassMacro.swift index bd5d2af..ddd62cb 100644 --- a/Sources/NodeAPIMacros/NodeClassMacro.swift +++ b/Sources/NodeAPIMacros/NodeClassMacro.swift @@ -31,9 +31,11 @@ struct NodeClassMacro: ExtensionMacro { } else { nil as TokenSyntax? } + if let identifier = identifier?.trimmed { + let key = member.decl.attributes?.findAttribute(named: "NodeName")?.nodeAttributes ?? "\(literal: identifier.text)" as ExprSyntax DictionaryElementSyntax( - key: "\(literal: identifier.text)" as ExprSyntax, + key: key, value: "$\(identifier)" as ExprSyntax ) } diff --git a/Sources/NodeAPIMacros/NodeNameMacro.swift b/Sources/NodeAPIMacros/NodeNameMacro.swift new file mode 100644 index 0000000..89a4c01 --- /dev/null +++ b/Sources/NodeAPIMacros/NodeNameMacro.swift @@ -0,0 +1,18 @@ +import SwiftSyntax +import SwiftSyntaxMacros + +struct NodeNameMacro: PeerMacro { + static func expansion( + of node: AttributeSyntax, + providingPeersOf declaration: some DeclSyntaxProtocol, + in context: some MacroExpansionContext + ) throws -> [DeclSyntax] { + guard let _ = node.nodeAttributes else { + context.diagnose(.init(node: Syntax(node), message: .expectedName)) + return [] + } + + // Processed by NodeClassMacro. + return [] + } +} diff --git a/Sources/NodeAPIMacros/Plugin.swift b/Sources/NodeAPIMacros/Plugin.swift index 04064e6..49d5491 100644 --- a/Sources/NodeAPIMacros/Plugin.swift +++ b/Sources/NodeAPIMacros/Plugin.swift @@ -8,5 +8,6 @@ import SwiftSyntaxMacros NodeConstructorMacro.self, NodeClassMacro.self, NodeModuleMacro.self, + NodeNameMacro.self, ] } diff --git a/Sources/NodeAPIMacros/SyntaxUtils.swift b/Sources/NodeAPIMacros/SyntaxUtils.swift index 7b51080..3e91189 100644 --- a/Sources/NodeAPIMacros/SyntaxUtils.swift +++ b/Sources/NodeAPIMacros/SyntaxUtils.swift @@ -1,14 +1,31 @@ import SwiftSyntax +extension DeclSyntax { + var attributes: AttributeListSyntax? { + if let function = self.as(FunctionDeclSyntax.self) { + function.attributes + } else if let property = self.as(VariableDeclSyntax.self) { + property.attributes + } else { + nil + } + } +} + extension AttributeListSyntax { - func hasAttribute(named name: String) -> Bool { - contains { + func findAttribute(named name: String) -> AttributeSyntax? { + lazy.compactMap { if case let .attribute(value) = $0 { - value.attributeName.as(IdentifierTypeSyntax.self)?.name.trimmed.text == name - } else { - false + if value.attributeName.as(IdentifierTypeSyntax.self)?.name.trimmed.text == name { + return value + } } - } + return nil + }.first + } + + func hasAttribute(named name: String) -> Bool { + findAttribute(named: name) != nil } } diff --git a/Tests/NodeAPIMacrosTests/NodeClassMacroTests.swift b/Tests/NodeAPIMacrosTests/NodeClassMacroTests.swift index 889aaae..599a54b 100644 --- a/Tests/NodeAPIMacrosTests/NodeClassMacroTests.swift +++ b/Tests/NodeAPIMacrosTests/NodeClassMacroTests.swift @@ -62,6 +62,42 @@ final class NodeClassMacroTests: XCTestCase { } } + func testCustomName() { + assertMacro { + #""" + @NodeClass final class Foo { + @NodeName("q") + @NodeProperty var x = 5 + @NodeName(NodeSymbol.someGlobalSymbol) + @NodeProperty var y = 6 + var z = 7 + + @NodeMethod func foo() {} + func bar() {} + @NodeMethod func baz() {} + } + """# + } expansion: { + """ + final class Foo { + @NodeName("q") + @NodeProperty var x = 5 + @NodeName(NodeSymbol.someGlobalSymbol) + @NodeProperty var y = 6 + var z = 7 + + @NodeMethod func foo() {} + func bar() {} + @NodeMethod func baz() {} + } + + extension Foo { + @NodeActor public static let properties: NodeClassPropertyList = ["q": $x, NodeSymbol.someGlobalSymbol: $y, "foo": $foo, "baz": $baz] + } + """ + } + } + func testNonClass() { assertMacro { #""" @@ -98,6 +134,7 @@ final class NodeClassMacroTests: XCTestCase { @NodeProperty(.enumerable) var y = "hello" var z = 7 + @NodeName("longerFooName") @NodeMethod func foo(_ x: String) async throws { throw SomeError(x) } @@ -157,7 +194,7 @@ final class NodeClassMacroTests: XCTestCase { } extension Foo { - @NodeActor public static let properties: NodeClassPropertyList = ["x": $x, "y": $y, "foo": $foo, "baz": $baz] + @NodeActor public static let properties: NodeClassPropertyList = ["x": $x, "y": $y, "longerFooName": $foo, "baz": $baz] } """# } From 3e3e90b208a02ed958ba0c5d81684b968e8b36e9 Mon Sep 17 00:00:00 2001 From: Jake Teton-Landis Date: Mon, 8 Jul 2024 14:56:47 -0400 Subject: [PATCH 2/8] Improve usability of NodeSymbol for method names - `NodeSymbol.wellKnown(propertyName: "name")` is equivalent to `Symbol['name']`. This is useful for implementing language-level protocols. - `NodeSymbol.global(for: "name")` is equivalent to `Symbol.for('name')` in JS. This is useful for implementing userland protocols. Both those static methods may throw, so there's also deferred versions which follow the existing convention of `deferredConstructor` to postpone building the NodeValue symbol until it's needed on the @NodeActor thread. --- Sources/NodeAPI/NodeSymbol.swift | 42 ++++++++++++++++++++++++++++++-- Sources/NodeAPI/NodeValue.swift | 16 ++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/Sources/NodeAPI/NodeSymbol.swift b/Sources/NodeAPI/NodeSymbol.swift index 5e2a4fc..0160105 100644 --- a/Sources/NodeAPI/NodeSymbol.swift +++ b/Sources/NodeAPI/NodeSymbol.swift @@ -1,11 +1,40 @@ @_implementationOnly import CNodeAPI public final class NodeSymbol: NodePrimitive, NodeName { - @_spi(NodeAPI) public let base: NodeValueBase @_spi(NodeAPI) public init(_ base: NodeValueBase) { self.base = base } + + public static func global(for name: String) throws -> NodeSymbol { + let ctx = NodeContext.current + let env = ctx.environment + let symbol = try env.global.Symbol.for(name) + if let nonNullSymbol = try symbol.nodeValue().as(NodeSymbol.self) { + return nonNullSymbol + } else { + throw NodeAPIError(.genericFailure, message: "globalThis.Symbol.for('\(name)') is not a symbol") + } + } + + public static func deferredGlobal(for name: String) -> NodeDeferredName { + NodeDeferredName { try global(for: name) } + } + + public static func wellKnown(propertyName name: String) throws -> NodeSymbol { + let ctx = NodeContext.current + let env = ctx.environment + let property = try env.global.Symbol[name].as(NodeSymbol.self) + if let property = property { + return property + } else { + throw NodeAPIError(.genericFailure, message: "globalThis.Symbol.\(name) is not a symbol") + } + } + + public static func deferredWellKnown(propertyName name: String) -> NodeDeferredName { + NodeDeferredName { try wellKnown(propertyName: name) } + } public init(description: String? = nil) throws { let ctx = NodeContext.current @@ -15,5 +44,14 @@ public final class NodeSymbol: NodePrimitive, NodeName { try env.check(napi_create_symbol(env.raw, descRaw, &result)) self.base = NodeValueBase(raw: result, in: ctx) } - } + +extension NodeSymbol { + /// Allows implementing the Iterable protocol for an object. + /// This method is called by the `for-of` statement or destructuring like `[...obj]`. + public static var iterator: NodeDeferredName { deferredWellKnown(propertyName: "iterator") } + + /// This symbol allows customizing how NodeJS formats objects in console.log. + /// See https://nodejs.org/api/util.html#custom-inspection-functions-on-objects + public static var utilInspectCustom: NodeDeferredName { deferredGlobal(for: "nodejs.util.inspect.custom")} +} \ No newline at end of file diff --git a/Sources/NodeAPI/NodeValue.swift b/Sources/NodeAPI/NodeValue.swift index 35fa249..39f42b1 100644 --- a/Sources/NodeAPI/NodeValue.swift +++ b/Sources/NodeAPI/NodeValue.swift @@ -116,6 +116,22 @@ public struct NodeDeferredValue: NodeValueConvertible, Sendable { } } +// Utility for APIs that take NodeValueConvertible: useful when you +// want to defer NodeValue creation to the API, for example for +// accessing global or well-known Symbols. +public struct NodeDeferredName: NodeValueConvertible, Sendable, NodeName { + let wrapper: @Sendable @NodeActor () throws -> NodeValue + + // thread-safe + public init(_ wrapper: @escaping @Sendable @NodeActor () throws -> NodeValue) { + self.wrapper = wrapper + } + + @NodeActor public func nodeValue() throws -> NodeValue { + try wrapper() + } +} + public protocol AnyNodeValueCreatable { @NodeActor static func from(_ value: NodeValue) throws -> Self? } From e6f35d70ee1df70b7be8228190ee1f11d434642c Mon Sep 17 00:00:00 2001 From: Jake Teton-Landis Date: Mon, 8 Jul 2024 15:12:39 -0400 Subject: [PATCH 3/8] Implement Javascript iterable protocol w/ NodeIterator Adds `NodeIterator` class and extension to NodeValueConvertible Sequence to support implementing the EcmaScript iterable protocol from Swift. Adds an iterable protocol integration test to demonstrate how the new features on this branch work together. Discussion point: none of the other internal classes seem to use macros, so I implemented NodeClass manually for NodeIterator. Is it okay to use the macros here? --- Sources/NodeAPI/NodeIterator.swift | 50 ++++++++++++++++++++++++++++++ test/suites/Test/Test.swift | 20 +++++++++++- test/suites/Test/index.js | 12 ++++++- 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 Sources/NodeAPI/NodeIterator.swift diff --git a/Sources/NodeAPI/NodeIterator.swift b/Sources/NodeAPI/NodeIterator.swift new file mode 100644 index 0000000..60240c9 --- /dev/null +++ b/Sources/NodeAPI/NodeIterator.swift @@ -0,0 +1,50 @@ +extension Sequence where Element: NodeValueConvertible { + @NodeActor public func nodeIterator() -> NodeIterator { + NodeIterator(self.lazy.map { $0 as NodeValueConvertible }.makeIterator()) + } +} + +public final class NodeIterator: NodeClass { + public struct Result: NodeValueConvertible, NodeValueCreatable { + public typealias ValueType = NodeObject + + let value: NodeValueConvertible? + let done: Bool? + + public func nodeValue() throws -> any NodeValue { + let obj = try NodeObject() + if let value = value { + try obj["value"].set(to: value) + } + if let done = done { + try obj["done"].set(to: done) + } + return obj + } + + public static func from(_ value: ValueType) throws -> Self { + Self( + value: try value.get("value"), + done: try value.get("done").as(Bool.self) + ) + } + } + + public static let properties: NodeClassPropertyList = [ + "next": NodeMethod(next), + ] + + private var iterator: any IteratorProtocol + public init(_ iterator: any IteratorProtocol) { + self.iterator = iterator + } + + public func next() -> Result { + if let value = iterator.next() { + return Result(value: value, done: false) + } else { + return Result(value: nil, done: true) + } + } +} + diff --git a/test/suites/Test/Test.swift b/test/suites/Test/Test.swift index 761ceac..fc75778 100644 --- a/test/suites/Test/Test.swift +++ b/test/suites/Test/Test.swift @@ -58,4 +58,22 @@ final class File: NodeClass { } } -#NodeModule(exports: ["File": File.deferredConstructor]) +final class SomeIterable: NodeClass { + typealias Element = String + + static let properties: NodeClassPropertyList = [ + NodeSymbol.iterator: NodeMethod(nodeIterator), + ] + + static let construct = NodeConstructor(SomeIterable.init(_:)) + init(_ args: NodeArguments) throws { } + + private let values: [String] = ["one", "two", "three"] + + func nodeIterator() throws -> NodeIterator { + values.nodeIterator() + } + +} + +#NodeModule(exports: ["File": File.deferredConstructor, "SomeIterable": SomeIterable.deferredConstructor]) diff --git a/test/suites/Test/index.js b/test/suites/Test/index.js index e2b6d4d..4c3c7b4 100644 --- a/test/suites/Test/index.js +++ b/test/suites/Test/index.js @@ -1,6 +1,6 @@ const assert = require("assert"); -const { File } = require("../../.build/Test.node"); +const { File, SomeIterable } = require("../../.build/Test.node"); assert.strictEqual(File.default().filename, "default.txt") @@ -23,3 +23,13 @@ file.unlink(); assert.strictEqual(file.reply("hi"), "You said hi"); assert.strictEqual(file.reply(null), "You said nothing"); assert.strictEqual(file.reply(undefined), "You said nothing"); + +const iterable = new SomeIterable() +const expected = ["one", "two", "three"] +assert.deepStrictEqual(Array.from(iterable), expected) +assert.deepStrictEqual([...iterable], expected) +let index = 0 +for (const item of iterable) { + assert.strictEqual(item, expected[index]) + index++ +} \ No newline at end of file From 147db9feea7e8d8a575f0b6bb24d30e076ce8b24 Mon Sep 17 00:00:00 2001 From: Jake Teton-Landis Date: Mon, 8 Jul 2024 15:21:16 -0400 Subject: [PATCH 4/8] Improve error when calling non-function values The dynamic call behavior makes it easy to call random fictional APIs in Swift code on NodeValue instances. I hit this error several times when trying to iron out NodeIterator stuff. I was mistakenly doing this: ```swift try nodeObj.set("prop", newValue) ``` which is legal from the compiler's perspective, but attempts to call a non-existant function `set` on `nodeObj`. The correct Swift syntax is: ```swift try nodeObj["prop"].set(to: newValue) ``` By including the DynamicProperty.key in the error message, it's much easier to find the Swift code responsible for the error throw in Javascript. --- Sources/NodeAPI/NodeValue.swift | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Sources/NodeAPI/NodeValue.swift b/Sources/NodeAPI/NodeValue.swift index 39f42b1..bf7e399 100644 --- a/Sources/NodeAPI/NodeValue.swift +++ b/Sources/NodeAPI/NodeValue.swift @@ -182,7 +182,7 @@ extension NodeCallable { @discardableResult public func dynamicallyCall(withArguments args: [NodeValueConvertible]) throws -> AnyNodeValue { guard let fn = try self.as(NodeFunction.self) else { - throw NodeAPIError(.functionExpected, message: "Cannot call a non-function") + throw NodeAPIError(.functionExpected, message: "Cannot call a non-function: \(try debugDescription())") } return try fn.call(on: receiver, args) } @@ -190,6 +190,14 @@ extension NodeCallable { public var new: NodeCallableConstructor { NodeCallableConstructor(callable: self) } + + internal func debugDescription() throws -> String { + let actual = "\(try self.nodeValue()) (\(self))" + if let dynamicProperty = self as? NodeObject.DynamicProperty { + return "\(receiver).\(dynamicProperty.key) is \(actual)" + } + return "is \(actual)" + } } // this type exists due to the aforementioned callAsFunction bug @@ -198,7 +206,7 @@ extension NodeCallable { let callable: NodeCallable public func dynamicallyCall(withArguments args: [NodeValueConvertible]) throws -> NodeObject { guard let fn = try callable.as(NodeFunction.self) else { - throw NodeAPIError(.functionExpected, message: "Cannot call a non-function") + throw NodeAPIError(.functionExpected, message: "Cannot call a non-function as constructor: \(try callable.debugDescription())") } return try fn.construct(withArguments: args) } From 082d531cddc5284100e723e1f5de257cced50bc2 Mon Sep 17 00:00:00 2001 From: Jake Teton-Landis Date: Mon, 8 Jul 2024 15:26:42 -0400 Subject: [PATCH 5/8] Don't clean runs of single integration tests For me, the test harness works fine without calling `builder.clean()` and significantly improves my iteration speed to not have to wait for a full rebuild whenever I run a single test suite. Running all tests should still clean by default, but in both cases this can be configured when running like `CLEAN=1 node index.js suite Test` or `CLEAN=0 node index.js all`. --- test/index.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/index.js b/test/index.js index cb03cb5..ee01511 100644 --- a/test/index.js +++ b/test/index.js @@ -2,6 +2,8 @@ const fs = require("fs").promises; const builder = require("../lib/builder"); const { spawnSync } = require("child_process"); +const CLEAN = process.env.CLEAN && process.env.CLEAN !== "0" + process.chdir(__dirname); function usage() { @@ -41,7 +43,7 @@ async function runAll() { switch (command) { case "all": if (process.argv.length > 3) usage(); - await builder.clean(); + if (CLEAN || CLEAN === undefined) await builder.clean(); await runAll(); break; case "_suite": @@ -50,7 +52,7 @@ async function runAll() { case "suite": if (process.argv.length !== 4) usage(); const suite = process.argv[3]; - if (!isChild) await builder.clean(); + if (!isChild && CLEAN) await builder.clean(); await runSuite(suite, isChild); break; default: From 869e58a401b71fee3d87d804199b159150f54bda Mon Sep 17 00:00:00 2001 From: Jake Teton-Landis Date: Mon, 8 Jul 2024 15:53:57 -0400 Subject: [PATCH 6/8] Fix test failure --- Tests/NodeAPIMacrosTests/NodeClassMacroTests.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/NodeAPIMacrosTests/NodeClassMacroTests.swift b/Tests/NodeAPIMacrosTests/NodeClassMacroTests.swift index 599a54b..b469a25 100644 --- a/Tests/NodeAPIMacrosTests/NodeClassMacroTests.swift +++ b/Tests/NodeAPIMacrosTests/NodeClassMacroTests.swift @@ -164,6 +164,7 @@ final class NodeClassMacroTests: XCTestCase { = NodeProperty(attributes: .enumerable, \_NodeSelf.y) var z = 7 + @NodeName("longerFooName") func foo(_ x: String) async throws { throw SomeError(x) } From 93a4d31807b6b1743ed4ef9a544d7cc1117b3692 Mon Sep 17 00:00:00 2001 From: Jake Teton-Landis Date: Mon, 8 Jul 2024 15:58:53 -0400 Subject: [PATCH 7/8] revert change to integration test runner --- test/index.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/index.js b/test/index.js index ee01511..cb03cb5 100644 --- a/test/index.js +++ b/test/index.js @@ -2,8 +2,6 @@ const fs = require("fs").promises; const builder = require("../lib/builder"); const { spawnSync } = require("child_process"); -const CLEAN = process.env.CLEAN && process.env.CLEAN !== "0" - process.chdir(__dirname); function usage() { @@ -43,7 +41,7 @@ async function runAll() { switch (command) { case "all": if (process.argv.length > 3) usage(); - if (CLEAN || CLEAN === undefined) await builder.clean(); + await builder.clean(); await runAll(); break; case "_suite": @@ -52,7 +50,7 @@ async function runAll() { case "suite": if (process.argv.length !== 4) usage(); const suite = process.argv[3]; - if (!isChild && CLEAN) await builder.clean(); + if (!isChild) await builder.clean(); await runSuite(suite, isChild); break; default: From 751b3ff2557afc9ca3a5f339323ac2d980ed343a Mon Sep 17 00:00:00 2001 From: Jake Teton-Landis Date: Mon, 8 Jul 2024 15:59:24 -0400 Subject: [PATCH 8/8] document using _suite to skip clean --- test/index.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/index.js b/test/index.js index cb03cb5..22517df 100644 --- a/test/index.js +++ b/test/index.js @@ -5,7 +5,10 @@ const { spawnSync } = require("child_process"); process.chdir(__dirname); function usage() { - console.log("Usage: test [all|suite ]"); + console.log("Usage: test [all | suite | _suite ]"); + console.log(" all: clean & run all tests"); + console.log(" suite: clean & run suite with given name"); + console.log(" _suite: run suite with given name without cleaning"); process.exit(1); }