pybind: pos validation and class attribute constants for S2CellId bindings

src/python/BUILD.bazel

@@ -100,6 +100,7 @@ pybind_library(
     srcs = ["s2cell_id_bindings.cc"],
     deps = [
         "//:s2",
+        "@abseil-cpp//absl/strings",
     ],
 )
 

src/python/README.md

@@ -144,7 +144,7 @@ To add bindings for a new class:
 Use the following sections to organize functions within the bindings files and tests. Secondarily, follow the order in which functions are declared in the C++ headers.
 
 1. **Constructors** - Default constructors and constructors with parameters
-1. **Constants** - Class-level constants in upper snake case (e.g., `S2CellId.MAX_LEVEL`, `S2CellId.NUM_FACES`)
+1. **Constants** - Class-level constants in upper snake case (e.g., `S2CellId.MAX_LEVEL`, `S2CellId.NUM_FACES`). Accessible as class attributes.
 1. **Factory methods** - Static factory methods (e.g., `from_degrees`, `from_radians`, `zero`, `invalid`)
 1. **Properties** - Mutable and read-only properties (e.g., coordinate accessors like `x`, `y`, `lo`, `hi`)
 1. **Predicates** - Simple boolean state checks (e.g., `is_empty`, `is_valid`, `is_full`)

src/python/s2cell_id_bindings.cc

@@ -60,10 +60,18 @@ void MaybeThrowChildPositionOutOfRange(int position) {
   }
 }
 
+void MaybeThrowPositionOutOfRange(uint64_t pos) {
+  if (pos > S2CellId::kMaxPosition) {
+    throw py::value_error(
+        absl::StrCat("pos ", pos, " out of range [0, ", S2CellId::kMaxPosition,
+                     "]"));
+  }
+}
+
 }  // namespace
 
 void bind_s2cell_id(py::module& m) {
-  py::class_<S2CellId>(m, "S2CellId",
+  auto cls = py::class_<S2CellId>(m, "S2CellId",
       "A 64-bit unsigned integer that uniquely identifies a cell in the\n"
       "S2 cell decomposition.\n\n"
       "See s2/s2cell_id.h for comprehensive documentation.")
@@ -88,12 +96,6 @@ void bind_s2cell_id(py::module& m) {
            py::arg("latlng"),
            "Construct a leaf cell containing the given S2LatLng.")
 
-      // Constants
-      .def_readonly_static("MAX_LEVEL", &S2CellId::kMaxLevel,
-           "Maximum cell subdivision level (30)")
-      .def_readonly_static("NUM_FACES", &S2CellId::kNumFaces,
-           "Number of cube faces (6)")
-
       // Factory methods
       .def_static("from_face", [](int face) {
                MaybeThrowFaceOutOfRange(face);
@@ -103,12 +105,13 @@ void bind_s2cell_id(py::module& m) {
            "Raises ValueError if face is out of range.")
       .def_static("from_face_pos_level", [](int face, uint64_t pos, int level) {
                MaybeThrowFaceOutOfRange(face);
+               MaybeThrowPositionOutOfRange(pos);
                MaybeThrowLevelOutOfRange(level, 0, S2CellId::kMaxLevel);
                return S2CellId::FromFacePosLevel(face, pos, level);
            },
            py::arg("face"), py::arg("pos"), py::arg("level"),
            "Return a cell given its face, Hilbert curve position, and level.\n\n"
-           "Raises ValueError if face or level is out of range.")
+           "Raises ValueError if face, pos, or level is out of range.")
       .def_static("from_token", [](const std::string& token) {
                S2CellId cell = S2CellId::FromToken(token);
                if (cell == S2CellId::None()) {
@@ -272,4 +275,8 @@ void bind_s2cell_id(py::module& m) {
         oss << id;
         return oss.str();
       });
+
+  cls.attr("MAX_LEVEL")    = S2CellId::kMaxLevel;
+  cls.attr("MAX_POSITION") = S2CellId::kMaxPosition;
+  cls.attr("NUM_FACES")    = S2CellId::kNumFaces;
 }

src/python/s2cell_id_test.py

@@ -57,6 +57,10 @@ def test_from_face_pos_level(self):
         self.assertEqual(cell.level, 0)
         self.assertEqual(cell.face, 0)
 
+    def test_from_face_pos_level_pos_out_of_range_raises(self):
+        with self.assertRaises(ValueError):
+            s2.S2CellId.from_face_pos_level(0, s2.S2CellId.MAX_POSITION + 1, 0)
+
     def test_from_token_roundtrip(self):
         cell = s2.S2CellId.from_face(3)
         token = cell.to_token()

src/s2/s2cell_id.h

@@ -99,6 +99,8 @@ class S2CellId {
   static constexpr int kMaxLevel =
       S2::kMaxCellLevel;  // Valid levels: 0..kMaxLevel
   static constexpr int kPosBits = 2 * kMaxLevel + 1;
+  static constexpr uint64_t kMaxPosition =
+      (~static_cast<uint64_t>(0)) >> kFaceBits;
   static constexpr int kMaxSize = 1 << kMaxLevel;
 
   explicit constexpr S2CellId(uint64_t id) : id_(id) {}

src/s2/s2cell_id_test.cc

@@ -101,6 +101,31 @@ TEST(S2CellId, FromFace) {
   }
 }
 
+TEST(S2CellId, MaxPositionIsValid) {
+  // kMaxPosition is the largest value that fits in the position field.
+  EXPECT_EQ(S2CellId::kMaxPosition, (~uint64_t{0}) >> S2CellId::kFaceBits);
+
+  // FromFacePosLevel with kMaxPosition produces a valid cell on the correct face.
+  for (int face = 0; face < S2CellId::kNumFaces; ++face) {
+    S2CellId id = S2CellId::FromFacePosLevel(face, S2CellId::kMaxPosition, 0);
+    EXPECT_TRUE(id.is_valid());
+    EXPECT_EQ(face, id.face());
+  }
+}
+
+TEST(S2CellId, PositionAboveMaxIsInvalid) {
+  // pos values above kMaxPosition overflow the position field into the face
+  // bits, producing an invalid cell.
+  //
+  // Replicate FromFacePosLevel to avoid its is_valid() DCHECK:
+  //   S2CellId cell((static_cast<uint64_t>(face) << kPosBits) + (pos | 1));
+  constexpr int face = 5;
+  constexpr uint64_t kOverflowPos = S2CellId::kMaxPosition + 1;
+  S2CellId overflow_id(
+      (uint64_t{face} << S2CellId::kPosBits) + (kOverflowPos | 1));
+  EXPECT_FALSE(overflow_id.is_valid());
+}
+
 TEST(S2CellId, ParentChildRelationships) {
   S2CellId id = S2CellId::FromFacePosLevel(3, 0x12345678,
                                            S2CellId::kMaxLevel - 4);