babashka / nbb

Scripting in Clojure on Node.js using SCI
Eclipse Public License 1.0
863 stars 52 forks source link

error when calling a macro referring to an ns qualified js symbol from another namespace #311

Closed ikappaki closed 1 year ago

ikappaki commented 1 year ago

version

nbb v1.2.167

platform

any

problem

error when calling a macro referring to an ns qualified js symbol from another namespace, e.g. Could not resolve symbol: fs/existsSync

  1. Create a nbb.edn file
    {:paths ["src"]}
  2. Create a src/macros.cljs flle with a macro calling to a namespace qualified js library fn, also for comparison purposes add the same for a cljs library namespace qualified symbol
    
    (ns macros
    (:require [cljs.test :as t]
            ["fs" :as fs]))

;; Can call a macro referring to a cljs library qualified symbol fine. ;; (defmacro t-test-is [] `(t/is (= 5 3))) (t-test-is) ;; => false (macroexpand-1 '(t-test-is)) ;; => (cljs.test/is (clojure.core/= 5 3))

;; Can call a macro referring to a js library qualified symbol fine. ;; (defmacro fs-exists-sync [] `(fs/existsSync ".")) (fs-exists-sync) ;; => true (macroexpand-1 '(fs-exists-sync)) ;; => (fs/existsSync ".")

3. Create a `src/issue.cljs` file that calls the macro referring to the namespace qualified js library symbol, and do the same for the namespace qualified cljs lib symbol for comparison purposes
``` clojure
(ns issue
  (:require [macros :as m]))

;; Can call macros referring to cljs libs  qualified symbol fine.
;;
(m/t-test-is)
;; => false
(macroexpand-1 '(m/t-test-is))
;; => (cljs.test/is (clojure.core/= 5 3))

;; (issue.case 1) but cannot call macros referring to js libs qualified symbol.
;; 
#_(m/fs-exists-sync)
;; => Could not resolve symbol: fs/existsSync
(macroexpand-1 '(m/fs-exists-sync))
;; => (fs/existsSync ".")
  1. Notice how the call to m/fs-exists-sync fails while the call tom/t-test-is succeeds.

expected behavior

I expect calling a macro referring to a namespace qualified js library symbol to succeed, as the namespace qualified cljs library symbol does.

Thanks

(This is a follow up to #309)

borkdude commented 1 year ago

I think this doesn't differ from how CLJS works. Can you give an example in proper CLJS that does work like you expect?

ikappaki commented 1 year ago

I think this doesn't differ from how CLJS works. Can you give an example in proper CLJS that does work like you expect?

You can't have macros in cljs calling either to cljs library fn with aliased namespace or js library fn with aliased namespaces, since to my understanding the macro compiler runs is Clojure mode and can't access any namespace information of the symbol under consideration.

For example, I don't think there i way that you could even make m/t-test-is above to work in cljs. This is a limitation of how the cljs compiler is implemented to use clojure for expanding macros.

nbb has the advantage that has first class support for macros, and I would have hoped it can be extended to properly support this case as it does for expanding t/is to cljs.test/is.

borkdude commented 1 year ago

I think this requires a change in SCI, perhaps it's possible... I'll have a look.

ikappaki commented 1 year ago

I think this requires a change in SCI, perhaps it's possible... I'll have a look.

Thanks

Here is an example how the ClojureScript compiler fails , unless the macro calls to a namespace qualified symbol, whose namespace qualifier is explicitly required in the current scope.

  1. deps.edn
    {:deps {org.clojure/clojurescript {:mvn/version "1.11.54"}}}
  2. src/macros.clj
    
    (ns macros)

(defmacro test-is [] '(is (= 6 3)))

(defmacro cljs-test-is [] '(cljs.test/is (= 6 3)))

(defmacro exists-sync [] '(existsSync "."))

(defmacro filesystem-exists-sync [] '(filesystem/existsSync "."))

3. `src/exp.cljs`
``` clojure
(ns exp
  (:require-macros [macros :as m])
  (:require ["fs" :as filesystem]
            [cljs.test]))

;; (cljs.case 1) cljs cannot expand an unqualified cljs library symbol
;; 
(println :me-test-is (macroexpand-1 '(m/test-is)))
;; => (is (= 6 3))
#_(println :testis (m/test-is))
;; => WARNING: Use of undeclared Var exp/is at line 4 src\exp.cljs

;; though it can if the cljs symbols namespace is fully
;; qualified (e.g. cljs.test/is) and that namespace was required here.
(println :me-cljs-test-is (macroexpand-1 '(m/cljs-test-is)))
;; => (cljs.test/is (= 6 3))
(println :me-cljs-test-is (m/cljs-test-is))
;; => false

;; (cljs.case 2) cljs cannot expand unqualified js library symbols.
(println :me-exists-sync (macroexpand-1 '(m/exists-sync)))
;; => (existsSync ".")
#_(println :exists-sync (m/exists-sync))
;; => WARNING: Use of undeclared Var exp/existsSync at line 10 src\exp.cljs

;; but can only expand qualified symbol if their namespace qualifier was required in scope.
(println :me-filesystem-exists-sync (macroexpand-1 '(m/filesystem-exists-sync)))
;; => (filesystem/existsSync ".")
(println :filesystem-exists-sync (m/filesystem-exists-sync))
;; => true
  1. compile and run
    
    clj -M --main cljs.main --target node --output-to main.js -c exp

node main.js

borkdude commented 1 year ago

Should work now in 1.2.168 (being built on CI)

ikappaki commented 1 year ago

Thanks, issue.case 1 now works.

There is one more edge case that fails at the moment when using$default, I'll open another issue, hopefully the last one.