bitemyapp / revise

RethinkDB client for Clojure
146 stars 8 forks source link

Name binding does not work properly with nested lambda's #14

Open danielytics opened 10 years ago

danielytics commented 10 years ago

When nesting lambda's, the name bindings of the outer lambda's get overwritten by the name bindings of the inner lambda's and cannot be accessed in the inner lambda's.

Nesting works as expected in the official javascript driver.


The test table looks like this:

{:id "40cd4261-aa72-4427-877b-da45abe4142b" :field [{:name "foo"}]}

The javascript query looks like this:

r.table('test').get('40cd4261-aa72-4427-877b-da45abe4142b')('field')
 .map(function(x){
  return {a: x('name'),
          b: r.expr(["x", "y", "z"]).map(function(y){
             return x
          })
       }
  })

And its output:

[
    {
        "a":  "foo" ,
        "b": [ 
            {
                "id":  "afc9cb64-af3d-4cae-8993-fc5d35ebd6ee" ,
                "name":  "foo" ,
            },
            {
                "id":  "afc9cb64-af3d-4cae-8993-fc5d35ebd6ee" ,
                "name":  "foo" ,
            },
            {
                "id":  "afc9cb64-af3d-4cae-8993-fc5d35ebd6ee" ,
                "name":  "foo" ,
            }
        ]
    }
]

However, when I try to run this in Revise, referncing x inside the nested lambda does not work.

The Clojure query:

(-> (r/table :test)
    (r/get "40cd4261-aa72-4427-877b-da45abe4142b")
    (r/get-field :field)
    (r/map
      (r/lambda [x]
        {:a (r/get-field x :name)
         :b (-> ["x" "y" "z"]
                (r/map
                  (r/lambda [y]
                    x))})))

And its output:

[{:a "foo"
  :b ["x" "y" "z"]}]

That is, the inner lambda seems to clobber the binding of the outer one.

Presumably this doesn't know about nested bindings and instead blindly replaces variables with the inner lambda's argument? I guess the comment is correct: not the best model of scope.. :(

danielytics commented 10 years ago

Ah, on closer inspection, I see that RethinkDB's variables are represented by unique integers, but that Revise simply assigns integers by incrementing from 1 for each variable found in the lambda argument list. So it always reuses the same variable numbers in each lambda. Looking at the spec for FUNC, it looks like they are local to each lambda, but the numbers used in the inner lambda override the numbers used for the outer lambda.


Here is the "quick fix" I'm using for now. I didn't put much effort into it, so should be considered a dirty hack and not particularly efficient. Also, ideally a better solution than keeping a global counter should be found:

(def global-id (atom 0))
(defn quick-fix-index-args
  [lambda-args]
  (let [ids (vec (clojure.core/map (fn [_] (swap! global-id inc)) lambda-args))]
  [ids
   (into {} (map (fn [k n]
                  [k
                   {:bitemyapp.revise.query/type :var
                    :bitemyapp.revise.query/number (r/parse-val (nth ids n))}])
                lambda-args
                (range)))]))

(defmacro lambda
  [arglist & body]
  (let [ret (last body)
        x (quick-fix-index-args arglist)
        ids (first x)
        arg-replacements (second x)]
    `(r/query :FUNC [~ids
                   ~(clojure.walk/postwalk-replace arg-replacements ret)])))
cesarbp commented 10 years ago

Yes, a better model of scope is needed. A better fix would involve not having a global atom as this library is inherently asynchronous.

danielytics commented 10 years ago

Yes - that's just a temporary hack to get my code working.

The first idea for a solution I had was to use a dynamic var that is bound when running the query:

(def ^:dynamic *global-id*)
(defn run-query [conn query]
    (binding [*global-id* (atom 0)]
        (run query conn)))

(run-query conn query)

This won't work because the query is evaluated on definition to construct the RethinkDB AST, not when run is called.

A possible solution would be to store the scope information in the tree (maybe something as simple as (str name "___" nesting, where name is the bound name and nesting is an integer that is incremented at each :FUNC), walk the tree in compile-term and replace the scope information in the :VAR's there. That is, it has global knowledge of all vars and their scopes in the context of a single query being run. It could simply generate a unique integer for each name.

cesarbp commented 10 years ago

compile-term handling the vars sounds good to me.