Missing ns-name data for queries in CLJS

Question

Missing ns-name data for queries in CLJS

alex-dixon opened this issue 7 years ago · 6 comments

Happened upon this discrepancy working on reloading rules in CLJS. :ns-name appears to be registered in the compiler env for rules but not queries.

Rule:

remove-entity___impl 
{:ns-name (quote precept.impl.rules), 
 :lhs (quote [{:type :remove-entity, :constraints [(= ?v (:v this))]} 
                     {:accumulator (precept.accumulators/all), 
                      :from {:type :all, :constraints [(= ?v (:e this))]}, 
                      :result-binding :?entity}]), 
 :rhs (quote (do (doseq [tuple ?entity] (precept.util/retract! tuple)))), 
 :props {:group :action}, 
 :name precept.app-ns/remove-entity___impl}

Query:

everything 
{:lhs (quote [{:accumulator (precept.accumulators/all), 
 :from {:type :all, :constraints []}, :result-binding :?facts}]), 
 :params #{}, 
 :name precept.app-ns/everything}

Relevant spots in the code appear to be:
https://github.com/cerner/clara-rules/blob/master/src/main/clojure/clara/rules/dsl.clj#L243
https://github.com/cerner/clara-rules/blob/master/src/main/clojure/clara/rules/dsl.clj#L272

Related, I also noticed the namespace that rules are keyed by appear to be different from the namespace in which they are declared. The following rule lives in precept.impl.rules but is indexed under the namespace that the session is compiled in (precept.app-ns):

{precept.app-ns 
 {clean-transients___impl {:ns-name (quote precept.impl.rules), 
                                            :lhs (quote [{:type :all, 
                                                                 :constraints [(= :transient (:e this))], 
                                                                 :fact-binding :?fact}]), 
                                            :rhs (quote (do (clara.rules/retract! ?fact))),

Discrepancy may be due to the use of (com/cljs-ns) via add-production here:
https://github.com/cerner/clara-rules/blob/master/src/main/clojure/clara/macros.clj#L51

...as opposed to a different method here:
https://github.com/cerner/clara-rules/blob/master/src/main/clojure/clara/rules/dsl.clj#L243

Answer 1 · 2017-07-19T19:14:46.000Z

The :ns-name was added to rules to address #178 which concerned incorrect qualifying of symbols on the RHS of rules. Since queries don't have a RHS they weren't impacted. Similar issues could occur in the LHS as well, but in practice they are less likely to run into this issue. It is still possible for the problem to happen there though, it just hasn't been a high enough priority to fix relative to other demands on our time.

I take it the map in your code blocks that has the discrepancy between namespace names is the ::productions map to which entries are added in add-productions? If so this result is rather curious and would suggest that the cljs-ns function may not be behaving correctly. This would warrant further investigation. I actually find it strange that the :ns-name for ClojureScript rules is being set by *ns* rather than the cljs-ns, since *ns* is part of the Clojure environment of the ClojureScript compiler rather than the ClojureScript code. Perhaps this is working out somehow though or I could be missing something. For example, when I discovered #300 I found that Clara Clojure namespaces could be compiled and available in the ClojureScript compiler environment, although relying on that sort of behavior seems unsafe.

Answer 2 · 2017-07-19T20:26:02.000Z

@alex-dixon , is this having an impact on the work you referenced to reload rules dynamically or did you just happen to notice it in the course of that work? If so, what kind of impact?

Answer 3 · 2017-07-19T21:05:51.000Z

@WilliamParker It'd really help to have it consistent. Right now everything's working because in all our examples we have our rules and our sessions defined in the same namespace, and I am choosing to use the namespace key from ::productions, which corresponds to the namespace of the session (not necessarily the namespace in which the rule was defined). That's because if I come across a query, I can't tell what namespace it lives in. I could use the session's namespace only when I don't have a namespace for a production, but I thought that would be harder to debug/track in a worse case scenario.

Couple places you might be able to see the impact are here, where I'm explicitly skipping a check on a query, and also using the rule-ns from the ::productions map for all the [ns rule-name] pairs I'm making.

If the map for queries were like rules (which do contain a :ns-name corresponding to the namespace in which they live), that'd be great and I'm more than happy to help implement according to whatever specification you'd like.

Thanks for your continued attention on these issues. :)

Answer 4 · 2017-07-24T17:57:58.000Z

@alex-dixon I agree that the behavior you describe is undesirable. So as I understand this you're seeing this behavior in a REPL environment? I wonder if that cause is something to do with the REPL (presumably Figwheel) setting the ClojureScript namespace to something other than what is being loaded. The adding of productions doesn't seem like it should have anything to do with the namespaces of any sessions using the productions in and of itself, but if you happen to be in the REPL in the namespace where the session is defined perhaps that could make a difference?

It actually seems to me like we may have a bug because of the use of *ns* i.e. the current namespace in the Clojure environment rather than the cljs-ns in the DSL rule parsing here. I'm doubtful that these will always align, although perhaps something is happening to make them do so.

If you could supply an exact list of steps, REPL or otherwise, that lead to the discrepancy that would be helpful.

Answer 5 · 2017-11-27T19:09:33.000Z

I ran into what may be a related issue. In CLJS, when the session was defined in a different namespace than the rules, symbols in the rule defs (e.g. fact types) would be qualified with the sesison namespace, not the rule namespace. The call to resolve-vars at https://github.com/cerner/clara-rules/blob/master/src/main/clojure/clara/macros.clj#L183 only binds the Clojure namespace. At this point in compilation, the CLJS namespace is not correct. Adding this to the binding fixes the issue: cljs.analyzer/*cljs-ns* (:ns-name production)

Answer 6 · 2017-11-27T20:40:17.000Z

@sparkofreason logged #359 (thanks for that by the way) for the issues discussed in his previous comment #327 (comment).

Regarding the behavior of the map of rules stored in the ClojureScript compiler environment not matching i.e.

(ns rules-ns)
(defrule rule1 ..)

(ns session-ns)
(defsession session1 'rules-ns)

and having a map of rules like

{session-ns [rule1-data-map]}

it isn't apparent to me how that would happen, since productions are added to the cache as part of the expansion of defrule and defquery apart from session compilation.

Some things that come to mind (that I realize we may not have answers for yet):

Does this always happen when rules are in another namespace than a session consuming them or only in certain circumstances?
Do we have a minimal reproducing case or is this only reproducible with other code included so far? For example Precept.
What happens if the rules namespace is consumed by sessions in multiple different namespaces? Are the rules duplicated across namespaces, is there "last one wins" behavior, etc.