Convert String Concatenation to String Interpolation #1551

AppAppWorks · 2024-07-03T21:14:51Z

It's only a draft as the support for multiline string literals is not yet implemented and more tests have to be written, I also wonder if it's a good idea to also perform the action in FormatRawStringLiteral when performing this conversion.

ahoppen

Thanks for adding the refactoring @AppAppWorks.

Depending on how you want to approach it, you can add support for multi-line string literals in this PR or make the refactoring unavailable if the sequence expression contains a multi-line string literal and then think about that case in a follow-up PR.

Sources/SourceKitLSP/Swift/CodeActions/ConvertStringConcatenationToStringInterpolation.swift

ahoppen · 2024-07-03T21:34:55Z

Sources/SourceKitLSP/Swift/CodeActions/ConvertStringConcatenationToStringInterpolation.swift

+}
+
+fileprivate extension ExprListSyntax {
+  func preflight() -> (componentsOnly: [ExprListSyntax.Element], commonPounds: TokenSyntax?)? {


Could you add a doc comment about what this method does and what componentsOnly and commonPounds represent?

Also, I would make this a static function on ConvertStringConcatenationToStringInterpolation instead of an extension on ExprListSyntax. I feel like extensions on a type should be generally useful but this preflight method is very specialized to the ConvertStringConcatenationToStringInterpolation refactoring.

ahoppen · 2024-07-03T21:40:46Z

Sources/SourceKitLSP/Swift/CodeActions/ConvertStringConcatenationToStringInterpolation.swift

+    var ret: StringLiteralSegmentListSyntax = []
+    for component in componentsOnly {
+      if var stringLiteral = StringLiteralExprSyntax(component) {
+        stringLiteral.pounds = commonPounds


Is this doing anything? We only extract the segments from the string literal, so it shouldn’t make a difference whether we set the pounds here. Also means we could remove the pounds property in the extension below.

Setting the pounds here also sets the pounds for all interpolations inside the literal, maybe I should refactor the setter of StringLiteralExprSyntax.pounds to make the intent clearer?

Tests/SourceKitLSPTests/CodeActionTests.swift

Sources/SourceKitLSP/Swift/CodeActions/ConvertStringConcatenationToStringInterpolation.swift

ahoppen · 2024-07-04T07:42:04Z

Tests/SourceKitLSPTests/CodeActionTests.swift

+              uri: [
+                TextEdit(
+                  range: positions["1️⃣"]..<positions["5️⃣"],
+                  newText: "##\"[\\##(key): \\##(d) \\##(value)]\"##"


I think this would be easier to read if you made it a multi-line string literal, because then you don’t need to escape the ".

AppAppWorks · 2024-07-04T09:10:40Z

@ahoppen please check again :)

ahoppen

Very nice

Tests/SourceKitLSPTests/CodeActionTests.swift

Sources/SourceKitLSP/Swift/CodeActions/ConvertStringConcatenationToStringInterpolation.swift

Tests/SourceKitLSPTests/CodeActionTests.swift

AppAppWorks · 2024-07-04T20:23:26Z

Could a follow-up action be converting concatenation of (some CustomStringConvertible).description into string interpolation?

e.g. 1.description + false.description -> "\(1)\(false)"

ahoppen

Looking great! I have a few minor comments that would make the code more readable in my opinion. After that, we’re ready to go.

Sources/SourceKitLSP/Swift/CodeActions/ConvertStringConcatenationToStringInterpolation.swift

ahoppen · 2024-07-16T19:59:08Z

Could a follow-up action be converting concatenation of (some CustomStringConvertible).description into string interpolation?

e.g. 1.description + false.description -> "\(1)\(false)"

I personally wouldn’t prioritize it because I don’t think it’s a pattern that’s used terribly often but I also wouldn’t object to a PR that implements it.

AppAppWorks · 2024-07-16T21:59:52Z

All done, please have a look.

ahoppen

Nice catch with the multi-line comments.

Sources/SourceKitLSP/Swift/CodeActions/ConvertStringConcatenationToStringInterpolation.swift

AppAppWorks · 2024-07-16T23:00:08Z

Revised, please check again!

ahoppen

Thank you for implementing the refactoring @AppAppWorks. Looks great!

Would you mind filing an issue to also support multi-line string literals. Just so we don’t forget about that as a follow-up thing that can be done.

ahoppen · 2024-07-17T17:57:02Z

@swift-ci Please test

ahoppen · 2024-07-17T18:00:53Z

@swift-ci Please test Windows

ahoppen · 2024-07-17T21:31:52Z

Sources/SourceKitLSP/CMakeLists.txt

@@ -31,6 +31,7 @@ target_sources(SourceKitLSP PRIVATE
  Swift/CodeActions/AddDocumentation.swift
  Swift/CodeActions/ConvertIntegerLiteral.swift
  Swift/CodeActions/ConvertJSONToCodableStruct.swift
+  Swift/CodeActions/ConvertStringConcatenationToStringInterpolation.swift,


I think this the Windows build is failing because this shouldn’t have a comma.

My bad, I've pushed a new commit, please test again.

added `ConvertStringConcatenationToStringInterpolation` to convert string concatenation to string interpolation: - the string concatenation must contain at least one string literal - the number of pound symbols in the resulting string interpolation is determined by the highest number of pound symbols among all string literals in the string concatenation - multiline string literals are not supported registered in `SyntaxCodeActions.allSyntaxCodeActions` registered in Sources/SourceKitLSP/CMakeLists created a test in `CodeActionTests`

ahoppen · 2024-07-17T22:13:57Z

@swift-ci Please test

ahoppen · 2024-07-17T22:14:56Z

@swift-ci Please test Windows

AppAppWorks requested a review from ahoppen as a code owner July 3, 2024 21:14

AppAppWorks marked this pull request as draft July 3, 2024 21:14

ahoppen reviewed Jul 4, 2024

View reviewed changes

AppAppWorks force-pushed the convert-string-concatenation-to-string-interpolation branch from 56081b7 to 1d64ead Compare July 4, 2024 09:09

ahoppen reviewed Jul 4, 2024

View reviewed changes

AppAppWorks force-pushed the convert-string-concatenation-to-string-interpolation branch 2 times, most recently from 37f0d8b to babf856 Compare July 8, 2024 20:55

AppAppWorks marked this pull request as ready for review July 9, 2024 23:39

ahoppen reviewed Jul 16, 2024

View reviewed changes

AppAppWorks force-pushed the convert-string-concatenation-to-string-interpolation branch from e0ce0ba to 50bb915 Compare July 16, 2024 21:59

ahoppen reviewed Jul 16, 2024

View reviewed changes

AppAppWorks force-pushed the convert-string-concatenation-to-string-interpolation branch from 50bb915 to b19e0a9 Compare July 16, 2024 22:59

ahoppen approved these changes Jul 17, 2024

View reviewed changes

ahoppen enabled auto-merge July 17, 2024 17:57

AppAppWorks mentioned this pull request Jul 17, 2024

Add support for multi-line string literals when converting string concatenation to string interpolation #1568

Open

ahoppen reviewed Jul 17, 2024

View reviewed changes

auto-merge was automatically disabled July 17, 2024 21:57
Head branch was pushed to by a user without write access

AppAppWorks force-pushed the convert-string-concatenation-to-string-interpolation branch from b19e0a9 to 17e33a6 Compare July 17, 2024 21:57

ahoppen enabled auto-merge July 17, 2024 22:13

ahoppen merged commit e4cb1bd into swiftlang:main Jul 18, 2024
3 checks passed

AppAppWorks mentioned this pull request Jul 18, 2024

Fix for trivia not preserved after string interpolation conversion #1575

Merged

Convert String Concatenation to String Interpolation #1551

Convert String Concatenation to String Interpolation #1551

Uh oh!

Conversation

AppAppWorks commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen Jul 3, 2024

Choose a reason for hiding this comment

Uh oh!

ahoppen Jul 3, 2024

Choose a reason for hiding this comment

Uh oh!

AppAppWorks Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

AppAppWorks commented Jul 4, 2024

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppAppWorks commented Jul 4, 2024

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen commented Jul 16, 2024

Uh oh!

AppAppWorks commented Jul 16, 2024

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppAppWorks commented Jul 16, 2024

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 17, 2024

Uh oh!

ahoppen commented Jul 17, 2024

Uh oh!

ahoppen Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

AppAppWorks Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 17, 2024

Uh oh!

ahoppen commented Jul 17, 2024

AppAppWorks commented Jul 3, 2024 •

edited

Loading