Fix `CodeGenerationFormat` to not add extra indentation, leave trailing spaces when adding newlines, or indent empty lines #2833

roopekv · 2024-08-31T17:44:26Z

This fixes multiple issues with CodeGenerationFormat. Files were re-generated with each commit, so the differences in formatting caused by each change to CodeGenerationFormat should be easy to review.

Fix CodeGenerationFormat to not add extra indentation

Indentation used to be all over the place when formatting children to be separated by newline, this should fix it.

Fix CodeGenerationFormat to not leave trailing spaces when adding newlines

Formatting children to be separated by newline used to leave spaces after each line's comma, this should trim them.

Fix CodeGenerationFormat to not indent empty lines

ensuringTwoLeadingNewlines used to add indented newlines, resulting in empty lines being indented. Not anymore.

ahoppen

Great changes. These have always bothered me but never enough that I set time aside to fix them. So, thank you for doing it 🙏🏽

First two commits look good, I have one comment about the indentation change.

ahoppen · 2024-09-04T19:56:23Z

CodeGeneration/Sources/Utils/CodeGenerationFormat.swift

-      self.rewrite($0.cast(SyntaxType.self)).cast(SyntaxType.self)
+    var formattedChildren = children.map { child in
+      var child = child.cast(SyntaxType.self)
+      child.leadingTrivia = Trivia(pieces: child.leadingTrivia.drop(while: \.isWhitespace))


If a child would span multiple lines, this only removes and re-adds whitespace for the first line and the subsequent lines would still be off. Can you think of a solution that immediately assigns the right indentation to the child? How many whitespaces (compared to indentedNewline) are we stripping here? Are we just off one indentation level? If so, could we fix that by arranging the calls to increaseIndentationLevel and decreaseIndentationLevel correctly?

Thanks for the notice. Calling increaseIndentationLevel() before rewriting the children seems to fix the issue of children that span multiple lines not getting indented ~~probably~~ properly.

Before:

swift-syntax/Sources/SwiftSyntaxBuilder/generated/BuildableNodes.swift

Lines 53 to 55 in 9b51a7b

body: bodyBuilder().map {

CodeBlockSyntax(statements: $0)

},

After:

swift-syntax/Sources/SwiftSyntaxBuilder/generated/BuildableNodes.swift

Lines 53 to 55 in b63eaaf

body: bodyBuilder().map {

CodeBlockSyntax(statements: $0)

},

Regarding the removal and re-adding of leading whitespace, it was to prevent adding newlines and indentation to children that were already separated by newlines. I changed it so that indented newlines are only added to children that don't already start on newlines instead of removing the whitespace and adding it back to all children, although it should work either way.

Both of these changes are included in:
Fix CodeGenerationFormat to correctly indent certain children that span multiple lines

However...
... I found another issue while inspecting the generated code:

swift-syntax/Sources/SwiftSyntax/generated/syntaxNodes/SyntaxNodesQRS.swift

Lines 854 to 861 in b63eaaf

return Syntax(self)

.replacingChild(

at: 1,

with: collection,

rawNodeArena: arena,

allocationArena: arena

)

.cast(SequenceExprSyntax.self)

The arguments and the right parenthesis of .replacingChild(...) are missing a level of indentation. Prior to this pull request this particular case looked OK, but I believe it was a false positive, as most of the code formatted by CodeGenerationFormat was over-indented.

I had fixed the over-indentation by overriding requiresIndent(_:),

swift-syntax/CodeGeneration/Sources/Utils/CodeGenerationFormat.swift

Lines 107 to 124 in b63eaaf

public override func requiresIndent(_ node: some SyntaxProtocol) -> Bool {

switch node.kind {

case .arrayElementList, .dictionaryElementList, .functionParameterList, .labeledExprList:

let indentManually = node.children(viewMode: .sourceAccurate).count > maxElementsOnSameLine

if indentManually {

return false

}

let startsOnNewline =

node.leadingTrivia.contains(where: \.isNewline)

|| node.previousToken(viewMode: .sourceAccurate)?.trailingTrivia.contains(where: \.isNewline) ?? false

if !startsOnNewline {

return false

}

default:

break

}

return super.requiresIndent(node)

}

which revealed that .replacingChild(...) is under-indented. The issue might have to do with the fact that the member function call starts on a newline, as it seems to be the only place where this issue manifests. Perhaps it has something to do with how BasicFormat handles the indentation for this case? Any insight on this issue @ahoppen ? Either way, I'll look into this issue during the weekend and try to resolve it.

Thanks for the detailed analysis.

My guess would be that this is because the function call is modeled as follows (simplified):

callee: Syntax(self).replacingChild

arguments: (at: 1 ….)

Thus, the call’s indentation is anchored at the base of the callee, which is the indentation level of Syntax here, not the indentation level of replacingChild. I don’t have an immediate suggestion of how to fix this but didn’t think about it a lot either. Maybe you can come up with something. If my guess is correct, this would be an issue in BasicFormat, not just CodeGenerationFormat.

Happy to take this PR as-is and fix that in a follow-up PR. That helps keep the reviews easier if the PRs are narrow in scope.

Thanks for the detailed analysis.

My guess would be that this is because the function call is modeled as follows (simplified):

callee: Syntax(self).replacingChild

arguments: (at: 1 ….)

Thus, the call’s indentation is anchored at the base of the callee, which is the indentation level of Syntax here, not the indentation level of replacingChild. I don’t have an immediate suggestion of how to fix this but didn’t think about it a lot either. Maybe you can come up with something. If my guess is correct, this would be an issue in BasicFormat, not just CodeGenerationFormat.

Alright, thanks for the insight 👍

Happy to take this PR as-is and fix that in a follow-up PR. That helps keep the reviews easier if the PRs are narrow in scope.

Sounds good to me. Shall I fix the merge conflicts now?

ahoppen

Looks good to me. Thanks! Also good to see that the latest commit fixed some more formatting issues.

Could you rebase your changes onto main to fix the merge conflict?

roopekv · 2024-09-06T00:28:42Z

@ahoppen is this good to go now?

ahoppen · 2024-09-06T00:53:19Z

Yes, let’s 🚢it

ahoppen · 2024-09-06T00:53:21Z

@swift-ci Please test

ahoppen · 2024-09-06T14:31:51Z

The macOS test failures are unrelated to your changes. I’m looking into them.

ahoppen · 2024-09-06T20:46:26Z

Sorry, there’s a merge conflict now. Could you rebase?

roopekv · 2024-09-06T20:48:52Z

Sure, just a moment.

…ng spaces when adding newlines, or indent empty lines

roopekv · 2024-09-06T21:02:43Z

Done

ahoppen · 2024-09-06T21:20:24Z

@swift-ci Please test

ahoppen · 2024-09-07T05:54:50Z

@swift-ci Please test

ahoppen · 2024-09-08T07:17:13Z

@swift-ci Please test macOS

ahoppen · 2024-09-09T19:04:44Z

@swift-ci Please test macOS

ahoppen · 2024-09-10T20:46:09Z

@swift-ci Please test macOS

roopekv requested review from ahoppen and bnbarham as code owners August 31, 2024 17:44

ahoppen reviewed Sep 4, 2024

View reviewed changes

ahoppen approved these changes Sep 5, 2024

View reviewed changes

roopekv force-pushed the fix-code-gen-list-format-indentation branch from b63eaaf to df8f459 Compare September 6, 2024 00:27

ahoppen enabled auto-merge September 6, 2024 00:53

Fix CodeGenerationFormat to not add extra indentation, leave traili…

29616eb

…ng spaces when adding newlines, or indent empty lines

auto-merge was automatically disabled September 6, 2024 21:02
Head branch was pushed to by a user without write access

roopekv force-pushed the fix-code-gen-list-format-indentation branch from df8f459 to 29616eb Compare September 6, 2024 21:02

ahoppen enabled auto-merge September 6, 2024 21:20

ahoppen merged commit 801b535 into swiftlang:main Sep 11, 2024
3 checks passed

roopekv deleted the fix-code-gen-list-format-indentation branch September 11, 2024 15:23

roopekv mentioned this pull request Sep 19, 2024

Fix CodeGenerationFormat to correctly indent LabeledExprListSyntax in method calls that start on a new line #2851

Merged

	return Syntax(self)
	.replacingChild(
	at: 1,
	with: collection,
	rawNodeArena: arena,
	allocationArena: arena
	)
	.cast(SequenceExprSyntax.self)

	public override func requiresIndent(_ node: some SyntaxProtocol) -> Bool {
	switch node.kind {
	case .arrayElementList, .dictionaryElementList, .functionParameterList, .labeledExprList:
	let indentManually = node.children(viewMode: .sourceAccurate).count > maxElementsOnSameLine
	if indentManually {
	return false
	}
	let startsOnNewline =
	node.leadingTrivia.contains(where: \.isNewline)
	\|\| node.previousToken(viewMode: .sourceAccurate)?.trailingTrivia.contains(where: \.isNewline) ?? false
	if !startsOnNewline {
	return false
	}
	default:
	break
	}
	return super.requiresIndent(node)
	}

Fix CodeGenerationFormat to not add extra indentation, leave trailing spaces when adding newlines, or indent empty lines #2833

Fix CodeGenerationFormat to not add extra indentation, leave trailing spaces when adding newlines, or indent empty lines #2833

Uh oh!

Conversation

roopekv commented Aug 31, 2024

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

roopekv Sep 5, 2024 • 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!

ahoppen Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

ahoppen Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

roopekv Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

roopekv Sep 5, 2024 • 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!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

roopekv commented Sep 6, 2024

Uh oh!

ahoppen commented Sep 6, 2024

Uh oh!

ahoppen commented Sep 6, 2024

Uh oh!

ahoppen commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Sep 6, 2024

Uh oh!

roopekv commented Sep 6, 2024

Uh oh!

roopekv commented Sep 6, 2024

Uh oh!

ahoppen commented Sep 6, 2024

Uh oh!

ahoppen commented Sep 7, 2024

Uh oh!

ahoppen commented Sep 8, 2024

Uh oh!

ahoppen commented Sep 9, 2024

Uh oh!

ahoppen commented Sep 10, 2024

Uh oh!

Uh oh!

Uh oh!

Fix `CodeGenerationFormat` to not add extra indentation, leave trailing spaces when adding newlines, or indent empty lines #2833

Fix `CodeGenerationFormat` to not add extra indentation, leave trailing spaces when adding newlines, or indent empty lines #2833

roopekv Sep 5, 2024 •

edited

Loading

roopekv Sep 5, 2024 •

edited

Loading

ahoppen commented Sep 6, 2024 •

edited

Loading