CommandLineParser improvements

Tokenizing used to snip the line and knit the args,
this commit denits the deknitting.

Adds a benchmark showing linear behavior and a unit test
to show correctness.

Departing from the previous notion of correctness,
internal quotes are respected. `"abc"xyz` is `abcxyz`.

Author: som-snytt

build.sbt

@@ -568,7 +568,7 @@ lazy val partest = configureAsSubproject(project)
   )
 
 lazy val bench = project.in(file("test") / "benchmarks")
-  .dependsOn(library)
+  .dependsOn(library, compiler)
   .settings(instanceSettings)
   .settings(disableDocs)
   .settings(disablePublishing)

src/compiler/scala/tools/cmd/CommandLineParser.scala

@@ -1,82 +1,103 @@
 /* NEST (New Scala Test)
- * Copyright 2007-2013 LAMP/EPFL
+ * Copyright 2007-2018 LAMP/EPFL
  * @author Paul Phillips
  */
-
-package scala.tools
-package cmd
+package scala.tools.cmd
 
 import scala.annotation.tailrec
 
-/** A simple (overly so) command line parser.
- *  !!! This needs a thorough test suite to make sure quoting is
- *  done correctly and portably.
+/** A simple enough command line parser.
  */
 object CommandLineParser {
-  // splits a string into a quoted prefix and the rest of the string,
-  // taking escaping into account (using \)
-  // `"abc"def` will match as `DoubleQuoted(abc, def)`
-  private class QuotedExtractor(quote: Char) {
-    def unapply(in: String): Option[(String, String)] = {
-      val del = quote.toString
-      if (in startsWith del) {
-        var escaped = false
-        val (quoted, next) = (in substring 1) span {
-          case `quote` if !escaped => false
-          case '\\'    if !escaped => escaped = true; true
-          case _                   => escaped = false; true
-        }
-        // the only way to get out of the above loop is with an empty next or !escaped
-        // require(next.isEmpty || !escaped)
-        if (next startsWith del) Some((quoted, next substring 1))
-        else None
-      } else None
+  private final val DQ = '"'
+  private final val SQ = '\''
+
+  /** Split the line into tokens separated by whitespace or quotes.
+   *
+   *  @return either an error message or reverse list of tokens
+   */
+  private def tokens(in: String) = {
+    import Character.isWhitespace
+    import java.lang.{StringBuilder => Builder}
+    import collection.mutable.ArrayBuffer
+
+    var accum: List[String] = Nil
+    var pos = 0
+    var start = 0
+    val qpos = new ArrayBuffer[Int](16)    // positions of paired quotes
+
+    def cur: Int  = if (done) -1 else in.charAt(pos)
+    def bump() = pos += 1
+    def done   = pos >= in.length
+
+    def skipToQuote(q: Int) = {
+      var escaped = false
+      def terminal = in.charAt(pos) match {
+        case _ if escaped => escaped = false ; false
+        case '\\'         => escaped = true ; false
+        case `q`          => true
+        case _            => false
+      }
+      while (!done && !terminal) pos += 1
+      !done
     }
-  }
-  private object DoubleQuoted extends QuotedExtractor('"')
-  private object SingleQuoted extends QuotedExtractor('\'')
-  object Word {
-    private val regex = """(\S+)""".r
-    def unapply(s: String): Option[(String, String)] = {
-      regex.findPrefixOf(s) match {
-        case Some(prefix) => Some(prefix, s.substring(prefix.length))
-        case None => None
+    def skipToDelim(): Boolean =
+      cur match {
+        case q @ (DQ | SQ)        => { qpos.append(pos); bump(); skipToQuote(q) } && { qpos.append(pos); bump(); skipToDelim() }
+        case -1                   => true
+        case c if isWhitespace(c) => true
+        case _                    => bump(); skipToDelim()
+      }
+    def skipWhitespace() = while (isWhitespace(cur)) pos += 1
+    def copyText() = {
+      val buf = new Builder
+      var p = start
+      var i = 0
+      while (p < pos) {
+        if (i >= qpos.size) {
+          buf.append(in, p, pos)
+          p = pos
+        } else if (p == qpos(i)) {
+          buf.append(in, qpos(i)+1, qpos(i+1))
+          p = qpos(i+1)+1
+          i += 2
+        } else {
+          buf.append(in, p, qpos(i))
+          p = qpos(i)
+        }
       }
+      buf.toString
     }
-  }
-
-  // parse `in` for an argument, return it and the remainder of the input (or an error message)
-  // (argument may be in single/double quotes, taking escaping into account, quotes are stripped)
-  private def argument(in: String): Either[String, (String, String)] = in match {
-    case DoubleQuoted(arg, rest) => Right((arg, rest))
-    case SingleQuoted(arg, rest) => Right((arg, rest))
-    case Word(arg, rest)         => Right((arg, rest))
-    case _                       => Left(s"Illegal argument: $in")
-  }
+    def text() = {
+      val res =
+        if (qpos.isEmpty) in.substring(start, pos)
+        else if (qpos(0) == start && qpos(1) == pos) in.substring(start+1, pos-1)
+        else copyText()
+      qpos.clear()
+      res
+    }
+    def badquote = Left("Unmatched quote")
 
-  // parse a list of whitespace-separated arguments (ignoring whitespace in quoted arguments)
-  @tailrec private def commandLine(in: String, accum: List[String] = Nil): Either[String, (List[String], String)] = {
-    val trimmed = in.trim
-    if (trimmed.isEmpty) Right((accum.reverse, ""))
-    else argument(trimmed) match {
-      case Right((arg, next)) =>
-        val leadingWhitespaceLen = next.prefixLength(Character.isWhitespace)
-        val rest = next.substring(leadingWhitespaceLen)
-        if (leadingWhitespaceLen == 0 && rest.nonEmpty)
-          Left("Arguments should be separated by whitespace.") // TODO: can this happen?
-        else
-          commandLine(rest, arg :: accum)
-      case Left(msg) => Left(msg)
+    @tailrec def loop(): Either[String, List[String]] = {
+      skipWhitespace()
+      start = pos
+      if (done) Right(accum)
+      else if (!skipToDelim()) badquote
+      else {
+        accum = text() :: accum
+        loop()
+      }
     }
+    loop()
   }
 
   class ParseException(msg: String) extends RuntimeException(msg)
 
-  def tokenize(line: String): List[String] = tokenize(line, x => throw new ParseException(x))
-  def tokenize(line: String, errorFn: String => Unit): List[String] = {
-    commandLine(line) match {
-      case Right((args, _)) => args
-      case Left(msg)        => errorFn(msg) ; Nil
+  def tokenize(line: String, errorFn: String => Unit): List[String] =
+    tokens(line) match {
+      case Right(args) => args.reverse
+      case Left(msg)   => errorFn(msg) ; Nil
     }
-  }
+
+  def tokenize(line: String): List[String] = tokenize(line, x => throw new ParseException(x))
 }

test/benchmarks/src/main/scala/scala/tools/cmd/CommandLineParserBenchmark.scala

@@ -0,0 +1,35 @@
+
+package scala.tools.cmd
+
+import java.util.concurrent.TimeUnit
+
+import org.openjdk.jmh.annotations._
+import org.openjdk.jmh.infra.Blackhole
+
+@BenchmarkMode(Array(Mode.AverageTime))
+@Fork(2)
+@Threads(1)
+@Warmup(iterations = 10)
+@Measurement(iterations = 10)
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+@State(Scope.Benchmark)
+class CommandLineParserBenchmark {
+
+  import CommandLineParser.tokenize
+
+  @Param(Array("1000", "10000", "100000"))
+  var argCount: Int = _
+  var line: String = _
+  var quotyline: String = _
+  var embedded: String = _
+
+  @Setup(Level.Trial) def init(): Unit = {
+    line = List.tabulate(argCount)(n => s"arg$n").mkString(" ")
+    val q = "\""
+    quotyline = List.tabulate(argCount)(n => s"${q}arg${n}${q}").mkString(" ")
+    embedded  = List.tabulate(argCount)(n => s"${n}${q}arg${q}${n}").mkString(" ")
+  }
+  @Benchmark def parsingBenchmark              = tokenize(line)
+  @Benchmark def quoteUnquoteParsingBenchmark  = tokenize(quotyline)
+  @Benchmark def embeddedQuoteParsingBenchmark = tokenize(embedded)
+}

test/junit/scala/tools/cmd/CommandLineParserTest.scala

@@ -0,0 +1,45 @@
+package scala.tools.cmd
+
+import org.junit.Assert._
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.junit.runners.JUnit4
+import scala.tools.testing.AssertUtil.assertThrows
+
+@RunWith(classOf[JUnit4])
+class CommandLineParserTest {
+  import CommandLineParser.{tokenize, ParseException}
+
+  @Test
+  def parserTokenizes(): Unit = {
+    assertEquals(Nil, tokenize(""))
+    assertEquals(List("x"), tokenize("x"))
+    assertEquals(List("x"), tokenize(" x "))
+    assertEquals(List("x","y"), tokenize("x y"))
+    assertEquals(List("x","y","z"), tokenize("x y z"))
+  }
+  @Test
+  def parserTrims(): Unit = {
+    assertEquals(Nil, tokenize(" "))
+    assertEquals(List("x"), tokenize(" x "))
+    assertEquals(List("x"), tokenize("\nx\n"))
+    assertEquals(List("x","y","z"), tokenize(" x y z "))
+  }
+  @Test
+  def parserQuotes(): Unit = {
+    assertEquals(List("x"), tokenize("'x'"))
+    assertEquals(List("x"), tokenize(""""x""""))
+    assertEquals(List("x","y","z"), tokenize("x 'y' z"))
+    assertEquals(List("x"," y ","z"), tokenize("x ' y ' z"))
+    assertEquals(List("x","y","z"), tokenize("""x "y" z"""))
+    assertEquals(List("x"," y ","z"), tokenize("""x " y " z"""))
+    // interior quotes
+    assertEquals(List("x y z"), tokenize("x' y 'z"))   // was assertEquals(List("x'","y","'z"), tokenize("x' y 'z"))
+    assertEquals(List("x\ny\nz"), tokenize("x'\ny\n'z"))
+    assertEquals(List("x'y'z"), tokenize("""x"'y'"z"""))
+    assertEquals(List("abcxyz"), tokenize(""""abc"xyz"""))
+    // missing quotes
+    assertThrows[ParseException](tokenize(""""x"""))         // was assertEquals(List("\"x"), tokenize(""""x"""))
+    assertThrows[ParseException](tokenize("""x'"""))
+  }
+}