DSL parsing derefs protocols instead of refering to the class of the protocol
Opened this issue · 4 comments
Per a discussion in the clara slack, there is an interesting behavior when a production tries to leverage a protocol as its fact-type.
For example,
(defprotocol MarkerProto)
(defrecord SomeFact []
MarkerProto)
(defrule some-rule
[?mp <- [MarkerProto]]
=>
(throw (ex-info "marker found" {:fact ?mp}))
(-> (mk-session)
(insert (->SomeFact))
fire-rules)
The execution of the session does not fail, this is because the way that the dsl resolves fact-types seen here. The protocol will resolve to a var, thus the parser will deref it into its map representation. Meaning that the ancestors call on the type of (->SomeFact) will not match the rule.
So, there is an open question here:
Should clara resolve the symbol and if its a protocol reference the class instead?
Unfortunately I don't think the question of how to behave here is entirely straightforward. I suspect that users who use a protocol as a type may be expecting the conditions to match on facts that extend the protocol, not just facts that implement the Java interface directly. So if this proposal is implemented, the first case below would match a MarkerProtocol condition but the second would not.
(defrecord FactOne []
MarkerProtocol)
(defrecord FactTwo [])
(extend MarkerProtocol
FactTwo)
At first glance the resolution could involve using the extenders function to find all types implementing the protocol, but this is potentially problematic if the types implementing the protocol were to change after the session was created though. However, this wouldn't be an entirely new problem since the default :ancestors-fn is seems likely to be susceptible to such issues when it works against Clojure's type hierarchy created via derive rather than the JVM's. I don't know how frequently that part of Clara's type dispatch is currently used in practice though. For any change like this we'd have to be careful of performance impacts, but since the get-alphas-fn that performs this dispatch is cached I think it would be OK from that point of view. I'd need to spend a bit more time reasoning through how this would be implemented but it seems like we could take a "snapshot" of the extenders at the time the session was created.
Another question: If we do want behaviour like this should it be opt-in or by default? Having a protocol map as an actual type in the rules seems like strange enough behaviour that I'd be tempted to either throw or log a warning if it occurs without the user having set their own :fact-type-fn and maybe :ancestors-fn, and then the users could choose what to do.
Unfortunately I don't think the question of how to behave here is entirely straightforward. I suspect that users who use a protocol as a type may be expecting the conditions to match on facts that extend the protocol, not just facts that implement the Java interface directly.
Overall, I consider using a defprotocol to be the "type" expressed to match our default fact type fn to be somewhat of an anti-pattern. I think anytime in Clojure that a defprotocol is used in a way that tries to utilize the host class/interface being created, is a misuse of defprotocol. The host interface on the JVM that defprotocol omits is primarily only an optimization, implementation detail.
The definterface macro instead, is what should be used when creating these things to be specifically a new class type to use.
That said, I think defprotocol has been used enough for the underlying interface it omits. It doesn't seem too crazy to me for Clara to just assume to use the interface instance instead of the protocol metadata map when resolving the symbol - if it resolves to a var instead of the JVM interface.
I'm not sure about caching protocol implementors etc. I know extenders types of ops are quite expensive. Perhaps it could be cached at startup, but then it would not be dynamic for a session and that may also be weird. Meaning, a later extend wouldn't be respected by the pre-created session. Even so, I think it's a stretch to support this sort of logic in the default :fact-type-fn and :ancestors-fn.
Quick comment from mobile:
"but then it would not be dynamic for a session and that may also be weird. Meaning, a later extend wouldn't be respected by the pre-created session".
Changes in the fact type dispatch during the scope of a rules session could have arbitrary behavior analogous to that when facts are mutated after being inserted into the session. For example, suppose a fact doesn't match a condition, the rules are fired, and then the type hierarchy changes so that it does match it. So I don't think trying to support that kind of dynamism is likely a good path.
@WilliamParker yeah that’s a fair point on the dynamism part. I guess that’s already sort of a possible restriction.