Exclude CR from comment tokens to fix an off-by-one Location#end_line on CRLF files - #3069
Open
tufusa wants to merge 3 commits into
Open
Exclude CR from comment tokens to fix an off-by-one Location#end_line on CRLF files#3069tufusa wants to merge 3 commits into
tufusa wants to merge 3 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Currently, the pattern for comment tokens is
"#" (. \ [\x00\uFFFD])*:rbs/src/lexer.re
Lines 61 to 66 in d89e4ad
This works correctly in LF environments, but the
.pattern excludes only LF, not CR; therefore\ris included at the end of the comment token in CRLF environments such as Windows.As a result, the value of
Location#end_linefor comments ends up being one line greater than actually is. This occurs because whileBuffer#rangesrbs/lib/rbs/buffer.rb
Lines 42 to 49 in d89e4ad
correctly removes
\r\n(L44),Buffer#pos_to_locwhich is passed a value forposthat is one greater than the actual valuerbs/lib/rbs/buffer.rb
Lines 56 to 59 in d89e4ad
incorrectly finds the next line.
This will cause incorrect behavior in tools built on
RBS::Parser.lex, for example,RBS/Layout/ExtraSpacingcop in rubocop-on-rbs (ksss/rubocop-on-rbs#152; in fact, I discovered this issue while investigating the false detection of this cop).This token behavior itself existed prior to RBS 4, but it looks to become apparent because, starting with RBS 4,
String#chompis called before the character count is performed withinBuffer#ranges.Reproduction
Changes
\rto the pattern's subtraction group:"#" (. \ [\x00\r\uFFFD])*. This causes the comment token to stop reading as soon as\rappears.RBS::ParserTest#test__lex_crlfnext totest__lex. This verifies that\ris treated as a single independentTRIVIAtoken as usual, even in the case of comments.Thank you!