(oracle-samples#426) Join binding followed by usage of binding leads …

…to unresolvable symbol at compilation

CHANGELOG.md

@@ -1,5 +1,8 @@
 This is a history of changes to clara-rules.
 
+# 0.20.0-SNAPSHOT
+* Correct implementation surrrouning bindings in ExpressionJoinNodes. See [Issue 426](https://github.com/cerner/clara-rules/issues/426)
+
 # 0.19.1
 * Added a new field to the clara.rules.engine/Accumulator record.  This could be a breaking change for any user durability implementations with low-level performance optimizations.  See [PR 410](https://github.com/cerner/clara-rules/pull/410) for details.
 * Performance improvements for :exists conditions.  See [issue 298](https://github.com/cerner/clara-rules/issues/298).

src/main/clojure/clara/macros.clj

@@ -174,8 +174,10 @@
                                                      ;; that a rule wouldn't, or at least shouldn't for
                                                      ;; clarity, start the names of other locals or vars
                                                      ;; with "?".
-                                                     (mapv (comp symbol name) all-bindings))]
-              (com/compile-action all-bindings
+                                                     (mapv (comp symbol name) (:bindings beta-node)))]
+              ;; using the :bindings defined on the beta-node rather than `all-bindings`, as all-bindings
+              ;; does not account for bindings that reside within expression join nodes
+              (com/compile-action (:bindings beta-node)
                                   ;; Using private function for now as a workaround.
                                   (if (:ns-name production)
                                     (if (com/compiling-cljs?)

src/main/clojure/clara/rules/compiler.clj

@@ -9,7 +9,6 @@
             [clojure.core.reducers :as r]
             [clojure.reflect :as reflect]
             [clojure.set :as s]
-            [clojure.set :as set]
             [clojure.string :as string]
             [clojure.walk :as walk]
             [schema.core :as sc]
@@ -803,17 +802,17 @@
 
         ;; Unsatisfied conditions remain, so find ones we can satisfy.
         (let [satisfied? (fn [classified-condition]
-                           (clojure.set/subset? (:unbound classified-condition)
-                                                bound-variables))
+                           (s/subset? (:unbound classified-condition)
+                                      bound-variables))
 
               ;; Find non-accumulator conditions that are satisfied. We defer
               ;; accumulators until later in the rete network because they
               ;; may fire a default value if all needed bindings earlier
               ;; in the network are satisfied.
               satisfied-non-accum? (fn [classified-condition]
                                      (and (not (:is-accumulator classified-condition))
-                                          (clojure.set/subset? (:unbound classified-condition)
-                                                               bound-variables)))
+                                          (s/subset? (:unbound classified-condition)
+                                                     bound-variables)))
 
               has-satisfied-non-accum (some satisfied-non-accum? remaining-conditions)
 
@@ -825,16 +824,16 @@
                                   (remove satisfied-non-accum? remaining-conditions)
                                   (remove satisfied? remaining-conditions))
 
-              updated-bindings (apply clojure.set/union bound-variables
+              updated-bindings (apply s/union bound-variables
                                       (map :bound newly-satisfied))]
 
           ;; If no existing variables can be satisfied then the production is invalid.
           (when (empty? newly-satisfied)
 
             ;; Get the subset of variables that cannot be satisfied.
-            (let [unsatisfiable (clojure.set/difference
-                                 (apply clojure.set/union (map :unbound still-unsatisfied))
-                                 bound-variables)]
+            (let [unsatisfiable (s/difference
+                                  (apply s/union (map :unbound still-unsatisfied))
+                                  bound-variables)]
               (throw (ex-info (str "Using variable that is not previously bound. This can happen "
                                    "when an expression uses a previously unbound variable, "
                                    "or if a variable is referenced in a nested part of a parent "
@@ -855,11 +854,30 @@
 (defn- non-equality-unifications
   "Returns a set of unifications that do not use equality-based checks."
   [constraints]
-  (let [[bound-variables unbound-variables] (classify-variables constraints)]
-    (into #{}
-          (for [constraint constraints
-                :when (non-equality-unification? constraint bound-variables)]
-            constraint))))
+  (let [[bound-variables _] (classify-variables constraints)]
+    (loop [[cur & more] constraints
+           constraints #{}
+           bound-variables bound-variables]
+      (if cur
+        (let [non-equity? (non-equality-unification? cur bound-variables)
+
+              ;; In the event that the unification is also a binding, then we will need to remove
+              ;; the bound variable from the previously bound variables. This prevents further constraints
+              ;; that use this variable from being considered "equality-unifications".
+              ;; See https://github.com/cerner/clara-rules/issues/426 for more info.
+              bound-variables (if (and non-equity?
+                                       (seq cur)
+                                       (equality-expression? cur))
+                                (s/difference bound-variables
+                                              (into #{}
+                                                    (filter is-variable?)
+                                                    cur))
+                                bound-variables)
+              constraints (if non-equity?
+                            (conj constraints cur)
+                            constraints)]
+          (recur more constraints bound-variables))
+        constraints))))
 
 (sc/defn condition-to-node :- schema/ConditionNode
   "Converts a condition to a node structure."
@@ -917,7 +935,7 @@
                         constraint-bindings)
 
         new-bindings (s/difference (variables-as-keywords (:constraints condition))
-                                             parent-bindings)
+                                   parent-bindings)
 
         join-filter-bindings (if join-filter-expressions
                                (variables-as-keywords join-filter-expressions)
@@ -1018,7 +1036,7 @@
           ;; have the necessary bindings. 
           ;; See https://github.com/cerner/clara-rules/issues/304 for more details
           ;; and a case that behaves incorrectly without this check.
-          ancestor-bindings-in-negation-expr (set/intersection
+          ancestor-bindings-in-negation-expr (s/intersection
                                               (variables-as-keywords negation-expr)
                                               ancestor-bindings)
 
@@ -1224,8 +1242,8 @@
                           ;; for use in descendent nodes.
                           {:beta-graph (:beta-graph new-result)
                            :new-ids (into (:new-ids previous-result) (:new-ids new-result))
-                           :bindings (set/union (:bindings previous-result)
-                                                (:bindings new-result))}))
+                           :bindings (s/union (:bindings previous-result)
+                                              (:bindings new-result))}))
 
                       ;; Initial reduce value, combining previous graph, parent ids, and ancestor variable bindings.
                       {:beta-graph beta-with-negations

src/main/clojure/clara/rules/testfacts.cljc

@@ -5,6 +5,7 @@
 ;; place them here as leiningen won't AOT compile test resources.
 (defrecord Temperature [temperature location])
 (defrecord WindSpeed [windspeed location])
+(defrecord WindChill [temperature location wind-chill])
 (defrecord Cold [temperature])
 (defrecord Hot [temperature])
 (defrecord ColdAndWindy [temperature windspeed])

src/test/common/clara/test_bindings.cljc

@@ -12,14 +12,15 @@
                                     query]]
 
                [clara.rules.testfacts :refer [->Temperature ->Cold ->WindSpeed
-                                              ->ColdAndWindy]]
+                                              ->ColdAndWindy ->WindChill]]
                [clojure.test :refer [is deftest run-tests testing use-fixtures]]
                [clara.rules.accumulators :as acc]
                [schema.test :as st])
      (:import [clara.rules.testfacts
                Temperature
                Cold
                WindSpeed
+               WindChill
                ColdAndWindy]))
 
    :cljs
@@ -33,6 +34,7 @@
                [clara.rules.testfacts :refer [->Temperature Temperature
                                               ->Cold Cold
                                               ->WindSpeed WindSpeed
+                                              ->WindChill WindChill
                                               ->ColdAndWindy ColdAndWindy]]
                [clara.rules.accumulators :as acc]
                [cljs.test]
@@ -369,3 +371,70 @@
              fire-rules
              (query cold-query)))
       "The query results should not be empty for a matching location"))
+
+;; https://github.com/cerner/clara-rules/issues/426
+;; A join binding used in the same condition but different constraint will fail to compile.
+(defn calc-wind-chill
+  [temp wind-speed]
+  (+ 35.74
+     (- (* 0.6215 temp)
+        (* 35.75
+           (Math/pow wind-speed 0.16)))
+     (* 0.4275 temp (Math/pow wind-speed 0.16))))
+
+(def-rules-test test-join-binding-followed-by-usage
+  {:rules [cold-windy-rule [[[Temperature
+                              (= ?t temperature)
+                              (= ?loc location)]
+                            [WindSpeed
+                             (= location ?loc)
+                             (= ?wc (calc-wind-chill ?t windspeed))
+                             (<= ?wc 32)]]
+                            (insert! (->WindChill ?t ?loc ?wc))]]
+   :queries [wind-chill-query [[]
+                               [[WindChill
+                                 (= ?t temperature)
+                                 (= ?wind-chill wind-chill)
+                                 (= ?loc location)]]]]
+   :sessions [empty-session [cold-windy-rule wind-chill-query] {}]}
+  ;; The overall intent of this test is to prove that the rules above compile and
+  ;; operate as expected.
+  (is (= [{:?loc "LHR"
+           :?t 36
+           :?wind-chill (calc-wind-chill 36 7)}]
+         (-> empty-session
+             (insert (->Temperature 36 "LHR"))
+             (insert (->WindSpeed 7 "LHR"))
+             fire-rules
+             (query wind-chill-query)))
+      ;; Not the intent of the test, but a distinct message in the event the test fails.
+      "The query should return the wind-chill for the given location since the wind chill is under 32 degrees"))
+
+;; https://github.com/cerner/clara-rules/issues/426
+;; Due to the way we compile the beta-network for cljs this test would fail for cljs because the binding
+;; expression join node(?wc) would not be recognised correctly in the production node. Thus ?wc would be
+;; assumed to be a variable defined in this ns and would resolve to nil at runtime.
+(def-rules-test test-join-binding-used-in-a-produciton-node
+  {:rules [cold-windy-rule [[[Temperature
+                              (= ?t temperature)
+                              (= ?loc location)]
+                             [WindSpeed
+                              (= location ?loc)
+                              (= ?wc (calc-wind-chill ?t windspeed))]]
+                            (insert! (->WindChill ?t ?loc ?wc))]]
+   :queries [wind-chill-query [[]
+                               [[WindChill
+                                 (= ?t temperature)
+                                 (= ?wind-chill wind-chill)
+                                 (= ?loc location)]]]]
+   :sessions [empty-session [cold-windy-rule wind-chill-query] {}]}
+  (is (= [{:?loc "LHR"
+           :?t 36
+           :?wind-chill (calc-wind-chill 36 7)}]
+         (-> empty-session
+             (insert (->Temperature 36 "LHR"))
+             (insert (->WindSpeed 7 "LHR"))
+             fire-rules
+             (query wind-chill-query)))
+      ;; Not the intent of the test, but a distinct message in the event the test fails.
+      "The query should return the wind-chill for the given location since there is a Temperature and WindSpeed"))