Fix wrong formatting of multiline string literal #1623

kimdv · 2023-05-01T15:15:41Z

I tried you comment here: #1616 (comment)

But it didn't work for me.
What I figured out what because if it have an open """ but not a closing """ then the formatter will only format on the closing one.
It sounds to me that works as intended. Only visit the tokens we fix.

Tests/SwiftParserTest/ExpressionTests.swift

Tests/SwiftParserTest/translated/AvailabilityQueryUnavailabilityTests.swift

Tests/SwiftParserTest/translated/UnclosedStringInterpolationTests.swift

Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift

Sources/SwiftBasicFormat/BasicFormat.swift

ahoppen

I’m not sure if it changes anything test-wise but since you are adding a trailing newline, that should also be reflected in previousTokenWillEndWithWhitespace. I.e. the return there should be replaced by the following (slightly refactored to improve readability.

      if previousToken.trailingTrivia.endsWithWhitespace {
        return true
      }
      if isMutable(previousToken) && (requiresWhitespace(between: previousToken, and: token) || requiresTrailingNewline(previousToken)) {
        return true
      }
      return false

ahoppen

Nice. This ended up being a pretty small change after all 😆

Unrelated to this PR, I think there’s still an error where we don’t insert the newline after the opening """:

assertBuildResult(
  StringLiteralExprSyntax(openQuote: .multilineStringQuoteToken(), content: "a", closeQuote: .multilineStringQuoteToken()),
  #"""
  """
  a
  """
  """#
)

Would you like to look into that as well? If you don’t want to look at it straight away, I will file an issue for it.

ahoppen · 2023-05-03T22:49:04Z

@swift-ci Please test

kimdv · 2023-05-04T05:00:09Z

@swift-ci please test

kimdv · 2023-05-04T06:27:28Z

@swift-ci please test linux

kimdv · 2023-05-04T06:27:34Z

@swift-ci please test Windows

kimdv · 2023-05-07T18:45:52Z

@swift-ci please test

kimdv · 2023-05-08T07:02:58Z

@swift-ci please test

kimdv · 2023-05-08T11:20:47Z

I don't understand why it says I haven't generated files.
When running locally there is nothing new ? 🤨

kimdv · 2023-05-08T17:33:08Z

I don't understand why it says I haven't generated files.
When running locally there is nothing new ? 🤨

Found the reason.
The reason that is that I had build and run code generation before. It did not look for a never version.
Looks a bit like a bug to me.

Tried to resolve and update but got those errors

kimdevos@dk-kimdevos12-8752 CodeGeneration % swift package resolve
Updating /Users/kimdevos/repository/kimdv/swift-src/swift-syntax
Updated /Users/kimdevos/repository/kimdv/swift-src/swift-syntax (0.47s)
error: could not find a branch named ‘HEAD’ in /Users/kimdevos/repository/kimdv/swift-src/swift-syntax
kimdevos@dk-kimdevos12-8752 CodeGeneration % swift package update 
Updating /Users/kimdevos/repository/kimdv/swift-src/swift-syntax
Updated /Users/kimdevos/repository/kimdv/swift-src/swift-syntax (0.17s)
error: could not find a branch named ‘HEAD’ in /Users/kimdevos/repository/kimdv/swift-src/swift-syntax

I deleted the .build and re-run the swift package resolve and it worked fine.

Looks a bit like a bug in SPM?

kimdv · 2023-05-08T17:33:14Z

@swift-ci please test

kimdv · 2023-05-08T17:34:54Z

@swift-ci please test windows

ahoppen · 2023-05-08T18:07:03Z

Found the reason. The reason that is that I had build and run code generation before. It did not look for a never version. Looks a bit like a bug to me.

I think that’s intentional. Unless you run swift package update, it won’t update your package version. Could you add a comment to our docs where we suggest swift run --package-path CodeGeneration generate-swiftsyntax Sources to run swift package update beforehand?

I deleted the .build and re-run the swift package resolve and it worked fine.

Looks a bit like a bug in SPM?

I was able to reproduce this as well but then couldn’t reproduce it again by creating a new commit and running swift package update. If you’ve got steps to reliably reproduce this, could you file an issue for SwiftPM?

Sources/SwiftBasicFormat/BasicFormat.swift

kimdv · 2023-05-17T12:34:21Z

@swift-ci please test

ahoppen

Wohooo. Nice. This looks really good now. I only have a few remarks mostly about naming and documentation.

Sources/SwiftBasicFormat/BasicFormat.swift

ahoppen · 2023-05-17T18:38:52Z

Sources/SwiftBasicFormat/BasicFormat.swift

-      }
-      if let ancestorsParent = ancestor.parent, childrenSeparatedByNewline(ancestorsParent) {
+    } else if let second {
+      if second.requiresLeadingNewline {


Definitely not part of this PR, but eventually I would like to remove requiresLeadingNewline from CodeGen altogether and just list the tokens that require a leading newline in this function. Same for childrenSeparatedByNewline. I don’t think we necessarily need to CodeGen it, it could just be implemented manually in this file.

Sources/SwiftBasicFormat/BasicFormat.swift

Tests/SwiftParserTest/ExpressionTests.swift

kimdv · 2023-05-18T12:23:22Z

@swift-ci please test

kimdv · 2023-05-18T12:23:45Z

@swift-ci please test windows

kimdv · 2023-05-19T07:18:21Z

@swift-ci please test

kimdv · 2023-05-19T07:18:41Z

@swift-ci please test windows

…ltiline-string-literal

kimdv requested a review from ahoppen as a code owner May 1, 2023 15:15

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch 2 times, most recently from e248934 to e49abe0 Compare May 1, 2023 15:16

kimdv commented May 1, 2023

View reviewed changes

Tests/SwiftParserTest/ExpressionTests.swift Show resolved Hide resolved

ahoppen reviewed May 1, 2023

View reviewed changes

kimdv mentioned this pull request May 2, 2023

Fix diagnostic for wrong where in if statement #1616

Merged

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch from e49abe0 to 06e701c Compare May 2, 2023 08:23

ahoppen reviewed May 2, 2023

View reviewed changes

Sources/SwiftBasicFormat/BasicFormat.swift Outdated Show resolved Hide resolved

Sources/SwiftBasicFormat/BasicFormat.swift Outdated Show resolved Hide resolved

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch from 06e701c to e021732 Compare May 2, 2023 19:25

ahoppen reviewed May 2, 2023

View reviewed changes

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch 3 times, most recently from e375328 to d84ceb3 Compare May 3, 2023 14:24

kimdv changed the title ~~Fix wrong foratting of multiline string literal~~ Fix wrong formatting of multiline string literal May 3, 2023

ahoppen approved these changes May 3, 2023

View reviewed changes

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch from d84ceb3 to 74aa602 Compare May 4, 2023 04:59

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch from c51b0d6 to fc2e999 Compare May 7, 2023 17:35

kimdv requested a review from ahoppen May 7, 2023 18:45

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch from fc2e999 to 3fccbe7 Compare May 8, 2023 17:30

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch 2 times, most recently from 124adcc to 6fb0cb9 Compare May 10, 2023 13:52

kimdv commented May 10, 2023

View reviewed changes

Sources/SwiftBasicFormat/BasicFormat.swift Outdated Show resolved Hide resolved

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch 5 times, most recently from 758daa8 to 08fecd9 Compare May 17, 2023 11:54

kimdv requested a review from ahoppen May 17, 2023 12:34

ahoppen approved these changes May 17, 2023

View reviewed changes

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch 2 times, most recently from 8b61a40 to 2521ba5 Compare May 18, 2023 08:12

kimdv added 2 commits May 18, 2023 12:40

Change test class name to match syntax type

60fa670

Fix wrong foratting of multiline string literal

3bd861f

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch from 2521ba5 to ac5dcb4 Compare May 18, 2023 12:20

kimdv enabled auto-merge May 18, 2023 12:24

kimdv disabled auto-merge May 18, 2023 13:26

kimdv added 2 commits May 19, 2023 09:14

Fix missing newline for opening quote for multilines strings

a64d0ef

Add documentation for local helper variables

6f41221

kimdv force-pushed the kimdv/fix-newline-for-multiline-string-literal branch from ac5dcb4 to 6f41221 Compare May 19, 2023 07:15

kimdv merged commit 0711d21 into swiftlang:main May 19, 2023

kimdv deleted the kimdv/fix-newline-for-multiline-string-literal branch May 19, 2023 13:07

kimdv added a commit to kimdv/swift-syntax that referenced this pull request May 20, 2023

Merge pull request swiftlang#1623 from kimdv/kimdv/fix-newline-for-mu…

a7ebf2f

…ltiline-string-literal

kimdv mentioned this pull request May 22, 2023

Remove requiresLeadingNewline from basic format #1690

Merged

kimdv added a commit to kimdv/swift-syntax that referenced this pull request May 23, 2023

Merge pull request swiftlang#1623 from kimdv/kimdv/fix-newline-for-mu…

5facc4d

…ltiline-string-literal

Fix wrong formatting of multiline string literal #1623

Fix wrong formatting of multiline string literal #1623

Uh oh!

Conversation

kimdv commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented May 3, 2023

Uh oh!

kimdv commented May 4, 2023

Uh oh!

kimdv commented May 4, 2023

Uh oh!

kimdv commented May 4, 2023

Uh oh!

kimdv commented May 7, 2023

Uh oh!

kimdv commented May 8, 2023

Uh oh!

kimdv commented May 8, 2023

Uh oh!

kimdv commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimdv commented May 8, 2023

Uh oh!

kimdv commented May 8, 2023

Uh oh!

ahoppen commented May 8, 2023

Uh oh!

Uh oh!

kimdv commented May 17, 2023

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahoppen May 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kimdv commented May 18, 2023

Uh oh!

kimdv commented May 18, 2023

Uh oh!

kimdv commented May 19, 2023

Uh oh!

kimdv commented May 19, 2023

Uh oh!

Uh oh!

kimdv commented May 1, 2023 •

edited

Loading

kimdv commented May 8, 2023 •

edited

Loading