Errors when session, rules, and fact types are defined in different namespaces and not explicitly referred by name
sparkofreason opened this issue · 3 comments
I've augmented the tests to illustrate the issue. Suppose I have the following two namespace definitions:
(ns clara.rule-defs
(:require-macros [cljs.test :refer (is deftest run-tests testing)]
[clara.test-rules-data])
(:require [cljs.test :as t]
[clara.rules.engine :as eng]
[clara.rules.accumulators :as acc]
[clara.rules :refer [insert retract fire-rules query insert!]
:refer-macros [defrule defsession defquery]]
[clara.rules.testfacts :as tf]))
(def simple-defrule-side-effect (atom nil))
(def other-defrule-side-effect (atom nil))
(defrule test-rule
[tf/Temperature (< temperature 20)]
=>
(reset! other-defrule-side-effect ?__token__)
(reset! simple-defrule-side-effect ?__token__))
(defquery cold-query
[]
[tf/Temperature (< temperature 20) (== ?t temperature)])
;; Accumulator for getting the lowest temperature.
(def lowest-temp (acc/min :temperature))
(defquery coldest-query
[]
[?t <- lowest-temp :from [tf/Temperature]])
(defrule is-cold-and-windy
"Rule to determine whether it is indeed cold and windy."
(tf/Temperature (< temperature 20) (== ?t temperature))
(tf/WindSpeed (> windspeed 30) (== ?w windspeed))
=>
(insert! (tf/->ColdAndWindy ?t ?w)))
(defquery find-cold-and-windy
[]
[?fact <- tf/ColdAndWindy])(ns clara.test-rules-require
(:require-macros [cljs.test :refer (is deftest run-tests testing)]
[clara.test-rules-data])
(:require [cljs.test :as t]
[clara.rules.engine :as eng]
[clara.rules.accumulators :as acc]
[clara.rules :refer [insert retract fire-rules query insert!]
:refer-macros [defrule defsession defquery]]
[clara.rule-defs :as rd]
[clara.rules.testfacts :as facts]))
(comment
;; Launch browser repl.
(cemerick.piggieback/cljs-repl :repl-env (cemerick.austin/exec-env))
)
(defn- has-fact? [token fact]
(some #{fact} (map first (:matches token))))
(defsession my-session 'clara.rule-defs)
(defsession my-session-map 'clara.rule-defs :fact-type-fn :type)
(deftest test-simple-defrule
(let [session (insert my-session (facts/->Temperature 10 "MCI"))]
(fire-rules session)
(is (has-fact? @rd/simple-defrule-side-effect (facts/->Temperature 10 "MCI")))
(is (has-fact? @rd/other-defrule-side-effect (facts/->Temperature 10 "MCI")))))
(deftest test-simple-query
(let [session (-> my-session
(insert (facts/->Temperature 15 "MCI"))
(insert (facts/->Temperature 10 "MCI"))
(insert (facts/->Temperature 80 "MCI"))
fire-rules)]
;; The query should identify all items that wer einserted and matchd the
;; expected criteria.
(is (= #{{:?t 15} {:?t 10}}
(set (query session rd/cold-query))))))
(deftest test-simple-accumulator
(let [session (-> my-session
(insert (facts/->Temperature 15 "MCI"))
(insert (facts/->Temperature 10 "MCI"))
(insert (facts/->Temperature 80 "MCI"))
fire-rules)]
;; Accumulator returns the lowest value.
(is (= #{{:?t 10}}
(set (query session rd/coldest-query))))))
(deftest test-simple-insert
(let [session (-> my-session
(insert (facts/->Temperature 15 "MCI"))
(insert (facts/->WindSpeed 45 "MCI"))
(fire-rules))]
(is (= #{{:?fact (facts/->ColdAndWindy 15 45)}}
(set
(query session rd/find-cold-and-windy))))))When executed with lein test, the compiler gives these warnings:
WARNING: Use of undeclared Var clara.test-rules-require/other-defrule-side-effect at line 17 src\test\clojurescript\clara\test_rules_require.cljs
WARNING: Use of undeclared Var clara.test-rules-require/simple-defrule-side-effect at line 18 src\test\clojurescript\clara\test_rules_require.cljs
WARNING: No such namespace: tf, could not locate tf.cljs, tf.cljc, or Closure namespace "" at line 37 src\test\clojurescript\clara\test_rules_require.cljs
WARNING: Use of undeclared Var tf/->ColdAndWindy at line 37 src\test\clojurescript\clara\test_rules_require.cljs
WARNING: Use of undeclared Var clara.test-rules-require/other-defrule-side-effect at line 17 src\test\clojurescript\clara\test_rules_require.cljs
WARNING: Use of undeclared Var clara.test-rules-require/simple-defrule-side-effect at line 18 src\test\clojurescript\clara\test_rules_require.cljs
WARNING: No such namespace: tf, could not locate tf.cljs, tf.cljc, or Closure namespace "" at line 37 src\test\clojurescript\clara\test_rules_require.cljs
WARNING: Use of undeclared Var tf/->ColdAndWindy at line 37 src\test\clojurescript\clara\test_rules_require.cljs
Along with several runtime errors such as
ERROR in (test-simple-accumulator) (:)
Uncaught exception, not in assertion.
expected: nil
actual: #error {:message "Exception in clara.rule-defs/test-rule with bindings {}", :data {:bindings {}, :name "clara.rule-defs/test-rule", :rhs (do (reset! other-defrule-side-effect ?token) (reset! simple-defrule-side-effect ?token)), :batched-logical-insertions [], :batched-unconditional-insertions [], :batched-rhs-retractions [], :listeners []}, :cause #object[Error Error: No protocol method IReset.-reset! defined for type undefined: ]}
ERROR in (test-simple-insert) (:)
Uncaught exception, not in assertion.
expected: nil
actual: #error {:message "Exception in clara.rule-defs/is-cold-and-windy with bindings {:?t 15, :?w 45}", :data {:bindings {:?t 15, :?w 45}, :name "clara.rule-defs/is-cold-and-windy", :rhs (do (insert! (tf/->ColdAndWindy ?t ?w))), :batched-logical-insertions [], :batched-unconditional-insertions [], :batched-rhs-retractions [], :listeners []}, :cause #object[ReferenceError ReferenceError: Can't find variable: tf]}
This issue is possibly related to #327.
I think the offending line here is in clara.rules.dsl/maybe-qualify
(if (com/compiling-cljs?)
;; Qualify the symbol using the CLJS analyzer.
(if-let [resolved (and (symbol? sym)
(not (env sym))
(com/resolve-cljs-sym (com/cljs-ns) sym))]
resolved
sym)
;; Then the Clojure implementation in the other branch of the top-level if
This makes no attempt to use the ClojureScript namespace of the rule in question, but rather just uses the current ClojureScript namespace. The call to clara.rules.dsl/resolve-vars occurs during the expansion of the defsession macro, specifically here, so it uses the namespace of the session being created to qualify the RHS. This is incorrect and I'd expect it to lead to the errors above.
In some form, the code snippet above needs to change in order to respect the ClojureScript namespace of the rule. This could be done via either binding the cljs namespace as suggested in #327 (comment) or by explicitly passing the namespace to use into resolve-vars (thus changing its signature). The only remaining usage of resolve-vars after #178 was fixed looks to be here in CLJS session compilation, in clara.rules.dsl/parse-rule*, and in clara.rules.dsl/parse-query*. My initial preference would be to favor explicit argument passing here over implicit dynamic variable bindings, but if others have different preferences I don't see it as a big issue either way. The existing usage would be pretty to easy to change.
Note that the namespace of the rule is currently set to the Clojure namespace, not the ClojureScript namespace. See here. This will need to change as well, probably to something like
(if (compiling-cljs?)
(cljs-ns)
*ns*
@sparkofreason did you want to work on a PR for this? I can get to it sometime if not, though perhaps not until after the December holidays.
I already have a PR pretty much ready with the binding approach. I also added the last bit you indicated.
Closed with merging of the PR.