Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ikappaki opened this issue Feb 24, 2023 · 6 comments

Comments

@ikappaki
Copy link
Contributor

ikappaki commented Feb 24, 2023

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"]}
  1. 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 ".")
  1. 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
(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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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

@ikappaki
Copy link
Contributor Author

ikappaki commented Feb 24, 2023

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"}}}
  1. 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 "."))
  1. src/exp.cljs
(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
Copy link
Collaborator

Should work now in 1.2.168 (being built on CI)

@ikappaki
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants