arichiardi / replumb

ClojureScript plumbing for your self-hosted REPLs.
http://clojurescript.io
Eclipse Public License 1.0
210 stars 16 forks source link

Requiring macros with alias - solved by CLJS-1521 #108

Closed tomasz-biernacki closed 8 years ago

tomasz-biernacki commented 8 years ago

In ClojureScript 1.7.170 the following does not work:

cljs.user=> (ns my.namespace (:require [foo.bar.baz :refer-macros [mul-baz]]))
Could not parse ns form my.namespace - Referred macro foo.bar.baz/mul-baz does not exist

But this works:

cljs.user=> (ns my.namespace (:use-macros [foo.bar.baz :only [mul-baz]]))
nil
my.namespace=> (ns my.namespace (:require [foo.bar.baz :refer-macros [mul-baz]]))
nil

This is important also when testing: if the refer-macros test follows the use-macros it will pass, but clearly it should't.

This could be a bug in ClojureScript 1.7.170.

See also another example in the comment below

mfikes commented 8 years ago
(ns my.namespace (:require [foo.bar.baz :refer-macros [mul-baz]]))

desugars to

(ns my.namespace 
  (:require [foo.bar.baz])
  (:require-macros [foo.bar.baz] :refer [mul-baz])

Thus you need a foo.bar.baz non-macro namespace and also a foo.bar.baz macro namespace to be successfully loaded at the same time. Thus you need CLJS-1504.

tomasz-biernacki commented 8 years ago

@mfikes Yes, so let me add that there is a baz.clj file with namespacefoo.bar.baz which contains the macro and a non-macro file baz.cljs with same foo.bar.baz namespace.

arichiardi commented 8 years ago

From this, it seems it will be solved upstream but after 1.7.189.

tomasz-biernacki commented 8 years ago

Ok, the require-macros maybe wasn't a good example as it has been solved in 1.7.202, so consider this (which doesn't work in 1.7.202), output from planck:

cljs.user=> (ns my.namespace (:require-macros [foo.bar.baz :refer [mul-baz]]))
nil
my.namespace=> (mul-baz 10 20)
200

but

cljs.user=> (require 'foo.bar.baz)
nil
cljs.user=> (ns my.namespace (:require-macros [foo.bar.baz :as f]))
nil
my.namespace=> (f/mul-baz 10 20)
WARNING: No such namespace: f, could not locate f.cljs, f.cljc, or Closure namespace "" at line 1 
WARNING: Use of undeclared Var f/mul-baz at line 1 
Can't find variable: f

my.namespace=> (ns my.namespace (:require-macros [foo.bar.baz :refer [mul-baz]]))
Referred macro foo.bar.baz/mul-baz does not exist
     cljs$core$ExceptionInfo (cljs/core.cljs:9891:11)
     cljs$core$IFn$_invoke$arity$3 (cljs/core.cljs:9924:5)
     cljs$core$IFn$_invoke$arity$3 (cljs/analyzer.cljs:1034:55)
     cljs$core$IFn$_invoke$arity$2 (cljs/analyzer.cljs:1030:57)
     cljs$analyzer$check_use_macros (cljs/analyzer.cljs:3022:56)
     cljs$js$load_macros (cljs/js.cljs:701:96)
     cljs$core$IFn$_invoke$arity$5 (cljs/js.cljs:469:96)
     cljs$js$load_macros (cljs/js.cljs:690:53)
     cljs$js$load_macros (cljs/js.cljs:701:96)
     cljs$core$IFn$_invoke$arity$5 (cljs/js.cljs:469:96)
     cljs$js$load_macros (cljs/js.cljs:690:53)
     cljs$js$check_uses_and_load_macros (cljs/js.cljs:796:27)
     cljs$core$IFn$_invoke$arity$6 (cljs/js.cljs:868:34)
     cljs$js$eval_str_STAR__$_compile_loop (cljs/js.cljs:1623:61)
     cljs$js$eval_str_STAR_ (cljs/js.cljs:1710:6)
     cljs$core$IFn$_invoke$arity$5 (cljs/js.cljs:1813:30)
     planck$repl$process_execute_source (planck/repl.cljs:785:8)
     planck$repl$execute_source (planck/repl.cljs:827:12)
     planck$repl$execute (planck/repl.cljs:833:4)

it works without the first (require 'foo.bar.baz) though

Please note that is the (ns my.namespace (:require-macros [foo.bar.baz :refer [mul-baz]])) that is not working (second example), although is should (first example).

mfikes commented 8 years ago

Yeah, I think it is definitely an upstream bug, perhaps something along the lines of a change similar to this patch (this doesn't work, but it illustrates what I'm thinking).

From a41b14dc7478be63f3bf6f6b16dce15f761b288c Mon Sep 17 00:00:00 2001
From: Mike Fikes <mike@fikesfarm.com>
Date: Wed, 23 Dec 2015 18:53:45 -0500
Subject: [PATCH] Idea on resolve-macro-var

---
 src/main/clojure/cljs/analyzer.cljc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/main/clojure/cljs/analyzer.cljc b/src/main/clojure/cljs/analyzer.cljc
index 2a343fd..706976e 100644
--- a/src/main/clojure/cljs/analyzer.cljc
+++ b/src/main/clojure/cljs/analyzer.cljc
@@ -767,6 +767,8 @@
       (when (and ev (not (-> ev :dynamic)))
         (warning :dynamic env {:ev ev :name (:name ev)})))))

+(declare macro-ns-name)
+
 (defn resolve-macro-var
   "Given env, an analysis environment, and sym, a symbol, resolve a macro."
   [env sym]
@@ -777,7 +779,11 @@
       (let [ns (namespace sym)
             ns (if (= "clojure.core" ns) "cljs.core" ns)
             full-ns (resolve-macro-ns-alias env ns)]
-        (get-in namespaces [full-ns :macros (symbol (name sym))]))
+        #?(:clj
+           (get-in namespaces [full-ns :macros (symbol (name sym))])
+           :cljs
+           {:name (get-in namespaces [(macro-ns-name full-ns) :defs (symbol (name sym)) :name])
+            :ns   (macro-ns-name full-ns)}))

       (get-in namespaces [ns :use-macros sym])
       (let [full-ns (get-in namespaces [ns :use-macros sym])]
-- 
2.5.4 (Apple Git-61)
arichiardi commented 8 years ago

We found a bug! It was exactly what I hoped for when I started replumb! Good job @tomasz-biernacki!

mfikes commented 8 years ago

Fix for using aliases is in http://dev.clojure.org/jira/browse/CLJS-1521

arichiardi commented 8 years ago

In order to test easily against other versions of clojurescript I created the branches cljs-1.7.189 and cljs-1.7.212

arichiardi commented 8 years ago

Applying patch CLJS-1521 passes the previous test with alias:

cljs.user=> (require 'foo.bar.baz) <- with and without this, should not matter
nil
cljs.user=> (ns my.namespace (:require-macros [foo.bar.baz :as f]))
nil
my.namespace=> (f/mul-baz 10 20)
200

A question for @tomasz-biernacki. Is the "alias" issue a different one from the one you stated in the very first post? I changed the title so that this one (with alias) is solved-upstream.

tomasz-biernacki commented 8 years ago

@arichiardi Yes, it's different but probably related. In https://github.com/ScalaConsultants/replumb/issues/90 I state that the functionality (:require-macros ... :as ... )does not work. In this issue I show that not only it does not work, but also leave the compiler in a faulty state: you can see that in the example in my comment above.

Solving issue https://github.com/ScalaConsultants/replumb/issues/90 probably will solve this as well.

tomasz-biernacki commented 8 years ago

Confirmed: As in https://github.com/ScalaConsultants/replumb/issues/90, by applying CLJS-1521, it works:

cljs.user=> (require 'foo.bar.baz)
nil
cljs.user=> (ns my.namespace (:require-macros [foo.bar.baz :as f]))
nil
my.namespace=> (f/mul-baz 10 20)
200
my.namespace=>  (ns my.namespace (:require-macros [foo.bar.baz :refer [mul-baz]]))
nil
my.namespace=> (mul-baz 12 21)
252