Accumulators not working inside negation conditions

[WilliamParker]

As a user I'd expect the following test to pass:

(def-rules-test test-exists-inside-negation

  {:rules [no-cold-exists [[[:not [:exists [Temperature (< temperature 20)]]]]
                           (insert! (->Hot :unknown))]]

   :queries [hot-query [[] [[Hot (= ?t temperature)]]]]

   :sessions [empty-session [no-cold-exists hot-query] {}]}

  (is (= [{:?t :unknown}]
         (-> empty-session
             fire-rules
             (query hot-query)))))

However, it fails with

clojure.lang.ExceptionInfo: Output of condition-to-node does not match schema: 

Of course, in this particular case one can remove the :exists clause without ill effect since such logic is implicit in negation. This came up in an automatically generated rule; there may be a more concise way to express this logic but the least surprising behavior would be for the rule to work.

As far as I can tell we aren't making any attempt to support this logical pattern at present. Both negation and accumulation nodes take their input directly from the alpha network. Note that :exists is implemented as an accumulator and I believe this problem is general to accumulators inside negation conditions, and I can envision cases where writing such rules would make more sense than in this :not :exists case. Some of them might end up being handled by the complex nested negation logic we added in #149 ; I'd need to explore that further. An approach similar to that used in #149 or possibly just a broadening of the criteria used to go down the path added there could be used here but I haven't thought about it in more than a shallow way yet.

[rbrush]

Agreed that this specific example should work since the exception is surprising, so this is a bug.

In general, accumulators inside of negations don't make a ton of sense to me. If in a negation, the result gathered and computed by the accumulator would never be visible to the user...so if the user has a negation, you'd think it should just contain the facts the accumulator would run over, not an accumulator in itself. Also, some accumulators always return something like an empty list or set...in which case the negation would never apply and the rule would never fire.

I'm open to debate on this, but for a quick fix we could:

1.) Handling this specific case, since it's not obviously an accumulator to the user. (We could just elide the :exists if it's in a :not...since a rete :not is really :not-exists...)
2.) Throw a better, descriptive error if we do see an accumulator in a negation, including a shorter version of the above reasoning. (You probably don't really want to be accumulating anything there.)

That all said, we should always act in the least surprising way from the perspective of the user, so if there are ideas for more clear semantics here than we can consider them.

[WilliamParker]

@rbrush What about a condition like the following?

[:not [:and [?fact1 <- (acc/newest) :from [FactType1]]
                   [FactType2 (= join-id (:join-id ?fact1))]]]

It would be possible to factor the FactType1 condition out and add an initial value to the accumulator, but it would be counterintuitive to me to have to do that.

However, I think that our complex negation extraction here will end up handling that. Might be worth adding a test for though; I don't think any currently exists. It would also be confusing to me for a complex nested expression to work if a more simple one didn't behave as expected.

Another situation I could see arising is if there was complex logic in the accumulator. Perhaps something like

(def net-impact-accumulator {:reduce-fn some-summation-fn
                         :convert-return-fn (fn [value] (nonzero-predicate? value))})
[:not [(net-impact-accumulator) :from [FactThatPotentiallyCancelEachOtherOut]]

Something like that could be expressed in an accumulator condition outside a negation and then have a test node, but for such a use-case I think I'd expect the negation/accumulator approach to work.

[rbrush]

Fair points, Will. Given these I think the least surprising behavior might be to catch this flow in the extract-negations function as you suggested earlier. (We could handle the special case of [:not [:exists ]] there too to offer a more efficient rule graph.