work on the ondemand iterators (#2590)

* work on the ondemand iterators

* guarding two SIMDJSON_ASSUME

* simplify following @jkeiser's comment

* adding safety rails to the iterators

* silencing a warning.

* updating the amalgamation files

CMakeLists.txt

@@ -117,7 +117,7 @@ if(SIMDJSON_BUILD_STATIC_LIB)
   add_library(simdjson_static STATIC ${SIMDJSON_SOURCES})
   add_library(simdjson::simdjson_static ALIAS simdjson_static)
   list(APPEND SIMDJSON_LIBRARIES simdjson_static)
-  
+
   # Reuse precompiled headers for static library
   if(CMAKE_VERSION VERSION_GREATER_EQUAL "3.16")
     target_precompile_headers(simdjson_static REUSE_FROM simdjson)
@@ -196,7 +196,12 @@ if(
       target_compile_options PRIVATE
       $<$<CONFIG:DEBUG>:-Og>
   )
-endif()
+  # We still want to enable development checks in Debug mode
+  simdjson_add_props(
+      target_compile_definitions PUBLIC
+      SIMDJSON_DEVELOPMENT_CHECKS
+  )
+  endif()
 
 if(SIMDJSON_ENABLE_THREADS)
   find_package(Threads REQUIRED)

doc/basics.md

@@ -354,7 +354,13 @@ the  macro `SIMDJSON_DEVELOPMENT_CHECKS` to 1 prior to including
 the `simdjson.h` header to enable these additional checks: just make sure you remove the
 definition once your code has been tested. When `SIMDJSON_DEVELOPMENT_CHECKS` is set to 1, the
 simdjson library runs additional (expensive) tests on your code to help ensure that you are
-using the library in a safe manner.
+using the library in a safe manner. We add asserts which may halt your program, helping
+you find the bad programming pattern.
+
+When `SIMDJSON_DEVELOPMENT_CHECKS`, some of our data structures contain extra data for
+tracking explicitly potential programming mistakes. Thus you should not relying on the
+size (`sizeof`) of our data structures to be constant: they may change depending on the
+compiler settings.
 
 Once your code has been tested, you can then run it in
 Release mode: under Visual Studio, it means having the `_DEBUG` macro undefined, and, for other
@@ -437,8 +443,8 @@ support for users who avoid exceptions. See [the simdjson error handling documen
 
   If you know the type of the value, you can cast it right there, too! `for (double value : array) { ... }`.
 
-  You may also use explicit iterators: `for(auto i = array.begin(); i != array.end(); i++) {}`. You can check that an array is empty with the condition `auto i = array.begin(); if (i == array.end()) {...}`.
-* **Object Iteration:** You can iterate through an object's fields, as well: `for (auto field : object) { ... }`. You may also use explicit iterators : `for(auto i = object.begin(); i != object.end(); i++) { auto field = *i; .... }`. You can check that an object is empty with the condition `auto i = object.begin(); if (i == object.end()) {...}`.
+  You may also use explicit iterators: `for(auto i = array.begin(); i != array.end(); i++) {}`. You can check that an array is empty with the condition `auto i = array.begin(); if (i == array.end()) {...}`. You should derefence (`*i`) an iterator at most once before incrementing it (`i++`), when compiling in debug mode with development checks, we add asserts to help you identify such a mistake.
+* **Object Iteration:** You can iterate through an object's fields, as well: `for (auto field : object) { ... }`.
   - `field.unescaped_key()` will get you the unescaped key string as a `std::string_view` instance. E.g., the JSON string `"\u00e1"` becomes the Unicode string `á`. Optionally,  you pass `true` as a parameter to the `unescaped_key` method if you want invalid escape sequences to be replaced by a default replacement character (e.g., `\ud800\ud801\ud811`): otherwise bad escape sequences lead to an immediate error.
   - `field.escaped_key()` will get you the key string as  as a `std::string_view` instance, but unlike `unescaped_key()`, the key is not processed, so no unescaping is done. E.g., the JSON string `"\u00e1"` becomes the Unicode string `\u00e1`. We expect that `escaped_key()` is faster than `field.unescaped_key()`.
   - `field.value()` will get you the value, which you can then use all these other methods on.
@@ -451,6 +457,10 @@ support for users who avoid exceptions. See [the simdjson error handling documen
 
   When you are iterating through an object, you are advancing through its keys and values. You should not also access the object or other objects. E.g. within a loop over `myobject`, you should not be accessing `myobject`. The following is an anti-pattern: `for(auto value: myobject) {myobject["mykey"]}`.
 
+  We discourage using the object iterators explicitly: `for(auto i = object.begin(); i != object.end(); i++) { auto field = *i; .... }`. In addition to the usual requirement to check against `end()` prior to dereferencing, you must also always dereference the pointer (`*it`) exactly once before you increment it (`it++`). You must also only deference the iterator once (never more than once). When compiling in
+  debug mode with development checks, we add asserts to help check whether you correctly
+  dereferenced the pointer before incrementing it.
+
   You should never reset an object as you are iterating through it. The following is an anti-pattern: `for(auto value: myobject) {myobject.reset()}`.
 * **Array Index:** Because it is forward-only, you cannot look up an array element by index. Instead,
   you should iterate through the array and keep an index yourself. Exceptionally, if need a single value

include/simdjson/common_defs.h

@@ -289,7 +289,9 @@ namespace std {
 // when the compiler is optimizing.
 // We only set SIMDJSON_DEVELOPMENT_CHECKS if both __OPTIMIZE__
 // and NDEBUG are not defined.
-#if !defined(__OPTIMIZE__) && !defined(NDEBUG)
+// We recognize _DEBUG as overriding __OPTIMIZE__ so that if both
+// __OPTIMIZE__ and _DEBUG are defined, we still set SIMDJSON_DEVELOPMENT_CHECKS.
+#if ((!defined(__OPTIMIZE__) || defined(_DEBUG)) && !defined(NDEBUG))
 #define SIMDJSON_DEVELOPMENT_CHECKS 1
 #endif // __OPTIMIZE__
 #endif // _MSC_VER

include/simdjson/error-inl.h

@@ -8,7 +8,7 @@
 namespace simdjson {
 
 inline bool is_fatal(error_code error) noexcept {
-  return error == TAPE_ERROR || error == INCOMPLETE_ARRAY_OR_OBJECT;
+  return error == TAPE_ERROR || error == INCOMPLETE_ARRAY_OR_OBJECT || error == OUT_OF_ORDER_ITERATION || error == DEPTH_ERROR;
 }
 
 namespace internal {

include/simdjson/generic/ondemand/array_iterator-inl.h

@@ -17,6 +17,10 @@ simdjson_inline array_iterator::array_iterator(const value_iterator &_iter) noex
 {}
 
 simdjson_inline simdjson_result<value> array_iterator::operator*() noexcept {
+#if SIMDJSON_DEVELOPMENT_CHECKS
+   SIMDJSON_ASSUME(!has_been_referenced);
+   has_been_referenced = true;
+#endif
   if (iter.error()) { iter.abandon(); return iter.error(); }
   return value(iter.child());
 }
@@ -27,6 +31,9 @@ simdjson_inline bool array_iterator::operator!=(const array_iterator &) const no
   return iter.is_open();
 }
 simdjson_inline array_iterator &array_iterator::operator++() noexcept {
+#if SIMDJSON_DEVELOPMENT_CHECKS
+   has_been_referenced = false;
+#endif
   error_code error;
   // PERF NOTE this is a safety rail ... users should exit loops as soon as they receive an error, so we'll never get here.
   // However, it does not seem to make a perf difference, so we add it out of an abundance of caution.

include/simdjson/generic/ondemand/array_iterator.h

@@ -2,6 +2,7 @@
 
 #ifndef SIMDJSON_CONDITIONAL_INCLUDE
 #define SIMDJSON_GENERIC_ONDEMAND_ARRAY_ITERATOR_H
+#include <iterator>
 #include "simdjson/generic/implementation_simdjson_result_base.h"
 #include "simdjson/generic/ondemand/base.h"
 #include "simdjson/generic/ondemand/value_iterator.h"
@@ -17,11 +18,17 @@ namespace ondemand {
  *
  * This is an input_iterator, meaning:
  * - It is forward-only
- * - * must be called exactly once per element.
+ * - * must be called at most once per element.
  * - ++ must be called exactly once in between each * (*, ++, *, ++, * ...)
  */
 class array_iterator {
 public:
+  using iterator_category = std::input_iterator_tag;
+  using value_type = simdjson_result<value>;
+  using difference_type = std::ptrdiff_t;
+  using pointer = void;
+  using reference = value_type;
+
   /** Create a new, invalid array iterator. */
   simdjson_inline array_iterator() noexcept = default;
 
@@ -65,6 +72,9 @@ public:
   simdjson_warn_unused simdjson_inline bool at_end() const noexcept;
 
 private:
+#if SIMDJSON_DEVELOPMENT_CHECKS
+   bool has_been_referenced{false};
+#endif
   value_iterator iter{};
 
   simdjson_inline array_iterator(const value_iterator &iter) noexcept;
@@ -82,6 +92,12 @@ namespace simdjson {
 
 template<>
 struct simdjson_result<SIMDJSON_IMPLEMENTATION::ondemand::array_iterator> : public SIMDJSON_IMPLEMENTATION::implementation_simdjson_result_base<SIMDJSON_IMPLEMENTATION::ondemand::array_iterator> {
+  using iterator_category = std::input_iterator_tag;
+  using value_type = simdjson_result<SIMDJSON_IMPLEMENTATION::ondemand::value>;
+  using difference_type = std::ptrdiff_t;
+  using pointer = void;
+  using reference = value_type;
+
   simdjson_inline simdjson_result(SIMDJSON_IMPLEMENTATION::ondemand::array_iterator &&value) noexcept; ///< @private
   simdjson_inline simdjson_result(error_code error) noexcept; ///< @private
   simdjson_inline simdjson_result() noexcept = default;

include/simdjson/generic/ondemand/json_iterator-inl.h

@@ -215,11 +215,10 @@ simdjson_inline void json_iterator::assert_more_tokens(uint32_t required_tokens)
 }
 
 simdjson_inline void json_iterator::assert_valid_position(token_position position) const noexcept {
+  (void)position; // Suppress unused parameter warning
 #ifndef SIMDJSON_CLANG_VISUAL_STUDIO
   SIMDJSON_ASSUME( position >= &parser->implementation->structural_indexes[0] );
   SIMDJSON_ASSUME( position < &parser->implementation->structural_indexes[parser->implementation->n_structural_indexes] );
-#else
-  (void)position; // Suppress unused parameter warning
 #endif
 }
 
@@ -358,7 +357,11 @@ simdjson_inline token_position json_iterator::position() const noexcept {
 simdjson_inline simdjson_result<std::string_view> json_iterator::unescape(raw_json_string in, bool allow_replacement) noexcept {
 #if SIMDJSON_DEVELOPMENT_CHECKS
   auto result = parser->unescape(in, _string_buf_loc, allow_replacement);
+#if !defined(SIMDJSON_VISUAL_STUDIO) && !defined(SIMDJSON_CLANG_VISUAL_STUDIO)
+  // Under Visual Studio, the next SIMDJSON_ASSUME fails with: the argument
+  // has side effects that will be discarded.
   SIMDJSON_ASSUME(!parser->string_buffer_overflow(_string_buf_loc));
+#endif // !defined(SIMDJSON_VISUAL_STUDIO) && !defined(SIMDJSON_CLANG_VISUAL_STUDIO)
   return result;
 #else
   return parser->unescape(in, _string_buf_loc, allow_replacement);
@@ -368,7 +371,11 @@ simdjson_inline simdjson_result<std::string_view> json_iterator::unescape(raw_js
 simdjson_inline simdjson_result<std::string_view> json_iterator::unescape_wobbly(raw_json_string in) noexcept {
 #if SIMDJSON_DEVELOPMENT_CHECKS
   auto result = parser->unescape_wobbly(in, _string_buf_loc);
+#if !defined(SIMDJSON_VISUAL_STUDIO) && !defined(SIMDJSON_CLANG_VISUAL_STUDIO)
+  // Under Visual Studio, the next SIMDJSON_ASSUME fails with: the argument
+  // has side effects that will be discarded.
   SIMDJSON_ASSUME(!parser->string_buffer_overflow(_string_buf_loc));
+#endif // !defined(SIMDJSON_VISUAL_STUDIO) && !defined(SIMDJSON_CLANG_VISUAL_STUDIO)
   return result;
 #else
   return parser->unescape_wobbly(in, _string_buf_loc);

include/simdjson/generic/ondemand/object.h

@@ -27,6 +27,13 @@ public:
    */
   simdjson_inline object() noexcept = default;
 
+  /**
+   * Get an iterator to the start of the object. We recommend using a range-based for loop.
+   *
+   * Using the iterator directly is also possible but error-prone and discouraged. In particular,
+   * you must dereference the iterator exactly once per iteration (before calling '++').
+   * Doing otherwise is unsafe and may lead to errors. You are responsible for ensuring
+   */
   simdjson_inline simdjson_result<object_iterator> begin() noexcept;
   simdjson_inline simdjson_result<object_iterator> end() noexcept;
   /**

include/simdjson/generic/ondemand/object_iterator-inl.h

@@ -21,6 +21,11 @@ simdjson_inline object_iterator::object_iterator(const value_iterator &_iter) no
 {}
 
 simdjson_inline simdjson_result<field> object_iterator::operator*() noexcept {
+#if SIMDJSON_DEVELOPMENT_CHECKS
+  // We must call * once per iteration.
+  SIMDJSON_ASSUME(!has_been_referenced);
+  has_been_referenced = true;
+#endif
   error_code error = iter.error();
   if (error) { iter.abandon(); return error; }
   auto result = field::start(iter);
@@ -39,6 +44,11 @@ simdjson_inline bool object_iterator::operator!=(const object_iterator &) const
 SIMDJSON_PUSH_DISABLE_WARNINGS
 SIMDJSON_DISABLE_STRICT_OVERFLOW_WARNING
 simdjson_inline object_iterator &object_iterator::operator++() noexcept {
+#if SIMDJSON_DEVELOPMENT_CHECKS
+   // Before calling ++, we must have called *.
+   SIMDJSON_ASSUME(has_been_referenced);
+   has_been_referenced = false;
+#endif
   // TODO this is a safety rail ... users should exit loops as soon as they receive an error.
   // Nonetheless, let's see if performance is OK with this if statement--the compiler may give it to us for free.
   if (!iter.is_open()) { return *this; } // Iterator will be released if there is an error

include/simdjson/generic/ondemand/object_iterator.h

@@ -32,9 +32,14 @@ public:
   // Assumes it's being compared with the end. true if depth >= iter->depth.
   simdjson_inline bool operator!=(const object_iterator &) const noexcept;
   // Checks for ']' and ','
+  // YOU MUST NOT CALL THIS IF operator* YIELDED AN ERROR.
+  // YOU MUST NOT CALL THIS WITHOUT A CORRESPONDING operator* CALL.
   simdjson_inline object_iterator &operator++() noexcept;
 
 private:
+#if SIMDJSON_DEVELOPMENT_CHECKS
+   bool has_been_referenced{false};
+#endif
   /**
    * The underlying JSON iterator.
    *

include/simdjson/generic/ondemand/value_iterator-inl.h

@@ -94,7 +94,6 @@ simdjson_warn_unused simdjson_inline error_code value_iterator::end_container()
 
 simdjson_warn_unused simdjson_inline simdjson_result<bool> value_iterator::has_next_field() noexcept {
   assert_at_next();
-
   // It's illegal to call this unless there are more tokens: anything that ends in } or ] is
   // obligated to verify there are more tokens if they are not the top level.
   switch (*_json_iter->return_current_and_advance()) {
@@ -967,6 +966,9 @@ simdjson_inline bool value_iterator::is_at_key() const noexcept {
   // Keys are at the same depth as the object.
   // Note here that we could be safer and check that we are within an object,
   // but we do not.
+  //
+  // As long as we are at the object's depth, in a valid document,
+  // we will only ever be at { , : or the actual string key: ".
   return _depth == _json_iter->_depth && *_json_iter->peek() == '"';
 }
 

include/simdjson/generic/ondemand/value_iterator.h

@@ -472,6 +472,7 @@ protected:
 
   friend class document;
   friend class object;
+  friend class object_iterator;
   friend class array;
   friend class value;
   friend class field;

include/simdjson/portability.h

@@ -210,7 +210,8 @@ using std::size_t;
 #define simdjson_strncasecmp strncasecmp
 #endif
 
-#if defined(NDEBUG) || defined(__OPTIMIZE__) || (defined(_MSC_VER) && !defined(_DEBUG))
+#if (defined(NDEBUG) || defined(__OPTIMIZE__) || (defined(_MSC_VER) && !defined(_DEBUG))) && !SIMDJSON_DEVELOPMENT_CHECKS
+// If SIMDJSON_DEVELOPMENT_CHECKS is undefined or 0, we consider that we are in release mode.
 // If NDEBUG is set, or __OPTIMIZE__ is set, or we are under MSVC in release mode,
 // then do away with asserts and use __assume.
 // We still recommend that our users set NDEBUG in release mode.
@@ -222,7 +223,7 @@ using std::size_t;
 #define SIMDJSON_ASSUME(COND) do { if (!(COND)) __builtin_unreachable(); } while (0)
 #endif
 
-#else // defined(NDEBUG) || defined(__OPTIMIZE__) || (defined(_MSC_VER) && !defined(_DEBUG))
+#else // defined(NDEBUG) || defined(__OPTIMIZE__) || (defined(_MSC_VER) && !defined(_DEBUG)) && !SIMDJSON_DEVELOPMENT_CHECKS
 // This should only ever be enabled in debug mode.
 #define SIMDJSON_UNREACHABLE() assert(0);
 #define SIMDJSON_ASSUME(COND) assert(COND)

tests/ondemand/ondemand_array_tests.cpp

@@ -6,7 +6,23 @@ using namespace simdjson;
 namespace array_tests {
   using namespace std;
   using simdjson::ondemand::json_type;
-
+  bool std_advance() {
+    TEST_START();
+    auto json = R"(   [  {"key" : "value1"}, {"key" : "value2"}  ]  )"_padded;
+    ondemand::parser parser;
+    ondemand::document doc;
+    ASSERT_SUCCESS(parser.iterate(json).get(doc));
+    ondemand::array arr;
+    ASSERT_SUCCESS(doc.get_array().get(arr));
+    auto iter = arr.begin();
+    std::advance(iter, 1);
+    ondemand::value v;
+    ASSERT_SUCCESS((*iter).get(v));
+    std::string_view sv;
+    ASSERT_SUCCESS(v["key"].get_string().get(sv));
+    ASSERT_EQUAL(sv, "value2");
+    TEST_SUCCEED();
+  }
   bool document_is_array_treated_as_object() {
     TEST_START();
     auto json = R"(   [  {"key" : "value"}  ]  )"_padded;
@@ -855,6 +871,7 @@ namespace array_tests {
 
   bool run() {
     return
+           std_advance() &&
            document_is_array_treated_as_object() &&
            issue1977() &&
            issue1876() &&

tests/ondemand/ondemand_object_tests.cpp

@@ -1,3 +1,4 @@
+#define SIMDJSON_VERBOSE_LOGGING 1
 #include "simdjson.h"
 #include "test_ondemand.h"
 
@@ -6,7 +7,6 @@ using namespace simdjson;
 namespace object_tests {
   using namespace std;
   using simdjson::ondemand::json_type;
-
   bool issue1979() {
     TEST_START();
     auto json = R"({
@@ -1015,6 +1015,26 @@ namespace object_tests {
     TEST_SUCCEED();
   }
 
+  bool manual_iterator_increment() {
+    // not recommended usage, but should work
+    TEST_START();
+    auto json = R"( {"1":1, "2":2, "3":3} )"_padded;
+    ondemand::parser parser;
+    ondemand::document doc;
+    ASSERT_SUCCESS(parser.iterate(json).get(doc));
+    ondemand::object obj;
+    ASSERT_SUCCESS(doc.get_object().get(obj));
+    std::cout << "object obtained.\n";
+    auto it = obj.begin();
+    *it; // essential !!!
+    ++it;
+    auto field = *it;
+    std::string_view key;
+    ASSERT_SUCCESS(field.unescaped_key().get(key));
+    std::cout << "Key: " << key << std::endl;
+    ASSERT_EQUAL(key, "2");
+    TEST_SUCCEED();
+  }
   bool issue1876() {
     TEST_START();
     auto json = R"( {} )"_padded;
@@ -1350,6 +1370,7 @@ namespace object_tests {
 #endif
            issue1974a() &&
            issue1974b() &&
+           manual_iterator_increment() &&
            issue1876a() &&
            issue1876() &&
            test_strager() &&