Skip to content

Generalize ScriptWhitespace #337

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.

Sign up for GitHub

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

Open
informarte wants to merge 1 commit into
base: master
Choose a base branch
from
+29 −6
Open

Generalize ScriptWhitespace #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions fastparse/src/fastparse/Whitespace.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ object MultiLineWhitespace {
* Whitespace syntax that supports # line-comments, as in the case in
* programming languages such as Bash, Ruby, or Python
*/
object ScriptWhitespace{
implicit object whitespace extends Whitespace {
object ScriptWhitespace {
def custom(delim: Char): Whitespace = new Whitespace {
def apply(ctx: ParsingRun[_]) = {
val input = ctx.input
@tailrec def rec(current: Int, state: Int): ParsingRun[Unit] = {
Expand All @@ -74,10 +74,12 @@ object ScriptWhitespace{
case 0 =>
(currentChar: @switch) match{

@He-Pin He-Pin Apr 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Performance issue: The inner (currentChar: @switch) match now has a problem. Originally it had 3 cases: whitespace chars, #, and case _. The @switch annotation compiles this to a tableswitch JVM instruction (O(1) dispatch).

After this change, case # is removed from the match and moved into case _ as a runtime if (currentChar == delim) check. This means:

  1. The @switch annotation on the inner match is still valid (2 cases + default), BUT
  2. Every non-whitespace character now pays an extra if check for delim before returning success. Previously case _ went directly to success. Now it does if (currentChar == delim) ... else success.

For the default ScriptWhitespace.whitespace (delim=#), this adds one extra character comparison per non-whitespace character encountered. In a hot path that parses whitespace frequently (between every token in a source file), this is a measurable regression.

Fix: Keep the hardcoded case # => in the switch AND still support custom delimiters. One approach: make custom return a different anonymous Whitespace subclass that has its own apply method with the delim hardcoded via a val captured at construction time, but the @switch match should still have the delim as a literal case. However, since delim is a runtime parameter, you cannot have it as a case literal.

Alternative: accept that custom delimiters incur a tiny overhead but document this, or use a macro/inline to generate the literal case at compile time.

case ' ' | '\t' | '\n' | '\r' => rec(current + 1, state)
case '#' => rec(current + 1, state = 1)
case _ =>
if (ctx.verboseFailures) ctx.reportTerminalMsg(current, Msgs.empty)
ctx.freshSuccessUnit(current)
if (currentChar == delim) rec(current + 1, state = 1)
else {
if (ctx.verboseFailures) ctx.reportTerminalMsg(current, Msgs.empty)
ctx.freshSuccessUnit(current)
}
}
case 1 => rec(current + 1, state = if (currentChar == '\n') 0 else state)
}
Expand All @@ -86,6 +88,7 @@ object ScriptWhitespace{
rec(current = ctx.index, state = 0)

@He-Pin He-Pin Apr 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Performance concern: implicit val whitespace = custom('#') allocates a new Whitespace instance on ScriptWhitespace initialization, same as implicit object. However, custom(delim) called by users with arbitrary chars allocates a new anonymous Whitespace instance per call.

This is minor since users typically call custom once and cache the result as an implicit val. But consider documenting that the result should be stored in a val, not called inline in a parse hot path.

}
}
implicit val whitespace: Whitespace = custom('#')
}

/**
Expand Down
22 changes: 21 additions & 1 deletion fastparse/test/src/fastparse/WhitespaceTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ object WhitespaceTests extends TestSuite{
def checkCommon(p0: Whitespace) = {
val p = p0.apply(_)

@He-Pin He-Pin Apr 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The function tests for line comments and block comments (lines 16-24), which are NOT supported by — it only supports a single configurable delimiter for line comments. These tests will fail when called with in the test (line 46). Either should be split into separate helpers for different whitespace types, or should not call .

val Parsed.Success((), 0) = parse("", p)
val Parsed.Success((), 4) = parse(" \t\r\n", p)
val Parsed.Success((), 0) = parse("/", p)
val Parsed.Success((), 1) = parse(" /", p)
val Parsed.Success((), 1) = parse(" / ", p)
Expand All @@ -22,13 +23,32 @@ object WhitespaceTests extends TestSuite{
val Parsed.Success((), 6) = parse("/****/", p)
val Parsed.Success((), 9) = parse("/** * **/", p)
val Parsed.Success((), 15) = parse("/** // * // **/", p)
val Parsed.Success((), 0) = parse("code", p)
val Parsed.Success((), 2) = parse(" code", p)
}
def checkScriptCommon(p0: Whitespace, delim: Char) = {
val p = p0.apply(_)
val Parsed.Success((), 0) = parse("", p)
val Parsed.Success((), 4) = parse(" \t\r\n", p)
val Parsed.Success((), 9) = parse(s"$delim comment", p)
val Parsed.Success((), 10) = parse(s"$delim$delim comment", p)
val Parsed.Success((), 10) = parse(s" $delim comment", p)
val Parsed.Success((), 19) = parse(s"$delim comment $delim comment", p)
val Parsed.Success((), 0) = parse("code", p)
val Parsed.Success((), 2) = parse(" code", p)
}
test("scala"){
checkCommon(ScalaWhitespace.whitespace)
// allow nested comments
val Parsed.Failure(_, 11, _) = parse("/** /* /**/", ScalaWhitespace.whitespace.apply(_))
val Parsed.Success((), 8) = parse("/*/**/*/", ScalaWhitespace.whitespace.apply(_))
}
test("standard_script"){
checkScriptCommon(ScriptWhitespace.whitespace, '#')
}
test("custom_script"){
checkScriptCommon(ScriptWhitespace.custom('%'), '%')
}
test("java"){
checkCommon(JavaWhitespace.whitespace)
// no nested comments
Expand All @@ -43,4 +63,4 @@ object WhitespaceTests extends TestSuite{
}
}

}
}