Make associative container out-of-range exception provide missing key
authorOgnjen Dragoljevic <plamenko@fb.com>
Wed, 1 Nov 2017 22:54:38 +0000 (15:54 -0700)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Wed, 1 Nov 2017 23:10:02 +0000 (16:10 -0700)
Summary:
When the key is missing, the standard associative containers throw an exception like `unordered_map::at: key not found`. That's nice, but it would be even better if we would actually know what key is missing. This is not an issue when one is accessing the container directly, but when that access happens several levels deep, such as here within folly formatting logic, we have no way of knowing what key was missing. This poses some difficulties in presenting error to the user.
This change makes folly format throw a subclass of `std::out_of_range` exception on a missing key when a string keyed associative container is used for providing parameters. That subclass stores the actual key used so it can be accessed in the exception handler. Existing callers can still catch `std::out_of_range` so they should be unaffected by this change.

Reviewed By: ot, yfeldblum

Differential Revision: D6202184

fbshipit-source-id: b8a6740aaccc5d8914ad7d099c8b901427f00083

folly/Format-inl.h
folly/Format.cpp
folly/Format.h
folly/test/FormatTest.cpp

index 60fdf24..b97375c 100644 (file)
@@ -27,6 +27,7 @@
 
 #include <folly/Exception.h>
 #include <folly/FormatTraits.h>
+#include <folly/MapUtil.h>
 #include <folly/Traits.h>
 #include <folly/portability/Windows.h>
 
@@ -950,7 +951,10 @@ struct KeyableTraitsAssoc : public FormatTraitsBase {
   typedef typename T::key_type key_type;
   typedef typename T::value_type::second_type value_type;
   static const value_type& at(const T& map, StringPiece key) {
-    return map.at(KeyFromStringPiece<key_type>::convert(key));
+    if (auto ptr = get_ptr(map, KeyFromStringPiece<key_type>::convert(key))) {
+      return *ptr;
+    }
+    detail::throwFormatKeyNotFoundException(key);
   }
   static const value_type&
   at(const T& map, StringPiece key, const value_type& dflt) {
index 380cddb..9ad5395 100644 (file)
@@ -356,4 +356,15 @@ void insertThousandsGroupingUnsafe(char* start_buffer, char** end_buffer) {
 }
 } // namespace detail
 
+FormatKeyNotFoundException::FormatKeyNotFoundException(StringPiece key)
+    : std::out_of_range(kMessagePrefix.str() + key.str()) {}
+
+constexpr StringPiece const FormatKeyNotFoundException::kMessagePrefix;
+
+namespace detail {
+[[noreturn]] void throwFormatKeyNotFoundException(StringPiece key) {
+  throw FormatKeyNotFoundException(key);
+}
+} // namespace detail
+
 } // namespace folly
index 3837d10..b8df86f 100644 (file)
@@ -18,6 +18,7 @@
 #define FOLLY_FORMAT_H_
 
 #include <cstdio>
+#include <stdexcept>
 #include <tuple>
 #include <type_traits>
 
@@ -306,6 +307,33 @@ inline std::string svformat(StringPiece fmt, Container&& container) {
   return vformat(fmt, std::forward<Container>(container)).str();
 }
 
+/**
+ * Exception class thrown when a format key is not found in the given
+ * associative container keyed by strings. We inherit std::out_of_range for
+ * compatibility with callers that expect exception to be thrown directly
+ * by std::map or std::unordered_map.
+ *
+ * Having the key be at the end of the message string, we can access it by
+ * simply adding its offset to what(). Not storing separate std::string key
+ * makes the exception type small and noexcept-copyable like std::out_of_range,
+ * and therefore able to fit in-situ in exception_wrapper.
+ */
+class FormatKeyNotFoundException : public std::out_of_range {
+ public:
+  explicit FormatKeyNotFoundException(StringPiece key);
+
+  char const* key() const noexcept {
+    return what() + kMessagePrefix.size();
+  }
+
+ private:
+  static constexpr StringPiece const kMessagePrefix = "format key not found: ";
+};
+
+namespace detail {
+[[noreturn]] void throwFormatKeyNotFoundException(StringPiece key);
+} // namespace detail
+
 /**
  * Wrap a sequence or associative container so that out-of-range lookups
  * return a default value rather than throwing an exception.
index 3bb739b..e84bc86 100644 (file)
@@ -162,6 +162,12 @@ TEST(Format, Simple) {
   EXPECT_EQ("worldXX", svformat("{hello:X<7}", defaulted(m2, "meow")));
   EXPECT_EQ("meowXXX", sformat("{[none]:X<7}", defaulted(m2, "meow")));
   EXPECT_EQ("meowXXX", svformat("{none:X<7}", defaulted(m2, "meow")));
+  try {
+    svformat("{none:X<7}", m2);
+    EXPECT_FALSE(true) << "svformat should throw on missing key";
+  } catch (const FormatKeyNotFoundException& e) {
+    EXPECT_STREQ("none", e.key());
+  }
 
   // Test indexing in strings
   EXPECT_EQ("61 62", sformat("{0[0]:x} {0[1]:x}", "abcde"));