Revise EvaluationContext annotation interface (#1295)

Signed-off-by: Juan Cruz Viotti <[email protected]>

src/evaluator/context.cc

@@ -13,10 +13,10 @@ auto EvaluationContext::prepare(const JSON &instance) -> void {
   assert(this->resources_.empty());
   this->instances_.clear();
   this->instances_.emplace_back(instance);
-  this->annotation_blacklist.clear();
-  this->annotations_.clear();
   this->labels.clear();
   this->property_as_instance = false;
+  this->annotation_blacklist.clear();
+  this->annotations_.clear();
 }
 
 auto EvaluationContext::push_without_traverse(
@@ -93,6 +93,14 @@ auto EvaluationContext::pop(const bool dynamic) -> void {
   }
 }
 
+// TODO: At least currently, we only need to mask if a schema
+// makes use of `unevaluatedProperties` or `unevaluatedItems`
+// Detect if a schema does need this so if not, we avoid
+// an unnecessary copy
+auto EvaluationContext::mask() -> void {
+  this->annotation_blacklist.push_back(this->evaluate_path_);
+}
+
 auto EvaluationContext::annotate(const WeakPointer &current_instance_location,
                                  const JSON &value)
     -> std::pair<std::reference_wrapper<const JSON>, bool> {
@@ -102,16 +110,48 @@ auto EvaluationContext::annotate(const WeakPointer &current_instance_location,
   return {*(result.first), result.second};
 }
 
-auto EvaluationContext::defines_annotation(
-    const WeakPointer &expected_instance_location,
-    const WeakPointer &base_evaluate_path,
+auto EvaluationContext::defines_any_annotation(
+    const std::string &expected_keyword) const -> bool {
+  const auto instance_location_result{
+      this->annotations_.find(this->instance_location_)};
+  if (instance_location_result == this->annotations_.end()) {
+    return false;
+  }
+
+  for (const auto &[schema_location, schema_annotations] :
+       instance_location_result->second) {
+    assert(!schema_location.empty());
+    const auto &keyword{schema_location.back()};
+
+    if (keyword.is_property() && expected_keyword == keyword.to_property() &&
+        !schema_annotations.empty() &&
+        schema_location.initial().starts_with(this->evaluate_path_)) {
+      bool blacklisted = false;
+      for (const auto &masked : this->annotation_blacklist) {
+        if (schema_location.starts_with(masked) &&
+            !this->evaluate_path_.starts_with(masked)) {
+          blacklisted = true;
+          break;
+        }
+      }
+
+      if (!blacklisted) {
+        return true;
+      }
+    }
+  }
+
+  return false;
+}
+
+auto EvaluationContext::defines_sibling_annotation(
     const std::vector<std::string> &keywords, const JSON &value) const -> bool {
   if (keywords.empty()) {
     return false;
   }
 
   const auto instance_location_result{
-      this->annotations_.find(expected_instance_location)};
+      this->annotations_.find(this->instance_location_)};
   if (instance_location_result == this->annotations_.end()) {
     return false;
   }
@@ -125,7 +165,7 @@ auto EvaluationContext::defines_annotation(
         std::find(keywords.cbegin(), keywords.cend(), keyword.to_property()) !=
             keywords.cend() &&
         schema_annotations.contains(value) &&
-        schema_location.initial().starts_with(base_evaluate_path)) {
+        schema_location.initial().starts_with(this->evaluate_path_.initial())) {
       bool blacklisted = false;
       for (const auto &masked : this->annotation_blacklist) {
         if (schema_location.starts_with(masked) &&
@@ -145,15 +185,12 @@ auto EvaluationContext::defines_annotation(
 }
 
 auto EvaluationContext::largest_annotation_index(
-    const WeakPointer &expected_instance_location,
-    const std::vector<std::string> &keywords,
-    const std::uint64_t default_value) const -> std::uint64_t {
+    const std::string &expected_keyword) const -> std::uint64_t {
   // TODO: We should be taking masks into account
-
-  std::uint64_t result{default_value};
+  std::uint64_t result{0};
 
   const auto instance_location_result{
-      this->annotations_.find(expected_instance_location)};
+      this->annotations_.find(this->instance_location_)};
   if (instance_location_result == this->annotations_.end()) {
     return result;
   }
@@ -162,12 +199,7 @@ auto EvaluationContext::largest_annotation_index(
        instance_location_result->second) {
     assert(!schema_location.empty());
     const auto &keyword{schema_location.back()};
-    if (!keyword.is_property()) {
-      continue;
-    }
-
-    if (std::find(keywords.cbegin(), keywords.cend(), keyword.to_property()) ==
-        keywords.cend()) {
+    if (!keyword.is_property() || expected_keyword != keyword.to_property()) {
       continue;
     }
 
@@ -255,14 +287,6 @@ auto EvaluationContext::mark(const std::size_t id,
   this->labels.try_emplace(id, children);
 }
 
-// TODO: At least currently, we only need to mask if a schema
-// makes use of `unevaluatedProperties` or `unevaluatedItems`
-// Detect if a schema does need this so if not, we avoid
-// an unnecessary copy
-auto EvaluationContext::mask() -> void {
-  this->annotation_blacklist.push_back(this->evaluate_path_);
-}
-
 auto EvaluationContext::jump(const std::size_t id) const noexcept
     -> const SchemaCompilerTemplate & {
   assert(this->labels.contains(id));

src/evaluator/evaluator.cc

@@ -749,10 +749,8 @@ auto evaluate_step(
         // TODO: It might be more efficient to get all the annotations we
         // potentially care about as a set first, and the make the loop
         // check for O(1) containment in that set?
-        if (context.defines_annotation(
-                context.instance_location(),
-                // TODO: Can we avoid doing this expensive operation on a loop?
-                context.evaluate_path().initial(), loop.value,
+        if (context.defines_sibling_annotation(
+                loop.value,
                 // TODO: This conversion implies a string copy
                 JSON{entry.first})) {
           continue;
@@ -778,9 +776,7 @@ auto evaluate_step(
     case IS_STEP(SchemaCompilerAnnotationLoopItemsUnmarked): {
       EVALUATE_BEGIN(loop, SchemaCompilerAnnotationLoopItemsUnmarked,
                      target.is_array() &&
-                         !context.defines_annotation(
-                             context.instance_location(),
-                             context.evaluate_path(), loop.value, JSON{true}));
+                         !context.defines_any_annotation(loop.value));
       // Otherwise you shouldn't be using this step?
       assert(!loop.value.empty());
       const auto &array{target.as_array()};
@@ -808,18 +804,16 @@ auto evaluate_step(
     case IS_STEP(SchemaCompilerAnnotationLoopItemsUnevaluated): {
       // TODO: This precondition is very expensive due to pointer manipulation
       EVALUATE_BEGIN(loop, SchemaCompilerAnnotationLoopItemsUnevaluated,
-                     target.is_array() && !context.defines_annotation(
-                                              context.instance_location(),
-                                              context.evaluate_path().initial(),
+                     target.is_array() && !context.defines_sibling_annotation(
                                               loop.value.mask, JSON{true}));
       const auto &array{target.as_array()};
       result = true;
       auto iterator{array.cbegin()};
 
       // Determine the proper start based on integer annotations collected for
       // the current instance location by the keyword requested by the user.
-      const std::uint64_t start{context.largest_annotation_index(
-          context.instance_location(), {loop.value.index}, 0)};
+      const std::uint64_t start{
+          context.largest_annotation_index(loop.value.index)};
 
       // We need this check, as advancing an iterator past its bounds
       // is considered undefined behavior
@@ -831,11 +825,9 @@ auto evaluate_step(
       for (; iterator != array.cend(); ++iterator) {
         const auto index{std::distance(array.cbegin(), iterator)};
 
-        if (context.defines_annotation(
-                context.instance_location(),
-                // TODO: Can we avoid doing this expensive operation on a loop?
-                context.evaluate_path().initial(), loop.value.filter,
-                JSON{static_cast<std::size_t>(index)})) {
+        // TODO: Can we avoid doing this expensive operation on a loop?
+        if (context.defines_sibling_annotation(
+                loop.value.filter, JSON{static_cast<std::size_t>(index)})) {
           continue;
         }
 

src/evaluator/include/sourcemeta/jsontoolkit/evaluator_context.h

@@ -77,11 +77,6 @@ class SOURCEMETA_JSONTOOLKIT_EVALUATOR_EXPORT EvaluationContext {
   auto resources() const noexcept -> const std::vector<std::string> &;
   auto mark(const std::size_t id, const SchemaCompilerTemplate &children)
       -> void;
-  // TODO: At least currently, we only need to mask if a schema
-  // makes use of `unevaluatedProperties` or `unevaluatedItems`
-  // Detect if a schema does need this so if not, we avoid
-  // an unnecessary copy
-  auto mask() -> void;
   auto jump(const std::size_t id) const noexcept
       -> const SchemaCompilerTemplate &;
   auto find_dynamic_anchor(const std::string &anchor) const
@@ -91,15 +86,17 @@ class SOURCEMETA_JSONTOOLKIT_EVALUATOR_EXPORT EvaluationContext {
   // Annotations
   ///////////////////////////////////////////////
 
+  // TODO: At least currently, we only need to mask if a schema
+  // makes use of `unevaluatedProperties` or `unevaluatedItems`
+  // Detect if a schema does need this so if not, we avoid
+  // an unnecessary copy
+  auto mask() -> void;
   auto annotate(const WeakPointer &current_instance_location, const JSON &value)
       -> std::pair<std::reference_wrapper<const JSON>, bool>;
-  auto defines_annotation(const WeakPointer &expected_instance_location,
-                          const WeakPointer &base_evaluate_path,
-                          const std::vector<std::string> &keywords,
-                          const JSON &value) const -> bool;
-  auto largest_annotation_index(const WeakPointer &expected_instance_location,
-                                const std::vector<std::string> &keywords,
-                                const std::uint64_t default_value) const
+  auto defines_any_annotation(const std::string &keyword) const -> bool;
+  auto defines_sibling_annotation(const std::vector<std::string> &keywords,
+                                  const JSON &value) const -> bool;
+  auto largest_annotation_index(const std::string &keyword) const
       -> std::uint64_t;
 
 public:
@@ -119,14 +116,15 @@ class SOURCEMETA_JSONTOOLKIT_EVALUATOR_EXPORT EvaluationContext {
   std::vector<std::pair<std::size_t, std::size_t>> frame_sizes;
   // TODO: Keep hashes of schema resources URI instead for performance reasons
   std::vector<std::string> resources_;
-  std::vector<WeakPointer> annotation_blacklist;
-  // We don't use a pair for holding the two pointers for runtime
-  // efficiency when resolving keywords like `unevaluatedProperties`
-  std::map<WeakPointer, std::map<WeakPointer, std::set<JSON>>> annotations_;
+  // TODO: Try unordered_map
   std::map<std::size_t,
            const std::reference_wrapper<const SchemaCompilerTemplate>>
       labels;
   bool property_as_instance{false};
+
+  // For annotations
+  std::vector<WeakPointer> annotation_blacklist;
+  std::map<WeakPointer, std::map<WeakPointer, std::set<JSON>>> annotations_;
 #if defined(_MSC_VER)
 #pragma warning(default : 4251 4275)
 #endif

src/evaluator/include/sourcemeta/jsontoolkit/evaluator_template.h

@@ -398,8 +398,7 @@ DEFINE_STEP_APPLICATOR(Annotation, LoopPropertiesUnevaluated,
 /// @ingroup evaluator_instructions
 /// @brief Represents a compiler step that loops over array items when the array
 /// is considered unmarked
-DEFINE_STEP_APPLICATOR(Annotation, LoopItemsUnmarked,
-                       SchemaCompilerValueStrings)
+DEFINE_STEP_APPLICATOR(Annotation, LoopItemsUnmarked, SchemaCompilerValueString)
 
 /// @ingroup evaluator_instructions
 /// @brief Represents a compiler step that loops over unevaluated array items

src/jsonschema/default_compiler_2019_09.h

@@ -254,7 +254,7 @@ auto compiler_2019_09_applicator_unevaluateditems(
   } else {
     return {make<SchemaCompilerAnnotationLoopItemsUnmarked>(
         true, context, schema_context, dynamic_context,
-        SchemaCompilerValueStrings{"unevaluatedItems"}, std::move(children))};
+        SchemaCompilerValueString{"unevaluatedItems"}, std::move(children))};
   }
 }