Respect indentation in the line for sub-tree in BasicFormat

[gibachan]

The pull request at #1791 emphasizes the need to indent sub-tree nodes while formatting.

I attempted to address another sub-tree indentation based on the comment in #1791 (comment).

[ahoppen]

Thanks a lot. I’ve got a couple comments/questions inline. It looks really good overall though.

[ahoppen]

I know that you just copied it, but could you fix this comment while you’re at it?

Suggested change

	/// In contrast to `indentation`, this does not walk to previous tokens to
	/// In contrast to `indentationOfLine`, this does not walk to previous tokens to

[gibachan]

Fixed 👍 b1b0e83

[ahoppen]

I’m not sure if my formulation of the comment is more helpful than yours but I think some information like the following would have helped me.

// If we are formatting a subtree and the line that the initial token occurs on is indented, use that line indentation for the first token in the subtree to format. For example, when formatting only the code block in the following, then the opening `{` should be indented by four spaces.
// ```
//     func test() {
//         print(1)
//     }
// ```

[gibachan]

Nice comment! Thank you 88fadc7

[ahoppen]

Why is this needed? I would expect that we should be able to always do

- tokens = tokens.map({ NoNewlinesFormat(viewMode: .all).visit($0) })
+ tokens = tokens.map({ NoNewlinesFormat(viewMode: .all).visit($0.detached) })

[gibachan]

I seem to have misunderstood, and now I agree with you.

However, this will cause a test to fail for this test case.

https://github.com/apple/swift-syntax/blob/e7068e8eb76542591a189f703f8b62881a520588/Tests/SwiftParserTest/AttributeTests.swift#L99-L113

The error message appears in this line of the DiagnosticSpec, stating that it outputs :false instead of : false.
https://github.com/apple/swift-syntax/blob/e7068e8eb76542591a189f703f8b62881a520588/Tests/SwiftParserTest/AttributeTests.swift#L106

The whitespace between : and false should be inserted by BasicFormat, and I believe the code responsible for it is here.

But requiresWhitespace(between:and:) returns false because the detached : token will not find false as its next token. As a result, the whitespace is not inserted as expected in the test case.

I am currently investigating this issue.

[ahoppen]

Ah, I see what’s going wrong g here: We definitely shouldn’t detach because otherwise we don’t know that : is followed by false in NoNewlinesFormat and thus can’t decide whether to add a space after it.

The issue is that inheriting the line’s indentation only makes sense if the formatted text should contain newlines. NoNewlinesFormat is a bit of an oddity here because it doesn’t do that. What I would do is to add the following property to BasicFormat and only perform your logic if inferInitialTokenIndentaiton is true. NoNewlinesFormat can then override it and set it to false.

/// If we are formatting a subtree and the line that the initial token occurs on is indented,
/// use that line indentation for the first token in the subtree to format.
/// 
/// For example, when formatting only the code block in the following,
/// then the opening `{` should be indented by four spaces.
/// ```
///     func test() {
///         print(1)
///     }
/// ```
open var inferInitialTokenIndentaiton: Bool { true }

And then, I think we also don’t need the inline comment in visit(_: TokenSyntax) anymore because the behavior is documented on inferInitialTokenIndentaiton.

[gibachan]

Wow, thank you for your great support!
I fixed it and squashed those commits into cc9f449

[ahoppen]

This looks good. Thanks 👍🏽

[ahoppen]

@swift-ci Please test

[gibachan]

Oh, I forgot to run "./format.py" for my changes, sorry. I fixed it! 1622dc6

[ahoppen]

@swift-ci Please test

[ahoppen]

When building swift-syntax for the compiler, we are building it using CMake and you need to add this file to Sources/SwiftBasicFormat/CMakeLists.txt.

[gibachan]

Added this file to Sources/SwiftBasicFormat/CMakeLists.txt. 73a21a8

[ahoppen]

Looking at this file again, is there a reason this extension needs to be public? If not, I’d prefer to keep it internal so we don’t increase our API surface area further.

[gibachan]

I did it to make var indentationOfLine: Trivia accessible from ParseDiagnosticsGenerator.swift which is in SwiftParserDiagnostics package.

I made this extension internal in 30bfccd. Please let me know if this was not your intention 😄

[ahoppen]

Ah, I meant if we could make indentationOfLine internal but I forgot that we use it from ParseDiagnosticsGenerator. Makes sense to be public 👍🏽

[ahoppen]

@swift-ci Please test