Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protoc: trailing comment on last line of file (with no subsequent newline) ignored #12081

Closed
jhump opened this issue Feb 28, 2023 · 0 comments
Closed

Comments

@jhump
Copy link
Contributor

jhump commented Feb 28, 2023

Apparently I introduced a bug in #10660. 🤦

If the last line in the file has a trailing comment and no trailing newline, that comment is effectively ignored (it gets interpreted as a detached comment before the EOF instead of a trailing comment on the preceding token).

Here's a simple example:

syntax = "proto3";
package foo.bar; // trailing comment on package.

Importantly, if the file above has no trailing newline (i.e. the last byte in the file is the period of the trailing comment), then that comment does not make it into source code info. One would expect it to be a trailing comment for the package statement.

@jhump jhump added the untriaged auto added to all issues by default when created. label Feb 28, 2023
copybara-service bot pushed a commit that referenced this issue Feb 28, 2023
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
copybara-service bot pushed a commit that referenced this issue Feb 28, 2023
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
copybara-service bot pushed a commit that referenced this issue Feb 28, 2023
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
copybara-service bot pushed a commit that referenced this issue Feb 28, 2023
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
deannagarcia pushed a commit to deannagarcia/protobuf that referenced this issue Feb 28, 2023
…ocolbuffers#12082)

Fixes protocolbuffers#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 protocolbuffers#12082

COPYBARA_INTEGRATE_REVIEW=protocolbuffers#12082 from jhump:jh/fix-trailing-comment-attribution 767e41c
PiperOrigin-RevId: 513046172
@googleberg googleberg removed the untriaged auto added to all issues by default when created. label Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants