feat: add more rules for parsing

src/facade/cmd_arg_parser.h

@@ -8,6 +8,7 @@
 #include <absl/strings/numbers.h>
 
 #include <cassert>
+#include <cmath>
 #include <concepts>
 #include <optional>
 #include <string_view>
@@ -21,6 +22,11 @@ namespace facade {
 
 // CmdArgParser — utility for parsing command option lists.
 //
+// Ideology: parse everything, then check the error ONCE at the end. The first error is latched and
+// every later read becomes a no-op, so intermediate `if (HasError())` guards are unnecessary. Fold
+// value checks into the read via Next<FInt<...>>() / Next<Validated<...>>() instead of post-parse
+// `if` blocks.
+//
 // Reading individual args:
 //   CmdArgParser parser(args);
 //   auto key = parser.Next<string_view>();                      // read one arg by type
@@ -29,6 +35,7 @@ namespace facade {
 //                                                               // (INVALID_INT if out of range)
 //   auto f  = parser.Next<FInt<1, 99>>("bad f");                // FInt with a custom out-of-range
 //                                                               // / non-integer error message
+//   auto s  = parser.Next<Validated<double, NotNan<kMsg>>>();   // parse + custom-error rule check
 //   auto count = parser.NextOrDefault<size_t>(10);              // read optional with default
 //   Range fields = parser.NextRange();                         // [N, e1..eN] counted list
 //   Range pairs  = parser.NextRange(2);                        // [N, f1,v1,..] N field/value pairs
@@ -77,34 +84,74 @@ namespace facade {
 //   parser.ReportCustom("bad option");                          // inject a custom error (no-op if
 //                                                               // one is already set)
 
-// A validated number for Next<T>(): a VNum-derived type that adds a static validate()
-// predicate. Convert<T>() parses the underlying value, runs validate(), and reports
-// INVALID_INT/INVALID_FLOAT on failure. FInt (below) is the built-in range-restricted integer;
-// domain-specific double validators are defined next to their callers.
+// Result of a validation rule: `failed` marks rejection. A non-empty `msg` is reported verbatim;
+// an empty one leaves the generic type error (INVALID_INT / INVALID_FLOAT), chosen by the caller.
+struct RuleError {
+  bool failed = false;
+  std::string_view msg = {};
+};
+
 template <class T>
-concept as_vnum = requires(T t, typename T::underlying_t v) {
-  static_cast<typename T::underlying_t>(t);
-  { T::validate(v) } -> std::same_as<bool>;
+concept as_vnum = requires(T t) {
+  static_cast<decltype(t.value)>(t);
+  { T::validate(t.value) } -> std::same_as<RuleError>;
 };
 
-// Base for as_vnum types: stores the parsed value and converts back to it. Derive and add a static
-// validate() predicate to define a new validated number.
+// Base for validated numbers: holds the parsed value and converts back to it.
 template <class T> struct VNum {
-  using underlying_t = T;
-  underlying_t value = {};
-  operator underlying_t() const {
+  T value = {};
+  operator T() const {
     return value;
   }
 };
 
-// Range-restricted integer used with Next<FInt<lo, hi>>() (INVALID_INT if out of range).
-template <auto min, auto max> struct FInt : VNum<decltype(min)> {
+// Validation rules for Next<Validated<T, Rules...>>(): free functions `RuleError rule(T)`. Reusable
+// ones take the message as a reference-to-constexpr NTTP. They also validate values from elsewhere:
+//   if (auto e = SomeRule<...>(v); e.failed) parser.ReportCustom(std::string{e.msg});
+
+// Out of [min, max] -> generic type error (FInt is the idiomatic spelling).
+template <auto min, auto max> RuleError InRange(decltype(min) v) {
   static_assert(std::is_same_v<decltype(min), decltype(max)>, "inconsistent types");
-  static constexpr bool validate(decltype(min) v) {
-    return v >= min && v <= max;
+  return {v < min || v > max, {}};
+}
+
+// Out of [min, max] -> custom Msg. Integer bounds only (floating-point NTTPs need clang 18+; use a
+// dedicated rule for float ranges).
+template <auto min, auto max, const auto& Msg> RuleError Bounded(decltype(min) v) {
+  static_assert(std::is_same_v<decltype(min), decltype(max)>, "inconsistent types");
+  return {!(v >= min && v <= max), Msg};
+}
+
+// v < 0 -> custom Msg; NaN is accepted (matches a plain `v < 0` guard).
+template <const auto& Msg, class V> RuleError NonNegative(V v) {
+  return {v < 0, Msg};
+}
+
+// Rejects NaN but accepts +/-inf.
+template <const auto& Msg, std::floating_point V> RuleError NotNan(V v) {
+  return {std::isnan(v), Msg};
+}
+
+template <const auto& Msg, std::floating_point V> RuleError Finite(V v) {
+  return {!std::isfinite(v), Msg};
+}
+
+template <auto Bad, const auto& Msg> RuleError NotEq(decltype(Bad) v) {
+  return {v == Bad, Msg};
+}
+
+// Accepts a value only if every rule accepts it; first rejection wins. Each rule is a function
+// T -> RuleError, e.g. Validated<int, Bounded<0, 9, kMsg>>.
+template <class T, RuleError (*... Rules)(T)> struct Validated : VNum<T> {
+  static RuleError validate(T v) {
+    RuleError e;
+    (void)((e = Rules(v)).failed || ...);
+    return e;
   }
 };
 
+template <auto min, auto max> using FInt = Validated<decltype(min), InRange<min, max>>;
+
 template <class T> constexpr bool is_optional = false;
 
 template <class U> constexpr bool is_optional<std::optional<U>> = true;
@@ -119,7 +166,7 @@ struct CmdArgParser {
     INVALID_CASES,
     INVALID_NEXT,
     UNPROCESSED,
-    CUSTOM_ERROR  // should be the last one
+    CUSTOM_ERROR  // keep last
   };
 
   struct ErrorInfo {
@@ -128,7 +175,7 @@ struct CmdArgParser {
     std::string custom_msg;
 
     operator bool() const {
-      return type != ErrorType::NO_ERROR;
+      return type != NO_ERROR;
     }
     ErrorReply MakeReply() const;
   };
@@ -242,13 +289,14 @@ struct CmdArgParser {
     }
   }
 
-  // Like Next<T>(), but on a failed read (non-numeric, out-of-range FInt, or missing arg) replaces
-  // the generic error with a caller-supplied CUSTOM_ERROR message. Pair with FInt<lo,hi> to attach
-  // a custom out-of-range / non-integer message: parser.Next<FInt<1u, 99u>>("bad f").
+  // Like Next<T>(), but replaces any read failure (bad value, missing arg, ...) with a caller-
+  // supplied CUSTOM_ERROR message. A rule's own message from a Validated<T, Rules...> passes
+  // through unchanged, so parser.Next<Timeout>(kNotAFloat) reports kNotAFloat for a non-float but
+  // keeps each rule's out-of-range / negative message.
   template <class T = std::string_view> auto Next(std::string_view err_msg) {
     bool prior = bool(error_);
     auto val = Next<T>();
-    if (!prior && error_ && !err_msg.empty()) {
+    if (!prior && !err_msg.empty() && error_ && error_.type != CUSTOM_ERROR) {
       error_.type = CUSTOM_ERROR;
       error_.custom_msg = std::string{err_msg};
     }
@@ -473,10 +521,15 @@ struct CmdArgParser {
     } else if constexpr (std::is_constructible_v<T, std::string_view>) {
       return static_cast<T>(SafeSV(idx));
     } else if constexpr (as_vnum<T>) {
-      using U = typename T::underlying_t;
+      using U = decltype(T::value);
       U val = Num<U>(idx);
-      if (!T::validate(val)) {
-        Report(std::is_floating_point_v<U> ? INVALID_FLOAT : INVALID_INT, idx);
+      if (error_)
+        return {};  // malformed number already reported
+      if (RuleError e = T::validate(val); e.failed) {
+        if (e.msg.empty())
+          Report(std::is_floating_point_v<U> ? INVALID_FLOAT : INVALID_INT, idx);
+        else
+          Report(CUSTOM_ERROR, idx, std::string{e.msg});
         return {};
       }
       return T{val};

src/facade/cmd_arg_parser_test.cc

@@ -495,6 +495,141 @@ TEST_F(CmdArgParserTest, ApplyOptional) {
   }
 }
 
+TEST_F(CmdArgParserTest, ValidatedRules) {
+  static constexpr char kOverflow[] = "overflow";
+  static constexpr char kNaN[] = "not a number";
+  static constexpr char kNonFinite[] = "not finite";
+
+  // NotEq: rejects only the sentinel value, with a custom message.
+  using NoMin = Validated<int64_t, NotEq<INT64_MIN, kOverflow>>;
+  {
+    auto parser = Make({"5"});
+    EXPECT_EQ(static_cast<int64_t>(parser.Next<NoMin>()), 5);
+    EXPECT_FALSE(parser.HasError());
+  }
+  {
+    auto parser = Make({"-9223372036854775808"});  // INT64_MIN
+    parser.Next<NoMin>();
+    auto err = parser.TakeError();
+    EXPECT_EQ(err.type, CmdArgParser::CUSTOM_ERROR);
+    EXPECT_EQ(err.MakeReply().ToSv(), "overflow");
+  }
+  {  // A malformed number reports the generic INVALID_INT, not the rule message.
+    auto parser = Make({"abc"});
+    parser.Next<NoMin>();
+    EXPECT_EQ(parser.TakeError().type, CmdArgParser::INVALID_INT);
+  }
+
+  // NotNan accepts +/-inf but rejects NaN; Finite rejects both.
+  {
+    auto parser = Make({"inf", "nan"});
+    EXPECT_TRUE(std::isinf(static_cast<double>(parser.Next<Validated<double, NotNan<kNaN>>>())));
+    EXPECT_FALSE(parser.HasError());
+    parser.Next<Validated<double, NotNan<kNaN>>>();
+    EXPECT_EQ(parser.TakeError().MakeReply().ToSv(), "not a number");
+  }
+  {
+    auto parser = Make({"inf"});
+    parser.Next<Validated<double, Finite<kNonFinite>>>();
+    EXPECT_EQ(parser.TakeError().MakeReply().ToSv(), "not finite");
+  }
+
+  // Composition: rules run in order, first non-empty message wins (NotNan rejects NaN first).
+  {
+    auto parser = Make({"nan"});
+    parser.Next<Validated<double, NotNan<kNaN>, Finite<kNonFinite>>>();
+    EXPECT_EQ(parser.TakeError().MakeReply().ToSv(), "not a number");
+  }
+
+  // The rules are reusable to validate a value parsed by other means (e.g. NextWithPrefix).
+  {
+    auto parser = Make({"#-9223372036854775808"});
+    bool prefixed = false;
+    int64_t off = parser.NextWithPrefix<int64_t>("#", &prefixed);
+    EXPECT_TRUE(prefixed);
+    if (auto e = NotEq<INT64_MIN, kOverflow>(off); e.failed)
+      parser.ReportCustom(std::string{e.msg});
+    EXPECT_EQ(parser.TakeError().MakeReply().ToSv(), "overflow");
+  }
+}
+
+TEST_F(CmdArgParserTest, BoundedRule) {
+  static constexpr char kMsg[] = "out of range";
+
+  // Integer range: in-range passes; below/above report the custom message; a non-integer stays
+  // generic INVALID_INT.
+  using Pct = Validated<int, Bounded<0, 100, kMsg>>;
+  {
+    auto parser = Make({"0", "100", "50"});
+    EXPECT_EQ(static_cast<int>(parser.Next<Pct>()), 0);
+    EXPECT_EQ(static_cast<int>(parser.Next<Pct>()), 100);
+    EXPECT_EQ(static_cast<int>(parser.Next<Pct>()), 50);
+    EXPECT_FALSE(parser.HasError());
+  }
+  {
+    auto parser = Make({"101"});
+    parser.Next<Pct>();
+    EXPECT_EQ(parser.TakeError().MakeReply().ToSv(), "out of range");
+  }
+  {
+    auto parser = Make({"abc"});
+    parser.Next<Pct>();
+    EXPECT_EQ(parser.TakeError().type, CmdArgParser::INVALID_INT);
+  }
+}
+
+TEST_F(CmdArgParserTest, NonNegativeRule) {
+  static constexpr char kNeg[] = "must not be negative";
+  using NonNeg = Validated<float, NonNegative<kNeg>>;
+
+  {
+    auto parser = Make({"0", "3.5"});
+    EXPECT_FLOAT_EQ(static_cast<float>(parser.Next<NonNeg>()), 0.0f);
+    EXPECT_FLOAT_EQ(static_cast<float>(parser.Next<NonNeg>()), 3.5f);
+    EXPECT_FALSE(parser.HasError());
+  }
+  {
+    auto parser = Make({"-0.1"});
+    parser.Next<NonNeg>();
+    EXPECT_EQ(parser.TakeError().MakeReply().ToSv(), "must not be negative");
+  }
+  {  // NaN is accepted (matches a plain `v < 0` guard); a non-float stays generic INVALID_FLOAT.
+    auto parser = Make({"nan"});
+    parser.Next<NonNeg>();
+    EXPECT_FALSE(parser.HasError());
+    auto p2 = Make({"abc"});
+    p2.Next<NonNeg>();
+    EXPECT_EQ(p2.TakeError().type, CmdArgParser::INVALID_FLOAT);
+  }
+}
+
+TEST_F(CmdArgParserTest, NextWithMessageOverride) {
+  static constexpr char kParseMsg[] = "custom parse error";
+  static constexpr char kRuleMsg[] = "must not be negative";
+  using NonNeg = Validated<float, NonNegative<kRuleMsg>>;
+
+  {  // A non-numeric value reports the caller message instead of the generic type error.
+    auto parser = Make({"abc"});
+    parser.Next<NonNeg>(kParseMsg);
+    EXPECT_EQ(parser.TakeError().MakeReply().ToSv(), "custom parse error");
+  }
+  {  // A rule violation keeps its own message; the caller message does not override it.
+    auto parser = Make({"-1"});
+    parser.Next<NonNeg>(kParseMsg);
+    EXPECT_EQ(parser.TakeError().MakeReply().ToSv(), "must not be negative");
+  }
+  {  // FInt out-of-range is a generic error, so the caller message applies to it too.
+    auto parser = Make({"5"});
+    parser.Next<FInt<0, 3>>(kParseMsg);
+    EXPECT_EQ(parser.TakeError().MakeReply().ToSv(), "custom parse error");
+  }
+  {  // A valid value produces no error.
+    auto parser = Make({"2.5"});
+    EXPECT_FLOAT_EQ(static_cast<float>(parser.Next<NonNeg>(kParseMsg)), 2.5f);
+    EXPECT_FALSE(parser.HasError());
+  }
+}
+
 TEST_F(CmdArgParserTest, FixedRangeInt) {
   {
     auto parser = Make({"10", "-10", "12"});
@@ -520,11 +655,10 @@ TEST_F(CmdArgParserTest, FixedRangeInt) {
   }
 }
 
-// A user-defined as_vnum: VNum<double> + a validate() predicate. Exercises the generic
-// validated-number path of Convert<T>() for a floating-point underlying type.
+// A user-defined validated number: VNum<double> + validate() reporting the generic INVALID_FLOAT.
 struct PositiveFinite : VNum<double> {
-  static bool validate(double v) {
-    return v > 0 && std::isfinite(v);
+  static RuleError validate(double v) {
+    return {!(v > 0 && std::isfinite(v)), {}};
   }
 };
 

src/facade/error.h

@@ -22,6 +22,9 @@ inline constexpr char kKeyNotFoundErr[] = "no such key";
 inline constexpr char kInvalidIntErr[] = "value is not an integer or out of range";
 inline constexpr char kInvalidFloatErr[] = "value is not a valid float";
 inline constexpr char kUintErr[] = "value is out of range, must be positive";
+inline constexpr char kTimeoutNegativeErr[] = "timeout is negative";
+inline constexpr char kTimeoutNotFloatErr[] = "timeout is not a float or out of range";
+inline constexpr char kTimeoutOutOfRangeErr[] = "timeout is out of range";
 inline constexpr char kInvalidNumFields[] = "Number of fields must be a positive integer";
 inline constexpr char kIncrOverflow[] = "increment or decrement would overflow";
 inline constexpr char kDbIndOutOfRangeErr[] = "DB index is out of range";

src/server/bitops_family.cc

@@ -34,6 +34,7 @@ namespace {
 
 using ShardStringResults = vector<OpResult<string>>;
 const int32_t OFFSET_FACTOR = 8;  // number of bits in byte
+constexpr char kBitArgErr[] = "The bit argument must be 1 or 0";
 const char* OR_OP_NAME = "OR";
 const char* XOR_OP_NAME = "XOR";
 const char* AND_OP_NAME = "AND";
@@ -525,40 +526,11 @@ void BitPos(facade::CmdArgParser parser, CommandContext* cmd_cntx) {
   auto* builder = cmd_cntx->rb();
 
   auto key = parser.Next<string_view>();
-  auto value = parser.Next<int32_t>();
+  int32_t value = parser.Next<Validated<int32_t, Bounded<int32_t{0}, int32_t{1}, kBitArgErr>>>();
 
-  if (auto err = parser.TakeError(); err) {
-    return builder->SendError(err.MakeReply());
-  }
-
-  if (value != 0 && value != 1) {
-    return builder->SendError("The bit argument must be 1 or 0");
-  }
-
-  int64_t start = 0;
-  int64_t end = std::numeric_limits<int64_t>::max();
-  bool as_bit = false;
-
-  if (parser.HasNext()) {
-    start = parser.Next<int64_t>();
-    if (auto err = parser.TakeError(); err) {
-      return builder->SendError(err.MakeReply());
-    }
-
-    if (parser.HasNext()) {
-      end = parser.Next<int64_t>();
-      if (auto err = parser.TakeError(); err) {
-        return builder->SendError(err.MakeReply());
-      }
-
-      if (parser.HasNext()) {
-        as_bit = parser.MapNext("BIT", true, "BYTE", false);
-        if (auto err = parser.TakeError(); err) {
-          return builder->SendError(kSyntaxErr);
-        }
-      }
-    }
-  }
+  int64_t start = parser.NextOrDefault<int64_t>();
+  int64_t end = parser.NextOrDefault<int64_t>(std::numeric_limits<int64_t>::max());
+  bool as_bit = parser.HasNext() ? parser.MapNext("BIT", true, "BYTE", false) : false;
 
   if (!parser.Finalize()) {
     return builder->SendError(parser.TakeError().MakeReply());