[SourceKit/CodeFormat] Re-work and improve the indentation implementation #30187

nathawes · 2020-03-03T21:11:09Z

This restructures the indentation logic around producing a single IndentContext
for the line being indented. An IndentContext has:

a ContextLoc, which points to a source location to indent relative to
a Kind, indicating whether we should indent relative to that location exactly or to the start of the content on its containing line, and
an IndentLevel with the relative number of levels to indent by.

It also improves the handling of:

chained and nested parens, braces, square brackets and angle brackets, and how those interact with the exact alignment of parameters, call arguments, and tuple, array and dictionary elements.
Indenting to the correct level after an incomplete expression, statement or decl.

Resolves:
rdar://problem/59135010
rdar://problem/25519439
rdar://problem/50137394
rdar://problem/48410444
rdar://problem/48643521
rdar://problem/42171947
rdar://problem/40130724
rdar://problem/41405163
rdar://problem/39367027
rdar://problem/36332430
rdar://problem/34464828
rdar://problem/33113738
rdar://problem/32314354
rdar://problem/30106520
rdar://problem/29773848
rdar://problem/27301544
rdar://problem/27776466
rdar://problem/27230819
rdar://problem/25490868
rdar://problem/23482354
rdar://problem/20193017
rdar://problem/47117735
rdar://problem/55950781
rdar://problem/55939440
rdar://problem/53247352
rdar://problem/54326612
rdar://problem/53131527
rdar://problem/48399673
rdar://problem/51361639
rdar://problem/58285950
rdar://problem/58286076
rdar://problem/53828204
rdar://problem/58286182
rdar://problem/58504167
rdar://problem/58286327
rdar://problem/53828026
rdar://problem/57623821
rdar://problem/56965360
rdar://problem/54470937
rdar://problem/55580761
rdar://problem/46928002
rdar://problem/35807378
rdar://problem/39397252
rdar://problem/26692035
rdar://problem/33760223
rdar://problem/48934744
rdar://problem/43315903
rdar://problem/24630624

nathawes · 2020-03-03T21:11:22Z

@swift-ci please test

swift-ci · 2020-03-03T21:16:48Z

Build failed
Swift Test Linux Platform
Git Sha - ce60ac6b8268a953933e27886b6eeef5d2fdb342

theblixguy · 2020-03-03T21:17:30Z

Wow, that’s a lot of radars!

nathawes · 2020-03-03T21:23:47Z

@swift-ci please test

swift-ci · 2020-03-03T21:24:08Z

Build failed
Swift Test Linux Platform
Git Sha - ce60ac6b8268a953933e27886b6eeef5d2fdb342

swift-ci · 2020-03-03T21:28:57Z

Build failed
Swift Test OS X Platform
Git Sha - ce60ac6b8268a953933e27886b6eeef5d2fdb342

nathawes · 2020-03-03T22:16:23Z

@swift-ci please test

swift-ci · 2020-03-03T22:18:58Z

Build failed
Swift Test Linux Platform
Git Sha - e15d8ea59836a166d74d497a783408da86965d7b

swift-ci · 2020-03-03T22:20:24Z

Build failed
Swift Test OS X Platform
Git Sha - e15d8ea59836a166d74d497a783408da86965d7b

nathawes · 2020-03-05T00:47:50Z

@swift-ci please test

swift-ci · 2020-03-05T00:50:18Z

Build failed
Swift Test Linux Platform
Git Sha - 923ae09ee80f917937d60cbbe257571c11030bda

swift-ci · 2020-03-05T01:39:55Z

Build failed
Swift Test OS X Platform
Git Sha - 923ae09ee80f917937d60cbbe257571c11030bda

rintaro · 2020-03-05T06:28:48Z

@swift-ci Please test

rintaro · 2020-03-05T07:24:14Z

lib/Parse/ParseExpr.cpp

@@ -568,9 +559,12 @@ ParserResult<Expr> Parser::parseExprKeyPath() {

  // FIXME: diagnostics
  ParserResult<Expr> rootResult, pathResult;
+  Optional<ParserStatus> parseStatus;


You can simply use ParserStatus parseStatus here. The default is success. What I was talking is that ParserResult cannot be success without the value. But ParserStatus can have simple "success" value.

swift-ci · 2020-03-05T08:10:08Z

Build failed
Swift Test Linux Platform
Git Sha - 9fce6ac6953586f4e93e8a24d5c48cbb18c9e9b9

swift-ci · 2020-03-05T08:24:03Z

Build failed
Swift Test OS X Platform
Git Sha - 9fce6ac6953586f4e93e8a24d5c48cbb18c9e9b9

nathawes · 2020-03-05T18:06:44Z

@swift-ci please test

swift-ci · 2020-03-05T18:07:07Z

Build failed
Swift Test Linux Platform
Git Sha - 9fce6ac6953586f4e93e8a24d5c48cbb18c9e9b9

swift-ci · 2020-03-05T18:11:06Z

Build failed
Swift Test OS X Platform
Git Sha - 9fce6ac6953586f4e93e8a24d5c48cbb18c9e9b9

nathawes · 2020-03-05T20:16:58Z

Failures seem unrelated.
@swift-ci please smoke test

nathawes · 2020-03-05T21:27:01Z

@swift-ci please smoke test OS X Platform

benlangmuir

Wow, this is a big patch. So far I've reviewed everything except the million overloads of FormatWalker:: getIndentContextFrom, of which I've sampled a handful of cases. Overall, I think the approach you've taken is better and more understandable than the previous version, and I'm happy with the overall structure. On the other hand it's not really possible to review every path here.

lib/IDE/Formatting.cpp

…tion. This restructures the indentation logic around producing a single IndentContext for the line being indented. An IndentContext has: - a ContextLoc, which points to a source location to indent relative to, - a Kind, indicating whether we should align with that location exactly, or with the start of the content on its containing line, and - an IndentLevel with the relative number of levels to indent by. It also improves the handling of: - chained and nested parens, braces, square brackets and angle brackets, and how those interact with the exact alignment of parameters, call arguments, and tuple, array and dictionary elements. - Indenting to the correct level after an incomplete expression, statement or decl. Resolves: rdar://problem/59135010 rdar://problem/25519439 rdar://problem/50137394 rdar://problem/48410444 rdar://problem/48643521 rdar://problem/42171947 rdar://problem/40130724 rdar://problem/41405163 rdar://problem/39367027 rdar://problem/36332430 rdar://problem/34464828 rdar://problem/33113738 rdar://problem/32314354 rdar://problem/30106520 rdar://problem/29773848 rdar://problem/27301544 rdar://problem/27776466 rdar://problem/27230819 rdar://problem/25490868 rdar://problem/23482354 rdar://problem/20193017 rdar://problem/47117735 rdar://problem/55950781 rdar://problem/55939440 rdar://problem/53247352 rdar://problem/54326612 rdar://problem/53131527 rdar://problem/48399673 rdar://problem/51361639 rdar://problem/58285950 rdar://problem/58286076 rdar://problem/53828204 rdar://problem/58286182 rdar://problem/58504167 rdar://problem/58286327 rdar://problem/53828026 rdar://problem/57623821 rdar://problem/56965360 rdar://problem/54470937 rdar://problem/55580761 rdar://problem/46928002 rdar://problem/35807378 rdar://problem/39397252 rdar://problem/26692035 rdar://problem/33760223 rdar://problem/48934744 rdar://problem/43315903 rdar://problem/24630624

…status correctly. We were always dropping the error status when returning from parseExprImpl. We were also incorrectly keeping error status after recovering by finding the right close token in parseList. This change fixes both, and also updates a few callers of parseList that assumed when they reported a failure parsing an element the list as a whole would get error status, which isn't true due to recovery.

…ontextLocs. - Rename several symbols to make it clearer whether the ranges they deal with are open or closed. - Add comments documenting the implementation of OutdentChecker::hasOutdent - Fix a bug where code after a doc coment block of the '/**' style was being indented 1 space. - Fix IsInStringLiteral not being set if the indent target was in a string segment of an interpolated multiline string. - Update OutdentChecker::hasOutdent to propagate indent contexts from parent parens/brackets/braces to child parens/brackets/braces that start later in the same line (like FormatWalker already does). This changes the braces in the example below to 'inherit' a ContextLoc from their parent square brackets, which have a ContextLoc at 'foo'. This makes the whole expression be correctly considered 'outdenting': foo(a: "hello" b: "hello")[x: { print("hello") }]

nathawes · 2020-03-11T04:20:06Z

@swift-ci please smoke test

nathawes · 2020-03-11T06:41:53Z

It looks like the swiftpm test failure is related and due to the parser changes. The test parses the code below and checks the reported diagnostic for expected ')' in expression list

import PackageDescription

let package = Package(
    name: "Trivial",
    targets: [
        .target(
            name: "foo",
            dependencies: []),
)

Prior to my parser changes it produced:

/tmp/t.swift:9:1: error: expected expression in container literal
)
^
/tmp/t.swift:9:2: error: expected ')' in expression list
)
 ^
/tmp/t.swift:2:22: note: to match this opening '('
let package = Package(
                     ^

And after:

/tmp/t.swift:9:1: error: expected expression in container literal
)
^

I think this is because when parsing the tuple arg of Package, parseExprList() calls parseExpr() which calls parseExprImpl() which gets an error status back when it tries to parse the collection. Prior to my changesparseExprImpl() was dropping that error status here, which meant we didn't take the true branch here in parseExprList() and still reported the missing right token diagnostics as well. With the error status propagating properly, the true branch is taken and they're suppressed.

Based on that it seems like the new behavior is desirable. @rintaro, do you agree? Should I just update the swiftpm test?

rintaro · 2020-03-11T16:13:21Z

I agree. Let's update the spm test.

…a diagnostic it was checking for. The test parses the code below and checks for a "expected ')' in expression list" diagnostic: ``` import PackageDescription let package = Package( name: "Trivial", targets: [ .target( name: "foo", dependencies: []), ) ``` Prior to the parser fix the invalid collection literal failed to parse but the error status wasn't propagated up to the containing argument list parsing code, so it produced an additional (and undesirable) missing right token diagnostic that this test was checking for. /tmp/t.swift:9:1: error: expected expression in container literal ) ^ /tmp/t.swift:9:2: error: expected ')' in expression list ) ^ /tmp/t.swift:2:22: note: to match this opening '(' let package = Package( ^ Afer the parser fix in swiftlang/swift#30187, we only report the first container literal error and suppress the rest: /tmp/t.swift:9:1: error: expected expression in container literal ) ^

lib/IDE/Formatting.cpp

benlangmuir · 2020-03-11T17:57:47Z

lib/IDE/Formatting.cpp

+    if (!isInCheckRange(L, R))
+      return true;
+
+    // The CheckRange is made outdenting by any parens/braces/brackets with:


Thanks, this is still complicated, but the examples help a lot to get a feeling for how it works!

lib/IDE/Formatting.cpp

benlangmuir · 2020-03-11T18:14:39Z

lib/IDE/Formatting.cpp

+  }
+
+  Optional<Trailing>
+  checkForTrailingTarget(SourceLoc End) {


I found the terminology "Trailing" hard to keep straight, since we have trailing closures, trailing where clauses, trailing targets ie. "Trailing". Would it be correct to call this something like PostTokenTarget? Maybe "trailing" is the right word, but given how overloaded it is, it would be nice if we could find the most specific term here.

The 'Token' part isn't quite right, since it can just be an empty line between two tokens. Do you mean "Trailing" specifically or the "TrailingTarget" term in general?

…opagate ContextLocs when necessary. We don't actually need to set a ContextOverride unless the ContextLoc and L paren/brace/bracket are on different lines. Combined with the fact that we only set them if the L and R parens/braces/brackets are on different lines to, it guarantees there will be at most one override that's applicable on any given line, which lets us simplify the logic somewhat.

nathawes · 2020-03-12T06:07:07Z

@swift-ci please test

swift-ci · 2020-03-12T06:09:46Z

Build failed
Swift Test Linux Platform
Git Sha - a1b5027a16672c2994a2e00cef5cac86c6fe74f2

swift-ci · 2020-03-12T06:11:22Z

Build failed
Swift Test OS X Platform
Git Sha - a1b5027a16672c2994a2e00cef5cac86c6fe74f2

benlangmuir

LGTM. I reviewed the most recent changes, scanned through the many specific indentation context sources, did a few more in-depth looks at specific cases in a debugger, and reviewed the new tests. We can discuss the "Trailing" terminology, but it shouldn't block the PR. Thanks!

nathawes · 2020-03-12T21:05:46Z

This was a huge PR, so thanks a lot for going through it all @benlangmuir.
@rintaro are you ok with the current state of the parser changes?

I'll do another run of the stress tester over the full compat suite (with just the format request enabled) to make sure the last two changes didn't break anything, and do a normal source compat run too just in case the parser changes somehow affect something before I merge.

nathawes · 2020-03-12T21:05:52Z

@swift-ci Please Test Source Compatibility

rintaro

Parser changes look good!

nathawes requested review from rintaro and benlangmuir March 3, 2020 21:11

nathawes force-pushed the indentation branch from ce60ac6 to e15d8ea Compare March 3, 2020 21:23

nathawes force-pushed the indentation branch from e15d8ea to 923ae09 Compare March 3, 2020 22:15

nathawes force-pushed the indentation branch 2 times, most recently from bf5bafe to 9fce6ac Compare March 5, 2020 00:43

rintaro reviewed Mar 5, 2020

View reviewed changes

nathawes force-pushed the indentation branch from 9fce6ac to a1b5027 Compare March 5, 2020 18:05

benlangmuir reviewed Mar 5, 2020

View reviewed changes

Nathan Hawes added 2 commits March 10, 2020 21:04

nathawes force-pushed the indentation branch from 64298de to aedafe9 Compare March 11, 2020 04:04

benlangmuir reviewed Mar 11, 2020

View reviewed changes

nathawes mentioned this pull request Mar 11, 2020

[test] Update PD5LoadingTests.swift for an upcoming parser fix that suppressed a diagnostic it was checking for. swiftlang/swift-package-manager#2654

Merged

benlangmuir approved these changes Mar 12, 2020

View reviewed changes

rintaro approved these changes Mar 12, 2020

View reviewed changes

nathawes merged commit fcf95ea into swiftlang:master Mar 13, 2020

nathawes mentioned this pull request May 22, 2020

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

Merged

[SourceKit/CodeFormat] Re-work and improve the indentation implementation #30187

[SourceKit/CodeFormat] Re-work and improve the indentation implementation #30187

Uh oh!

Conversation

nathawes commented Mar 3, 2020

Uh oh!

nathawes commented Mar 3, 2020

Uh oh!

swift-ci commented Mar 3, 2020

Uh oh!

theblixguy commented Mar 3, 2020

Uh oh!

nathawes commented Mar 3, 2020

Uh oh!

swift-ci commented Mar 3, 2020

Uh oh!

swift-ci commented Mar 3, 2020

Uh oh!

nathawes commented Mar 3, 2020

Uh oh!

swift-ci commented Mar 3, 2020

Uh oh!

swift-ci commented Mar 3, 2020

Uh oh!

nathawes commented Mar 5, 2020

Uh oh!

swift-ci commented Mar 5, 2020

Uh oh!

swift-ci commented Mar 5, 2020

Uh oh!

rintaro commented Mar 5, 2020

Uh oh!

rintaro Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Mar 5, 2020

Uh oh!

swift-ci commented Mar 5, 2020

Uh oh!

nathawes commented Mar 5, 2020

Uh oh!

swift-ci commented Mar 5, 2020

Uh oh!

swift-ci commented Mar 5, 2020

Uh oh!

nathawes commented Mar 5, 2020

Uh oh!

nathawes commented Mar 5, 2020

Uh oh!

benlangmuir 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!

Uh oh!

nathawes commented Mar 11, 2020

Uh oh!

nathawes commented Mar 11, 2020

Uh oh!

rintaro commented Mar 11, 2020

Uh oh!

Uh oh!

benlangmuir Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benlangmuir Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

nathawes Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathawes Mar 12, 2020 •

edited

Loading