Override trailing comma for elements in ResultBuilders

[evnik]

Test coverage is added for some of ResultBuilders
I think that should be enough for now and more coverage could be added later in subsequent PRs

[ahoppen]

Thanks, this is a great improvement and exactly the kind of thing SwiftSyntaxBuilder needs 👍

I think adding the trailing comma shouldn’t be done via the create* methods though. I put up a more detailed comment inline.

[ahoppen]

Nitpick: IIUC if you explicitly set a trailing comma on the last element, this will remove it, right? I think that’s surprising. If we used something like the following, the convenience initializer would only add trailing commas but never remove any.

Suggested change

	element.create${element_type.buildable_base_name()}().withTrailingComma(index < lastIndex)
	var node = element.create${element_type.buildable_base_name()}()
	if index < lastIndex {
	node = node.withTrailingComma(true)
	}
	return node

I think it would also be worth adding a test case for this.

[evnik]

Originally I was expecting to unconditionally override trailing commas in the Result Builders. create*(isLastElement: Bool?) methods required to apply commas in both Result Builders and Element List types initializers. Now with the separate withTrailingComma method it's possible to limit that impact to Result Builders, which I missed in my recent commit. So I'm going to move that logic again, in that way trailing commas will be preserved in the Element List types initializers (so there is a way to define them manually), but always overridden in the Result Builders. Does it make sense?

[ahoppen]

Thanks for catching this. Yes, I think limiting the automatic trailing commas to result builders makes sense to me 👍 I think you should be able to implement that in the buildFinalResult of the result builder.

[ahoppen]

I think your indentation is wrong here. This should be indented two more spaces because it’s inside the for loop started in line 23. This also applies to line 138.

[ahoppen]

I think I mentioned this before but unconditionally calling withTrailingComma here effectively prevents you from adding a trailing comma using result builders (because the result builder will remove the comma on the last element).

I think we don’t have this problem if this would be something like

Suggested change

	element.create${element_type.buildable_base_name()}().withTrailingComma(index < lastIndex)
	var buildable = element.create${element_type.buildable_base_name()}()
	if index < lastIndex {
	buildable = buildable.withTrailingComma(index < lastIndex)
	}
	return buildable

I think it would also be worth adding a test case that explicitly adds a trailing comma to the last element and assert that the trailing comma is retained through the result builder.

[evnik]

I thought you've agreed that it's fine if trailing commas are preserved in the Element List types initializers, but always overridden in the Result Builders (in both ways added and removed).
I've added a test case checking that explicitly defined trailing commas are preserved by the Element List initializer

[ahoppen]

Sorry, I should have maybe been clearer (or I’m misunderstanding you). What I mean is that I think you should be able to generate e.g. an array literal with trailing comma like [1,2,] using the result builder syntax using something like the following (pseudo-code written in GitHub, probably doesn’t compile)

ArrayExpr {
  ArrayElement(IntegerLiteralExpr(1))
  ArrayElement(IntegerLiteralExpr(2), trailingComma: .comma)
}

So basically my thinking is that the result builder should only add commas but never remove any. My thinking is that some people prefer to have trailing commas in their array/dictionary literals and I don’t think we should deprive them of that ability.

[evnik]

I don't see much value in it because Result Builders don't allow explicit trivias on non-last trailing commas anyway. I think list type initializer should be used in the scenario you described. But since you insist, I've updated the PR with that and I've added a test for it as well.

[ahoppen]

I’m totally open to debate here. I would have just been surprised if the example I put above dropped the trailing comma. Do you disagree? Or do you have an example where the behavior you implemented before would be less surprising than what I’m suggesting?

[evnik]

Trailing comma in a single line array literal (like [1, 2, 3, ]) doesn't make much sense to me, however I see the point in case of multiline:

[
  1,
  2,
  3,
]

However current implementation won't allow you to generate code like the second example with result builder, because it'll overwrite trailing commas removing explicit new line trivias.
It might be surprising, but I'd say that it works as designed: you either use convenience of result builder with no way to customize the output, or you use more verbose Element List initializer and has full control on the output.
Finally, I'd like to merge this PR ASAP and then the logic could be improved as a separate change

[ahoppen]

OK, now I understand what you meant by “don't allow explicit trivias on non-last trailing commas”. With that in mind, I don’t have strong opinions in either direction anymore.

I’ve merged the PR as-is for now. If you want, I won’t object to reverting the behavior back to what you originally implemented. 😉

[ahoppen]

@swift-ci Please test