Skip to content

[Macros] Granular diagnostics when macro type is not found in a plugin #2268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 17, 2023
Merged

[Macros] Granular diagnostics when macro type is not found in a plugin #2268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f413c51
[Macros] Granular diagnostics when macro type is not found in a plugin
rintaro Sep 20, 2023
e4e6a95
[CompilerPlugin] remove _resolveMacro
rintaro Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions Sources/SwiftCompilerPlugin/CompilerPlugin.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ public protocol CompilerPlugin {
}

extension CompilerPlugin {
func resolveMacro(moduleName: String, typeName: String) -> Macro.Type? {
@_spi(Testing)
public func resolveMacro(moduleName: String, typeName: String) throws -> Macro.Type {
let qualifedName = "\(moduleName).\(typeName)"

for type in providingMacros {
Expand All @@ -74,12 +75,9 @@ extension CompilerPlugin {
return type
}
}
return nil
}

// @testable
public func _resolveMacro(moduleName: String, typeName: String) -> Macro.Type? {
resolveMacro(moduleName: moduleName, typeName: typeName)
let pluginPath = CommandLine.arguments.first ?? Bundle.main.executablePath ?? ProcessInfo.processInfo.processName
throw CompilerPluginError(message: "macro implementation type '\(moduleName).\(typeName)' could not be found in executable plugin '\(pluginPath)'")
}
}

Expand All @@ -88,8 +86,8 @@ struct MacroProviderAdapter<Plugin: CompilerPlugin>: PluginProvider {
init(plugin: Plugin) {
self.plugin = plugin
}
func resolveMacro(moduleName: String, typeName: String) -> Macro.Type? {
plugin.resolveMacro(moduleName: moduleName, typeName: typeName)
func resolveMacro(moduleName: String, typeName: String) throws -> Macro.Type {
try plugin.resolveMacro(moduleName: moduleName, typeName: typeName)
}
}

Expand Down Expand Up @@ -242,3 +240,10 @@ private extension FileHandle {
}
}
}

struct CompilerPluginError: Error, CustomStringConvertible {
var description: String
init(message: String) {
self.description = message
}
}
Comment on lines +244 to +249
Copy link
Member

Choose a reason for hiding this comment

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

Do you think there would be value in structuring the errors and making CompilerPluginError an enum with a computed description property instead of shoving an entire error message in there?

Copy link
Member Author

@rintaro rintaro Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not at this point. As long as we only need the message, I don't think making it an enum benefits anything.

Copy link
Contributor

@bnbarham bnbarham Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was also considering commenting on this. It would be nice if rather than passing a random string (which we've duplicated in many places) into the eventual diagnostic, that we instead passed the paths/macro type/plugin kind along. I'm not sure how difficult that would be or if it's even feasible though so maybe it's not worth it.

As an example, one of the messages in this PR is macro implementation type '\(moduleName).\(typeName)' could not be found in executable plugin '\(pluginPath)' and another has just type instead. And IMO I think it would be better with neither.

We then have duplicates of these messages for the plugin server and then more for in-process vs executable (all slightly different).

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a plan to consolidate logics of plugin-server and in-process in the follow-ups (because they are both library plugins). In that change, I think we can consolidate this error message too between all three of executable, server, and in-process.

Copy link
Member

Choose a reason for hiding this comment

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

It’s not a blocker for me. It was just something that crossed my mind but I’ll leave it up to you to decide how to model the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that would be nice 👍

2 changes: 1 addition & 1 deletion Sources/SwiftCompilerPluginMessageHandling/CompilerPluginMessageHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public enum PluginFeature: String {
/// A type that provides the actual plugin functions.
public protocol PluginProvider {
/// Resolve macro type by the module name and the type name.
func resolveMacro(moduleName: String, typeName: String) -> Macro.Type?
func resolveMacro(moduleName: String, typeName: String) throws -> Macro.Type

/// Load dynamic link library at `libraryPath`. Implementations can use
/// `moduleName` to associate the loaded library with it.
Expand Down
13 changes: 4 additions & 9 deletions Sources/SwiftCompilerPluginMessageHandling/Macros.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ import SwiftSyntaxMacros

extension CompilerPluginMessageHandler {
/// Get concrete macro type from a pair of module name and type name.
private func resolveMacro(_ ref: PluginMessage.MacroReference) -> Macro.Type? {
provider.resolveMacro(moduleName: ref.moduleName, typeName: ref.typeName)
private func resolveMacro(_ ref: PluginMessage.MacroReference) throws -> Macro.Type {
try provider.resolveMacro(moduleName: ref.moduleName, typeName: ref.typeName)
}

/// Expand `@freestainding(XXX)` macros.
Expand All @@ -43,10 +43,7 @@ extension CompilerPluginMessageHandler {
guard let macroSyntax = syntax.asProtocol(FreestandingMacroExpansionSyntax.self) else {
throw MacroExpansionError.freestandingMacroSyntaxIsNotMacro
}
guard let macroDefinition = resolveMacro(macro) else {
throw MacroExpansionError.macroTypeNotFound(macro)
}

let macroDefinition = try resolveMacro(macro)
let macroRole: MacroRole
if let pluginMacroRole {
macroRole = MacroRole(messageMacroRole: pluginMacroRole)
Expand Down Expand Up @@ -113,9 +110,7 @@ extension CompilerPluginMessageHandler {
// TODO: Make this a 'String?' and remove non-'hasExpandMacroResult' branches.
let expandedSources: [String]?
do {
guard let macroDefinition = resolveMacro(macro) else {
throw MacroExpansionError.macroTypeNotFound(macro)
}
let macroDefinition = try resolveMacro(macro)
let role = MacroRole(messageMacroRole: macroRole)

let expansions = SwiftSyntaxMacroExpansion.expandAttachedMacroWithoutCollapsing(
Expand Down
14 changes: 7 additions & 7 deletions Tests/SwiftCompilerPluginTest/CompilerPluginTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//
//===----------------------------------------------------------------------===//

import SwiftCompilerPlugin
@_spi(Testing) import SwiftCompilerPlugin
import SwiftSyntax
import SwiftSyntaxMacros
import XCTest
Expand Down Expand Up @@ -51,20 +51,20 @@ public class CompilerPluginTests: XCTestCase {
func testResolveMacro() {
let plugin = MyPlugin()

let registeredMacro = plugin._resolveMacro(
let registeredMacro = try? plugin.resolveMacro(
moduleName: "SwiftCompilerPluginTest",
typeName: "RegisteredMacro"
)
XCTAssertNotNil(registeredMacro)
XCTAssertTrue(registeredMacro == RegisteredMacro.self)

/// Test the plugin doesn't provide unregistered macros.
let dummyMacro = plugin._resolveMacro(
moduleName: "SwiftCompilerPluginTest",
typeName: "DummyMacro"
XCTAssertThrowsError(
try plugin.resolveMacro(
moduleName: "SwiftCompilerPluginTest",
typeName: "DummyMacro"
)
)
XCTAssertNil(dummyMacro)
XCTAssertFalse(dummyMacro == DummyMacro.self)

}
}