abort instead of throwing an exception on bad format strings

Summary:
Errors that happen during format are almost always programmer
error in the form of a bad format string. Abort instead of throwing in
these cases.

Test Plan: automated tests, added a couple new tests

Reviewed By: tudorb@fb.com

FB internal diff: D947470

diff --git a/folly/Format-inl.h b/folly/Format-inl.h
index 70672d476ebedc710194fe14abdb67c239e8984b..620557a6b435a80b139b241975952f9545d8dafb 100644 (file)
--- a/folly/Format-inl.h
+++ b/folly/Format-inl.h
@@ -168,10 +168,7 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
       out(StringPiece(p, q));
       p = q;
 
-      if (p == end || *p != '}') {
-        throw std::invalid_argument(
-            "folly::format: single '}' in format string");
-      }
+      CHECK(p != end && *p == '}') << "single '}' in format string";
       ++p;
     }
   };
@@ -188,10 +185,7 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
     outputString(StringPiece(p, q));
     p = q + 1;
 
-    if (p == end) {
-      throw std::invalid_argument(
-          "folly::format: '}' at end of format string");
-    }
+    CHECK(p != end) << "'{' at end of format string";
 
     // "{{" -> "{"
     if (*p == '{') {
@@ -202,9 +196,7 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
 
     // Format string
     q = static_cast<const char*>(memchr(p, '}', end - p));
-    if (q == end) {
-      throw std::invalid_argument("folly::format: missing ending '}'");
-    }
+    CHECK(q != end) << "missing ending '}'";
     FormatArg arg(StringPiece(p, q));
     p = q + 1;
 
@@ -226,17 +218,16 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
         try {
           argIndex = to<int>(piece);
         } catch (const std::out_of_range& e) {
-          arg.error("argument index must be integer");
+          LOG(FATAL) << "argument index must be integer";
         }
-        arg.enforce(argIndex >= 0, "argument index must be non-negative");
+        CHECK(argIndex >= 0)
+          << arg.errorStr("argument index must be non-negative");
         hasExplicitArgIndex = true;
       }
     }
 
-    if (hasDefaultArgIndex && hasExplicitArgIndex) {
-      throw std::invalid_argument(
-          "folly::format: may not have both default and explicit arg indexes");
-    }
+    CHECK(!hasDefaultArgIndex || !hasExplicitArgIndex)
+      << "may not have both default and explicit arg indexes";
 
     doFormat(argIndex, arg, out);
   }
@@ -400,8 +391,8 @@ class FormatValue<
       uval = val_;
       sign = '\0';
 
-      arg.enforce(arg.sign == FormatArg::Sign::DEFAULT,
-                  "sign specifications not allowed for unsigned values");
+      CHECK(arg.sign == FormatArg::Sign::DEFAULT)
+        << arg.errorStr("sign specifications not allowed for unsigned values");
     }
 
     // max of:
@@ -428,9 +419,9 @@ class FormatValue<
     switch (presentation) {
     case 'n':  // TODO(tudorb): locale awareness?
     case 'd':
-      arg.enforce(!arg.basePrefix,
-                  "base prefix not allowed with '", presentation,
-                  "' specifier");
+      CHECK(!arg.basePrefix)
+        << arg.errorStr("base prefix not allowed with '", presentation,
+                        "' specifier");
       if (arg.thousandsSeparator) {
         useSprintf("%'ju");
       } else {
@@ -440,21 +431,21 @@ class FormatValue<
       }
       break;
     case 'c':
-      arg.enforce(!arg.basePrefix,
-                  "base prefix not allowed with '", presentation,
-                  "' specifier");
-      arg.enforce(!arg.thousandsSeparator,
-                  "thousands separator (',') not allowed with '",
-                  presentation, "' specifier");
+      CHECK(!arg.basePrefix)
+        << arg.errorStr("base prefix not allowed with '", presentation,
+                        "' specifier");
+      CHECK(!arg.thousandsSeparator)
+        << arg.errorStr("thousands separator (',') not allowed with '",
+                        presentation, "' specifier");
       valBufBegin = valBuf + 3;
       *valBufBegin = static_cast<char>(uval);
       valBufEnd = valBufBegin + 1;
       break;
     case 'o':
     case 'O':
-      arg.enforce(!arg.thousandsSeparator,
-                  "thousands separator (',') not allowed with '",
-                  presentation, "' specifier");
+      CHECK(!arg.thousandsSeparator)
+        << arg.errorStr("thousands separator (',') not allowed with '",
+                        presentation, "' specifier");
       valBufEnd = valBuf + valBufSize - 1;
       valBufBegin = valBuf + detail::uintToOctal(valBuf, valBufSize - 1, uval);
       if (arg.basePrefix) {
@@ -463,9 +454,9 @@ class FormatValue<
       }
       break;
     case 'x':
-      arg.enforce(!arg.thousandsSeparator,
-                  "thousands separator (',') not allowed with '",
-                  presentation, "' specifier");
+      CHECK(!arg.thousandsSeparator)
+        << arg.errorStr("thousands separator (',') not allowed with '",
+                        presentation, "' specifier");
       valBufEnd = valBuf + valBufSize - 1;
       valBufBegin = valBuf + detail::uintToHexLower(valBuf, valBufSize - 1,
                                                     uval);
@@ -476,9 +467,9 @@ class FormatValue<
       }
       break;
     case 'X':
-      arg.enforce(!arg.thousandsSeparator,
-                  "thousands separator (',') not allowed with '",
-                  presentation, "' specifier");
+      CHECK(!arg.thousandsSeparator)
+        << arg.errorStr("thousands separator (',') not allowed with '",
+                        presentation, "' specifier");
       valBufEnd = valBuf + valBufSize - 1;
       valBufBegin = valBuf + detail::uintToHexUpper(valBuf, valBufSize - 1,
                                                     uval);
@@ -490,9 +481,9 @@ class FormatValue<
       break;
     case 'b':
     case 'B':
-      arg.enforce(!arg.thousandsSeparator,
-                  "thousands separator (',') not allowed with '",
-                  presentation, "' specifier");
+      CHECK(!arg.thousandsSeparator)
+        << arg.errorStr("thousands separator (',') not allowed with '",
+                        presentation, "' specifier");
       valBufEnd = valBuf + valBufSize - 1;
       valBufBegin = valBuf + detail::uintToBinary(valBuf, valBufSize - 1,
                                                   uval);
@@ -503,7 +494,7 @@ class FormatValue<
       }
       break;
     default:
-      arg.error("invalid specifier '", presentation, "'");
+      LOG(FATAL) << arg.errorStr("invalid specifier '", presentation, "'");
     }
 
     if (sign) {
@@ -645,7 +636,7 @@ class FormatValue<double> {
       }
       break;
     default:
-      arg.error("invalid specifier '", arg.presentation, "'");
+      LOG(FATAL) << arg.errorStr("invalid specifier '", arg.presentation, "'");
     }
 
     int len = builder.position();
@@ -702,9 +693,9 @@ class FormatValue<
   void format(FormatArg& arg, FormatCallback& cb) const {
     if (arg.keyEmpty()) {
       arg.validate(FormatArg::Type::OTHER);
-      arg.enforce(arg.presentation == FormatArg::kDefaultPresentation ||
-                  arg.presentation == 's',
-                  "invalid specifier '", arg.presentation, "'");
+      CHECK(arg.presentation == FormatArg::kDefaultPresentation ||
+            arg.presentation == 's')
+        << arg.errorStr("invalid specifier '", arg.presentation, "'");
       format_value::formatString(val_, arg, cb);
     } else {
       FormatValue<char>(val_.at(arg.splitIntKey())).format(arg, cb);
@@ -724,8 +715,8 @@ class FormatValue<std::nullptr_t> {
   template <class FormatCallback>
   void format(FormatArg& arg, FormatCallback& cb) const {
     arg.validate(FormatArg::Type::OTHER);
-    arg.enforce(arg.presentation == FormatArg::kDefaultPresentation,
-                "invalid specifier '", arg.presentation, "'");
+    CHECK(arg.presentation == FormatArg::kDefaultPresentation)
+      << arg.errorStr("invalid specifier '", arg.presentation, "'");
     format_value::formatString("(null)", arg, cb);
   }
 };
@@ -775,8 +766,8 @@ class FormatValue<
     } else {
       // Print as a pointer, in hex.
       arg.validate(FormatArg::Type::OTHER);
-      arg.enforce(arg.presentation == FormatArg::kDefaultPresentation,
-                  "invalid specifier '", arg.presentation, "'");
+      CHECK(arg.presentation == FormatArg::kDefaultPresentation)
+        << arg.errorStr("invalid specifier '", arg.presentation, "'");
       arg.basePrefix = true;
       arg.presentation = 'x';
       if (arg.align == FormatArg::Align::DEFAULT) {
@@ -796,7 +787,7 @@ class TryFormatValue {
  public:
   template <class FormatCallback>
   static void formatOrFail(T& value, FormatArg& arg, FormatCallback& cb) {
-    arg.error("No formatter available for this type");
+    LOG(FATAL) << arg.errorStr("No formatter available for this type");
   }
 };
 
@@ -1037,7 +1028,7 @@ class FormatValue<std::pair<A, B>> {
       FormatValue<typename std::decay<B>::type>(val_.second).format(arg, cb);
       break;
     default:
-      arg.error("invalid index for pair");
+      LOG(FATAL) << arg.errorStr("invalid index for pair");
     }
   }
 
@@ -1055,7 +1046,7 @@ class FormatValue<std::tuple<Args...>> {
   template <class FormatCallback>
   void format(FormatArg& arg, FormatCallback& cb) const {
     int key = arg.splitIntKey();
-    arg.enforce(key >= 0, "tuple index must be non-negative");
+    CHECK(key >= 0) << arg.errorStr("tuple index must be non-negative");
     doFormat(key, arg, cb);
   }
 
@@ -1065,7 +1056,7 @@ class FormatValue<std::tuple<Args...>> {
   template <size_t K, class Callback>
   typename std::enable_if<K == valueCount>::type
   doFormatFrom(size_t i, FormatArg& arg, Callback& cb) const {
-    arg.enforce("tuple index out of range, max=", i);
+    LOG(FATAL) << arg.errorStr("tuple index out of range, max=", i);
   }
 
   template <size_t K, class Callback>

diff --git a/folly/Format.cpp b/folly/Format.cpp
index 385a9bb8b9d215bd1dfecd7cb78b18ec2a1f08d6..169eca087192066ddc1963c547727a81b70052d7 100644 (file)
--- a/folly/Format.cpp
+++ b/folly/Format.cpp
@@ -70,7 +70,7 @@ void FormatArg::initSlow() {
     }
 
     if (*p == '0') {
-      enforce(align == Align::DEFAULT, "alignment specified twice");
+      CHECK(align == Align::DEFAULT) << errorStr("alignment specified twice");
       fill = '0';
       align = Align::PAD_AFTER_SIGN;
       if (++p == end) return;
@@ -105,31 +105,31 @@ void FormatArg::initSlow() {
     if (++p == end) return;
   }
 
-  error("extra characters in format string");
+  LOG(FATAL) << "extra characters in format string";
 }
 
 void FormatArg::validate(Type type) const {
-  enforce(keyEmpty(), "index not allowed");
+  CHECK(keyEmpty()) << "index not allowed";
   switch (type) {
   case Type::INTEGER:
-    enforce(precision == kDefaultPrecision,
-            "precision not allowed on integers");
+    CHECK(precision == kDefaultPrecision)
+      << errorStr("precision not allowed on integers");
     break;
   case Type::FLOAT:
-    enforce(!basePrefix,
-            "base prefix ('#') specifier only allowed on integers");
-    enforce(!thousandsSeparator,
-            "thousands separator (',') only allowed on integers");
+    CHECK(!basePrefix)
+      << errorStr("base prefix ('#') specifier only allowed on integers");
+    CHECK(!thousandsSeparator)
+      << errorStr("thousands separator (',') only allowed on integers");
     break;
   case Type::OTHER:
-    enforce(align != Align::PAD_AFTER_SIGN,
-            "'='alignment only allowed on numbers");
-    enforce(sign == Sign::DEFAULT,
-            "sign specifier only allowed on numbers");
-    enforce(!basePrefix,
-            "base prefix ('#') specifier only allowed on integers");
-    enforce(!thousandsSeparator,
-            "thousands separator (',') only allowed on integers");
+    CHECK(align != Align::PAD_AFTER_SIGN)
+      << errorStr("'='alignment only allowed on numbers");
+    CHECK(sign == Sign::DEFAULT)
+      << errorStr("sign specifier only allowed on numbers");
+    CHECK(!basePrefix)
+      << errorStr("base prefix ('#') specifier only allowed on integers");
+    CHECK(!thousandsSeparator)
+      << errorStr("thousands separator (',') only allowed on integers");
     break;
   }
 }

diff --git a/folly/FormatArg.h b/folly/FormatArg.h
index 124d621d732f0fbf0511e584c1091c7e59647664..d48aad7a05b277f5b3c1866d7399a49f71b83e8a 100644 (file)
--- a/folly/FormatArg.h
+++ b/folly/FormatArg.h
@@ -71,8 +71,11 @@ struct FormatArg {
     }
   }
 
+  template <typename... Args>
+  std::string errorStr(Args&&... args) const;
   template <typename... Args>
   void error(Args&&... args) const FOLLY_NORETURN;
+
   /**
    * Full argument string, as passed in to the constructor.
    */
@@ -185,16 +188,21 @@ struct FormatArg {
   NextKeyMode nextKeyMode_;
 };
 
+template <typename... Args>
+inline std::string FormatArg::errorStr(Args&&... args) const {
+  return to<std::string>(
+    "invalid format argument {", fullArgString, "}: ",
+    std::forward<Args>(args)...);
+}
+
 template <typename... Args>
 inline void FormatArg::error(Args&&... args) const {
-  throw std::invalid_argument(to<std::string>(
-      "folly::format: invalid format argument {", fullArgString, "}: ",
-      std::forward<Args>(args)...));
+  throw std::invalid_argument(errorStr(std::forward<Args>(args)...));
 }
 
 template <bool emptyOk>
 inline StringPiece FormatArg::splitKey() {
-  enforce(nextKeyMode_ != NextKeyMode::INT, "integer key expected");
+  CHECK(nextKeyMode_ != NextKeyMode::INT) << errorStr("integer key expected");
   return doSplitKey<emptyOk>();
 }
 
@@ -202,16 +210,12 @@ template <bool emptyOk>
 inline StringPiece FormatArg::doSplitKey() {
   if (nextKeyMode_ == NextKeyMode::STRING) {
     nextKeyMode_ = NextKeyMode::NONE;
-    if (!emptyOk) {  // static
-      enforce(!nextKey_.empty(), "non-empty key required");
-    }
+    CHECK(emptyOk || !nextKey_.empty()) << errorStr("non-empty key required");
     return nextKey_;
   }
 
   if (key_.empty()) {
-    if (!emptyOk) {  // static
-      error("non-empty key required");
-    }
+    CHECK(emptyOk) << errorStr("non-empty key required");
     return StringPiece();
   }
 
@@ -221,7 +225,7 @@ inline StringPiece FormatArg::doSplitKey() {
   if (e[-1] == ']') {
     --e;
     p = static_cast<const char*>(memchr(b, '[', e - b));
-    enforce(p, "unmatched ']'");
+    CHECK(p) << errorStr("unmatched ']'");
   } else {
     p = static_cast<const char*>(memchr(b, '.', e - b));
   }
@@ -231,9 +235,8 @@ inline StringPiece FormatArg::doSplitKey() {
     p = e;
     key_.clear();
   }
-  if (!emptyOk) {  // static
-    enforce(b != p, "non-empty key required");
-  }
+  CHECK(emptyOk || b != p) << errorStr("non-empty key required");
+
   return StringPiece(b, p);
 }
 
@@ -245,7 +248,7 @@ inline int FormatArg::splitIntKey() {
   try {
     return to<int>(doSplitKey<true>());
   } catch (const std::out_of_range& e) {
-    error("integer key required");
+    LOG(FATAL) << errorStr("integer key required");
     return 0;  // unreached
   }
 }

diff --git a/folly/test/FormatTest.cpp b/folly/test/FormatTest.cpp
index 96de6bf68cafa7f7bd1ea6430a10d15c3ea061e6..0c41d7ae8d9a36e1946d2fc08d9665242197c112 100644 (file)
--- a/folly/test/FormatTest.cpp
+++ b/folly/test/FormatTest.cpp
@@ -275,20 +275,6 @@ TEST(Format, Custom) {
   EXPECT_NE("", fstr("{}", &kv));
 }
 
-namespace {
-
-struct Opaque {
-  int k;
-};
-
-} // namespace
-
-TEST(Format, Unformatted) {
-  Opaque o;
-  EXPECT_NE("", fstr("{}", &o));
-  EXPECT_THROW(fstr("{0[0]}", &o), std::invalid_argument);
-}
-
 TEST(Format, Nested) {
   EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, format("{} {}", 3, 4)));
   //
@@ -297,6 +283,16 @@ TEST(Format, Nested) {
   EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, saved));
 }
 
+TEST(Format, OutOfBounds) {
+  std::vector<int> ints{1, 2, 3, 4, 5};
+  EXPECT_EQ("1 3 5", fstr("{0[0]} {0[2]} {0[4]}", ints));
+  EXPECT_THROW(fstr("{[5]}", ints), std::out_of_range);
+
+  std::map<std::string, int> map{{"hello", 0}, {"world", 1}};
+  EXPECT_EQ("hello = 0", fstr("hello = {[hello]}", map));
+  EXPECT_THROW(fstr("{[nope]}", map), std::out_of_range);
+}
+
 int main(int argc, char *argv[]) {
   testing::InitGoogleTest(&argc, argv);
   google::ParseCommandLineFlags(&argc, &argv, true);