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

dmhts · 2024-07-08T07:05:31Z

Executing swift package add-target my-target --type executable breaks the updated package because the executable target name is not a valid Swift identifier.

Motivation:

After executing swift package add-target my-target --type executable, I expect the generated stub to not break the modified package. However, in this case, the resulting stub file would contain invalid Swift code:

@main
struct my-targetMain {
    static func main() {
        print("Hello, world")
    }
}

The same issue applies to macro and test target types. It's also important to note that dash case is quite often used for target names, like e.g. in swift-argument-parser.

Additionaly, when AddTarget is consumed via an API (as opposed to the CLI), the following parsing error is thrown into stdout:

Modifications:

I believe there are at least two ways to solve this issue:

Use a predefined type name in the generated stub, such as struct ExampleMain, so it doesn't depend on the target name.
Introduce a sanitizer function to ensure that the target name is always a valid Swift identifier. For example, a function that would convert "$my-target-name%" into "myTargetName".

For the sake of simplicity, I opted for approach #1 in the proposed PR. However, if it's necessary to preserve target names in the generated stubs, we can certainly go with approach #2.

Result:

swift package add-target my-target --type executable

@main
struct ExampleMain {
    static func main() {
        print("Hello, world")
    }
}

dmhts · 2024-07-08T10:33:24Z

Tests/PackageModelSyntaxTests/ManifestEditTests.swift

+    func testAddExecutableTargetWithNameAsInvalidSwiftIdentifier() throws {
+        try assertManifestRefactor("""


To reduce the amount of duplicated code in tests, we could also use a "non-valid Swift identifier target name" instead of "MyProgram" in the original testAddExecutableTargetWithDependencies test (the same goes for testAddSwiftTestingTestTargetWithNameAsInvalidSwiftIdentifier test below).

Alternatively, we could extend the assertManifestRefactor function to handle multiple editing operations at once, allowing us to add multiple targets with different names within a single test.

I like the idea of using a "non-valid Swift identifier target name" in existing tests, because it gives us better coverage without duplication. We also often create new tests from old ones, so it'll help us make sure we cover this case whenever we are introducing a new test (without always introducing two tests).

Good points! I updated the tests. One thing though, the only special characters I've ever seen used for target names are dashes and spaces. It’s still based on my anecdotal experience, but I’m not sure if we need to overcomplicate the tests with every possible unlikely input.

grynspan · 2024-07-10T12:05:38Z

Sources/PackageModelSyntax/AddTarget.swift

@@ -234,10 +234,10 @@ public struct AddTarget {
        case .macro:
            """
            \(imports)
-            struct \(raw: target.name): Macro {


How about using c99name instead?

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?

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

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

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

…t-with-name-as-invalid-identifier

DougGregor · 2024-08-05T16:53:16Z

@swift-ci please test

dmhts · 2024-08-06T13:06:20Z

Sources/PackageModelSyntax/AddTarget.swift

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


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.

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.

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?

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.

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

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.

dmhts · 2024-08-06T13:48:24Z

Sources/PackageModelSyntax/AddTarget.swift

+    var sanitizedName: String {
+        name
+            .spm_mangledToC99ExtendedIdentifier()
+            .capitalizingFirstLetter()
+    }


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).

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.

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

dmhts · 2024-08-06T15:30:19Z

Tests/PackageModelSyntaxTests/ManifestEditTests.swift

@@ -538,33 +538,33 @@ class ManifestEditTests: XCTestCase {
            )
            """,
            expectedAuxiliarySources: [
-                RelativePath("Sources/MyMacro/MyMacro.swift") : """
+                RelativePath("Sources/MyMacro target-name/MyMacro target-name.swift") : """


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

dmhts · 2024-08-23T13:24:38Z

@DougGregor just a friendly reminder when you have time. I believe this PR can be already merged once the CI tests have passed.

Sidenote: I've been using the new package editing API quite a lot in my own project, and I've come across a few other bugs that I'd be glad to fix in follow-up PRs. For example, sanitizing imports, avoiding the use of product names in imports (only targets can be imported), etc.

I could also address some FIXMEs, such as plugin and resources support, as I currently have to resort to workarounds to bypass them.

dschaefer2 · 2024-08-23T15:03:54Z

@swift-ci please test

dschaefer2 · 2024-08-23T15:05:56Z

Looks good. Sorry about the delay.

DougGregor · 2024-08-23T16:35:26Z

@DougGregor just a friendly reminder when you have time. I believe this PR can be already merged once the CI tests have passed.

Thank you for the ping, and I'm sorry I lost track of this. Your changes look great to me and we'll get them merged once CI passes. Thank you!

…t-with-name-as-invalid-identifier

dmhts · 2024-08-23T19:14:33Z

I just fixed some conflicts that popped up only during the tests (which succeeded). Next time, I'll trust GitHub's UI less and merge main beforehand.

dschaefer2 · 2024-08-23T19:20:19Z

@swift-ci please test

dschaefer2 · 2024-08-23T19:20:32Z

@swift-ci please test windows

dmhts · 2024-08-23T19:41:13Z

--- bootstrap: error: Command '['env', 'SDKROOT=/...' returned non-zero exit status 1.`

It seems to be a CI issue :(

dschaefer2 · 2024-08-23T21:00:47Z

@swift-ci please test

dschaefer2 · 2024-08-23T21:12:17Z

--- bootstrap: error: Command '['env', 'SDKROOT=/...' returned non-zero exit status 1.`

It seems to be a CI issue :(

Yup CI is down at the moment. We're trying to get it back.

dschaefer2 · 2024-08-23T21:41:48Z

I should say working to get it back. I think we've identified the issue. I'll retrigger the tests once I know we're clear.

dschaefer2 · 2024-08-24T00:32:18Z

@swift-ci please test

DougGregor · 2024-08-25T04:49:19Z

All clear, merging! Thank you @dmhts

Use "Example" prefix instead of actual target name

9e15797

dmhts requested review from bnbarham, MaxDesiatov, jakepetroules, francescomikulis and xedin as code owners July 8, 2024 07:05

dmhts commented Jul 8, 2024

View reviewed changes

grynspan reviewed Jul 10, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into dmhts/fix-add-targe…

e515806

…t-with-name-as-invalid-identifier

Introduce target name sanitized name

627b625

dmhts commented Aug 6, 2024

View reviewed changes

Use uppercase characters and spaces in tests

88ffe20

dmhts commented Aug 6, 2024

View reviewed changes

Use localized capitalizing

3160ed5

dschaefer2 approved these changes Aug 23, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into dmhts/fix-add-targe…

d8d4e3f

…t-with-name-as-invalid-identifier

dmhts requested review from shawnhyam, bripeticca, plemarquand and owenv as code owners August 23, 2024 19:09

DougGregor merged commit 742157a into swiftlang:main Aug 25, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dmhts commented Jul 8, 2024 •

edited

Loading

dmhts Jul 8, 2024 •

edited

Loading

DougGregor Aug 5, 2024

dmhts Aug 6, 2024 •

edited

Loading

grynspan Jul 10, 2024

dmhts Jul 10, 2024 •

edited

Loading

grynspan Jul 10, 2024

dmhts Jul 10, 2024

DougGregor Jul 10, 2024

DougGregor commented Aug 5, 2024

dmhts Aug 6, 2024

dmhts Aug 6, 2024 •

edited

Loading

grynspan Aug 6, 2024

dmhts Aug 8, 2024 •

edited

Loading

grynspan Aug 8, 2024

dmhts Aug 9, 2024 •

edited

Loading

dmhts Aug 6, 2024 •

edited

Loading

bnbarham Aug 6, 2024

dmhts Aug 8, 2024

dmhts Aug 6, 2024 •

edited

Loading

dmhts commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

DougGregor commented Aug 23, 2024

dmhts commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

dmhts commented Aug 23, 2024 •

edited

Loading

dschaefer2 commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

dschaefer2 commented Aug 24, 2024

DougGregor commented Aug 25, 2024

		func testAddExecutableTargetWithNameAsInvalidSwiftIdentifier() throws {
		try assertManifestRefactor("""

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

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

Conversation

dmhts commented Jul 8, 2024 • edited Loading

Motivation:

Modifications:

Result:

dmhts Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmhts Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmhts Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DougGregor commented Aug 5, 2024

Choose a reason for hiding this comment

dmhts Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmhts Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmhts Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

dmhts Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmhts Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

dmhts commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

DougGregor commented Aug 23, 2024

dmhts commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

dmhts commented Aug 23, 2024 • edited Loading

dschaefer2 commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

dschaefer2 commented Aug 23, 2024

dschaefer2 commented Aug 24, 2024

DougGregor commented Aug 25, 2024

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

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

dmhts commented Jul 8, 2024 •

edited

Loading

dmhts Jul 8, 2024 •

edited

Loading

dmhts Aug 6, 2024 •

edited

Loading

dmhts Jul 10, 2024 •

edited

Loading

dmhts Aug 6, 2024 •

edited

Loading

dmhts Aug 8, 2024 •

edited

Loading

dmhts Aug 9, 2024 •

edited

Loading

dmhts Aug 6, 2024 •

edited

Loading

dmhts Aug 6, 2024 •

edited

Loading

dmhts commented Aug 23, 2024 •

edited

Loading