Skip to content

Conversation

natecook1000
Copy link
Member

Uses the existing logic for determining whether a binding can be moved into a tuple or function call context, and then rewrites the tuple element list or argument list to including the binding keyword. Additionally, improves the diagnostic by using the binding specifier in the source code (i.e. let vs var).

This change enables swift format to rewrite case statements like the following:

     switch (start.representation, end.representation) {
-    case let (.element(element), .separator(next: separator)):
+    case (.element(let element), .separator(next: let separator)):
       return 2 * base.distance(from: element, to: separator) - 1
-    case let (.separator(next: separator), .element(element)):
+    case (.separator(next: let separator), .element(let element)):
       return 2 * base.distance(from: separator, to: element) + 1

Everything looks great for the initial case item in a switch, but for subsequent items under the same case statement, the rewritten syntax kind of gets exploded onto multiple lines. Is this a behavior that anyone recognizes? Am I just missing a parameter to tell it not to do this?

-    case let (.element(start), .element(end)),
-      let (.separator(start), .separator(end)):
+    case (.element(let start), .element(let end)),
+      (
+        .separator(
+          let start),
+        .separator(
+          let end)
+      ):
       return 2 * base.distance(from: start, to: end)

if case let x as SomeType = someValue {}
""",
input: """
if case 1️⃣let .labeled(label, value) = DataPoint.labeled("hello", 100) {}
Copy link
Member

Choose a reason for hiding this comment

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

thoughts on adding a test for case .labeled(let label, var value) we should avoid flagging this pattern

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the existing tests (the next two lines after this) cover that pattern, where binding keywords are already used adjacent to the bound variables.

Copy link
Member

@allevato allevato left a comment

Choose a reason for hiding this comment

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

Nice, thanks! I was trying to remember if there was a specific reason that we didn't make this a rewriter previously (i.e., concerns whether there would be cases where the automatic fix would break something), but nothing comes to mind.

@ahoppen Do we need to do anything with the API breakage check failure? The rule implementations themselves are obviously not external API and are only public as @_spi.

@natecook1000 natecook1000 marked this pull request as ready for review February 24, 2025 17:40
@natecook1000
Copy link
Member Author

Fixed the issue with re-wrapping subsequent case items in the latest commit – I was accidentally preserving the leading whitespace on the let keyword when applying it to the local bindings.

Uses the existing logic for determining whether a binding can be moved
into a tuple or function call context, and then rewrites the tuple
element list or argument list to including the binding keyword.
Additionally, improves the diagnostic by using the binding specifier
in the source code (i.e. `let` vs `var`).
@allevato
Copy link
Member

Not an issue with this PR at all, but commenting for posterity: Those required changes to api-breakages.txt are a real bummer.

Once we drop support for 5.8, I guess we can change a lot of those SPIs to package visibility instead. Then I guess it'll freak out because we're deleting a bunch of "public" APIs, but maybe we can take a new baseline then or something.

@allevato allevato merged commit f8e608e into swiftlang:main Feb 24, 2025
19 checks passed
@natecook1000 natecook1000 deleted the bound-case-formatter branch February 25, 2025 14:40
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.

3 participants