Add docCommentValue property to Trivia for cleaned comments

[zyadtaha]

This PR continues the work from #2578, addressing the review comments and refining the behavior of Trivia.commentValue.

[ahoppen]

Thanks for addressing the issue, @zyadtaha. I think the PR still needs some polishing regarding indentation handling. I left a few comments inline.

[zyadtaha]

Thanks for the feedback, @ahoppen! I've addressed the comments—let me know if anything else needs attention.

[ahoppen]

Thanks for the adjustments. I left a few more edge cases to consider as comments.

[zyadtaha]

Hey @ahoppen, just wanted to check if there's anything else needed from my side on this PR. Let me know if you have any concerns. Thanks!

[ahoppen]

Thanks for the updates. I think there are still a few more issues that need to be addressed.

[zyadtaha]

Hey @ahoppen, I've pushed my updates. For your last two comments, I've added responses—let me know your feedback when you get a chance. Thanks!

[ahoppen]

I addressed my own review comments in a 2197103, which I pushed to your branch. Could you check if these changes make sense to you?

[zyadtaha]

I addressed my own review comments in a 2197103, which I pushed to your branch. Could you check if these changes make sense to you?

Thank you! I pulled the changes and reviewed the new logic and tests, and everything looks great to me. I really appreciated the detailed doc comments; they helped me understand your approach much better. I also liked how you split the tests into separate functions since it makes things much clearer.

I just had one question: why did you move the commentValue property to a separate file, and similarly, the test cases to a separate file?"

Also, if you have any feedback on my code, communication, or anything I could improve, I’d be very grateful. I want to make sure I’m not wasting your time and that I'm learning as much as possible through this process.

[ahoppen]

Thank you! I pulled the changes and reviewed the new logic and tests, and everything looks great to me. I really appreciated the detailed doc comments; they helped me understand your approach much better. I also liked how you split the tests into separate functions since it makes things much clearer.

Thanks for checking that the changes look good to you.

I just had one question: why did you move the commentValue property to a separate file, and similarly, the test cases to a separate file?"

The implementation of commentValue ended up being non-trivial and since it isn’t all that related to the other Trivia functionality (which is a lot more low-level), I decided to put it into its own file. This way we can also communicate that the extensions defined in that file are only intended for the commentValue property.

Also, if you have any feedback on my code, communication, or anything I could improve, I’d be very grateful. I want to make sure I’m not wasting your time and that I'm learning as much as possible through this process.

You did great work to get things started. I just figured that it was easier to fix the edge cases instead of repeatedly pointing out edge cases and then discovering more after you addressed them. Thinking of all possible edge cases is a big part of developing compiler-related tooling and it’s hard to teach that general way of thinking in pull request comments.

[rintaro]

I'm not sure about concatenating the pieces. The current behavior sounds fragile and not predictable.

TriviaPiece.commentValue: String? (as #1890 requested) would make more sense and we should implement that at least. But it's also debatable what to do with the space after //.

Trivia.commentValue: String? should be IMO a high-level API based on TriviaPiece.commentValue, but the concatenating behavior mixing trivia kinds should be discussed more.

Maybe Trivia.docCommentValue: String? that would only handle .docLineComment/.docBlockComment makes more sense.

[rintaro]

Here we allow

/// Foo Bar /// This is a doc-comment. // @_spi(Testing) public fooBar(){}

resulting "Foo Bar\nThis is a doc-comment\n@_spi(Testing)", but not

/** * Foo Bar * This is a doc-comment. */ // @_spi(Testing) public fooBar(){}

Not sure about this case.

[zyadtaha]

TriviaPiece.commentValue: String? (as #1890 requested) would make more sense and we should implement that at least. But it's also debatable what to do with the space after //.

I appreciate the thoughtful feedback! I agree that TriviaPiece.commentValue: String? (per #1890) is a great starting point. On the space after //, I'm not sure we should preserve it. In my view, something like TriviaPiece.lineComment("// Hello") could simply result in commentValue = "Hello", not " Hello". Keeping the space feels unnecessary with the likely intent of accessing the actual comment content. What’s your take on this?

Trivia.commentValue: String? should be IMO a high-level API based on TriviaPiece.commentValue, but the concatenating behavior mixing trivia kinds should be discussed more.

Maybe Trivia.docCommentValue: String? that would only handle .docLineComment/.docBlockComment makes more sense.

Your suggestion of Trivia.docCommentValue: String? for .docLineComment and .docBlockComment makes sense to me. As Alex noted (#2966 (comment)), it could return nil for multiple block comments or mixed comment types. Similarly, Trivia.commentValue: String? could focus on .lineComment and .blockComment, returning nil in those same cases. What do you think?

[ahoppen]

@Rinataro and I just chatted about this and agreed that it would be best if we focus on docCommentValue first – its semantics are a lot clearer and we don’t need to decide how to concatenate mixtures of line comments and doc line comments etc.

The doc line comment should return the value of the last doc comment that’s inside the Trivia:

• For line comments, this means that it should return the last consecutive block of doc line comments, stopping at empty lines or lines with other trivia types. In all of the cases below, we should return X Y and not A

/// not included /// included /// also included func test(){} /// not included // /// included func test(){} /// not included /* abc */ /// included func test(){} /** not included */ /// included func test(){} /// included // ignored func test(){}

• For block comments, we were less sure whether we should only return the contents of the last doc block comment or whether we should concatenate the different doc block comments. We were leaning towards concatenating them because that’s what the current compiler does, though the concatenation behavior is still up for debate. Again, details to be decided, I think that if there are multiple block comments, we should pretend that the text between */ and /** doesn’t exist.

[zyadtaha]

Hi @ahoppen and @rintaro,
Sorry for the late reply - I was busy with final exams, but summer is finally here and I'm excited to get back to contributing!

I've just pushed my latest changes. The implementation now handles concatenation for doc block comments as the compiler does. If no doc block comments are found, it falls back to returning the last consecutive block of doc line comments. Please let me know if there are any new details!

[ahoppen]

No worries for the time gap. I hope your exams went well.

Thanks for continuing to work on this PR. Things are looking good to me, I just left a few minor comments inline.

[ahoppen]

Nice work. Just came back to this and this is looking really good. One super minor comment and one open question about how the API should behave.

Could you also format your changes as described in https://github.com/swiftlang/swift-syntax/blob/main/CONTRIBUTING.md#formatting?

[ahoppen]

This could now be of type [String], right? We don’t ever add substrings to this array if I understand correctly.

[ahoppen]

Do we need the variable hasBlockComment? If I understand correctly, we never set it to false and if it’s false then we always have comments == [], which means that the else-block is equivalent to this if branch.

[ahoppen]

Sorry in case we discussed this before and I just don’t remember but my gut feeling is that the result of this should be def, not abc\ndef. @rintaro What do you think?

[rintaro]

I vote for def, i.e. only use the last block doc-comment.
FWIW, the current SourceKit seems to concat them as long as there's no blank lines between them, but that's also true for this case.

/** not included */ /// included