Fixes github #7533 (#7539)

* Fixes #7533

Fixes problem with SparseIntVect and SparseBitVect where the intention is to
be able to store all possible values of the index type: in these cases
you could not store (or access) the final element.

* increase the verbosity of the DataStructs tests
so that we can see what's going on

Code/DataStructs/CMakeLists.txt

@@ -35,6 +35,9 @@ rdkit_test(testFPB testFPB.cpp
 rdkit_test(testMultiFPB testMultiFPB.cpp
            LINK_LIBRARIES DataStructs )
 
+rdkit_catch_test(catchDataStructs catch_tests.cpp
+           LINK_LIBRARIES DataStructs )
+
 if(RDK_BUILD_PYTHON_WRAPPERS)
 add_subdirectory(Wrap)
 endif()

Code/DataStructs/SparseBitVect.cpp

@@ -1,6 +1,5 @@
-// $Id$
 //
-// Copyright (c) 2001-2008 greg Landrum and Rational Discovery LLC
+// Copyright (c) 2001-2024 greg Landrum and other RDKit contributors
 //
 //  @@ All Rights Reserved @@
 //  This file is part of the RDKit.
@@ -14,7 +13,6 @@
 #include "base64.h"
 #include <RDGeneral/StreamOps.h>
 #include <sstream>
-#include <limits>
 
 #ifdef WIN32
 #include <ios>
@@ -150,7 +148,7 @@ SparseBitVect SparseBitVect::operator~() const {
 //
 // """ -------------------------------------------------------
 bool SparseBitVect::getBit(const unsigned int which) const {
-  if (which >= d_size) {
+  if (!checkIndex(which)) {
     throw IndexErrorException(which);
   }
   return dp_bits->count(which) > 0u;
@@ -163,7 +161,7 @@ bool SparseBitVect::getBit(const unsigned int which) const {
 //
 // """ -------------------------------------------------------
 bool SparseBitVect::getBit(const IntVectIter which) const {
-  if (*which < 0 || static_cast<unsigned int>(*which) >= d_size) {
+  if (!checkIndex(which)) {
     throw IndexErrorException(*which);
   }
   return dp_bits->count(*which) > 0u;
@@ -176,7 +174,7 @@ bool SparseBitVect::getBit(const IntVectIter which) const {
 //
 // """ -------------------------------------------------------
 bool SparseBitVect::getBit(const IntSetIter which) const {
-  if (*which < 0 || static_cast<unsigned int>(*which) >= d_size) {
+  if (!checkIndex(which)) {
     throw IndexErrorException(*which);
   }
   return dp_bits->count(*which) > 0u;
@@ -193,11 +191,10 @@ bool SparseBitVect::setBit(const unsigned int which) {
   if (!dp_bits) {
     throw ValueErrorException("BitVect not properly initialized.");
   }
-  std::pair<IntSetIter, bool> res;
-  if (which >= d_size) {
+  if (!checkIndex(which)) {
     throw IndexErrorException(which);
   }
-  res = dp_bits->insert(which);
+  auto res = dp_bits->insert(which);
   return !(res.second);
 }
 
@@ -213,11 +210,10 @@ bool SparseBitVect::setBit(const IntSetIter which) {
   if (!dp_bits) {
     throw ValueErrorException("BitVect not properly initialized.");
   }
-  std::pair<IntSetIter, bool> res;
-  if (*which < 0 || static_cast<unsigned int>(*which) >= d_size) {
+  if (!checkIndex(which)) {
     throw IndexErrorException(*which);
   }
-  res = dp_bits->insert(*which);
+  auto res = dp_bits->insert(*which);
   return !(res.second);
 }
 
@@ -232,7 +228,7 @@ bool SparseBitVect::unsetBit(const unsigned int which) {
   if (!dp_bits) {
     throw ValueErrorException("BitVect not properly initialized.");
   }
-  if (which >= d_size) {
+  if (!checkIndex(which)) {
     throw IndexErrorException(which);
   }
 

Code/DataStructs/SparseBitVect.h

@@ -1,5 +1,5 @@
 //
-// Copyright (c) 2003-2008 greg Landrum and Rational Discovery LLC
+//  Copyright (C) 2007-2024 Greg Landrum and other RDKit contributors
 //
 //  @@ All Rights Reserved @@
 //  This file is part of the RDKit.
@@ -17,6 +17,7 @@
 using std::set;
 #include <iterator>
 #include <algorithm>
+#include <limits>
 
 typedef set<int> IntSet;
 typedef IntSet::iterator IntSetIter;
@@ -96,6 +97,14 @@ class RDKIT_DATASTRUCTS_EXPORT SparseBitVect : public BitVect {
  private:
   unsigned int d_size{0};
   void _initForSize(const unsigned int size) override;
+  bool checkIndex(const unsigned int idx) const {
+    return idx < d_size || (idx == d_size &&
+                            d_size == std::numeric_limits<unsigned int>::max());
+  }
+  template <typename T>
+  bool checkIndex(const T which) const {
+    return *which >= 0 && static_cast<unsigned int>(*which) < d_size;
+  }
 };
 
 #endif

Code/DataStructs/SparseIntVect.h

@@ -1,6 +1,5 @@
-// $Id$
 //
-//  Copyright (C) 2007-2008 Greg Landrum
+//  Copyright (C) 2007-2024 Greg Landrum and other RDKit contributors
 //
 //  @@ All Rights Reserved @@
 //  This file is part of the RDKit.
@@ -19,6 +18,7 @@
 #include <RDGeneral/Exceptions.h>
 #include <RDGeneral/StreamOps.h>
 #include <cstdint>
+#include <limits>
 
 const int ci_SPARSEINTVECT_VERSION =
     0x0001;  //!< version number to use in pickles
@@ -75,7 +75,7 @@ class SparseIntVect {
 #endif
   //! return the value at an index
   int getVal(IndexType idx) const {
-    if (idx < 0 || idx >= d_length) {
+    if (!checkIndex(idx)) {
       throw IndexErrorException(static_cast<int>(idx));
     }
     int res = 0;
@@ -88,7 +88,7 @@ class SparseIntVect {
 
   //! set the value at an index
   void setVal(IndexType idx, int val) {
-    if (idx < 0 || idx >= d_length) {
+    if (!checkIndex(idx)) {
       throw IndexErrorException(static_cast<int>(idx));
     }
     if (val != 0) {
@@ -410,6 +410,13 @@ class SparseIntVect {
       d_data[tVal] = val;
     }
   }
+  bool checkIndex(IndexType idx) const {
+    if (idx < 0 || idx > d_length ||
+        (idx == d_length && d_length < std::numeric_limits<IndexType>::max())) {
+      return false;
+    }
+    return true;
+  }
 };
 
 template <typename IndexType, typename SequenceType>

Code/DataStructs/catch_tests.cpp

@@ -0,0 +1,29 @@
+//
+//  Copyright (C) 2024 Greg Landrum and other RDKit contributors
+//
+//  @@ All Rights Reserved @@
+//  This file is part of the RDKit.
+//  The contents are covered by the terms of the BSD license
+//  which is included in the file license.txt, found at the root
+//  of the RDKit source tree.
+//
+
+#include <catch2/catch_all.hpp>
+
+#include <RDGeneral/test.h>
+#include "BitVects.h"
+#include "BitOps.h"
+#include "BitVectUtils.h"
+#include "SparseIntVect.h"
+#include <limits>
+
+using namespace RDKit;
+
+TEST_CASE("special cases for the limits of sparse vectors") {
+  SECTION("SparseBitVect") {
+    SparseBitVect sbv(std::numeric_limits<unsigned int>::max());
+    CHECK(sbv.getNumBits() == std::numeric_limits<unsigned int>::max());
+    CHECK(!sbv.setBit(std::numeric_limits<unsigned int>::max()));
+    CHECK(sbv.getBit(std::numeric_limits<unsigned int>::max()) == 1);
+  }
+}

Code/DataStructs/testDatastructs.cpp

@@ -1,6 +1,5 @@
-// $Id$
 //
-//  Copyright (C) 2001-2014 Greg Landrum and Rational Discovery LLC
+//  Copyright (C) 2001-2024 Greg Landrum and other RDKit contributors
 //
 //  @@ All Rights Reserved @@
 //  This file is part of the RDKit.
@@ -1453,6 +1452,7 @@ void test17Github3994() {
 
 int main() {
   RDLog::InitLogs();
+  boost::logging::enable_logs("rdApp.info");
   try {
     throw IndexErrorException(3);
   } catch (IndexErrorException &) {

Code/GraphMol/Fingerprints/catch_tests.cpp

@@ -794,3 +794,11 @@ TEST_CASE(
     }
   }
 }
+
+TEST_CASE("github #7533: IndexError while computing fingerprint") {
+  auto mol = "CC1C(B(C)C)S1(C)(C)=O"_smiles;
+  REQUIRE(mol);
+  auto fp = MorganFingerprints::getFingerprint(*mol, 2);
+  REQUIRE(fp);
+  CHECK(fp->getLength() == std::numeric_limits<unsigned>::max());
+}