Skip to content

Improve CRLF support in lexer - #1414

Merged
jiribenes merged 7 commits into
mainfrom
jiribenes/fix/lexer-CRLF
Jul 31, 2026
Merged

Improve CRLF support in lexer#1414
jiribenes merged 7 commits into
mainfrom
jiribenes/fix/lexer-CRLF

Conversation

@jiribenes

@jiribenes jiribenes commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This change should help with resolving #1183, though I can't confirm it's solved completely while being on a Mac.

I also changed the Windows smoke tests to run the case studies -- hopefully this means a slightly bigger subset of the language being checked on Windows CI :)

@jiribenes

Copy link
Copy Markdown
Contributor Author

private def advance(): Char =
val ret = currentChar
currentChar = nextChar
nextChar = if charIterator.hasNext then charIterator.next() else '\u0000'
position = position.advance(ret == '\n')
ret

is likely also a source of inconsistencies (if it doesn't "magically" work out somehow)

@jiribenes
jiribenes marked this pull request as ready for review July 28, 2026 15:23
Comment on lines 715 to 719
private def shebang(): TokenKind =
advanceWhile { (curr, _) => curr != '\n' }
advanceWhile { (curr, _) => curr != '\n' && curr != '\r' }
val command = getCurrentSlice(skipAfterStart = 2) // Remove `#!`
TokenKind.Shebang(command)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I don't expect too many CRLF enjoyers using shebangs, we should have a consistent "until the end of the line" predicate, hence this funny-looking change.

@martin-ilgner

Copy link
Copy Markdown
Contributor

And there are still some tests failing that probably should work, like effekt.ParserTests.Correct multiline span

@jiribenes

This comment was marked as outdated.

@jiribenes

Copy link
Copy Markdown
Contributor Author

@martin-ilgner if you have the time in the future, could you please try to run the tests again on this branch with an additional patch that changes:

def sourceAndPositions: (Source, Seq[Int]) = {
val lines = content.stripMargin.split("\n").toBuffer
val positions = scala.collection.mutable.ArrayBuffer[Int]()

into:

def sourceAndPositions: (Source, Seq[Int]) = {
  val normalized = content.stripMargin.replace("\r\n", "\n")
  val lines = normalized.split("\n", -1).toBuffer
  val positions = = scala.collection.mutable.ArrayBuffer[Int]() 
  ...

I looked at the ParserTests and the Position.advance and it feels like the tests are wrong[er]

@martin-ilgner

Copy link
Copy Markdown
Contributor

@jiribenes I fixed the test (also had to normalize the expected value). I'm still confused by the regex above though.

Comment on lines 232 to 239
val textWithoutSpan =
raw"""There is some content here.
|And here.""".stripMargin
|And here.
|""".stripMargin.replace("\r\n", "\n")

assertEquals(span.from, 4)
assertEquals(span.to, 33)
assertEquals(source.content, textWithoutSpan)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alt. we could use assertNoDiff(source.content, textWithoutSpan)

@jiribenes

jiribenes commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

I'm still confused by the regex above though.

WDYM? 🤔
the .split("\n", -1) is Java's "split newlines and keep [trailing] empty lines"

@jiribenes
jiribenes merged commit 85bb24b into main Jul 31, 2026
6 of 7 checks passed
@jiribenes
jiribenes deleted the jiribenes/fix/lexer-CRLF branch July 31, 2026 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants