Skip to content

Commit

Permalink
Fix trailing comment attribution when file has no final newline (#12082)
Browse files Browse the repository at this point in the history
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=#12082 from jhump:jh/fix-trailing-comment-attribution 767e41c
FUTURE_COPYBARA_INTEGRATE_REVIEW=#12082 from jhump:jh/fix-trailing-comment-attribution 767e41c
PiperOrigin-RevId: 513017635
  • Loading branch information
deannagarcia authored and copybara-github committed Feb 28, 2023
1 parent 7cc46f5 commit 15e2f3d
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 17 deletions.
3 changes: 2 additions & 1 deletion src/google/protobuf/io/tokenizer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -942,7 +942,8 @@ bool Tokenizer::NextWithComments(std::string* prev_trailing_comments,
// makes no sense to attach a comment to the following token.
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
// even if a multi-line trailing comment ends on the same line
// as this token, it's unclear to what token the comment
Expand Down
62 changes: 46 additions & 16 deletions src/google/protobuf/io/tokenizer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,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",

"",
Expand Down Expand Up @@ -777,10 +789,11 @@ DocCommentCase kDocCommentCases[] = {
" will be handled "},

{R"pb(
prev /* a single block comment
that spans multiple lines
is detached if it ends
on the same line as next */ next"
prev /* a single block comment
that spans multiple lines
is detached if it ends
on the same line as next */
next
)pb",

"",
Expand All @@ -791,23 +804,38 @@ DocCommentCase kDocCommentCases[] = {
""},

{R"pb(
prev /* trailing */ /* leading */ next"
prev /* trailing */ /* leading */ next
)pb",

" trailing ",
{},
" leading "},

{R"pb(
prev /* multi-line
trailing */ /* an oddly
placed detached */ /* an oddly
placed leading */ next"
prev /* multi-line
trailing */
/* an oddly
placed detached */
/* an oddly
placed leading */
next
)pb",

" multi-line\ntrailing ",
{" an oddly\nplaced detached "},
" 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"},
""},
};

TEST_2D(TokenizerTest, DocComments, kDocCommentCases, kBlockSizes) {
Expand All @@ -822,20 +850,22 @@ TEST_2D(TokenizerTest, DocComments, kDocCommentCases, kBlockSizes) {
kDocCommentCases_case.input.size(), kBlockSizes_case);
Tokenizer tokenizer2(&input2, &error_collector);

tokenizer.Next();
tokenizer2.Next();
EXPECT_TRUE(tokenizer.Next());
EXPECT_TRUE(tokenizer2.Next());

EXPECT_EQ("prev", tokenizer.current().text);
EXPECT_EQ("prev", tokenizer2.current().text);

std::string prev_trailing_comments;
std::vector<std::string> detached_comments;
std::string next_leading_comments;
tokenizer.NextWithComments(&prev_trailing_comments, &detached_comments,
&next_leading_comments);
tokenizer2.NextWithComments(NULL, NULL, NULL);
EXPECT_EQ("next", tokenizer.current().text);
EXPECT_EQ("next", tokenizer2.current().text);
bool has_next = tokenizer.NextWithComments(
&prev_trailing_comments, &detached_comments, &next_leading_comments);
EXPECT_EQ(has_next, tokenizer2.NextWithComments(NULL, NULL, NULL));
if (has_next) {
EXPECT_EQ("next", tokenizer.current().text);
EXPECT_EQ("next", tokenizer2.current().text);
}

EXPECT_EQ(kDocCommentCases_case.prev_trailing_comments,
prev_trailing_comments);
Expand Down

0 comments on commit 15e2f3d

Please sign in to comment.