From 60b1ac7ac4c92909f932a42af1677d1cedabcd0f Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 24 Jun 2019 21:28:58 -0700 Subject: [PATCH 1/2] Improve multi parser performance The multi parser had very poor performance if there were many commands. Evaluating the expansion of something like "compile;" * 30 could cause sbt to hang indefinitely. I believe this was due to excessive backtracking due to the optional `(parser <~ semi.?).?` part of the parser in the non-leading semicolon case. I also reworked the implementation so that the multi command now has a name. This allows us to partition the commands into multi and non-multi commands more easily in State while still having multi in the command list. With this change, builds and plugins can exclude the multi parser if they wish. Using the partitioned parsers, I removed the high/priority low priority distinction. Instead, I made it so that the multi command will actually check if the first command is a named command, like '~'. If it is, it will pass the raw command argument with the named command stripped out into the parser for the named command. If that is parseable, then we directly apply the effect. Otherwise we prefix each multi command to the state. --- .../src/main/scala/sbt/BasicCommands.scala | 93 ++++++++++++++----- main-command/src/main/scala/sbt/Command.scala | 7 +- main-command/src/main/scala/sbt/State.scala | 23 +---- main/src/main/scala/sbt/Main.scala | 1 + .../main/scala/sbt/internal/Continuous.scala | 6 +- .../sbt-test/actions/multi-command/build.sbt | 2 + .../multi-command/src/main/scala/Foo.scala | 1 + sbt/src/sbt-test/actions/multi-command/test | 4 + sbt/src/sbt-test/watch/command-parser/test | 3 + .../sbt/scriptedtest/ScriptedTests.scala | 2 + 10 files changed, 97 insertions(+), 45 deletions(-) create mode 100644 sbt/src/sbt-test/actions/multi-command/src/main/scala/Foo.scala diff --git a/main-command/src/main/scala/sbt/BasicCommands.scala b/main-command/src/main/scala/sbt/BasicCommands.scala index 3a01ac7b2..04e0a60b8 100644 --- a/main-command/src/main/scala/sbt/BasicCommands.scala +++ b/main-command/src/main/scala/sbt/BasicCommands.scala @@ -157,39 +157,88 @@ object BasicCommands { val nonSemi = charClass(_ != ';', "not ';'") val semi = token(OptSpace ~> ';' ~> OptSpace) val nonDelim = charClass(c => c != '"' && c != '{' && c != '}', label = "not '\"', '{', '}'") - val cmdPart = OptSpace ~> matched( - token( - (nonSemi & nonDelim).map(_.toString) | StringEscapable | braces('{', '}'), - hide = const(true) - ).+ - ) <~ OptSpace - val strictParser: Option[Parser[String]] = - state.map(s => OptSpace ~> matched(s.nonMultiParsers) <~ OptSpace) - val parser = strictParser.map(sp => sp & cmdPart).getOrElse(cmdPart) + val components = ((nonSemi & nonDelim) | StringEscapable | braces('{', '}')).+ + val cmdPart = matched(components) + + val completionParser: Option[Parser[String]] = + state.map(s => OptSpace ~> matched(s.nonMultiParser) <~ OptSpace) + val cmdParser = completionParser.map(sp => sp & cmdPart).getOrElse(cmdPart).map(_.trim) + val multiCmdParser: Parser[String] = semi ~> cmdParser /* * There are two cases that need to be handled separately: - * 1) There are multiple commands separated by at least one semicolon with an optional - * leading semicolon. - * 2) There is a leading semicolon, but only on one command + * 1) leading semicolon with one or more commands separated by a semicolon + * 2) no leading semicolon and at least one command followed by a trailing semicolon + * and zero or more commands separated by a semicolon + * Both cases allow an optional trailing semi-colon. + * * These have to be handled separately because the performance degrades badly if the first * case is implemented with the following parser: * (semi.? ~> ((combinedParser <~ semi).* ~ combinedParser <~ semi.?) */ - (semi.? ~> (parser <~ semi).+ ~ (parser <~ semi.?).?).flatMap { - case (prefix, last) => - (prefix ++ last).toList.map(_.trim).filter(_.nonEmpty) match { - case Nil => Parser.failure("No commands were parsed") - case cmds => Parser.success(cmds) - } - } | semi ~> parser.map(_.trim :: Nil) <~ semi.? + val noLeadingSemi = (cmdParser ~ (multiCmdParser.+ | semi.map(_ => Nil))).map { + case (prefix, last) => (prefix :: Nil ::: last.toList).filter(_.nonEmpty) + } + val leadingSemi = multiCmdParser.+.map(_.toList.filter(_.nonEmpty)) + ((leadingSemi | noLeadingSemi) <~ semi.?).flatMap { + case Nil => Parser.failure("No commands were parsed") + case commands => Parser.success(commands) + } } def multiParser(s: State): Parser[List[String]] = multiParserImpl(Some(s)) - def multiApplied(s: State): Parser[() => State] = - Command.applyEffect(multiParser(s))(_ ::: s) + def multiApplied(state: State): Parser[() => State] = + Command.applyEffect(multiParserImpl(Some(state))) { + // the (@ _ :: _) ensures tail length >= 1. + case commands @ first :: (tail @ _ :: _) => + require(first.head != ' ', s"Commands must be trimmed. Received: '$first'.") + // Note: scalafmt refuses to align on '*' using multiline /*...*/ here + // + // This case is only executed if the multi parser actually returns multiple commands to run. + // Otherwise we just prefix the single extracted command with the semicolon stripped to the + // state. Since there is no semicolon in the stripped command, the multi command parser will + // fail to parse that single command so we do not end up in a loop. + // + // If there are multiple commands, we give a named command a chance to parse the raw input + // and possibly directly evaluate the side effects. This is desirable if, for example, + // the command runs other commands. The continuous (~) command is one such example. If the + // input command is `~foo; bar`, the multi parser would extract "~foo" :: "bar" :: Nil. + // If we naively just prepended the state with both of these commands, it would run '~foo' + // and then when watch exited, it'd run 'bar', which is likely unexpected. To address this, + // we search for a named command whose name is a prefix of the first command in the list. + // In this case, we'd find '~' and then pass 'foo;bar' into its parser. If this succeeds, + // which it will in the case of continuous (so long as foo and bar are valid commands), + // then we directly evaluate the `() => State` returned by the parser. Otherwise, we + // fall back to prefixing the multi commands to the state. + // + state.nonMultiCommands.view.flatMap { + case command => + command.nameOption match { + case Some(commandName) if first.startsWith(commandName) => + // A lot of commands expect leading semicolons in their parsers. In order to + // ensure that they are multi-command capable, we strip off any leading spaces. + // Without doing this, we could run simple commands like `set` with the full + // input. This would likely fail because `set` doesn't know how to handle + // semicolons. This is a bit of a hack that is specifically there + // to handle `~` which doesn't require a space before its argument. Any command + // whose parser accepts multi commands without a leading space should be accepted. + // All other commands should be rejected. Note that `alias` is also handled by + // this case. + val commandArgs = + (first.drop(commandName.length).trim :: Nil ::: tail).mkString(";") + parse(commandArgs, command.parser(state)).toOption + case _ => None + } + case _ => None + }.headOption match { + case Some(s) => s() + case _ => commands ::: state + } + case commands => commands ::: state + } - def multi: Command = Command.custom(multiApplied, Help(Multi, MultiBrief, MultiDetailed)) + val multi: Command = + Command.custom(multiApplied, Help(Multi, MultiBrief, MultiDetailed), Multi) lazy val otherCommandParser: State => Parser[String] = (s: State) => token(OptSpace ~> combinedLax(s, NotSpaceClass ~ any.*)) diff --git a/main-command/src/main/scala/sbt/Command.scala b/main-command/src/main/scala/sbt/Command.scala index 89ba7a375..8fd7e9acc 100644 --- a/main-command/src/main/scala/sbt/Command.scala +++ b/main-command/src/main/scala/sbt/Command.scala @@ -52,8 +52,11 @@ private[sbt] final class SimpleCommand( private[sbt] final class ArbitraryCommand( val parser: State => Parser[() => State], val help: State => Help, - val tags: AttributeMap + val tags: AttributeMap, + override val nameOption: Option[String] ) extends Command { + def this(parser: State => Parser[() => State], help: State => Help, tags: AttributeMap) = + this(parser, help, tags, None) def tag[T](key: AttributeKey[T], value: T): ArbitraryCommand = new ArbitraryCommand(parser, help, tags.put(key, value)) } @@ -124,6 +127,8 @@ object Command { def customHelp(parser: State => Parser[() => State], help: State => Help): Command = new ArbitraryCommand(parser, help, AttributeMap.empty) + private[sbt] def custom(parser: State => Parser[() => State], help: Help, name: String): Command = + new ArbitraryCommand(parser, const(help), AttributeMap.empty, Some(name)) def custom(parser: State => Parser[() => State], help: Help = Help.empty): Command = customHelp(parser, const(help)) diff --git a/main-command/src/main/scala/sbt/State.scala b/main-command/src/main/scala/sbt/State.scala index d83d76b1c..28be57a98 100644 --- a/main-command/src/main/scala/sbt/State.scala +++ b/main-command/src/main/scala/sbt/State.scala @@ -41,25 +41,10 @@ final case class State( currentCommand: Option[Exec], next: State.Next ) extends Identity { - /* - * The `~` and `alias` commands effectively run other commands so they need to be run before - * the multi parser. For example, if the user runs `~foo;bar` and the multi parser runs before - * the `~` parser, then then it will be parsed as two commands `~foo` and `bar`. By running - * the high priority commands before the multi parser, we ensure that the high priority commands - * parse the full command input. Any other command that runs other commands would likely need - * to be added to this list but at the time of writing this comment {~, alias} are the two - * commands that we know need this special treatment. - * - * TODO: add a structured way of indicating that a command needs to run before the multi parser. - */ - private[this] val highPriorityCommands = Set("~", "alias") - private[this] lazy val (highPriority, regularPriority) = - definedCommands.partition(_.nameOption.exists(highPriorityCommands)) - private[this] lazy val highPriorityParser = Command.combine(highPriority)(this) - private[this] lazy val lowPriorityParser = Command.combine(regularPriority)(this) - private[sbt] lazy val nonMultiParsers = highPriorityParser | lowPriorityParser - lazy val combinedParser = - highPriorityParser | BasicCommands.multiApplied(this) | lowPriorityParser + private[sbt] lazy val (multiCommands, nonMultiCommands) = + definedCommands.partition(_.nameOption.contains(BasicCommandStrings.Multi)) + private[sbt] lazy val nonMultiParser = Command.combine(nonMultiCommands)(this) + lazy val combinedParser = multiCommands.foldRight(nonMultiParser)(_.parser(this) | _) def source: Option[CommandSource] = currentCommand match { diff --git a/main/src/main/scala/sbt/Main.scala b/main/src/main/scala/sbt/Main.scala index f36fc1322..e6b99a808 100644 --- a/main/src/main/scala/sbt/Main.scala +++ b/main/src/main/scala/sbt/Main.scala @@ -190,6 +190,7 @@ object BuiltinCommands { def DefaultCommands: Seq[Command] = Seq( + multi, about, tasks, settingsCommand, diff --git a/main/src/main/scala/sbt/internal/Continuous.scala b/main/src/main/scala/sbt/internal/Continuous.scala index 35bd50f20..80e948781 100644 --- a/main/src/main/scala/sbt/internal/Continuous.scala +++ b/main/src/main/scala/sbt/internal/Continuous.scala @@ -146,9 +146,9 @@ private[sbt] object Continuous extends DeprecatedContinuous { val ocp = BasicCommands.multiParserImpl(Some(state)) | BasicCommands.otherCommandParser(state).map(_ :: Nil) (digitParser.? ~ ocp).flatMap { - case (i, cmds) if cmds.exists(_.nonEmpty) => - Parser.success((i.getOrElse(0), cmds.filter(_.nonEmpty))) - case (_, cmds) => Parser.failure("Couldn't parse any commands") + case (i, commands) if commands.exists(_.nonEmpty) => + Parser.success((i.getOrElse(0), commands.filter(_.nonEmpty))) + case (_, _) => Parser.failure("Couldn't parse any commands") } } diff --git a/sbt/src/sbt-test/actions/multi-command/build.sbt b/sbt/src/sbt-test/actions/multi-command/build.sbt index 5db2dc44f..6b673176b 100644 --- a/sbt/src/sbt-test/actions/multi-command/build.sbt +++ b/sbt/src/sbt-test/actions/multi-command/build.sbt @@ -18,3 +18,5 @@ checkInput := checkInputImpl.evaluated val dynamicTask = taskKey[Unit]("dynamic input task") dynamicTask := { println("not yet et") } + +crossScalaVersions := "2.11.12" :: "2.12.8" :: Nil diff --git a/sbt/src/sbt-test/actions/multi-command/src/main/scala/Foo.scala b/sbt/src/sbt-test/actions/multi-command/src/main/scala/Foo.scala new file mode 100644 index 000000000..43c42f145 --- /dev/null +++ b/sbt/src/sbt-test/actions/multi-command/src/main/scala/Foo.scala @@ -0,0 +1 @@ +class Foo \ No newline at end of file diff --git a/sbt/src/sbt-test/actions/multi-command/test b/sbt/src/sbt-test/actions/multi-command/test index 0e0e84d40..99a77cee8 100644 --- a/sbt/src/sbt-test/actions/multi-command/test +++ b/sbt/src/sbt-test/actions/multi-command/test @@ -34,3 +34,7 @@ > checkInput foo > compile; checkInput foo + +> ++ 2.11.12 compile; setStringValue bar; checkStringValue bar + +> ++2.12.8 compile; setStringValue foo; checkStringValue foo diff --git a/sbt/src/sbt-test/watch/command-parser/test b/sbt/src/sbt-test/watch/command-parser/test index e8733214f..55f3bff5f 100644 --- a/sbt/src/sbt-test/watch/command-parser/test +++ b/sbt/src/sbt-test/watch/command-parser/test @@ -19,3 +19,6 @@ > ~; compile; setStringValue string.txt baz; checkStringValue string.txt baz # Ensure that trailing semi colons work > ~ compile; setStringValue string.txt baz; checkStringValue string.txt baz; + +# Ensure that no space is required between '~' and the command +> ~compile;setStringValue string.txt foo;checkStringValue string.txt foo diff --git a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala index cf077a326..c92e29e12 100644 --- a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala +++ b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala @@ -185,6 +185,8 @@ final class ScriptedTests( case "actions/cross-multiproject" => LauncherBased // tbd case "actions/cross-multi-parser" => LauncherBased // java.lang.ClassNotFoundException: javax.tools.DiagnosticListener when run with java 11 and an old sbt launcher + case "actions/multi-command" => + LauncherBased // java.lang.ClassNotFoundException: javax.tools.DiagnosticListener when run with java 11 and an old sbt launcher case "actions/external-doc" => LauncherBased // sbt/Package$ case "actions/input-task" => LauncherBased // sbt/Package$ case "actions/input-task-dyn" => LauncherBased // sbt/Package$ From 4e2c1858f2a5333977a438514582241e75e21da0 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 24 Jun 2019 22:43:57 -0700 Subject: [PATCH 2/2] Don't append empty comp.append In some cases, comp.append could be an empty string. This would happen if a parser was something like `(token(foo) <~ ;).+ <~ fo.?` because there were no completions for the `fo` available anchor. The effect of this was that tab would never complete foo;f to foo;foo, even though that was the only possible completion. It would, _display_, foo as a possible completion though. This came up because the multi parser has a similar parser to that described above and it broke tab completion to the right of a semi colon. --- .../scala/sbt/internal/util/complete/JLineCompletion.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/util-complete/src/main/scala/sbt/internal/util/complete/JLineCompletion.scala b/internal/util-complete/src/main/scala/sbt/internal/util/complete/JLineCompletion.scala index eb955ffd2..fc9f2f49a 100644 --- a/internal/util-complete/src/main/scala/sbt/internal/util/complete/JLineCompletion.scala +++ b/internal/util-complete/src/main/scala/sbt/internal/util/complete/JLineCompletion.scala @@ -83,7 +83,8 @@ object JLineCompletion { val (insert, display) = ((Set.empty[String], Set.empty[String]) /: cs) { case (t @ (insert, display), comp) => - if (comp.isEmpty) t else (insert + comp.append, appendNonEmpty(display, comp.display)) + if (comp.isEmpty) t + else (appendNonEmpty(insert, comp.append), appendNonEmpty(display, comp.display)) } (insert.toSeq, display.toSeq.sorted) }