Test condition on equality of variables bound in previous accumulator conditions is ignored

[WilliamParker]

@sparkofreason reported that the following rule does not behave as expected; specifically, it ignores the :test condition.

(defrule lost-game
  [?ships <- (acc/count) :from [Ship]]
  [?destroyed <- (acc/count) :from [Destroyed]]
  [:test (= ?ships ?destroyed)]
  =>
  (insert! (->Loser)))

The rules network built from this rule doesn't have the :test condition as a descendant of the alpha roots; since facts are provided to the alpha roots and then propagate downward toward rule (ProductionNode) and query (QueryNode) nodes the :test condition will be ignored.

clara.test-rules> (clara.rules/defrule lost-game
  [?ships <- (acc/count) :from [Ship]]
  [?destroyed <- (acc/count) :from [Destroyed]]
  [:test (= ?ships ?destroyed)]
  =>
  (insert! (->Loser)))
#'clara.test-rules/lost-game
clara.test-rules> (-> (mk-session [lost-game]) .rulebase :alpha-roots (get Ship))
({:env nil,
  :children
  ({:id 2,
    :accum-condition {:accumulator (clara.rules.accumulators/count), :from {:type clara.test_rules.Ship, :constraints []}},
    :accumulator
    {:initial-value 0,
     :reduce-fn #function[clara.rules.accumulators/count/fn--16091],
     :combine-fn #function[clojure.core/+],
     :convert-return-fn #function[clojure.core/identity],
     :retract-fn #function[clara.rules.accumulators/count/fn--16093]},
    :result-binding :?ships,
    :children
    ({:id 3,
      :accum-condition {:accumulator (clara.rules.accumulators/count), :from {:type clara.test_rules.Destroyed, :constraints []}},
      :accumulator
      {:initial-value 0,
       :reduce-fn #function[clara.rules.accumulators/count/fn--16091],
       :combine-fn #function[clojure.core/+],
       :convert-return-fn #function[clojure.core/identity],
       :retract-fn #function[clara.rules.accumulators/count/fn--16093]},
      :result-binding :?destroyed,
      :children
      ({:id 4,
        :production
        {:ns-name clara.test-rules,
         :lhs
         [{:accumulator (clara.rules.accumulators/count), :from {:type clara.test_rules.Ship, :constraints []}, :result-binding :?ships}
          {:accumulator (clara.rules.accumulators/count), :from {:type clara.test_rules.Destroyed, :constraints []}, :result-binding :?destroyed}
          {:constraints [(= ?ships ?destroyed)]}],
         :rhs (do (insert! (->Loser))),
         :name "clara.test-rules/lost-game"},
        :rhs #function[clara.test-rules/eval17812/fn--17813]}),
      :binding-keys #{},
      :new-bindings #{}}),
    :binding-keys #{},
    :new-bindings #{}}),
  :activation #function[clara.test-rules/eval17828/fn--17829]})

@Zylox looked into this and it seems that sort-conditions places the :test condition above the accumulator conditions. It seems that sort-conditions assumes that all equality join variables in a condition can be created by that condition, but that breaks down in this case since we're relating equality join variables to each other, not to fields on a fact, constants, etc. One possible route to solve this would be to replace the usage of analyze-condition with a function that specifically targeted whether a condition can create a binding, not merely whether the binding is an equality join binding.

For anyone experiencing this issue, it can be avoided by using indirection on =. Since Clara special-cases the circumstance in which the first symbol is = (see the equality-expression? function in the compiler something like the following will work:

(defn equals-indirection [& args] (apply = args))
clara.test-rules> (clara.rules/defrule lost-game
  [?ships <- (acc/count) :from [Ship]]
  [?destroyed <- (acc/count) :from [Destroyed]]
  [:test (equals-indirection ?ships ?destroyed)]
  =>
  (insert! (->Loser)))

Obviously this is confusing from a user's point of view though and it should be fixed; the example directly above should be considered a temporary workaround.

[WilliamParker]

I have submitted a PR for this at #407. The fix is to simply consider any binding variable used a :test node to be an unbound variable. Since TestNode simply passes on a subset of the tokens it receives I don't see any way for it actually create bindings. If such behavior is desired in future I think :let bindings, discussed in #375, would be a better way to approach the problem than a "sneaky" approach like a filter (which is essentially what a :test node is) having a side effect of creating derived data.

There are other issues around condition sorting and binding analysis discussed at #373 but the issues there are much more extensive and their solutions ambiguous. The fix here is much quicker and the desirable behavior unambiguous so I think we can apply this targeted fix for the moment.

[WilliamParker]

Closing since I have merged the PR.