go/parser: incorrect comment end position with CRLF line endings

[stamblerre]

Full repro case here: https://play.golang.org/p/JwZQ7Zjaypp.

go/parser produces incorrect position information for multi-line comments in files with CRLF line endings. This caused #41057 in gopls.

[dmitshur]

/cc @griesemer per owners.

[findleyr]

This is a bit tricky to fix. The comment literal has '\r' stripped (from scanner.scanComment: "matching the pre-existing behavior of the scanner"), and this literal is used to compute the comment end position on the AST. In other words the comment text has normalized line endings, but positions do not, so the AST doesn't have enough information to accurately compute End for the ast.Comment.

This could of course be fixed by simply not stripping the \r from comments, but that should probably be considered a breaking change. Alternatively, I suppose we'd have to add an additional piece of information to ast.Comment -- perhaps EndSlash -- to capture the missing information. In order to populate this information and not change the Scanner, we'd have to use a similar workaround to gopls (count lines and reverse-engineer the token.Pos).

[gopherbot]

Change https://golang.org/cl/255657 mentions this issue: go/ast,go/parser: add ast.Comment.EndSlash to fix End calculation

[findleyr]

After jotting down the above CL, I realized that solving this bug may not be possible.

We cannot change comment text to include carriage returns. That ship has sailed. The CL above instead captures more information: the position of the 'EndSlash' in the original file. On the surface, this avoids a breaking change by simply adding new information to the AST. But of course, this information is not independent: for newline separated text, we must have the invariant that c.Slash + token.Pos(len(c.Text)) == c.EndSlash (for /*-style comments).

What I realized is that there is already a moderate amount of code out there (some in the stdlib, some not) that implicitly relies on the invariant that c.Slash + token.Pos(len(c.Text)) == c.End(). One example of such an implicit reliance would be any code that modifies ast.Comment.Text. That code is not currently doing anything wrong, and could break if we implemented the above fix.

Unfortunately, on first principles I don't think there's a fix out there that avoids this pitfall. We could be slightly more 'independent' by, for example, storing the number of carriage returns in the comment text, rather than the exact position of the ending slash. But that still breaks any rewrite that changes this number.

So this is probably not a bug that can be fixed, and code that needs exact end positions of the comment node must recompute it using the original file, as gopls does.

Unless anyone has other ideas, I'll send a CL updating the documentation for ast.Comment to make this limitation clearer, and close this issue as unfixable.

[gopherbot]

Change https://golang.org/cl/270938 mentions this issue: go/ast: document invalid comment end positions with CRLF line endings