:Or condition with optional bindings can cause different values for a binding in different conditions

Question

:Or condition with optional bindings can cause different values for a binding in different conditions

Opened this issue 6 years ago · 3 comments

I'd expect the following test to pass or possibly fail to compile, depending on the resolution of issue 373.

(deftest test-boolean-or-bindings
  (let [r (dsl/parse-rule [[:or
                            [?h <- Hot]
                            [Cold (= ?c temperature)]]
                           [:test (nil? ?c)]
                           [?c <- (acc/all) :from [Cold]]]
                          (insert! (->Temperature ?c "MCI")))

        q (dsl/parse-query [] [[Temperature (= ?t temperature)]])

        s (mk-session [r q] :cache false)]

    (is (empty? (-> s
                    (insert (->Hot 10) (->Cold 10))
                    fire-rules
                    (query q))))))

Note that the binding ?c is required to be nil for the rule to match, but the acc/all accumulator will not return a nil top-level value. Instead, this test fails:

clara.test-rules> (test-boolean-or-bindings)

FAIL in (test-boolean-or-bindings) (test_rules.clj:2694)
expected: (empty? (-> s (insert (->Hot 10) (->Cold 10)) fire-rules (query q)))
  actual: (not (empty? ({:?t [#clara.rules.testfacts.Cold{:temperature 10}]})))
nil

Note the sorting order in this rule and the condition analysis outputs in this rule.

clara.test-rules> (-> r :lhs com/sort-conditions)
([:or
  {:type clara.rules.testfacts.Hot,
   :constraints [],
   :fact-binding :?h}
  {:type clara.rules.testfacts.Cold,
   :constraints [(= ?c temperature)]}]
 {:constraints [(nil? ?c)]}
 {:accumulator (clara.rules.accumulators/all),
  :from {:type clara.rules.testfacts.Cold, :constraints []},
  :result-binding :?c})

This is the original order of the conditions in the rule.

clara.test-rules> (->> r :lhs (map com/analyze-condition) (map #(select-keys % [:bound :unbound])))
({:bound #{?h ?c}, :unbound #{}}
 {:bound #{}, :unbound #{?c}}
 {:bound #{?c}, :unbound #{}})

I think what happens here is as follows:

The condition sorting allows the :or condition to remain first; note that condition sorting is done before the transformation to DNF.
We then end up with two rules like the following from this reduce over disjunctions:

[[?h <- Hot]
 [:test (nil? ?c)]
 [?c <- (acc/all) :from [Cold]]]
 (insert! (->Temperature ?c "MCI"))

[[Cold (= ?c temperature)]
 [:test (nil? ?c)]
 [?c <- (acc/all) :from [Cold]]]
 (insert! (->Temperature ?c "MCI"))

In the first rule, when the :test condition is evaluated, the binding :?c doesn't exist. From the test filter function generating function it can be seen that when a binding isn't present it will just take the default value of a lookup in a map for an absent key, namely nil. Therefore the test passes, but the binding still isn't present.
The accumulator condition creates the binding here and passes it downstream.
As a result ?c in the RHS assumes a different value than it had in the :test condition earlier, which seems like a bug to me.

Note that in the absence of an :or condition, this behavior does not occur as verified by the following test that passes:

(deftest test-similar-case-without-or
  (let [r (dsl/parse-rule [[Cold (= ?c temperature)]
                           [?c <- (acc/all) :from [Cold]]]
                          (insert! (->Temperature ?c "MCI")))


        q (dsl/parse-query [] [[Temperature (= ?t temperature)]])

        s (mk-session [r q] :cache false)]

    (is (empty? (-> s
                    (insert (->Cold nil))
                    fire-rules
                    (query q))))))

Firing when the result binding from an accumulator contradicts a binding from a prior condition was fixed in issue 132 by this when check. This comment on issue 132 clarifies why this fix works.

Answer 1 · 2018-01-19T17:57:23.000Z

I think there is probably an issue here. However, it seems like this example is just so confusing that it is hard to even know what the "right semantics" should be.
I think it'd be nice to have a more realistic case so that demonstrating the problem was clearer.

For example: Does an accumulator have to be used to arrive the sort of problem behavior here?

Answer 2 · 2018-01-19T17:59:57.000Z

However, looking back to #373 it does seem to me that just having the order of conditions and order when bindings are present vs not be based on the order of conditions seems like it'd make a lot of things much clearer.
e.g.

rule 1
[A (= ?x x)]
[:test (something ?x)]  ;; ok

rule 2
[:test (something ?x)]  ;; not ok - throw compile exception
[A (= ?x x)]

Answer 3 · 2018-01-19T18:34:08.000Z

Regarding

For example: Does an accumulator have to be used to arrive the sort of problem behavior here?

I don't think so, but it makes it more consistent since accumulator conditions are sorted below non-accumulator conditions when possible.

The problem can be generically stated as:

Condition A is considered to create binding X, but actually doesn't.
Condition B depends on the value of binding X in some way.
Condition C actually does create binding X.

where the conditions are ordered [A B C]. If there is no accumulation there is no expectation of the ordering, which means the ordering could create this issue or not.