Skip to content

Add diagnostic for label with string segment #1864

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

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Jun 28, 2023

Resolves #1831

@kimdv kimdv requested a review from ahoppen as a code owner June 28, 2023 20:13
@kimdv kimdv force-pushed the kimdv/add-diagnostics-for-string-interpolates-message-label branch 4 times, most recently from a74cceb to 53fbbe3 Compare June 30, 2023 05:48
@kimdv kimdv force-pushed the kimdv/add-diagnostics-for-string-interpolates-message-label branch 2 times, most recently from e5cc28a to 59e16b6 Compare July 7, 2023 21:23
@kimdv kimdv force-pushed the kimdv/add-diagnostics-for-string-interpolates-message-label branch 3 times, most recently from e830959 to ea4b71f Compare July 19, 2023 12:08
@kimdv kimdv force-pushed the kimdv/add-diagnostics-for-string-interpolates-message-label branch from ea4b71f to 7c0220b Compare July 20, 2023 09:56
@kimdv
Copy link
Contributor Author

kimdv commented Jul 20, 2023

@swift-ci please test

@kimdv kimdv requested a review from ahoppen July 20, 2023 09:58
@kimdv
Copy link
Contributor Author

kimdv commented Jul 20, 2023

@ahoppen testDiagnoseAvailability18 fails.

It fails formatting the same code, this is the result

@available(*, unavailable, message: """
  foobar message.
  
""") func multilineMessage() {
}
multilineMessage()

A quick debug I think it's because we do some post processing of multiline string when parsing a RawStringLiteralExprSyntax.

We can use the same method, but that require we convert the RawStringSegmentSyntax to a RawStringLiteralSegmentsSyntax.Element and then back to RawStringSegmentSyntax.

Or we should try to extract the postProcessMultilineStringLiteral into multiple functions so we better can reuse it.

What do you think?

@ahoppen
Copy link
Member

ahoppen commented Jul 20, 2023

I think we should try just re-using postProcessMultilineStringLiteral. Mapping between RawStringLiteralExprSyntax and RawStringLiteralSegmentsSyntax.Element doesn’t result in an allocation on the bump allocator, so we shouldn’t be leaking any memory that way, so the mapping should be fine.

@kimdv kimdv force-pushed the kimdv/add-diagnostics-for-string-interpolates-message-label branch 2 times, most recently from 40ac1bf to fdcb47f Compare August 4, 2023 18:45
@kimdv
Copy link
Contributor Author

kimdv commented Aug 4, 2023

@ahoppen I was just debugging and trying to figure out why we get this error

I've tried to debug, but I can't find the reason

Trees do not match due to token in:
stringSegment("  foobar message.") [2:1...2:18]
when expecting:
stringSegment("foobar message.") [2:3...2:18]

@ahoppen
Copy link
Member

ahoppen commented Aug 4, 2023

As the failure message says: What we do is remove all the trivia of the tree, run BasicFormat and check that we end up with the same tree. This basically tests BasicFormat to check that we indeed separate different tokens by space.

But when you remove the leading spaces from the closing """, you are really changing the semantic meaning of the string literal because the two spaces in front of foobar message. now become part of the string literal.

That’s why in SwiftParserTest/Assertions.swift:735 (inside TriviaRemover, we check if the token is inside a StringLiteralSyntax, in which case we just don’t remove trivia from inside this subtree. You just need to add SimpleStringLiteralExprSyntax here.

And as I’m looking at the code, I’m realizing that we could actually simplify TriviaRemover

/// Removes trivia from all tokens that don’t occur inside multiline string
/// literals.
///
/// We keep trivia inside multiline string literals because the indentation of
/// the closing quote of a multi-line string literals has impact on how much
/// leading trivia is stripped from the literal’s content.
class TriviaRemover: SyntaxRewriter {
  override func visit(_ node: StringLiteralExprSyntax) -> ExprSyntax {
    if node.openingQuote == .multilineStringQuoteToken() {
      return ExprSyntax(node)
    } else {
      return super.visit(node)
    }
  }

  override func visit(_ node: SimpleStringLiteralExprSyntax) -> ExprSyntax {
    if node.openingQuote == .multilineStringQuoteToken() {
      return ExprSyntax(node)
    } else {
      return super.visit(node)
    }
  }

  override func visit(_ token: TokenSyntax) -> TokenSyntax {
    return token.with(\.leadingTrivia, []).with(\.trailingTrivia, [])
  }
}

@kimdv kimdv force-pushed the kimdv/add-diagnostics-for-string-interpolates-message-label branch 2 times, most recently from cb87d07 to f6dbe22 Compare August 5, 2023 07:40
@kimdv
Copy link
Contributor Author

kimdv commented Aug 5, 2023

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/add-diagnostics-for-string-interpolates-message-label branch from f6dbe22 to cf8d598 Compare August 5, 2023 10:38
@kimdv
Copy link
Contributor Author

kimdv commented Aug 5, 2023

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/add-diagnostics-for-string-interpolates-message-label branch from cf8d598 to a795ee7 Compare August 5, 2023 13:21
@kimdv
Copy link
Contributor Author

kimdv commented Aug 5, 2023

@swift-ci please test

@ahoppen
Copy link
Member

ahoppen commented Aug 5, 2023

@swift-ci Please test Windows

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

A couple comments to prevent round-trip failures, otherwise LGTM.

@kimdv kimdv force-pushed the kimdv/add-diagnostics-for-string-interpolates-message-label branch 4 times, most recently from 57e0dea to fd9c925 Compare August 8, 2023 13:25
@kimdv
Copy link
Contributor Author

kimdv commented Aug 8, 2023

@swift-ci Please test

@kimdv kimdv force-pushed the kimdv/add-diagnostics-for-string-interpolates-message-label branch from fd9c925 to 4226e7a Compare August 9, 2023 12:15
@kimdv
Copy link
Contributor Author

kimdv commented Aug 9, 2023

@swift-ci Please test

@kimdv
Copy link
Contributor Author

kimdv commented Aug 9, 2023

@swift-ci Please test windows

@kimdv kimdv merged commit 3b727c6 into swiftlang:main Aug 9, 2023
@kimdv kimdv deleted the kimdv/add-diagnostics-for-string-interpolates-message-label branch August 9, 2023 18:30
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.

#sourceLocation should not allow string literals with string interpolation
2 participants