util-app: roll up flag parsing errors

Problem

When parsing flags, we escape on the first parsing failure.
This behavior results in poor user experience and odd
side effects in certain scenarios.

For the user, if they have
multiple flag errors, they are forced to iterate and fix the
errors 1-by-1. Without knowing that there are other parse
failures to fix - this is a slow and time consuming feedback
loop.

In instances where an app references flag values as part of
the lifecycle (i.e. as part of cleaning up/closing the app), a
parse failure can result in remaining flags not being parsed,
which cascades when we fail when flags are referenced, but
not yet parsed, resulting in unclean shutdown of the app.

Solution

We will ensure that all flags get parsed consistently and that
we roll-up errors as a single error result. We will also print
a help message as part of a parse error, so that users are given
the reason their flags were failed to be parsed, along with the
expected usage for defined/available flags.

Result

A more streamlined experience for users attempting to remedy
mistaken flag definitions and better behavior when flags are
referenced during various phases of the application lifecycle.

JIRA Issues: CSL-11231

Differential Revision: https://phabricator.twitter.biz/D729700

Author: enbnt

Diff for: CHANGELOG.rst

@@ -6,14 +6,12 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com
 
 Unreleased
 ----------
+
 New Features
 ~~~~~~~~~~~~
 
 * util-stats: Counter, Gauge, and Stat can be instrumented with descriptions. ``PHAB_ID = D615481``
 
-New Features
-~~~~~~~~~~~~
-
 * util-cache: Experimentally crossbuilds with Scala 3. ``PHAB_ID=D714304``.
 
 * util-cache-guava: Experimentally crossbuilds with Scala 3. ``PHAB_ID=D716101``.
@@ -28,6 +26,12 @@ New Features
 
 * util-zk-test: Experimentally crossbuilds with Scala 3. ``PHAB_ID=D720603``
 
+* util-app: Flags parsing will now roll-up multiple flag parsing errors into a single
+  error message. When an error is encountered, flag parsing will continue to collect parse error
+  information instead of escaping on the first flag failure. After parsing all flags, if any errors
+  are present, a message containing all of the failed flags and their error reason,
+  along with the `help` usage message will be emitted. ```PHAB_ID=D729700``
+
 Runtime Behavior Changes
 ~~~~~~~~~~~~~~~~~~~~~~~~
 

Diff for: util-app/src/main/scala/com/twitter/app/Flags.scala

@@ -49,7 +49,9 @@ object Flags {
    *
    * @param reason A string explaining the error that occurred.
    */
-  case class Error(reason: String) extends FlagParseResult
+  case class Error(reason: String) extends FlagParseResult {
+    override def toString: String = reason
+  }
 }
 
 /**
@@ -128,6 +130,7 @@ final class Flags(argv0: String, includeGlobal: Boolean, failFastUntilParsed: Bo
     synchronized {
       reset()
       val remaining = new ArrayBuffer[String]
+      val errors = new ArrayBuffer[Error]
       var i = 0
       while (i < args.length) {
         val a = args(i)
@@ -144,7 +147,7 @@ final class Flags(argv0: String, includeGlobal: Boolean, failFastUntilParsed: Bo
               if (allowUndefinedFlags)
                 remaining += a
               else
-                return Error(
+                errors += Error(
                   "Error parsing flag \"%s\": %s".format(k, FlagUndefinedMessage)
                 )
 
@@ -153,7 +156,7 @@ final class Flags(argv0: String, includeGlobal: Boolean, failFastUntilParsed: Bo
               if (allowUndefinedFlags)
                 remaining += a
               else
-                return Error(
+                errors += Error(
                   "Error parsing flag \"%s\": %s".format(k, FlagUndefinedMessage)
                 )
 
@@ -163,7 +166,7 @@ final class Flags(argv0: String, includeGlobal: Boolean, failFastUntilParsed: Bo
 
             // Mandatory argument without a value and with no more arguments.
             case Array(k) if i == args.length =>
-              return Error(
+              errors += Error(
                 "Error parsing flag \"%s\": %s".format(k, FlagValueRequiredMessage)
               )
 
@@ -173,7 +176,7 @@ final class Flags(argv0: String, includeGlobal: Boolean, failFastUntilParsed: Bo
               try flag(k).parse(args(i - 1))
               catch {
                 case NonFatal(e) =>
-                  return Error(
+                  errors += Error(
                     "Error parsing flag \"%s\": %s".format(k, e.getMessage)
                   )
               }
@@ -182,8 +185,8 @@ final class Flags(argv0: String, includeGlobal: Boolean, failFastUntilParsed: Bo
             case Array(k, v) =>
               try flag(k).parse(v)
               catch {
-                case e: Throwable =>
-                  return Error(
+                case NonFatal(e) =>
+                  errors += Error(
                     "Error parsing flag \"%s\": %s".format(k, e.getMessage)
                   )
               }
@@ -196,6 +199,8 @@ final class Flags(argv0: String, includeGlobal: Boolean, failFastUntilParsed: Bo
 
       if (helpFlag())
         Help(usage)
+      else if (errors.nonEmpty)
+        Error(s"Error parsing flags: ${errors.mkString("[\n  ", ",\n  ", "\n]")}\n\n$usage")
       else
         Ok(remaining.toSeq)
     }

Diff for: util-app/src/test/scala/com/twitter/app/FlagsTest.scala

@@ -165,6 +165,18 @@ class FlagsTest extends AnyFunSuite {
     )
   }
 
+  test("Flag: multiple parse errors roll-up") {
+    val ctx = new Ctx
+    import ctx._
+    flag.parseArgs(Array("-undefined", "-foo", "blah")) match {
+      case Flags.Error(reason) =>
+        assert(reason.contains("Error parsing flag \"undefined\": flag undefined"))
+        assert(reason.contains("Error parsing flag \"foo\": For input string: \"blah\""))
+        assert(reason.contains("usage:"))
+      case other => fail(s"expected a flag error, but received $other")
+    }
+  }
+
   test("formatFlagValues") {
 
     val flagWithGlobal = new Flags("my", includeGlobal = true)