Fix trailing comment attribution when file has no final newline (#12082)

Fixes #12081.

The issue was the call to `MaybeDetachComment`: the conditional assumed that there was a next token, which was on the same line as the previous one, making attribution unclear. However, if there is no next token, we should not detach.

The actual fix is a one-liner. The rest of this PR is updates to the tests to verify this behavior under a handful of scenarios.

Closes #12082

COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/12082 from jhump:jh/fix-trailing-comment-attribution 767e41cb05
PiperOrigin-RevId: 513046172
pull/12072/head
Joshua Humphries 2 years ago committed by Copybara-Service
parent 723bd4c3c1
commit 2b462ae58d
  1. 3
      src/google/protobuf/io/tokenizer.cc
  2. 48
      src/google/protobuf/io/tokenizer_unittest.cc

@ -942,7 +942,8 @@ bool Tokenizer::NextWithComments(std::string* prev_trailing_comments,
// makes no sense to attach a comment to the following token. // makes no sense to attach a comment to the following token.
collector.Flush(); collector.Flush();
} }
if (prev_line == line_ || trailing_comment_end_line == line_) { if (result &&
(prev_line == line_ || trailing_comment_end_line == line_)) {
// When previous token and this one are on the same line, or // When previous token and this one are on the same line, or
// even if a multi-line trailing comment ends on the same line // even if a multi-line trailing comment ends on the same line
// as this token, it's unclear to what token the comment // as this token, it's unclear to what token the comment

@ -643,6 +643,7 @@ inline std::ostream& operator<<(std::ostream& out,
return out << absl::CEscape(test_case.input); return out << absl::CEscape(test_case.input);
} }
// clang-format off
DocCommentCase kDocCommentCases[] = { DocCommentCase kDocCommentCases[] = {
{"prev next", {"prev next",
@ -650,6 +651,18 @@ DocCommentCase kDocCommentCases[] = {
{}, {},
""}, ""},
{"prev // no next token\n",
" no next token\n",
{},
""},
{"prev // no next token and no trailing newline",
" no next token and no trailing newline",
{},
""},
{"prev /* detached */ next", {"prev /* detached */ next",
"", "",
@ -780,7 +793,7 @@ DocCommentCase kDocCommentCases[] = {
prev /* a single block comment prev /* a single block comment
that spans multiple lines that spans multiple lines
is detached if it ends is detached if it ends
on the same line as next */ next" on the same line as next */ next
)pb", )pb",
"", "",
@ -791,7 +804,7 @@ DocCommentCase kDocCommentCases[] = {
""}, ""},
{R"pb( {R"pb(
prev /* trailing */ /* leading */ next" prev /* trailing */ /* leading */ next
)pb", )pb",
" trailing ", " trailing ",
@ -802,13 +815,26 @@ DocCommentCase kDocCommentCases[] = {
prev /* multi-line prev /* multi-line
trailing */ /* an oddly trailing */ /* an oddly
placed detached */ /* an oddly placed detached */ /* an oddly
placed leading */ next" placed leading */ next
)pb", )pb",
" multi-line\ntrailing ", " multi-line\ntrailing ",
{" an oddly\nplaced detached "}, {" an oddly\nplaced detached "},
" an oddly\nplaced leading "}, " an oddly\nplaced leading "},
{R"pb(
prev // trailing with newline
// detached
/* another detached */
// leading but no next token to attach it to
)pb",
" trailing with newline\n",
{" detached\n", " another detached ",
" leading but no next token to attach it to\n"},
""},
}; };
// clang-format on
TEST_2D(TokenizerTest, DocComments, kDocCommentCases, kBlockSizes) { TEST_2D(TokenizerTest, DocComments, kDocCommentCases, kBlockSizes) {
// Set up the tokenizer. // Set up the tokenizer.
@ -822,8 +848,8 @@ TEST_2D(TokenizerTest, DocComments, kDocCommentCases, kBlockSizes) {
kDocCommentCases_case.input.size(), kBlockSizes_case); kDocCommentCases_case.input.size(), kBlockSizes_case);
Tokenizer tokenizer2(&input2, &error_collector); Tokenizer tokenizer2(&input2, &error_collector);
tokenizer.Next(); EXPECT_TRUE(tokenizer.Next());
tokenizer2.Next(); EXPECT_TRUE(tokenizer2.Next());
EXPECT_EQ("prev", tokenizer.current().text); EXPECT_EQ("prev", tokenizer.current().text);
EXPECT_EQ("prev", tokenizer2.current().text); EXPECT_EQ("prev", tokenizer2.current().text);
@ -831,11 +857,13 @@ TEST_2D(TokenizerTest, DocComments, kDocCommentCases, kBlockSizes) {
std::string prev_trailing_comments; std::string prev_trailing_comments;
std::vector<std::string> detached_comments; std::vector<std::string> detached_comments;
std::string next_leading_comments; std::string next_leading_comments;
tokenizer.NextWithComments(&prev_trailing_comments, &detached_comments, bool has_next = tokenizer.NextWithComments(
&next_leading_comments); &prev_trailing_comments, &detached_comments, &next_leading_comments);
tokenizer2.NextWithComments(NULL, NULL, NULL); EXPECT_EQ(has_next, tokenizer2.NextWithComments(nullptr, nullptr, nullptr));
EXPECT_EQ("next", tokenizer.current().text); if (has_next) {
EXPECT_EQ("next", tokenizer2.current().text); EXPECT_EQ("next", tokenizer.current().text);
EXPECT_EQ("next", tokenizer2.current().text);
}
EXPECT_EQ(kDocCommentCases_case.prev_trailing_comments, EXPECT_EQ(kDocCommentCases_case.prev_trailing_comments,
prev_trailing_comments); prev_trailing_comments);

Loading…
Cancel
Save