noprompt / meander

Tools for transparent data transformation
MIT License
921 stars 55 forks source link

Unquoting fails inside guardrails in ClojureScript #165

Closed psagers closed 3 years ago

psagers commented 3 years ago

The following fails to compile to the intended JavaScript:

(ns user
  (:require [meander.epsilon :as m]
            [com.fulcrologic.guardrails.core :refer [>defn =>]])

(>defn match-my-map
  [m x]
  [map? any? => vector?]
  (m/match m
    {:x ~x :y ?y}
    [:okay ?y]

    _
    [:fail]))

For some reason, Guardrails turns ~x into (unquote x) rather than (clojure.core/unquote x). Interestingly, even putting the fully qualified (clojure.core/unquote x) directly into the function definition doesn't resolve the issue. I'm not sure exactly what's going on in Guardrails, although the simple fix in Meander is to have meander.syntax.epsilon/parse-seq accept unqualified unquoting symbols (as with the unqualified quote symbol). I'm not sure if these were omitted deliberately for safety or simply because they appeared unnecessary in practice.

jimmyhmiller commented 3 years ago

@psagers I'm not able to recreate this. I tried both in clojure and in clojurescript. Can you run the following and tell me what you get?

clj -Sdeps '{:deps {meander/epsilon {:mvn/version "0.0.588"} com.fulcrologic/guardrails {:mvn/version "1.1.2"} org.clojure/core.async {:mvn/version "1.3.610"} org.clojure/clojurescript {:mvn/version "1.10.758"}}}' \
-M   \
--main cljs.main \
--repl-env node \
-e " \
(require '[meander.epsilon :as m])                            \
(require '[com.fulcrologic.guardrails.core :refer [>defn]])   \
(>defn match-my-map                                           \
  [m x]                                                       \
  [map? any? => vector?]                                      \
  (m/match m                                                  \
    {:x ~x :y ?y}                                             \
    [:okay ?y]                                                \
                                                              \
    _                                                         \
    [:fail]))                                                 \
                                                              \
    (println                                                  \
       (match-my-map {:x 1 :y 2} 1)                            \
       (match-my-map {:x 1 :y 2} 3))                          "

I get

[:okay 2] [:fail]

which is the expected output. Do you get something different?

psagers commented 3 years ago

Guardrails actually needs to be activated explicitly. I've set up a repro case at https://github.com/psagers/meander-165.

Thanks

jimmyhmiller commented 3 years ago

Thanks I will look into it a bit later and see if I can understand what is going on.

To be honest, this sounds like a guard rails bug, not a meander one. Maybe I will find out differently. But we wouldn't want to change meander to just work on unqualified unquote. It is very intentional to have this only work on clojure.core/unquote. Otherwise we could be giving up on hygiene.

psagers commented 3 years ago

That's fair, and it certainly wasn't obvious where to mention this first.

That said, guardrails (essentially a fork of ghostwheel) is essentially just wrapping a function in some instrumentation and passing the body through as-is. Perhaps there's some subtlety that they've overlooked. And if that's the case, it would suggest that there are likely other defn-replacement macros out there—public and private—with the same issue, which would make meander's use of unquote something of a ticking time bomb, regardless of its theoretical tidiness.

Perhaps I'm a little out of my depth in macro-fu, but I'm frankly a bit uneasy with the assumptions being made about quoting in general here. An API that took a map of fixed values for selected logic variables—more like datalog—would feel more straightforward and reliable. Even if you decide to support naked unquote symbols, after this I'm on the fence about playing it safe and, with no small regret, just relying on specter.

Thanks

jimmyhmiller commented 3 years ago

So I can at least confirm this is guardrails causing this issue. This unquote stuff happens even if meander is not in the mix. Also, that guard rails, when enabled, is not passing the body through as is. When it is disabled, it is in fact passing the body through as is, which is why this works without an issue then.

This also only appears to be in issue in clojurescript with guardrails. I haven't been able to dive into the exact root cause yet but I plan on doing so.

I totally understand being wary of macro based solutions. We all have to make the technology choices that make sense to us. We use macros here because otherwise we would not get the performance we want. But we did just release an experimental interpreter that we will continue to improve.

As for switching to specter, we don't consider anything in the clojure ecosystem competition and are happy when people use what works best for them. It might be that guardrails in meander don't play together well. Hopefully I can help make that not the case soon. :)

psagers commented 3 years ago

I guess I didn't check, but I just assumed that guardrails was exhibiting this quoting behavior no matter what. It would be interesting to know more about that, although it may be academic.

As far as meander goes, the question I would be most interested in is whether the documented use of the ~ reader form actually has guaranteed behavior. Not having done all the research, I wouldn't be able to rule out the possibility that using ~ outside of a syntax quote is off-label and its behavior is officially undefined, at least as far as the qualified/unqualified question goes. Maybe that's not the case and this is just a bug in the end; I don't envy you facing the other possibility.

I've been using both specter and meander and I have often found them delightfully complementary. And hopefully will again in the future.

jimmyhmiller commented 3 years ago

@psagers Luckily this has nothing to do with the reader macros.

Here is the macro definition for guardrails >defn.

https://github.com/fulcrologic/guardrails/blob/9b166925103a6c615479db06681b4bf3ece1dc27/src/main/com/fulcrologic/guardrails/core.cljc#L710-L731

You can see on line 720 that if it is cljs it calls clj->cljs. That can be found here.

https://github.com/fulcrologic/guardrails/blob/9b166925103a6c615479db06681b4bf3ece1dc27/src/main/com/fulcrologic/guardrails/utils.cljc#L27-L45

This code intentionally strips off clojure.core from symbols it finds. So this problem happens regardless of use of the reader macro or not.

I am honestly not sure why guardrails is doing this, git blame didn't help me. I have never seen this pattern before and it is not one I would advise. This can lead to all sorts of incorrect programs. Anything that needs to refer to clojure core things is now broken. Here is a really simple example.

(>defn greet-person 
  [name]
  [string? => string?]
  (str "Hello " 
       (if (keyword? name)
         (clojure.core/name name)
         name)))

(greet-person :jimmy)

;; =>

"Hello "

If you look at the macroexpanded version you can see why.

(fn [name] 
  (str "Hello " 
       (if (keyword? name) 
         (name name) 
         name)))

Now :jimmy is being passed itself, which returns nil and that is being added to "Hello ".

Hopefully this is helpful in understanding what is going on and why sadly there is nothing we can do in meander to get around this unfortunate behavior in guardrails.

psagers commented 3 years ago

Fascinating. I was definitely on the wrong scent there. Sorry to drag you into this.

If this was inherited from ghostwheel, that might explain why its origin is obscure. Now it seems like the real mystery is how something so fundamental could have gone undetected so long.

Thanks