Fix Dash-Separated Compound Commands

Summary:
As I've been working on the e2e cli test suite, I've seen that there are issues with the way that the CLI Parses compound commands, which means that the parsing can terminate early and not parse the entire string.

I think this highlights the folly in 'implicit' chaining of compound commands. For example,

```
list boot listen --http 1000 shutdown
```

Should result in the same parsed command as with dash-separated compound commands:

```
list -- boot -- listen --http 1000 -- shutdown
```

But it does not, as the dash separated style will bail out in parsing halfway through and provided a half-parsed result.

I want to correct the existing behaviour, then remove 'implicit' parsing:

1) Introduce tests (and fixes) for dash-separated compound commands
2) Make parsing of compound commands 'strict'. This means that `list -- boot ThisIsABadArgument` should fail to parse instead of just parsing `list` and then continuing. This is important as this can result in very surprising behaviour otherwise.
3) Update any documentation and dependencies to use the new dash-separation of compound commands.
4) Remove the behaviour for implicit chaining of compound commands. Iinvert the tests so that dash-separation succeeds to parse, implicit chaining fails.

This diff does #1 and #2 in unison, since only doing #1 without #2 results in a very confusing parser.

Reviewed By: asm89

Differential Revision: D3592485

fbshipit-source-id: 00d3164c921082638d65076329b117c19ee57bf8

fbsimctl/FBSimulatorControlKit/Sources/CommandParsers.swift

@@ -130,6 +130,10 @@ extension Parser {
   public static var ofDate: Parser<NSDate> { get {
     return Parser<NSDate>.ofDouble.fmap { NSDate(timeIntervalSince1970: $0) }
   }}
+
+  public static var ofDashSeparator: Parser<NSNull> { get {
+    return Parser<NSNull>.ofString("--", NSNull())
+  }}
 }
 
 extension OutputOptions : Parsable {
@@ -339,7 +343,7 @@ extension Command : Parsable {
         Configuration.parser,
         FBiOSTargetQueryParsers.parser.optional(),
         FBiOSTargetFormatParsers.parser.optional(),
-        Parser.manyCount(1, Action.parser)
+        self.compoundActionParser
       )
       .fmap { (configuration, query, format, actions) in
         return Command(
@@ -351,6 +355,13 @@ extension Command : Parsable {
       }
     }
   }
+
+  static var compoundActionParser: Parser<[Action]> { get {
+    return Parser.alternative([
+      Parser.exhaustive(Parser.manyCount(1, Action.parser)),
+      Parser.exhaustive(Parser.manySepCount(1, Action.parser, Parser<NSNull>.ofDashSeparator)),
+    ])
+  }}
 }
 
 extension Server : Parsable {
@@ -806,9 +817,10 @@ struct FBProcessLaunchConfigurationParsers {
   }}
 
   static var argumentParser: Parser<[String]> { get {
-    return Parser.manyTill(
-      Parser<String>.ofString("--", "--"),
-      Parser<String>.ofAny
-    )
+    return Parser
+      .manyTill(
+        Parser<NSNull>.ofDashSeparator,
+        Parser<String>.ofAny
+      )
   }}
 }

fbsimctl/FBSimulatorControlKit/Sources/Parser.swift

@@ -75,14 +75,24 @@ extension Parser {
   func optional() -> Parser<A?> {
     return Parser<A?>(self.matchDescription) { tokens in
       do {
-        let (nextTokens, value) = try self.parse(tokens)
-        return (nextTokens, Optional.Some(value))
+        let (tokens, value) = try self.parse(tokens)
+        return (tokens, Optional.Some(value))
       } catch {
         return (tokens, nil)
       }
     }
   }
 
+  func handle(f: ParseError -> A) -> Parser<A> {
+    return Parser<A>(self.matchDescription) { tokens in
+      do {
+        return try self.parse(tokens)
+      } catch let error as ParseError {
+        return (tokens, f(error))
+      }
+    }
+  }
+
   func sequence<B>(p: Parser<B>) -> Parser<B> {
     return self
       .bind({ _ in p })
@@ -94,17 +104,6 @@ extension Parser {
   Derivatives
 */
 extension Parser {
-  func handle(f: () -> A) -> Parser<A> {
-    return self
-      .optional()
-      .fmap { optionalValue in
-        guard let value = optionalValue else {
-          return f()
-        }
-        return value
-      }
-  }
-
   func fallback(a: A) -> Parser<A> {
     return self.handle { _ in a }
   }
@@ -113,6 +112,21 @@ extension Parser {
     return Parser(description, output: self.output)
   }
 
+  static var passthrough: Parser<NSNull> { get {
+    return Parser<NSNull>("") { tokens in
+      return (tokens, NSNull())
+    }
+  }}
+
+  static var noRemaining: Parser<NSNull> { get {
+    return Parser<NSNull>("No Remaining") { tokens in
+      if tokens.count > 0 {
+        throw ParseError.Custom("There were remaining tokens \(tokens)")
+      }
+      return ([], NSNull())
+    }
+  }}
+
   static func fail(error: ParseError) -> Parser<A> {
     return Parser<A>("fail Parser") { _ in
       throw error
@@ -199,6 +213,10 @@ extension Parser {
   }
 
   static func manyCount(count: Int, _ parser: Parser<A>) -> Parser<[A]> {
+    return self.manySepCount(count, parser, Parser.passthrough)
+  }
+
+  static func manySepCount<B>(count: Int, _ parser: Parser<A>, _ separator: Parser<B>) -> Parser<[A]> {
     assert(count >= 0, "Count should be >= 0")
     return Parser<[A]>("At least \(count) of \(parser)") { tokens in
       var values: [A] = []
@@ -207,10 +225,15 @@ extension Parser {
 
       do {
         while (runningArgs.count > 0) {
-          let output = try parser.parse(runningArgs)
+          // Extract the main parsed value
+          let (remainder, value) = try parser.parse(runningArgs)
           parseCount += 1
-          runningArgs = output.0
-          values.append(output.1)
+          runningArgs = remainder
+          values.append(value)
+
+          // Add the separator, will break out if separator parse fails
+          let (nextRemainder, _) = try separator.parse(runningArgs)
+          runningArgs = nextRemainder
         }
       } catch { }
 
@@ -228,8 +251,7 @@ extension Parser {
 
       while (runningArgs.count > 0) {
         do {
-          let output = try terminatingParser.parse(runningArgs)
-          runningArgs = output.0
+          try terminatingParser.parse(runningArgs)
           break
         } catch {
           let output = try parser.parse(runningArgs)
@@ -280,6 +302,14 @@ extension Parser {
         return accumulator
       }
   }
+
+  static func exhaustive(parser: Parser<A>) -> Parser<A> {
+    return Parser
+      .ofTwoSequenced(parser, Parser.noRemaining)
+      .fmap { (original, _) in
+        return original
+      }
+  }
 }
 
 public protocol Parsable {

fbsimctl/FBSimulatorControlKitTests/Tests/Unit/CommandParsersTests.swift

@@ -163,14 +163,12 @@ let validActions: [([String], Action)] = [
   (["erase"], Action.Erase),
   (["install", Fixtures.application.path], Action.Install(Fixtures.application.path)),
   (["launch", "--stderr", "com.foo.bar", "--foo", "-b", "-a", "-r"], Action.LaunchApp(FBApplicationLaunchConfiguration(bundleID: "com.foo.bar", bundleName: nil, arguments: ["--foo", "-b", "-a", "-r"], environment: [:], options: .WriteStderr))),
-  (["launch", "--stdout", "com.foo.bar", "--foo", "--", "--something-else"], Action.LaunchApp(FBApplicationLaunchConfiguration(bundleID: "com.foo.bar", bundleName: nil, arguments: ["--foo"], environment: [:], options: .WriteStdout))),
   (["launch", "com.foo.bar"], Action.LaunchApp(FBApplicationLaunchConfiguration(bundleID: "com.foo.bar", bundleName: nil, arguments: [], environment: [:], options: FBProcessLaunchOptions()))),
   (["launch", "--stderr", Fixtures.application.path], Action.LaunchApp(FBApplicationLaunchConfiguration(bundleID: Fixtures.application.bundleID, bundleName: nil, arguments: [], environment: [:], options: .WriteStderr))),
   (["launch", Fixtures.application.path], Action.LaunchApp(FBApplicationLaunchConfiguration(bundleID: Fixtures.application.bundleID, bundleName: nil, arguments: [], environment: [:], options: FBProcessLaunchOptions()))),
   (["launch", Fixtures.binary.path, "--foo", "-b", "-a", "-r"], Action.LaunchAgent(FBAgentLaunchConfiguration(binary: Fixtures.binary, arguments: ["--foo", "-b", "-a", "-r"], environment: [:], options: FBProcessLaunchOptions()))),
   (["launch", Fixtures.binary.path], Action.LaunchAgent(FBAgentLaunchConfiguration(binary: Fixtures.binary, arguments: [], environment: [:], options: FBProcessLaunchOptions()))),
   (["launch_xctest", "/usr/bin", "com.foo.bar", "--foo", "-b", "-a", "-r"], Action.LaunchXCTest(FBApplicationLaunchConfiguration(bundleID: "com.foo.bar", bundleName: nil, arguments: ["--foo", "-b", "-a", "-r"], environment: [:], options: FBProcessLaunchOptions()), "/usr/bin")),
-  (["launch_xctest", "/usr/bin", "com.foo.bar", "--foo", "--", "--something-else"], Action.LaunchXCTest(FBApplicationLaunchConfiguration(bundleID: "com.foo.bar", bundleName: nil, arguments: ["--foo"], environment: [:], options: FBProcessLaunchOptions()), "/usr/bin")),
   (["launch_xctest", "/usr/bin", "com.foo.bar"], Action.LaunchXCTest(FBApplicationLaunchConfiguration(bundleID: "com.foo.bar", bundleName: nil, arguments: [], environment: [:], options: FBProcessLaunchOptions()), "/usr/bin")),
   (["launch_xctest", "/usr/bin", Fixtures.application.path], Action.LaunchXCTest(FBApplicationLaunchConfiguration(bundleID: Fixtures.application.bundleID, bundleName: nil, arguments: [], environment: [:], options: FBProcessLaunchOptions()), "/usr/bin")),
   (["launch_xctest", "/usr/bin", Fixtures.application.path], Action.LaunchXCTest(FBApplicationLaunchConfiguration(bundleID: Fixtures.application.bundleID, bundleName: nil, arguments: [], environment: [:], options: FBProcessLaunchOptions()), "/usr/bin")),
@@ -236,18 +234,29 @@ class CommandParserTests : XCTestCase {
   }
 
   func testParsesListBootListenShutdown() {
+    let compoundComponents = [
+      ["list"], ["boot"], ["listen", "--http", "1000"], ["shutdown"],
+    ]
     let actions: [Action] = [Action.List, Action.Boot(nil), Action.Listen(Server.Http(1000)), Action.Shutdown]
-    let suffix: [String] = ["list", "boot", "listen", "--http", "1000", "shutdown"]
-    self.assertWithDefaultActions(actions, suffix: suffix)
+    self.assertParsesImplodingCompoundActions(actions, compoundComponents: compoundComponents)
   }
 
   func testParsesListBootListenShutdownDiagnose() {
+    let compoundComponents = [
+      ["list"], ["create", "iPhone 5"], ["boot", "--direct-launch"], ["listen", "--http", "8090"], ["shutdown"], ["diagnose"],
+    ]
     let launchConfiguration = FBSimulatorLaunchConfiguration.withOptions(FBSimulatorLaunchOptions.EnableDirectLaunch)
     let creationConfiguration = CreationConfiguration(osVersion: nil, deviceType: FBControlCoreConfiguration_Device_iPhone5(), auxDirectory: nil)
     let diagnoseAction = Action.Diagnose(FBSimulatorDiagnosticQuery.all(), DiagnosticFormat.CurrentFormat)
     let actions: [Action] = [Action.List, Action.Create(creationConfiguration), Action.Boot(launchConfiguration), Action.Listen(Server.Http(8090)), Action.Shutdown, diagnoseAction]
-    let suffix: [String] = ["list", "create", "iPhone 5", "boot", "--direct-launch", "listen", "--http", "8090", "shutdown", "diagnose"]
-    self.assertWithDefaultActions(actions, suffix: suffix)
+    self.assertParsesImplodingCompoundActions(actions, compoundComponents: compoundComponents)
+  }
+
+  func testFailsToParseDanglingTokens() {
+    let compoundComponents = [
+      ["list"], ["create", "iPhone 5"], ["boot", "--direct-launch"], ["listen", "--http", "8090"], ["YOLO"],
+    ]
+    self.assertFailsToParseImplodingCompoundActions(compoundComponents)
   }
 
   func testParsesMultipleConsecutiveLaunches() {
@@ -259,7 +268,7 @@ class CommandParserTests : XCTestCase {
   }
 
   func assertWithDefaultAction(action: Action, suffix: [String]) {
-    assertWithDefaultActions([action], suffix: suffix)
+    self.assertWithDefaultActions([action], suffix: suffix)
   }
 
   func assertWithDefaultActions(actions: [Action], suffix: [String]) {
@@ -274,10 +283,30 @@ class CommandParserTests : XCTestCase {
     ])
   }
 
+  func assertParsesImplodingCompoundActions(actions: [Action], compoundComponents: [[String]]) {
+    for suffix in CommandParserTests.implodeCompoundActions(compoundComponents) {
+      self.assertWithDefaultActions(actions, suffix: suffix)
+    }
+  }
+
+  func assertFailsToParseImplodingCompoundActions(compoundComponents: [[String]]) {
+    self.assertFailsToParseAll(
+      Command.parser,
+      CommandParserTests.implodeCompoundActions(compoundComponents)
+    )
+  }
+
   func unzipAndAssert(actions: [Action], suffix: [String], extras: [([String], FBiOSTargetQuery?, FBiOSTargetFormat?)]) {
     let pairs = extras.map { (tokens, query, format) in
       return (tokens + suffix, Command(configuration: Configuration.defaultValue, actions: actions, query: query, format: format))
     }
     self.assertParsesAll(Command.parser, pairs)
   }
+
+  static func implodeCompoundActions(compoundComponents: [[String]]) -> [[String]] {
+    return [
+      Array(compoundComponents.joinWithSeparator(["--"])),
+      Array(compoundComponents.joinWithSeparator([]))
+    ]
+  }
 }