From 196318e6192a167da1359ee77b4d6bba2d7afb06 Mon Sep 17 00:00:00 2001
From: Ethan Atkins <contact@ethanatkins.com>
Date: Sat, 27 Jul 2019 21:40:48 -0700
Subject: [PATCH] Fix multi parser performance regression

It was reported in https://github.com/sbt/sbt/issues/4890 that cosmetic
white space could cause problems for the paser. I tracked this down to
primarily being because of the
`val semi = token(OptSpace ~> ';' ~> OptSpace)` line. This would cause
excessive backtracking. I added a test for a multi line command with a
lot of cosmetic whitespace that was adapted from #4890 except that I
made it even more taxing by running adding 100 commands instead of the
roughly 10 in the report. Before the parser changes, the test would
more or less block indefinitely. I never saw it successfully complete.
After these changes, it completes in 30-50ms (which drops to about 2-3
ms if the number of commands is dropped from 100 to 3).

I verified manually in a different project that a number of different
multi command completions still worked. In particular, I tested that
`~foo/test; foo/tes` would expand to `~foo/test; foo/test` which is one
of the hardest cases to get right.

I also added a few extra test cases for the parser since I wasn't sure
what the impact of removing the OptSpace ~> from the semi parser would
be.
---
 .../src/main/scala/sbt/BasicCommands.scala    |  7 +++----
 .../src/test/scala/sbt/MultiParserSpec.scala  | 20 +++++++++++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/main-command/src/main/scala/sbt/BasicCommands.scala b/main-command/src/main/scala/sbt/BasicCommands.scala
index 73c0184e5..819a71b2e 100644
--- a/main-command/src/main/scala/sbt/BasicCommands.scala
+++ b/main-command/src/main/scala/sbt/BasicCommands.scala
@@ -155,15 +155,14 @@ object BasicCommands {
 
   private[sbt] def multiParserImpl(state: Option[State]): Parser[List[String]] = {
     val nonSemi = charClass(_ != ';', "not ';'")
-    val semi = token(OptSpace ~> ';' ~> OptSpace)
+    val semi = token(';')
     val nonDelim = charClass(c => c != '"' && c != '{' && c != '}', label = "not '\"', '{', '}'")
     val components = ((nonSemi & nonDelim) | StringEscapable | braces('{', '}')).+
     val cmdPart = matched(components).examples()
 
     val completionParser: Option[Parser[String]] =
-      state.map(s => OptSpace ~> matched(s.nonMultiParser) <~ OptSpace)
-    val cmdParser =
-      completionParser.map(sp => (sp & cmdPart) | cmdPart).getOrElse(cmdPart).map(_.trim)
+      state.map(s => OptSpace ~> matched((s.nonMultiParser & cmdPart) | cmdPart) <~ OptSpace)
+    val cmdParser = completionParser.getOrElse(cmdPart).map(_.trim)
     val multiCmdParser: Parser[String] = semi ~> cmdParser
     /*
      * There are two cases that need to be handled separately:
diff --git a/main-command/src/test/scala/sbt/MultiParserSpec.scala b/main-command/src/test/scala/sbt/MultiParserSpec.scala
index 1beb4059d..1ea3c8556 100644
--- a/main-command/src/test/scala/sbt/MultiParserSpec.scala
+++ b/main-command/src/test/scala/sbt/MultiParserSpec.scala
@@ -7,6 +7,7 @@
 
 package sbt
 
+import scala.concurrent.duration._
 import org.scalatest.FlatSpec
 import sbt.internal.util.complete.Parser
 
@@ -28,6 +29,15 @@ class MultiParserSpec extends FlatSpec {
   }
   it should "parse single command with leading spaces" in {
     assert(";     foo".parse == Seq("foo"))
+    assert("     ;     foo".parse == Seq("foo"))
+    assert("      foo;".parse == Seq("foo"))
+  }
+  it should "parse single command with trailing spaces" in {
+    assert(";     foo      ".parse == Seq("foo"))
+    assert(";foo      ".parse == Seq("foo"))
+    assert("foo;      ".parse == Seq("foo"))
+    assert("    foo;    ".parse == Seq("foo"))
+    assert("      foo   ;    ".parse == Seq("foo"))
   }
   it should "parse multiple commands with leading spaces" in {
     assert(";     foo;bar".parse == Seq("foo", "bar"))
@@ -103,4 +113,14 @@ class MultiParserSpec extends FlatSpec {
     val unmatchedEmptyBraces = "{{{{}}}"
     assert(s"compile; $unmatchedEmptyBraces".parseEither.isLeft)
   }
+  it should "handle cosmetic whitespace" in {
+    val commands = (1 to 100).map(_ => "compile")
+    val multiLine = commands.mkString("      \n      ;", "       \n       ;", "    \n        ")
+    val start = System.nanoTime
+    assert(multiLine.parse == commands)
+    val elapsed = System.nanoTime - start
+    // Make sure this took less than 10 seconds. It takes about 30 milliseconds to run with
+    // 100 commands and 3 milliseconds with 3 commands. With a bad parser, it will run indefinitely.
+    assert(elapsed.nanoseconds < 10.seconds)
+  }
 }