Duplicate unifications within the same condition cause rebinding

[EthanEChristian]

(defrecord A [a])
(r/defrule r-1 
  [?b <- A (= ?a a) (= ?a 1)]
  =>
  (println ?b))
(-> (r/mk-session) (r/insert (->A 1)) (r/insert (->A 2)) (r/fire-rules))

Based on the semantics of unifications with resect to Joins, ie. first usage is unification and subsequent are equivalence, i would expect this to print:

#user.A{:a 1}

However, due to how the alpha node is compiled the actual result is:

#user.A{:a 1}
#user.A{:a 2}

because the constraints get compiled to:

(clojure.core/fn
              [?__fact__ ?__env__]
              (clojure.core/let
               [this ?__fact__ a (.-a ?__fact__) ?__bindings__ (clojure.core/atom {:?b ?__fact__})]
               (clojure.core/let
                [?a a]
                (clojure.core/swap! ?__bindings__ clojure.core/assoc :?a ?a)
                (clojure.core/let
                 [?a 1]
                 (clojure.core/swap! ?__bindings__ clojure.core/assoc :?a ?a)
                 (clojure.core/deref ?__bindings__)))))

While the recommended usage would be (= ?a a 1), i feel like the scenario above should either follow the unification semantics of Join scenarios, or it should be an exception.

@WilliamParker / @mrrodriguez / @rbrush /community,
how do you think this scenario should work?

[WilliamParker]

Good find!

Looking at the condition

[?b <- A (= ?a a) (= ?a 1)]

I'd take that to mean:

• Create the ?a binding if it is not present or inherit it from the ancestors if it is.
  Then find set of all A such that
• ?a is equal to the field a.
• ?a is equal to 1.

Basically I see a condition as an expression of a subset of a set of facts, and the matches for the production(rule/query) as the Cartesian join of those subsets. Accumulators are of course a little different in producing a smaller number of values than the number of facts, but the logic underlying a join is still the same.

In this instance, there is one fact that satisfies the condition, namely (->A 1), so I'd expect the rule to fire once. If this was handwritten the user could use more arguments to = as you say, but I don't see why we should require this. If nothing else we should continue supporting Clara as a compilation target, so even patterns that look slightly odd to a human eye but have a clear meaning should work IMO.

It might be user-friendlier to throw exceptions or log a warning in cases where rules are written that can't ever fire and we can detect it, but even this would probably need to be optional behaviour in case of rules generated by a compiler where there might not be a clear mapping between a user DSL to single rules.

This also makes me curious what would happen if ?b were used in the constraints as well, I'll plan to look at that later.

[mrrodriguez]

I'd think

[?b <- A (= ?a a) (= ?a 1)]

should work with the least surprising behavior. There was already support added a while ago to bind computed values to variable bindings (ie. not just unifying on a direct field value) to be used in unification throughout the conditions. In a natural sequential flow, the least surprising behavior would be for unification to "happen" once past (= ?a a), so then (= ?a 1) should be able utilize that binding - ie, it is "in scope".

The current compiler behavior is certainly surprising in that it causes the (= ?a 1) form to override the value of ?a, which results in a variable binding that apparently has more than 1 value, which breaks the concept of unification.

Hopefully fixing the compiler to achieve this wouldn't be too involved. From the looks of it, I don't believe it is.

[EthanEChristian]

Closing this as #424 is now merged

[eraserhd]

I ran into this in a slightly different form:

(defrule no-dependency-cycles
  {:salience -10000}
  [TransitiveTaskdefDependency (= from ?a) (= to ?a)]
  =>
  (error! ?__token__
          "The taskdefs have a dependency cycle"
          ?a :taskdef/depends-on))

The above rule would bind to any instance of TransitiveTaskdefDependency. #424 did fix this problem. (Leaving this here for future travellers.)