Issue 383: Remove unused let-bindings from the generated RHS code (#384)

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

@@ -73,6 +73,12 @@
                         ;; Function that takes facts and determines what alpha nodes they match.
                         get-alphas-fn])
 
+(defn- is-variable?
+  "Returns true if the given expression is a variable (a symbol prefixed by ?)"
+  [expr]
+  (and (symbol? expr)
+       (.startsWith (name expr) "?")))
+
 (def ^:private reflector
   "For some reason (bug?) the default reflector doesn't use the
    Clojure dynamic class loader, which prevents reflecting on
@@ -309,8 +315,7 @@
   "Returns a set of the symbols in the given s-expression that start with '?' as keywords"
   [expression]
   (into #{} (for [item (flatten-expression expression)
-                  :when (and (symbol? item)
-                             (= \? (first (name item))))]
+                  :when (is-variable? item)]
               (keyword  item))))
 
 (defn field-name->accessors-used
@@ -386,7 +391,19 @@
 (defn compile-action
   "Compile the right-hand-side action of a rule, returning a function to execute it."
   [binding-keys rhs env]
-  (let [assignments (mapcat #(list (symbol (name %)) (list 'get-in '?__token__ [:bindings %])) binding-keys)
+  (let [;; Avoid creating let bindings in the compile code that aren't actually used in the body.
+        ;; The bindings only exist in the scope of the RHS body, not in any code called by it,
+        ;; so this scanning strategy will detect all possible uses of binding variables in the RHS.
+        ;; Note that some strategies with macros could introduce bindings, but these aren't something
+        ;; we're trying to support.  If necessary a user could macroexpand their RHS code manually before
+        ;; providing it to Clara.
+        rhs-bindings-used (variables-as-keywords rhs)
+
+        assignments (sequence
+                     (comp
+                      (filter rhs-bindings-used)
+                      (mapcat #(list (symbol (name %)) (list '-> '?__token__ :bindings %))))
+                     binding-keys)
 
         ;; The destructured environment, if any.
         destructured-env (if (> (count env) 0)
@@ -609,12 +626,6 @@
 
     node-type))
 
-(defn- is-variable?
-  "Returns true if the given expression is a variable (a symbol prefixed by ?)"
-  [expr]
-  (and (symbol? expr)
-       (.startsWith (name expr) "?")))
-
 (defn- extract-exists
   "Converts :exists operations into an accumulator to detect
    the presence of a fact and a test to check that count is

src/test/common/clara/test_performance_optimizations.cljc

@@ -0,0 +1,62 @@
+;; These tests validate that operations that the rules engine should optimize
+;; away are in fact optimized away.  The target here is not the actual execution time,
+;; which will vary per system, but verification that the action operations in question are not performed.
+#?(:clj
+   (ns clara.test-performance-optimizations
+     (:require [clara.tools.testing-utils :refer [def-rules-test
+                                                  side-effect-holder] :as tu]
+               [clara.rules :refer [fire-rules
+                                    insert
+                                    insert!
+                                    query]]
+
+               [clara.rules.testfacts :refer [->Cold ->ColdAndWindy]]
+               [clojure.test :refer [is deftest run-tests testing use-fixtures]]
+               [clara.rules.accumulators]
+               [schema.test :as st])
+     (:import [clara.rules.testfacts
+               Cold
+               ColdAndWindy]))
+
+   :cljs
+   (ns clara.test-performance-optimizations
+     (:require [clara.rules :refer [fire-rules
+                                    insert
+                                    insert!
+                                    query]]
+               [clara.rules.testfacts :refer [->Cold Cold
+                                              ->ColdAndWindy ColdAndWindy]]
+               [clara.rules.accumulators]
+               [cljs.test]
+               [schema.test :as st]
+               [clara.tools.testing-utils :refer [side-effect-holder] :as tu])
+     (:require-macros [clara.tools.testing-utils :refer [def-rules-test]]
+                      [cljs.test :refer [is deftest run-tests testing use-fixtures]])))
+
+(use-fixtures :once st/validate-schemas #?(:clj tu/opts-fixture))
+(use-fixtures :each tu/side-effect-holder-fixture)
+
+#?(:clj
+   (defmacro true-if-binding-absent
+     []
+     (not (contains? &env '?unused-binding))))
+
+;; See issue https://github.com/cerner/clara-rules/issues/383
+;; This validates that we don't create let bindings for binding
+;; variables that aren't used.  Doing so both imposes runtime costs
+;; and increases the size of the generated code that must be evaluated.
+(def-rules-test test-unused-rhs-binding-not-bound
+
+  {:rules [cold-windy-rule [[[ColdAndWindy (= ?used-binding temperature) (= ?unused-binding windspeed)]]
+                            (when (true-if-binding-absent)
+                              (insert! (->Cold ?used-binding)))]]
+
+   :queries [cold-query [[] [[Cold (= ?c temperature)]]]]
+
+   :sessions [empty-session [cold-windy-rule cold-query] {}]}
+
+  (is (= [{:?c 0}]
+         (-> empty-session
+             (insert (->ColdAndWindy 0 0))
+             fire-rules
+             (query cold-query)))))