Skip to content
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

StringLiteralExprSyntax adds whitespace in front of closingQuote #2906

Open
Jeehut opened this issue Dec 2, 2024 · 11 comments · May be fixed by #2922
Open

StringLiteralExprSyntax adds whitespace in front of closingQuote #2906

Jeehut opened this issue Dec 2, 2024 · 11 comments · May be fixed by #2922
Labels
bug Something isn't working

Comments

@Jeehut
Copy link
Contributor

Jeehut commented Dec 2, 2024

Description

I'm using version 600.0.1 and wrote this code as part of a macro expression:

      let defaultValueLiteral = StringLiteralExprSyntax(
         openingQuote: .stringQuoteToken(),
         segments: StringLiteralSegmentListSyntax([.stringSegment(.init(content: "Some Text"))]),
         closingQuote: .stringQuoteToken()
      )

When I return it in my ExpressionMacro wrapped into an ExprSyntax, the expanded code looks like this:

"Some Text "

But I expect it to be:

"Some Text"

Am I missing something?

Steps to Reproduce

Here's a more complete code example:

public struct TranslationKey: ExpressionMacro {
   public static func expansion(
      of node: some FreestandingMacroExpansionSyntax,
      in context: some MacroExpansionContext
   ) throws -> ExprSyntax {
      // constructing: `"Some Text"`
      let defaultValueLiteral = StringLiteralExprSyntax(
         openingQuote: .stringQuoteToken(),
         segments: StringLiteralSegmentListSyntax([.stringSegment(.init(content: "Some Text"))]),
         closingQuote: .stringQuoteToken()
      )

      // I have actually more code here (just in case you were wondering)

      return ExprSyntax(defaultValueLiteral)
   }
}
@Jeehut Jeehut added the bug Something isn't working label Dec 2, 2024
@ahoppen
Copy link
Member

ahoppen commented Dec 3, 2024

Synced to Apple’s issue tracker as rdar://140829044

@ahoppen
Copy link
Member

ahoppen commented Dec 3, 2024

My bet would be that there’s something in // I have actually more code here (just in case you were wondering) where you are building a DeclSyntax node that contains an expression or an ExprSyntax that actually contains multiple expressions (or something of the kind). When testing your macro, could you look for any log messages in the debug console or check if a breakpoint gets hit here:

let diagnostics = ParseDiagnosticsGenerator.diagnostics(for: self)
?

@Jeehut
Copy link
Contributor Author

Jeehut commented Dec 4, 2024

Firstly: The code I posted above is exactly the code with which I can reproduce the issue. The comment was just merely to tell you that this is not my final macro and there's more work I need to do. So it can't be related to any DeclSyntax stuff I'm doing somewhere else. Did you try this code on your end and didn't run into the same issue?

Secondly: I do not get any log messages when running my tests. I set a breakpoint inside logStringInterpolationParsingError at the line you specified (it was line 62 for me) but when running my tests, the breakpoint didn't get hit.

I stripped down my Swift package to a minimal reproducible state and attached it for you. This way you should be able to fully reproduce my reported issue by simply running the tests:
TranslateKit-Issue-2906.zip

Note that I tested the macro also in a real-world project and hit the same issue, so I don't think it's related to how the test is written.

@ahoppen
Copy link
Member

ahoppen commented Dec 4, 2024

Ah, sorry. My hunch was wrong then. I just debugged this and the issue is that when you are calling StringSegmentSyntax(content: "Some Text), you pass a string literal as the content parameter. That content parameter is a TokenSyntax and creating a TokenSyntax from a string literal creates an identifier token. But a string segment should not be an identifier token but a string segment token, ie. this should be StringSegmentSyntax(content: .stringSegment("Some Text")). That way BasicFormat will pick up the correct token type and not insert a whitespace after the string segment.

@Jeehut
Copy link
Contributor Author

Jeehut commented Dec 5, 2024

@ahoppen Awesome, thank you! That actually fixed the issue I've had. But the whole API is not very intuitive. Is there any good place to learn about all these things, like a book or at least some unit tests which I can read to learn things like this?

@ahoppen
Copy link
Member

ahoppen commented Dec 5, 2024

Unfortunately I don’t have any good single resource that I can recommend to learn about these sorts of details.

@Jeehut
Copy link
Contributor Author

Jeehut commented Dec 5, 2024

@ahoppen Would it be useful to report an issue explicitly complaining about the lack of detailed documentation or a learning path? If that helps prioritizing internally, I'm happy to report it. 😁 Or this issue itself could be turned into that issue since the main problem was the lack of documentation? If not, I think this can be closed.

But if it's currently too early for some reason (e.g. because some bigger refactoring are planned in the near future), I can understand that you might need som extra time.

@ahoppen
Copy link
Member

ahoppen commented Dec 5, 2024

A general “Need more documentation” issue will not be very helpful or actionable but I think that more concrete questions that are left unanswered by the documentation at https://swiftpackageindex.com/swiftlang/swift-syntax/main/documentation/swiftsyntax can be helpful. Also: If you have found an answer to one of these questions and would like to share it with the community, PRs are always very welcome and I’m happy to help polish it, if necessary.

@Jeehut
Copy link
Contributor Author

Jeehut commented Dec 5, 2024

@ahoppen I see, that makes sense. I think, what would have helped me most in this specific situation would have been if each Decl had some common usage examples both for reading from and creation of each Declaration. In my particular case, if StringLiteralExprSyntax had an example in the type documentation how to create one with a string (and other common ways if there are any), I could have discovered it right within Xcode when playing around with the APIs, cause I often alt-click on types or initializers to understand them better if I don't get what I expected from the API naming itself.

So, you would suggest that I just document what I've learned this way inside the respective types right within the project and create a pull request right away without creating a separate issue for each? I may actually create a bigger PR documenting a whole bunch of APIs with some examples I've run into myself. Cause why not if I'm already at it. Is a "big" documentation-adding PR fine with you or rather small ones documenting only few types each time?

@ahoppen
Copy link
Member

ahoppen commented Dec 6, 2024

Yes, a PR without an issue is perfectly fine. Thank you in advance for doing it 🙏🏽. As long as the PR doesn’t get massive (I usually fine 1000 LOC a point when things start to get hard to review), one PR is just fine.

And I would attach the documentation for all the types to the type nodes, like here:

documentation: """
An `associatedtype` declaration
An example of an associatedtype declaration is
```swift
associatedtype Item
```
An associated type declaration may contain a type initializer clause which represents a default type assignment for the associated type.
```swift
associatedtype Item = Int
```
An associated type declaration may be declared with an inheritance clause which specifies the required conformances.
```swift
associatedtype Iterator: IteratorProtocol
```
A generic where clause may be present, here is an example which shows an associated type containing an inheritance clauses and a generic where clause.
```swift
associatedtype Iterator: IteratorProtocol where Iterator.Element == Item
```
""",

@Jeehut
Copy link
Contributor Author

Jeehut commented Dec 18, 2024

@ahoppen Here we go (probably done wrong though): #2922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants