Don't allow CR to act as newline when it is followed by LF

[DanTup]

This fixes an error I encountered (dart-lang/tools#1797) when scanning a zero-length match when between a CR and LF. It removes some special handling of \r's that seemed to complicate things. Now, \r is just ignored and only \n is treated as the end of a line.

Note: This partially reverts some changes made in 27d20a3 that seemed to be the cause of the error - however the changes added in that change still pass with my changes, and I added some additional tests.

Fixes dart-lang/tools#1797

[DanTup]

@nex3 interested in your thoughts on this since you seem to have committed here recently and made the commit noted above that this changes.

[DanTup]

Looking at 27d20a3 again, I think the intention may have been to also handle a \r that is not followed by \n as a new line:

https://github.com/dart-lang/string_scanner/blob/2139417ffcd0392bde3ba9bc83ee13eaa5fbed01/lib/src/line_scanner.dart#L94

So it's possible this change is not completely consistent (and more tests are needed). However, I'm unsure about this behaviour - should a bare \r really start a new line? I feel like it should not, and some testing from a terminal on Windows does not treat it like a new line:

(note: PowerShell uses backticks for escape characters)

PS D:\> echo "abc`ndef"
abc
def
PS D:\> echo "abc`r`ndef"
abc
def
PS D:\> echo "abc`rdef"
def

We should agree what the behaviour should be here and add additional tests.

[devoncarew]

should a bare \r really start a new line

I think practically only \r\n and \n should be treated as line terminators (historically - and not something to take into account here - I remember macos before OS X used \r as a terminator).

I'm less familiar w/ this package specifically but am comforted by existing + new tests passing. If there are more tests you feel we should have, please add them :)

[natebosch]

This line has likely been a bug since this landed. It was introduced during code review.

[natebosch]

comforted by existing + new tests passing

I'm less comforted to find that we might be missing tests covering that behavior, since the behavior changed during code review and no one noticed.
Of course it's also a good sign that it's been behaving differently than was originally intended for almost a decade and no one noticed, so it's probably not an edge of the behavior we're really relying on.

I will run this against a larger set of tests and see if anything crops up.

Edit: Although I don't know if it adds much confidence, since I won't be running any windows tests.

[nex3]

The intention was that \r\n would be considered a newline, and that \r followed by anything other than \n would be considered a newline. While modern OSes don't use \r as a stand-alone newline, it is supported by modern software, including for example the CSS specification. I think any changes here should preserve that behavior.

[DanTup]

I have pushed changes to handle \r that is not followed by \n as a newline. While I'm not sure if all users would expect this behaviour, it appears to be the original intention (and mostly supported).

The changes were a little complex when moving the position backwards, because the original code searched for the last newline when computing the new column, but if that newline was two characters, the column would be out by one (because it used the offset of the \r and therefore counted the \n as the first character of the line).

I've added many more tests and everything is passing, but please review carefully and let me know if you think there are additional cases we need tests for.

[natebosch]

This looks great. Thanks for following through with the solution that handles solo /r

[DanTup]

I think I found a bug here, going to add some new tests and fix if necessary.

[DanTup]

Pushed some additional tests + fixes for:

• moving backwards with position from between \r\n would incorrectly handle the \r\n because _newlinesIn always assumed the string it was checking ended at position (but it doesn't when moving backwards), so now the end position is passed in.
• moving forwards from a non-zero character would compute the wrong character because it used the offset of the last newline (match.end) without accounting for the search string only being a subset of the full string (this was an existing bug so I also added to the changelog)