Fixing bugs exposed by fixed ACI tests (#84)

Fixing several impactful bugs exposed by fixing the ACI tests. **Bug fixes** - The ACI tests were incorrectly written in such a way that they would always pass. This has been addressed, and safeguards put in place in the test driver to detect this in the future. - Some statements nested in an every block would break the conditions if the item sequence was an array for which multiple elements could be identified by the some that were valid (see the new `every_some` test for a minimal reproduction). This has been fixed. - Dead links were being incorrectly identified in some cases. This has been addressed by making the check more precise and moving it to the functions pass. - Var lookups into a virtual or base document would resolve the entire document, causing needless recursion. This has been fixed. --------- Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>
microsoft · Sep 20, 2023 · 5be2edf · 5be2edf
1 parent e07e37f
commit 5be2edf
Show file tree

Hide file tree

Showing 18 changed files with 372 additions and 260 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,5 +1,19 @@
 # Changelog
 
+## 2023-09-20 - Version 0.3.9
+Fixing several impactful bugs.
+
+**Bug fixes**
+- The ACI tests were incorrectly written in such a way that they would always pass. This has been addressed,
+  and safeguards put in place in the test driver to detect this in the future.
+- Some statements nested in an every block would break the conditions if the item sequence was an array for
+  which multiple elements could be identified by the some that were valid (see the new `every_some` test for
+  a minimal reproduction). This has been fixed.
+- Dead links were being incorrectly identified in some cases. This has been addressed by making the check
+  more precise and moving it to the functions pass.
+- Var lookups into a virtual or base document would resolve the entire document, causing needless recursion.
+  This has been fixed. 
+
 ## 2023-09-18 - Version 0.3.8
 Adding new examples for Python and Rust usage.
 

diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-0.3.8
+0.3.9
diff --git a/examples/rust/Cargo.toml b/examples/rust/Cargo.toml
@@ -6,5 +6,5 @@ edition = "2021"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-regorust = "0.3.8"
+regorust = "0.3.9"
 clap = { version = "4.0", features = ["derive"] }
diff --git a/src/internal.cc b/src/internal.cc
@@ -779,4 +779,9 @@ namespace rego
   {
     return Set << set_members;
   }
+
+  bool is_module(const Node& var)
+  {
+    return var->type().in({Submodule, DataItem, Data});
+  }
 }
diff --git a/src/internal.hh b/src/internal.hh
@@ -50,6 +50,7 @@ namespace rego
   bool is_truthy(const Node& node);
   bool is_undefined(const Node& node);
   bool is_ref_to_type(const Node& var, const std::set<Token>& types);
+  bool is_module(const Node& var);
   std::string strip_quotes(const std::string_view& str);
   std::string type_name(const Token& type, bool specify_number = false);
   std::string type_name(const Node& node, bool specify_number = false);
@@ -112,6 +113,7 @@ namespace rego
     static Node set_difference(const Node& lhs, const Node& rhs);
     static Nodes resolve_varseq(const Node& varseq);
     static Nodes object_lookdown(const Node& object, const Node& query);
+    static Nodes module_lookdown(const Node& module, const std::string& name);
     static Node inject_args(const Node& rulefunc, const Nodes& args);
     static Node membership(
       const Node& index, const Node& item, const Node& itemseq);

diff --git a/src/passes/constants.cc b/src/passes/constants.cc
@@ -3,7 +3,6 @@
 namespace rego
 {
   // Converts all rules with constant terms to be DataTerm nodes.
-  // Also reshapes ExprEvery nodes so they can be lifted later.
   PassDef constants()
   {
     return {
@@ -103,72 +102,6 @@ namespace rego
           return DataObjectItem << (DataTerm << key) << (DataTerm << val);
         },
 
-      In(Expr) *
-          (T(ExprEvery)([](auto& n) {
-             return is_in(*n.first, {Policy}) && is_in(*n.first, {UnifyBody});
-           })
-           << ((T(VarSeq) << (T(Var)[Val] * End)) * T(UnifyBody)[UnifyBody] *
-               (T(IsIn) << T(Expr)[Expr]))) >>
-        [](Match& _) {
-          Location item = _.fresh({"item"});
-          Location every = _.fresh({"every"});
-
-          return ExprEvery
-            << (UnifyBody
-                << (Local << (Var ^ item) << Undefined)
-                << (LiteralNot
-                    << (UnifyBody
-                        << (LiteralEnum << (Var ^ item) << _(Expr))
-                        << (Literal
-                            << (Expr
-                                << (RefTerm << _(Val)) << Unify
-                                << (RefTerm
-                                    << (Ref
-                                        << (RefHead << (Var ^ item))
-                                        << (RefArgSeq
-                                            << (RefArgBrack
-                                                << (Scalar << (Int ^ "1"))))))))
-                        << (LiteralNot << _(UnifyBody)))));
-        },
-
-      In(Expr) *
-          (T(ExprEvery)([](auto& n) {
-             return is_in(*n.first, {Policy}) && is_in(*n.first, {UnifyBody});
-           })
-           << ((T(VarSeq) << (T(Var)[Idx] * T(Var)[Val] * End)) *
-               T(UnifyBody)[UnifyBody] * (T(IsIn) << T(Expr)[Expr]))) >>
-        [](Match& _) {
-          Location item = _.fresh({"item"});
-          Location every = _.fresh({"every"});
-
-          return ExprEvery
-            << (UnifyBody
-                << (Local << (Var ^ item) << Undefined)
-                << (LiteralNot
-                    << (UnifyBody
-                        << (LiteralEnum << (Var ^ item) << _(Expr))
-                        << (Literal
-                            << (Expr
-                                << (RefTerm << _(Idx)->clone()) << Unify
-                                << (RefTerm
-                                    << (Ref
-                                        << (RefHead << (Var ^ item))
-                                        << (RefArgSeq
-                                            << (RefArgBrack
-                                                << (Scalar << (Int ^ "0"))))))))
-
-                        << (Literal
-                            << (Expr
-                                << (RefTerm << _(Val)->clone()) << Unify
-                                << (RefTerm
-                                    << (Ref
-                                        << (RefHead << (Var ^ item))
-                                        << (RefArgSeq
-                                            << (RefArgBrack
-                                                << (Scalar << (Int ^ "1"))))))))
-                        << (LiteralNot << _(UnifyBody)))));
-        },
-
       In(ModuleSeq) * (T(Module)[Lhs] * T(Module)[Rhs])([](auto& n) {
         Node lhs = n.first[0];
         Node rhs = n.first[1];

diff --git a/src/passes/functions.cc b/src/passes/functions.cc
@@ -199,6 +199,17 @@ namespace rego
           (T(RefTerm)
            << (T(SimpleRef) << (T(Var)[Var] * (T(RefArgDot)[RefArgDot])))) >>
         [](Match& _) {
+          auto defs = _(Var)->lookup();
+          if (!defs.empty() && defs.front()->type().in({Submodule, Data}))
+          {
+            // At this point all possible documents are fully qualified and in
+            // the symbol table. As such, a reference such as this, which points
+            // to a top-level module or rule, is a dead link and can be
+            // replaced.
+            Location dead = _.fresh({"dead"});
+            return Var ^ dead;
+          }
+
           Location field_name = _(RefArgDot)->front()->location();
           Node arg = Scalar << (JSONString ^ field_name);
           return Function << (JSONString ^ "apply_access")
@@ -219,6 +230,17 @@ namespace rego
           }
           else
           {
+            auto defs = _(Var)->lookup();
+            if (!defs.empty() && defs.front()->type().in({Submodule, Data}))
+            {
+              // At this point all possible documents are fully qualified and in
+              // the symbol table. As such, a reference such as this, which
+              // points to a top-level module or rule, is a dead link and can be
+              // replaced.
+              Location dead = _.fresh({"dead"});
+              return Var ^ dead;
+            }
+
             return Function << (JSONString ^ "apply_access")
                             << (ArgSeq << _(Var) << (Term << arg));
           }

diff --git a/src/passes/simple_refs.cc b/src/passes/simple_refs.cc
@@ -100,16 +100,6 @@ namespace rego
                   (T(RefArgSeq) << (RefArg[Head] * RefArg++[Tail])))) >>
         [](Match& _) {
           LOG("ref.a/ref[a]");
-          if (_(Var)->location().view() == "data")
-          {
-            // At this point all possible documents are fully qualified and in
-            // the symbol table. As such, a reference such as this, which points
-            // to a top-level module or rule, is a dead link and can be
-            // replaced.
-            Location dead = _.fresh({"dead"});
-            return RefTerm << (Var ^ dead);
-          }
-
           NodeRange tail = _[Tail];
           Location ref = _.fresh({"ref"});
           Node seq =

diff --git a/src/passes/strings.cc b/src/passes/strings.cc
@@ -20,10 +20,6 @@ namespace
           buf << "\\\\";
           break;
 
-        case '/':
-          buf << "\\/";
-          break;
-
         case '\b':
           buf << "\\b";
           break;

diff --git a/src/passes/symbols.cc b/src/passes/symbols.cc
@@ -385,25 +385,29 @@ namespace rego
                                              << (Scalar << (Int ^ "1"))))))))
             << *_[UnifyBody];
 
-          return Seq << (Lift << UnifyBody
-                              << (Local << (Var ^ itemseq) << Undefined))
-                     << (Lift << UnifyBody
-                              << (Literal
-                                  << (Expr << (RefTerm << (Var ^ itemseq))
-                                           << Unify << *_[Expr])))
-                     << (ExprCall
-                         << (RuleRef << (Var ^ "count"))
+          return Seq
+            << (Lift << UnifyBody << (Local << (Var ^ itemseq) << Undefined))
+            << (Lift << UnifyBody
+                     << (Literal
+                         << (Expr << (RefTerm << (Var ^ itemseq)) << Unify
+                                  << *_[Expr])))
+            << (ExprCall << (RuleRef << (Var ^ "count"))
                          << (ArgSeq
                              << (Expr
                                  << (Term
-                                     << (ArrayCompr
+                                     << (SetCompr
                                          << (Expr
                                              << (RefTerm << _(Val)->clone()))
                                          << rulebody)))))
-                     << Equals
-                     << (ExprCall
-                         << (RuleRef << (Var ^ "count"))
-                         << (ArgSeq << (Expr << (RefTerm << (Var ^ itemseq)))));
+            << Equals
+            << (ExprCall
+                << (RuleRef << (Var ^ "count"))
+                << (ArgSeq
+                    << (Expr
+                        << (ExprCall
+                            << (RuleRef << (Var ^ "cast_set"))
+                            << (ArgSeq
+                                << (Expr << (RefTerm << (Var ^ itemseq))))))));
         },
 
       In(Expr) *

diff --git a/src/resolver.cc b/src/resolver.cc
@@ -496,43 +496,14 @@ namespace rego
       return object_lookdown(container, query);
     }
 
-    if (container->type() == Data || container->type() == DataModule)
+    if (
+      container->type() == Data || container->type() == DataItem ||
+      container->type() == Submodule)
     {
       Node key = arg->front();
-      std::string key_str = strip_quotes(to_json(key));
-      Nodes defs = container->lookdown(key_str);
-      if (defs.size() == 0)
-      {
-        return Nodes({err(container, "No definition found for " + key_str)});
-      }
-
-      if (defs[0]->type() == RuleComp || defs[0]->type() == RuleFunc)
-      {
-        return defs;
-      }
-
-      Nodes nodes;
-      for (auto& def : defs)
-      {
-        if (def->type() == DataItem)
-        {
-          nodes.push_back(def / Val);
-        }
-        else if (def->type() == ObjectItem)
-        {
-          nodes.push_back(def / Val);
-        }
-        else if (def->type() == Submodule)
-        {
-          nodes.push_back(def / DataModule);
-        }
-        else
-        {
-          throw std::runtime_error("Not implemented");
-        }
-      }
-
-      return nodes;
+      std::string key_str = std::string((container / Key)->location().view());
+      key_str += "." + strip_quotes(to_json(key));
+      return module_lookdown(container, key_str);
     }
 
     if (container->type() == Set)
@@ -1047,6 +1018,54 @@ namespace rego
     return rulefunc;
   }
 
+  Nodes Resolver::module_lookdown(
+    const Node& container, const std::string& query)
+  {
+    Node module = container;
+    if (
+      module->type() == Submodule || module->type() == Data ||
+      module->type() == DataItem)
+    {
+      module = module / Val;
+    }
+
+    if (module->type() != DataModule)
+    {
+      return {err(container, "Not a module")};
+    }
+
+    Nodes rules;
+    for (auto& rule : *module)
+    {
+      if (rule->type() == RuleFunc)
+      {
+        Node args = rule / RuleArgs;
+        if (args->size() > 0)
+        {
+          // no way to include this without arguments
+          continue;
+        }
+      }
+
+      Location name;
+      if (rule->type() == Submodule)
+      {
+        name = (rule / Key)->location();
+      }
+      else
+      {
+        name = (rule / Var)->location();
+      }
+
+      if (name == query)
+      {
+        rules.push_back(rule);
+      }
+    }
+
+    return rules;
+  }
+
   Nodes Resolver::object_lookdown(const Node& object, const Node& query)
   {
     Nodes terms;

diff --git a/src/unifier.cc b/src/unifier.cc
@@ -766,7 +766,12 @@ namespace rego
         source_values.begin(),
         source_values.end(),
         std::back_inserter(values),
-        [var](auto& source_value) {
+        [var, this](auto& source_value) {
+          if (is_module(source_value->node()))
+          {
+            Node module_obj = resolve_module(source_value->node());
+            return ValueDef::copy_to(ValueDef::create(module_obj), var);
+          }
           return ValueDef::copy_to(source_value, var);
         });
     }
@@ -910,19 +915,18 @@ namespace rego
         }
         else if (def->type() == Data)
         {
-          values.push_back(ValueDef::create(resolve_module(def)));
+          values.push_back(ValueDef::create(def));
         }
         else if (def->type() == RuleFunc)
         {
-          // these will always be an argument to apply_access
           values.push_back(ValueDef::create(def));
         }
         else if (def->type() == DataItem)
         {
           Node value = def / Val;
           if (value->type() == DataModule)
           {
-            values.push_back(ValueDef::create(resolve_module(def)));
+            values.push_back(ValueDef::create(def));
           }
           else
           {
@@ -966,7 +970,6 @@ namespace rego
     {
       values.push_back(ValueDef::create(var, container, sources));
     }
-
     else
     {
       auto maybe_nodes = Resolver::apply_access(container, args[1]->node());