Skip to content

Add infrastructure to write tests for Swift macros #1367

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.

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 1 commit into from
Jun 1, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented May 29, 2024

Add a default package manifest that defines two targets:

  • A macro target named MyMacro
  • And executable target named MyMacroClient

It builds the macro using the swift-syntax that was already built as part of the SourceKit-LSP build.
Re-using the SwiftSyntax modules that are already built is significantly faster than building swift-syntax in each test case run and does not require internet access.

@ahoppen ahoppen requested review from bnbarham and hamishknight May 29, 2024 23:44
@ahoppen ahoppen requested a review from benlangmuir as a code owner May 29, 2024 23:44
@ahoppen
Copy link
Member Author

ahoppen commented May 29, 2024

@swift-ci Please test

@ahoppen ahoppen force-pushed the macro-test-project branch from d2c1488 to d8ecc21 Compare May 31, 2024 06:42
Comment on lines 94 to 100
var objectFiles: [String] = []
for moduleName in swiftSyntaxModulesToLink {
let dir = buildDirectory.appendingPathComponent("\(moduleName).build")
let enumerator = FileManager.default.enumerator(at: dir, includingPropertiesForKeys: nil)
while let file = enumerator?.nextObject() as? URL {
if file.pathExtension == "o" {
objectFiles.append(file.path)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a directory traversal, I suppose you could consult SourceKitLSPPackageTests.product/Objects.LinkFileList. Not sure if that's a significant improvement though.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would also link in all sorts of other object files that we don’t need, so I don’t really prefer it. Also it has been causing some undefined symbol issues for me (_del_curterm) that I didn’t debug any further. We’d probably need to link it some more system libraries if we wanted to go the Objects.LinkedFileList idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, I meant still keeping the logic to only use the object files for the syntax modules we want. But not to worry if that misses object files

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. Let’s leave it as-is for now. If we read object files, we need to deal with un-quoting lines in there and the added complexity of that doesn’t seem worth it.

// Never skip tests in CI. Toolchain should be up-to-date
checkResult = .featureSupported
} else {
return try await skipUnlessSupported(file: file, line: line) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to pass through featureName?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes. Thanks for noticing. This would have been a fun failure mode to debug.

Comment on lines 53 to 75
let swiftSyntaxSearchPaths = [
buildDirectory
.deletingLastPathComponent() // arm64-apple-macosx
.deletingLastPathComponent() // debug
.appendingPathComponent("checkouts"),
URL(fileURLWithPath: #filePath)
.deletingLastPathComponent() // SwiftPMTestProject.swift
.deletingLastPathComponent() // SKTestSupport
.deletingLastPathComponent() // Sources
.deletingLastPathComponent(), // sourcekit-lsp
]

let swiftSyntaxCShimsModulemap =
swiftSyntaxSearchPaths.map { swiftSyntaxSearchPath in
swiftSyntaxSearchPath
.appendingPathComponent("swift-syntax")
.appendingPathComponent("Sources")
.appendingPathComponent("_SwiftSyntaxCShims")
.appendingPathComponent("include")
.appendingPathComponent("module.modulemap")
}
.first { FileManager.default.fileExists(atPath: $0.path) }

guard let swiftSyntaxCShimsModulemap else {
throw SwiftSyntaxCShimsModulemapNotFoundError()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, unfortunate this is needed, shame we don't have a non-deprecated way of using @_implementationOnly in a non-resilient module

@ahoppen ahoppen force-pushed the macro-test-project branch from d8ecc21 to f466568 Compare May 31, 2024 23:54
@ahoppen ahoppen changed the title Add infrastructure to write tests for Swift macros 🚥 #1366 Add infrastructure to write tests for Swift macros May 31, 2024
@ahoppen
Copy link
Member Author

ahoppen commented May 31, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented May 31, 2024

@swift-ci Please test Windows

Add a default package manifest that defines two targets:
 - A macro target named `MyMacro`
 - And executable target named `MyMacroClient`

It builds the macro using the swift-syntax that was already built as part of the SourceKit-LSP build.
Re-using the SwiftSyntax modules that are already built is significantly faster than building swift-syntax in each test case run and does not require internet access.
@ahoppen ahoppen force-pushed the macro-test-project branch from f466568 to 6599292 Compare June 1, 2024 04:37
@ahoppen
Copy link
Member Author

ahoppen commented Jun 1, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Jun 1, 2024

@swift-ci Please test Windows

@ahoppen
Copy link
Member Author

ahoppen commented Jun 1, 2024

@swift-ci Please test macOS

@ahoppen ahoppen merged commit 98718d4 into swiftlang:main Jun 1, 2024
3 checks passed
@ahoppen ahoppen deleted the macro-test-project branch June 1, 2024 15:02
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