Fix signed to unsigned conversion

Summary:
`folly::to<uint32_t>()` and `folly::to<uint64_t>()` were silently failing
and truncating the input when negative values were passed:

```
int8_t x = -1;
folly::to<uint8_t>(x); // THROWS

int16_t x = -1;
folly::to<uint16_t>(x); // THROWS
```

all throw folly::ConversionError, but

```
int32_t x = -1;
folly::to<uint32_t>(x); // DOES NOT THROW

int64_t x = -1;
folly::to<uint64_t>(x); // DOES NOT THROW
```

The actual bug is in `less_than<>()`, from Traits.h, which in these cases relies that `(uint32)0 <= std::numeric_limits<int32_t>::min()`, which doesn't hold according to the standard.

The fixes in this diff are:

- Fix `less_than<>()` to properly support signed to unsigned conversions
- Fix `less_than<>()` to properly support unsigned to signed conversions
- Fix `greater_than<>()` to properly support signed to unsigned conversions
- Fix `greater_than<>()` to properly support unsigned to signed conversions
- Add unit tests to cover all these cases.

Reviewed By: yfeldblum

Differential Revision: D19511557

fbshipit-source-id: 48649dfcdadeaa652c8f57f7b11481de44a88927

folly/Traits.h

@@ -688,6 +688,30 @@ using IsOneOf = StrictDisjunction<std::is_same<T, Ts>...>;
  * @author: Marcelo Juchem <marcelo@fb.com>
  */
 
+// same as `x < 0`
+template <typename T>
+constexpr bool is_negative(T x) {
+  return std::is_signed<T>::value && x < T(0);
+}
+
+// same as `x <= 0`
+template <typename T>
+constexpr bool is_non_positive(T x) {
+  return !x || folly::is_negative(x);
+}
+
+// same as `x > 0`
+template <typename T>
+constexpr bool is_positive(T x) {
+  return !is_non_positive(x);
+}
+
+// same as `x >= 0`
+template <typename T>
+constexpr bool is_non_negative(T x) {
+  return !x || is_positive(x);
+}
+
 namespace detail {
 
 // folly::to integral specializations can end up generating code
@@ -705,6 +729,9 @@ template <typename RHS, RHS rhs, typename LHS>
 bool less_than_impl(LHS const lhs) {
   // clang-format off
   return
+      // Ensure signed and unsigned values won't be compared directly.
+      (!std::is_signed<RHS>::value && is_negative(lhs)) ? true :
+      (!std::is_signed<LHS>::value && is_negative(rhs)) ? false :
       rhs > std::numeric_limits<LHS>::max() ? true :
       rhs <= std::numeric_limits<LHS>::min() ? false :
       lhs < rhs;
@@ -715,6 +742,9 @@ template <typename RHS, RHS rhs, typename LHS>
 bool greater_than_impl(LHS const lhs) {
   // clang-format off
   return
+      // Ensure signed and unsigned values won't be compared directly.
+      (!std::is_signed<RHS>::value && is_negative(lhs)) ? false :
+      (!std::is_signed<LHS>::value && is_negative(rhs)) ? true :
       rhs > std::numeric_limits<LHS>::max() ? false :
       rhs < std::numeric_limits<LHS>::min() ? true :
       lhs > rhs;
@@ -725,30 +755,6 @@ FOLLY_POP_WARNING
 
 } // namespace detail
 
-// same as `x < 0`
-template <typename T>
-constexpr bool is_negative(T x) {
-  return std::is_signed<T>::value && x < T(0);
-}
-
-// same as `x <= 0`
-template <typename T>
-constexpr bool is_non_positive(T x) {
-  return !x || folly::is_negative(x);
-}
-
-// same as `x > 0`
-template <typename T>
-constexpr bool is_positive(T x) {
-  return !is_non_positive(x);
-}
-
-// same as `x >= 0`
-template <typename T>
-constexpr bool is_non_negative(T x) {
-  return !x || is_positive(x);
-}
-
 template <typename RHS, RHS rhs, typename LHS>
 bool less_than(LHS const lhs) {
   return detail::

folly/test/ConvTest.cpp

@@ -1041,9 +1041,59 @@ std::string prefixWithType(V value) {
   EXPECT_CONV_ERROR_QUOTE(                       \
       to<type>(val), code, prefixWithType<type>(val).c_str(), false)
 
+template <typename TUnsigned>
+void unsignedUnderflow() {
+  EXPECT_CONV_ERROR_ARITH(
+      TUnsigned, std::numeric_limits<int8_t>::min(), ARITH_NEGATIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      TUnsigned, std::numeric_limits<int16_t>::min(), ARITH_NEGATIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      TUnsigned, std::numeric_limits<int32_t>::min(), ARITH_NEGATIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      TUnsigned, std::numeric_limits<int64_t>::min(), ARITH_NEGATIVE_OVERFLOW);
+}
+
 TEST(Conv, ConversionErrorIntToInt) {
-  EXPECT_CONV_ERROR_ARITH(signed char, 128, ARITH_POSITIVE_OVERFLOW);
-  EXPECT_CONV_ERROR_ARITH(unsigned char, -1, ARITH_NEGATIVE_OVERFLOW);
+  // Test overflow upper bound. First unsigned to signed.
+  EXPECT_CONV_ERROR_ARITH(
+      int8_t, std::numeric_limits<uint8_t>::max(), ARITH_POSITIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      int16_t, std::numeric_limits<uint16_t>::max(), ARITH_POSITIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      int32_t, std::numeric_limits<uint32_t>::max(), ARITH_POSITIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      int64_t, std::numeric_limits<uint64_t>::max(), ARITH_POSITIVE_OVERFLOW);
+
+  // Signed to signed.
+  EXPECT_CONV_ERROR_ARITH(
+      int8_t, std::numeric_limits<int16_t>::max(), ARITH_POSITIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      int16_t, std::numeric_limits<int32_t>::max(), ARITH_POSITIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      int32_t, std::numeric_limits<int64_t>::max(), ARITH_POSITIVE_OVERFLOW);
+
+  // Unsigned to unsigned.
+  EXPECT_CONV_ERROR_ARITH(
+      uint8_t, std::numeric_limits<uint16_t>::max(), ARITH_POSITIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      uint16_t, std::numeric_limits<uint32_t>::max(), ARITH_POSITIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      uint32_t, std::numeric_limits<uint64_t>::max(), ARITH_POSITIVE_OVERFLOW);
+
+  // Test underflows from signed to unsigned data types. Make sure we test all
+  // combinations.
+  unsignedUnderflow<uint8_t>();
+  unsignedUnderflow<uint16_t>();
+  unsignedUnderflow<uint32_t>();
+  unsignedUnderflow<uint64_t>();
+
+  // Signed to signed.
+  EXPECT_CONV_ERROR_ARITH(
+      int8_t, std::numeric_limits<int16_t>::min(), ARITH_NEGATIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      int16_t, std::numeric_limits<int32_t>::min(), ARITH_NEGATIVE_OVERFLOW);
+  EXPECT_CONV_ERROR_ARITH(
+      int32_t, std::numeric_limits<int64_t>::min(), ARITH_NEGATIVE_OVERFLOW);
 }
 
 TEST(Conv, ConversionErrorFloatToFloat) {

folly/test/TraitsTest.cpp

@@ -173,10 +173,31 @@ TEST(Traits, relational) {
   EXPECT_FALSE((folly::less_than<uint8_t, 255u, uint8_t>(255u)));
   EXPECT_TRUE((folly::less_than<uint8_t, 255u, uint8_t>(254u)));
 
+  // Making sure signed to unsigned comparisons are not truncated.
+  EXPECT_TRUE((folly::less_than<uint8_t, 0, int8_t>(-1)));
+  EXPECT_TRUE((folly::less_than<uint16_t, 0, int16_t>(-1)));
+  EXPECT_TRUE((folly::less_than<uint32_t, 0, int32_t>(-1)));
+  EXPECT_TRUE((folly::less_than<uint64_t, 0, int64_t>(-1)));
+
+  EXPECT_FALSE((folly::less_than<int8_t, -1, uint8_t>(0)));
+  EXPECT_FALSE((folly::less_than<int16_t, -1, uint16_t>(0)));
+  EXPECT_FALSE((folly::less_than<int32_t, -1, uint32_t>(0)));
+  EXPECT_FALSE((folly::less_than<int64_t, -1, uint64_t>(0)));
+
   EXPECT_FALSE((folly::greater_than<uint8_t, 0u, uint8_t>(0u)));
   EXPECT_TRUE((folly::greater_than<uint8_t, 0u, uint8_t>(254u)));
   EXPECT_FALSE((folly::greater_than<uint8_t, 255u, uint8_t>(255u)));
   EXPECT_FALSE((folly::greater_than<uint8_t, 255u, uint8_t>(254u)));
+
+  EXPECT_FALSE((folly::greater_than<uint8_t, 0, int8_t>(-1)));
+  EXPECT_FALSE((folly::greater_than<uint16_t, 0, int16_t>(-1)));
+  EXPECT_FALSE((folly::greater_than<uint32_t, 0, int32_t>(-1)));
+  EXPECT_FALSE((folly::greater_than<uint64_t, 0, int64_t>(-1)));
+
+  EXPECT_TRUE((folly::greater_than<int8_t, -1, uint8_t>(0)));
+  EXPECT_TRUE((folly::greater_than<int16_t, -1, uint16_t>(0)));
+  EXPECT_TRUE((folly::greater_than<int32_t, -1, uint32_t>(0)));
+  EXPECT_TRUE((folly::greater_than<int64_t, -1, uint64_t>(0)));
 }
 
 #if FOLLY_HAVE_INT128_T