Skip to content

[5.3][SourceKit/CodeFormat] Fix var/let declaration continuation lines appearing over-indented. #31945

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

Merged

Conversation

nathawes
Copy link
Contributor

@nathawes nathawes commented May 21, 2020

Cherry-pick of #31941 for 5.3

  • Explanation:
    The indentation of pattern binding decls was changed in 5.3 to improve their readability when they include multiple entries by column-aligning the entries and indenting their continuation lines relative to that column too. This change made pattern binding decls with a single entry that continued onto a second line seem over-indented, though, both relative to their surrounding code and to how they were treated in the 5.2 release, and single-entry pattern binding decls are used far more commonly than multiple-entry ones. This change fixes the over-indentation issue by not column-aligning the entries if any entry starts on the same line as the var/let and continues onto a later line. For example:

    // Previous behavior (with the over-indentation issue):
    let foo = someItem
          .getValue(), // Column-alignment (relative to `foo`) looks ok here...
        bar = otherItem
          .getValue()
    
    getAThing()
      .andDoStuffWithIt()
    let foo = someItem
          .getValue() // but looks over-indented with a single entry.
    getOtherThing()
      .andDoStuffWithIt()
    
    // New behavior after this fix:
    getAThing()
      .andDoStuffWithIt()
    let foo = someItem
      .getValue() // No column alignment in this case (good)...
    doOtherThing()
    
    let foo = someItem
      .getValue(), // or in this case (unfortunate, but far less common)...
      bar = otherItem
        .getValue()
    
    let foo = someItem.getValue(),
        bar = otherItem.getValue() // but still column-aligned in this case (good).

    Using this rule, as opposed to handling single and multi-entry PBDs differently, ensures that the as-typed-out indentation (where it's unclear how many entries the user intends to write) matches the selected-and-reindented indentation for multi-entry pattern binding decls.

  • Scope of issue: The over-indentation affects all multi-line var/let declarations in Swift source files, and I expect many users would consider it a regression relative to the 5.2 behavior.

  • Origination: Since the indentation implementation was overhauled in [SourceKit/CodeFormat] Re-work and improve the indentation implementation #30187

  • Risk: Low. This only affects sourcekitd (not the compiler), and specifically only its CodeFormat request. It is a targeted fix that only affects the indentation within pattern binding declarations (var/let declarations).

  • Testing: Updated and added regression tests for this change, regression tests pass, and I manually tested the changes in Xcode.

  • Reviewer: @benlangmuir (on the master branch PR)

Resolves rdar://problem/63309288

…in certain cases.

Don't column align PBD entries if any entry spans from the same line as
the var/let to another line. E.g.

```
// Previous behavior:
let foo = someItem
      .getValue(), // Column-alignment looks ok here, but...
    bar = otherItem
      .getValue()

getAThing()
  .andDoStuffWithIt()
let foo = someItem
      .getValue() // looks over-indented here, which is more common.
getOtherThing()
  .andDoStuffWithIt()

// New behavior
getAThing()
  .andDoStuffWithIt()
let foo = someItem
  .getValue() // No column alignment in this case...
doOtherThing()

let foo = someItem
   .getValue(), // Or in this case (unfortunate, but less common)...
   bar = otherItem
       .getValue()

let foo = someItem.getValue(),
    bar = otherItem.getValue() // but still column-aligned in this case.
```

Resolves rdar://problem/63309288
@nathawes nathawes added the r5.3 label May 21, 2020
@nathawes
Copy link
Contributor Author

@swift-ci please test

@nathawes nathawes changed the title [WIP][5.3][SourceKit/CodeFormat] Don't column-align PatternBindingDecl entries in certain cases. [5.3][SourceKit/CodeFormat] Fix var/let declaration continuation lines appearing over-indented. May 22, 2020
@nathawes nathawes marked this pull request as ready for review May 22, 2020 00:11
@nathawes nathawes requested a review from a team as a code owner May 22, 2020 00:11
@tkremenek tkremenek merged commit 32b2521 into swiftlang:release/5.3 May 22, 2020
@AnthonyLatsis AnthonyLatsis added swift 5.3 🍒 release cherry pick Flag: Release branch cherry picks labels Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 release cherry pick Flag: Release branch cherry picks swift 5.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants