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 4 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 @@ -234,10 +234,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 @@ -255,8 +255,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 @@ -266,8 +266,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 @@ -284,7 +284,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 @@ -313,9 +313,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 @@ -414,3 +414,15 @@ fileprivate extension PackageDependency {
)
}
}

fileprivate extension TargetDescription {
var sanitizedName: String {
name
.spm_mangledToC99ExtendedIdentifier()
.capitalizingFirstLetter()
}
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.

Using the Keyword enum turned out to be not that trivial for this task, as there are many exceptions due to the keyword's context-specific nature, namely:

  • objc, escaping and some others are considered keywords only when they are used in combination with the leading @ character.
  • The same goes for available and selector but in combination with the # character
  • metadata and module - I'm not even sure in which context they would be considered actual language keywords.
  • etc.

As simple solutions are generally better, I preferred to use c99name in combination with a capitalization of the first letter. With the capitalization strategy, we can feed two birds with one scone:

  • It forces example type names to follow the overall type naming convention: "sanitizer" -> "struct Sanitizer"
  • Reserved words are turned into valid identifiers when capitalized: "macro" -> "struct Macro"

I believe the combination of these two safeguards covers all known edge cases (to the best of my knowledge).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, I missed the discussion above. We actually added isValidSwiftIdentifier in swift-syntax for the purpose you described above. Having said that, IMO c99name + uppercase is probably good enough here anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks! That's really nice to know.

}

fileprivate extension String {
func capitalizingFirstLetter() -> String { prefix(1).uppercased() + dropFirst() }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using both prefix(1) and dropFirst() is absolutely safe with strings of any length, including empty ones, as both are concerned with a maximum number of characters to process, not a minimum.

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.

I've decided to introduce capitalizingFirstLetter() instead of using the existing capitalized() function because the latter has an undesirable by-design behavior. It lowercases all remaining letters in a word, so "MyNewShinyTarget" would be turned into "Mynewshinytarget". This makes it a rather poor option for camel-case notation, which is the most popular convention for target names, from my experience.

On the other hand, capitalizing only the first letter would turn "my-new-shiny-target" into "My_new_shiny_target" instead of the potentially preferred "My_New_Shiny_Target" (which capitalized() would produce), but using underscores would be a non-preferred Swift naming convention anyway. Therefore I would consider it a very minor downside (if a downside at all) given that we're talking about example names here.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, worth taking a moment to write some unit tests where the macro name does not use the English Latin alphabet. For instance, does the French "être" correctly capitalize to "Être"? Does the Japanese "マクロ" stay unmodified? These are trivial test cases but worth adding.

More complex to solve would be something like the "Turkish I" capitalization problem (uppercase "i" in Turkish is "İ", not "I", and lowercase "I" is "ı".) Should this code attempt to take the user's localization settings into consideration?

Copy link
Contributor Author

@dmhts dmhts Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, that's a good point again. I switched to localizedCapitalized instead of uppercased(), which addresses all the cases mentioned above while taking the current user's locale into account. Interestingly, the case with the Turkish "İ" is specifically covered in Foundation :) I'm surprised that the tests there are not as rigorous as expected.

I'd love to write more tests that cover localization edge cases. However, this would require some refactoring of the test helpers (as I mentioned here) if we want to avoid significant duplication in tests.
An alternative approach could be to use the Turkish "I" in the existing test target names to confirm that at least some localization is taken into account. However, that would require some additional test setup to manipulate Locale.current.

At the same time, what we have might already be good enough for this specific use case, as the main goal of this PR was to prevent broken code in modified packages in the first place.

@DougGregor I'd also like to hear your opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I'm not sure Locale.current reports anything other than en_US for a command-line tool like SwiftPM anyway. 🤔

Copy link
Contributor Author

@dmhts dmhts Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just checked, and Locale.current correctly reports en_DE in my case, which is aligned with the system settings:

defaults read ~/Library/Preferences/.GlobalPreferences AppleLocale
en_DE

At the same time, the system locale utility reports the default en_US, as you said:

locale
LANG="en_US.UTF-8"

Meaning, String.localizedCapitalized should work as expected in the CLI context.

}
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 @@ -586,19 +586,19 @@ class ManifestEditTests: XCTestCase {
],
targets: [
.testTarget(
name: "MyTest",
name: "MyTest target-name",
dependencies: [ .product(name: "Testing", package: "swift-testing") ]
),
]
)
""",
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 @@ -607,7 +607,7 @@ class ManifestEditTests: XCTestCase {
]) { manifest in
try AddTarget.addTarget(
TargetDescription(
name: "MyTest",
name: "MyTest target-name",
type: .test
),
to: manifest,
Expand Down