Skip to content

add-target breaks resulting package when target name is not valid Swift identifier #7764

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 6 commits into from
Aug 25, 2024
Merged

add-target breaks resulting package when target name is not valid Swift identifier #7764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9e15797
Use "Example" prefix instead of actual target name
dmhts Jul 8, 2024
e515806
Merge remote-tracking branch 'upstream/main' into dmhts/fix-add-targe…
dmhts Aug 4, 2024
627b625
Introduce target name sanitized name
dmhts Aug 6, 2024
88ffe20
Use uppercase characters and spaces in tests
dmhts Aug 6, 2024
3160ed5
Use localized capitalizing
dmhts Aug 8, 2024
d8d4e3f
Merge remote-tracking branch 'upstream/main' into dmhts/fix-add-targe…
dmhts Aug 23, 2024
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
30 changes: 21 additions & 9 deletions Sources/PackageModelSyntax/AddTarget.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,10 @@ public struct AddTarget {
case .macro:
"""
\(imports)
struct \(raw: target.name): Macro {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using c99name instead?

Copy link
Contributor Author

@dmhts dmhts Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's a very good point. c99name would certainly cover most edge cases, such as using dash-case, special characters, and digits in the first character. Well, it's already used for language-level target name sanitizing so it must be robust enough for this use case.

The only remaining concern is that c99 (obviously) doesn't cover Swift's reserved words. However, in that case, we could "just" escape the reserved word based on a list taken e.g. from swift-syntax, which SwiftPM already depends on.

I believe combining these two safeguards should be a good enough way to proceed. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just visiting. :) There may be a better way to avoid reserved names—the package owners probably have ideas!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright :) @DougGregor, maybe you have some ideas about this?

Copy link
Member

Choose a reason for hiding this comment

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

Both of your ideas together (cc9name and using the swift-syntax keyword checking) sound reasonable to me.

struct \(raw: target.sanitizedName): Macro {
/// TODO: Implement one or more of the protocols that inherit
/// from Macro. The appropriate macro protocol is determined
/// by the "macro" declaration that \(raw: target.name) implements.
/// by the "macro" declaration that \(raw: target.sanitizedName) implements.
/// Examples include:
/// @freestanding(expression) macro --> ExpressionMacro
/// @attached(member) macro --> MemberMacro
Expand All @@ -238,8 +238,8 @@ public struct AddTarget {
case .xctest:
"""
\(imports)
class \(raw: target.name): XCTestCase {
func test\(raw: target.name)() {
class \(raw: target.sanitizedName)Tests: XCTestCase {
func test\(raw: target.sanitizedName)() {
XCTAssertEqual(42, 17 + 25)
}
}
Expand All @@ -249,8 +249,8 @@ public struct AddTarget {
"""
\(imports)
@Suite
struct \(raw: target.name)Tests {
@Test("\(raw: target.name) tests")
struct \(raw: target.sanitizedName)Tests {
@Test("\(raw: target.sanitizedName) tests")
func example() {
#expect(42 == 17 + 25)
}
Expand All @@ -267,7 +267,7 @@ public struct AddTarget {
"""
\(imports)
@main
struct \(raw: target.name)Main {
struct \(raw: target.sanitizedName)Main {
static func main() {
print("Hello, world")
}
Expand Down Expand Up @@ -296,9 +296,9 @@ public struct AddTarget {
import SwiftCompilerPlugin

@main
struct \(raw: target.name)Macros: CompilerPlugin {
struct \(raw: target.sanitizedName)Macros: CompilerPlugin {
let providingMacros: [Macro.Type] = [
\(raw: target.name).self,
\(raw: target.sanitizedName).self,
]
}
"""
Expand Down Expand Up @@ -364,3 +364,15 @@ fileprivate extension PackageDependency {
)
}
}

fileprivate extension TargetDescription {
var sanitizedName: String {
name
.spm_mangledToC99ExtendedIdentifier()
.localizedFirstWordCapitalized()
}
}

fileprivate extension String {
func localizedFirstWordCapitalized() -> String { prefix(1).localizedCapitalized + dropFirst() }
}
34 changes: 17 additions & 17 deletions Tests/PackageModelSyntaxTests/ManifestEditTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ class ManifestEditTests: XCTestCase {
// These are the targets
.target(name: "MyLib"),
.executableTarget(
name: "MyProgram",
name: "MyProgram target-name",
dependencies: [
.product(name: "SwiftSyntax", package: "swift-syntax"),
.target(name: "TargetLib"),
Expand All @@ -479,13 +479,13 @@ class ManifestEditTests: XCTestCase {
)
""",
expectedAuxiliarySources: [
RelativePath("Sources/MyProgram/MyProgram.swift") : """
RelativePath("Sources/MyProgram target-name/MyProgram target-name.swift") : """
import MyLib
import SwiftSyntax
import TargetLib

@main
struct MyProgramMain {
struct MyProgram_target_nameMain {
static func main() {
print("Hello, world")
}
Expand All @@ -494,7 +494,7 @@ class ManifestEditTests: XCTestCase {
]) { manifest in
try AddTarget.addTarget(
TargetDescription(
name: "MyProgram",
name: "MyProgram target-name",
dependencies: [
.product(name: "SwiftSyntax", package: "swift-syntax"),
.target(name: "TargetLib", condition: nil),
Expand Down Expand Up @@ -528,7 +528,7 @@ class ManifestEditTests: XCTestCase {
],
targets: [
.macro(
name: "MyMacro",
name: "MyMacro target-name",
dependencies: [
.product(name: "SwiftCompilerPlugin", package: "swift-syntax"),
.product(name: "SwiftSyntaxMacros", package: "swift-syntax")
Expand All @@ -538,33 +538,33 @@ class ManifestEditTests: XCTestCase {
)
""",
expectedAuxiliarySources: [
RelativePath("Sources/MyMacro/MyMacro.swift") : """
RelativePath("Sources/MyMacro target-name/MyMacro target-name.swift") : """
Copy link
Contributor Author

@dmhts dmhts Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Though spaces work for file names, we could consider sanitizing them too.

import SwiftCompilerPlugin
import SwiftSyntaxMacros

struct MyMacro: Macro {
struct MyMacro_target_name: Macro {
/// TODO: Implement one or more of the protocols that inherit
/// from Macro. The appropriate macro protocol is determined
/// by the "macro" declaration that MyMacro implements.
/// by the "macro" declaration that MyMacro_target_name implements.
/// Examples include:
/// @freestanding(expression) macro --> ExpressionMacro
/// @attached(member) macro --> MemberMacro
}
""",
RelativePath("Sources/MyMacro/ProvidedMacros.swift") : """
RelativePath("Sources/MyMacro target-name/ProvidedMacros.swift") : """
import SwiftCompilerPlugin

@main
struct MyMacroMacros: CompilerPlugin {
struct MyMacro_target_nameMacros: CompilerPlugin {
let providingMacros: [Macro.Type] = [
MyMacro.self,
MyMacro_target_name.self,
]
}
"""
]
) { manifest in
try AddTarget.addTarget(
TargetDescription(name: "MyMacro", type: .macro),
TargetDescription(name: "MyMacro target-name", type: .macro),
to: manifest
)
}
Expand All @@ -582,17 +582,17 @@ class ManifestEditTests: XCTestCase {
let package = Package(
name: "packages",
targets: [
.testTarget(name: "MyTest"),
.testTarget(name: "MyTest target-name"),
]
)
""",
expectedAuxiliarySources: [
RelativePath("Tests/MyTest/MyTest.swift") : """
RelativePath("Tests/MyTest target-name/MyTest target-name.swift") : """
import Testing

@Suite
struct MyTestTests {
@Test("MyTest tests")
struct MyTest_target_nameTests {
@Test("MyTest_target_name tests")
func example() {
#expect(42 == 17 + 25)
}
Expand All @@ -601,7 +601,7 @@ class ManifestEditTests: XCTestCase {
]) { manifest in
try AddTarget.addTarget(
TargetDescription(
name: "MyTest",
name: "MyTest target-name",
type: .test
),
to: manifest,
Expand Down