let-bound names may be inappropriately fully qualified

[stephenbrady]

Just wanted to report this issue.

This compiles fine:

(ns anamespace)

(defrule a-rule
  [AType]
  =>
  (let [foo 1]
    (inc foo)))

This will fail at clara.rules.compiler/try-eval

(ns anamespace)

(def foo nil)

(defrule a-rule
  [AType]
  =>
  (let [foo 1]
    (inc foo)))

I've traced this to foo being incorrectly expanded to anamespace/foo because the heuristic during parsing in clara.rules.dsl/maybe-qualify checks for whether it's a symbol and then down here attempts to resolve it to determine whether to qualify the name. In the above example, foo will resolve and therefore get namespace qualified, yielding:

(let [anamespace/foo 1]
  (inc anamespace/foo))

The let causes an exception to be thrown during try-eval. Perhaps more dangerous, the body of the let resolves to the global var foo and silently goes on its way, though the Clojure compiler does throw on the let binding first.

I didn't see a straight-forward, robust fix, so I'm just posting this issue in case this isn't known (though I see related issues to name resolution and the environment have come up before).

[mrrodriguez]

I've thought about this one quite a bit before and I'm really not convinced there is a simple solution due to the inability to detect locals shadowing vars in the RHS (right hand side) of a rule. The RHS allows for mostly unconstrained and arbitrary Clojure forms. This means that it is not something that can be simply parsed to understand the AST semantics of things like local let-bindings that shadow vars of the namespace.

let-bindings can come from any arbitrary number of forms since there can be any macro that expands to let's. Common examples are doseq, for, if-let, etc. it can even be non-core macros of course too.

The only solution I've explored that I do think works, but is not light weight, is to take advantage of a library that is aligned with Clojure's compilers analyzer, because Clojure's itself is fairly closed off for use by others. The tools.analyzer project is capable of this and I have used it to successful qualify vars in rule RHS while still respecting local shadowing semantics. Although this is quite a lot to take on as a dependency and piece of the Clara DSL parser/compiler.

I do think that you won't typically have any silent failures from this issue though. Clara DSL parser will qualify all of the vars, which means it will qualify any symbols used in locally bindings as well if they are shadowing a var of the ns. The Clojure compiler will in turn fail due to trying to local bind a qualified symbol. It'd be difficult (impossible?)to set up a RHS that didn't fail compilations like that. It is an unfortunately confusing error message to get though. Clara can't do much to detect it either as it stands, short of try to catch the exception to give better feedback.

Clara qualifies all symbols that need it during parsing so that they can be compiler later in potentially different ns contexts. One alternative on the RHS could be to avoid qualifying vars and instead just track the originating ns to rebind later when eval'ing the form. I've had some success with an approach like this before as well.

Anyways perhaps someone else has further insight into this.

[rbrush]

Thanks for reporting this. I like @mrrodriguez's idea of using the originating namespace when eval-ing the right-hand side...which basically just lets Clojure qualify the symbols itself. I'm traveling and am not able to jump on this immediately but will take a closer look next week.

[mrrodriguez]

@rbrush I can work on this in the next few days.

I think this would be useful for a few cases we had as well.

[mrrodriguez]

I was wrong about this being impossible to ever be a problem too.
With http://dev.clojure.org/jira/browse/CLJ-1318 "Support destructuring maps with namespaced keywords" of Clojure 1.6, things can go wrong with qualified symbols and shadowing. This whole feature allows strange things, but still.

(def x :bad)

(clara.rules/defrule trial
  [:test]
  =>
  (let [{:keys [x]} :good]
    (println x)))

(-> (clara.rules/mk-session [trial])
    (clara.rules/insert ^{:type :test} {})
    (clara.rules/fire-rules))

;; :bad

trial
;;= {:lhs [{:constraints []}], :rhs (do (let [{:keys [user/x]} :good] (println user/x))), :name "user/trial"}

Something like this actually snuck up on me recently. :keys destructuring doesn't stop you from having qualified local bindings. So the clj compiler didn't report an error, but the body of the RHS was incorrect.

I'm just posted here in response to my own comment above #178 (comment) where I thought it was impossible to get a real problem int he RHS that passed the clj compiler without exception. I was wrong.
:)

I'll still plan to work on that fix for this.

[mrrodriguez]

Just as a heads up. I have done some work on this. I've been a little slow at having enough time for it, but should be done soon. My biggest hurdle has just been dealing with the ClojureScript-side tests. I should be able to get that sorted out soon.

[rbrush]

I think @mrrodriguez fixed this in #179. I might go ahead and close this in prep for a 0.11.1 fix release with this included.

Mike's change didn't include similar issues on the rule left-hand side, but we can track that separately. A different approach is needed there since the same LHS may be use across rules and namespaces, and it's hard to imagine using let bindings there, anyway.

[rbrush]

Closing per my previous comment, targeted for 0.11.1. We can open new issues if anything else pops up. Thanks to Stephen for reporting this, and Mike for fixing it!