ClojureScript

Simplify macro usage

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

The usage of macros from CLJS is very explicit and users of any given namespace must remember whether it comes with macros and which vars are macros. This leads to rather verbose :require statements which are confusing and lead to unexpected results if incomplete (eg. missing :refer-macros).

(ns my-ns
  (:require [my-app.other-ns :as other :refer (something) :refer-macros (something) :include-macros true]))

(something) ;; macro (when :refer-macros, otherwise no macro)
(other/thing) ;; maybe macro (only when :include-macros, otherwise no macro)

I think the user should not need to know whether something is a macro or not, they should just be able to use it.

I implemented a proof-of-concept that

(ns my-ns
  (:require [my-app.other-ns :refer (something)])

will work just find and do the correct thing with regards to macros (assuming the namespace has a corresponding clj namespace, :require-macros is unaffected). No changes to any code are necessary as the implementation uses ana/passes, it is also fully backwards compatible.

Implementation points which should be discussed

  • The CLJS namespace with macros currently has to opt-in through a bit of metadata (eg. (ns my-ns-with-macros {:load-macros true})), that might not be necessary and maybe should default to true.
  • The implementation assumes compilation happens in dependency order. shadow-build always does this, I'm not too sure about CLJS though. Given ana/analyze-deps equals true that is guaranteed but I'm not sure that is always the case.

If there is any interest I can provide a patch, until then refer to the proof-of-concept [1].

[1] https://github.com/thheller/shadow-build/blob/f37cfa598f1e90dd66e333d1e45580ea25650025/src/clj/shadow/cljs/passes.clj#L30-L82

  1. cljs-948.diff
    07/Jan/15 8:37 AM
    7 kB
    Thomas Heller
  2. cljs-948-no-passes.diff
    08/Jan/15 9:42 AM
    5 kB
    Thomas Heller

Activity

Hide
David Nolen added a comment -

Thomas the previous environment gets restored, https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1840.

Because parse-ns is not recursive by default there are no problems.

Like I said sorry I wasn't clear about what I wanted to see implemented. The :refer bit is just nice sugar and not fundamental. Please open a separate ticket.

Show
David Nolen added a comment - Thomas the previous environment gets restored, https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1840. Because parse-ns is not recursive by default there are no problems. Like I said sorry I wasn't clear about what I wanted to see implemented. The :refer bit is just nice sugar and not fundamental. Please open a separate ticket.
Hide
Thomas Heller added a comment -

parse-ns calls analyze [1] which calls (defmethod parse 'ns ...) which does a swap! [2] and therefore touches the compiler environment?

[1] https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1818
[2] https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1357

When opening the issue my goal was to make everything as close to Clojure as possible which included the transparent handling of :refer for macros. All my patches included that functionality so I'm slightly confused that this was not "under consideration". Especially since my example basically consisted of exactly that use-case. We can take this to another issue if we must ...

Show
Thomas Heller added a comment - parse-ns calls analyze [1] which calls (defmethod parse 'ns ...) which does a swap! [2] and therefore touches the compiler environment? [1] https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1818 [2] https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1357 When opening the issue my goal was to make everything as close to Clojure as possible which included the transparent handling of :refer for macros. All my patches included that functionality so I'm slightly confused that this was not "under consideration". Especially since my example basically consisted of exactly that use-case. We can take this to another issue if we must ...
Hide
David Nolen added a comment - - edited

parse-ns does not change the compiler environment by default nor is it recursive by default. All that it is used for in this context is to extract namespace information, nothing more or less.

Expecting :refer to work like that for macros was never under consideration for this issue. I should have been more clear about that. We can open a new minor enhancement issue for this addition.

Show
David Nolen added a comment - - edited parse-ns does not change the compiler environment by default nor is it recursive by default. All that it is used for in this context is to extract namespace information, nothing more or less. Expecting :refer to work like that for macros was never under consideration for this issue. I should have been more clear about that. We can open a new minor enhancement issue for this addition.
Hide
Thomas Heller added a comment -

I take that back, I no longer like the solution since you added the call to parse-ns, which basically supersedes the *analyze-deps* option.

Since you cannot guarantee that files are processed in dependency order the extra parse-ns call will create an entry in the compiler env for the namespace, analyze-deps will then skip the analyze-file since parse-ns already created the entry (which is incomplete since it stopped at the ns)

In addition the sequence of calls is wrong and check-uses does not work correctly since it does not check for macros.

(ns macro-use.bug
  (:require [macro-use.core :as core :refer (a-macro)]))

Since check-uses now throws this will result in

clojure.lang.ExceptionInfo: Referred var macro-use.core/a-macro does not exist at line 1 src/macro_use/bug.cljs {:tag :cljs/analysis-error, :file "src/macro_use/bug.cljs", :line 1, :column 1}

Demo repo here:
https://github.com/thheller/macro-use-bug/blob/master/src/macro_use/bug.cljs

Show
Thomas Heller added a comment - I take that back, I no longer like the solution since you added the call to parse-ns, which basically supersedes the *analyze-deps* option. Since you cannot guarantee that files are processed in dependency order the extra parse-ns call will create an entry in the compiler env for the namespace, analyze-deps will then skip the analyze-file since parse-ns already created the entry (which is incomplete since it stopped at the ns) In addition the sequence of calls is wrong and check-uses does not work correctly since it does not check for macros.
(ns macro-use.bug
  (:require [macro-use.core :as core :refer (a-macro)]))
Since check-uses now throws this will result in
clojure.lang.ExceptionInfo: Referred var macro-use.core/a-macro does not exist at line 1 src/macro_use/bug.cljs {:tag :cljs/analysis-error, :file "src/macro_use/bug.cljs", :line 1, :column 1}
Demo repo here: https://github.com/thheller/macro-use-bug/blob/master/src/macro_use/bug.cljs
Hide
Thomas Heller added a comment -

Nice work, your solution looks much better.

Show
Thomas Heller added a comment - Nice work, your solution looks much better.
Hide
Thomas Heller added a comment -

As per IRC the *load-macros* issues does not present in CLJS since it is true when it needs to be.

Show
Thomas Heller added a comment - As per IRC the *load-macros* issues does not present in CLJS since it is true when it needs to be.
Hide
Thomas Heller added a comment -

The *load-macros* issue manifests as follows:

(ns demo.ns1
  (:require-macros [demo.ns1 :as m]))

(ns demo.ns2
  (:require [demo.ns1 :as x :refer (a-macro)]))

Compile 1: Everything is fine, cache is written.
Compile 2:

  • ns1 not modified, parse-ns decides to use cached version
  • ns2 modified, compiler attempts to compile, fails with NullPointerException
Caused by: java.lang.NullPointerException
	at cljs.analyzer$get_expander.invoke(analyzer.clj:1532)
	at cljs.analyzer$macroexpand_1.invoke(analyzer.clj:1543)
	... 80 more

This is because the load from cache does not require the macro namespace. Before these changes demo.ns2 would have had a :require-macros or equivalent in the ns form which would then have require'd the macro ns. Now only demo.ns1 would trigger the require, but since it loads from cache it doesn't UNLESS *load-macros* is true. Since parse-ns does revive the cache (I think) it is not.

The require must be triggered somewhere before actual compilation happens. In shadow-build this is pretty simple but I'm not sure where to put this for CLJS.

Show
Thomas Heller added a comment - The *load-macros* issue manifests as follows:
(ns demo.ns1
  (:require-macros [demo.ns1 :as m]))

(ns demo.ns2
  (:require [demo.ns1 :as x :refer (a-macro)]))
Compile 1: Everything is fine, cache is written. Compile 2:
  • ns1 not modified, parse-ns decides to use cached version
  • ns2 modified, compiler attempts to compile, fails with NullPointerException
Caused by: java.lang.NullPointerException
	at cljs.analyzer$get_expander.invoke(analyzer.clj:1532)
	at cljs.analyzer$macroexpand_1.invoke(analyzer.clj:1543)
	... 80 more
This is because the load from cache does not require the macro namespace. Before these changes demo.ns2 would have had a :require-macros or equivalent in the ns form which would then have require'd the macro ns. Now only demo.ns1 would trigger the require, but since it loads from cache it doesn't UNLESS *load-macros* is true. Since parse-ns does revive the cache (I think) it is not. The require must be triggered somewhere before actual compilation happens. In shadow-build this is pretty simple but I'm not sure where to put this for CLJS.
Hide
Thomas Heller added a comment -

Attached the no-passes version. The does get a bit simpler since it doesn't have to manually patch the compiler env.

I'm not totally sure how the @reload logic works, maybe some adjustments are required.

Show
Thomas Heller added a comment - Attached the no-passes version. The does get a bit simpler since it doesn't have to manually patch the compiler env. I'm not totally sure how the @reload logic works, maybe some adjustments are required.
Hide
David Nolen added a comment - - edited

The difference for inter-types is that is something that needs to be run for a large number of nodes - it's a real global pass.

Let's go with moving the logic in to parse 'ns. I still don't understand the *load-macros* issue you are describing. We absolutely need to be able to disable this and dependency analysis for legitimate uses of parse-ns.

Show
David Nolen added a comment - - edited The difference for inter-types is that is something that needs to be run for a large number of nodes - it's a real global pass. Let's go with moving the logic in to parse 'ns. I still don't understand the *load-macros* issue you are describing. We absolutely need to be able to disable this and dependency analysis for legitimate uses of parse-ns.
Hide
Thomas Heller added a comment -

Hmm, you are right. Will move it out of the passes.

The only reason it was in there in the first place is due to shadow-build. It was the only place I could put it to make it work without patching CLJS. But since we agree that this should be in core the passes option really doesn't make sense any more.

Going to deliver a new patch soon.

Show
Thomas Heller added a comment - Hmm, you are right. Will move it out of the passes. The only reason it was in there in the first place is due to shadow-build. It was the only place I could put it to make it work without patching CLJS. But since we agree that this should be in core the passes option really doesn't make sense any more. Going to deliver a new patch soon.
Hide
Thomas Heller added a comment -

Well, yeah we could to away with the passes and do it all in (defmethod parse 'ns ...) but that doesn't change the issue as long as there is a way to toggle this at all (ie. :load-macros or load-macros). It is not really about whether it is in a pass or not.

I just liked using passes since it sort of allows cherry-picking what you want and don't want to run. infer-types doesn't need to run in parse-ns for example, but it always did before this.

But I'm not committed to passes, happy to move everything so it runs in the parse method.

Let me know, if I should rewrite the patch.

Show
Thomas Heller added a comment - Well, yeah we could to away with the passes and do it all in (defmethod parse 'ns ...) but that doesn't change the issue as long as there is a way to toggle this at all (ie. :load-macros or load-macros). It is not really about whether it is in a pass or not. I just liked using passes since it sort of allows cherry-picking what you want and don't want to run. infer-types doesn't need to run in parse-ns for example, but it always did before this. But I'm not committed to passes, happy to move everything so it runs in the parse method. Let me know, if I should rewrite the patch.
Hide
David Nolen added a comment - - edited

Thomas to me this illustrates the pitfalls of trying to bring a multipass approach to an existing compiler that isn't already fundamentally multipass. It seems to me that an alternative approach is possible without bothering at all about passes. In the analyzer parse 'ns case for each required CLJS lib you could leverage parse-ns to see if it requires a macro file of the same name - if it does add an implicit :require-macros.

While I like the passes approach in theory it will just make it harder for contributors to understand the structure of the compiler, they have to look at two different approaches to determine where things happen.

Show
David Nolen added a comment - - edited Thomas to me this illustrates the pitfalls of trying to bring a multipass approach to an existing compiler that isn't already fundamentally multipass. It seems to me that an alternative approach is possible without bothering at all about passes. In the analyzer parse 'ns case for each required CLJS lib you could leverage parse-ns to see if it requires a macro file of the same name - if it does add an implicit :require-macros. While I like the passes approach in theory it will just make it harder for contributors to understand the structure of the compiler, they have to look at two different approaches to determine where things happen.
Hide
Thomas Heller added a comment -

One issue I found is related to caching. Depends on how/where cljs.analyzer/parse-ns is called.

If the new passes aren't executed and the result of that analysis are used to write out the cache it won't contain the new macros information. I believe cljs.analyzer/parse-ns does this.

On the next run if it was decided to use the cache, we are missing some information AND won't require the macros. This was true before the changes but since the dependant namespaces would require the macros on their own the issue was somewhat hidden. Now they assume that macros were already loaded, therefore the must require macros when reconstructing from cache.

How to do that best in CLJS I don't know since shadow-build does its own caching. The passes could be refactored so they can be called with the cached information as well.

Note that this issue was basically introduced by load-macros (CLJS-940), but since that flag is already gone it is now moved to the :load-macros option of parse-ns.

The simplest way to address this would be to always run the new passes, basically how it was before CLJS-940.

Show
Thomas Heller added a comment - One issue I found is related to caching. Depends on how/where cljs.analyzer/parse-ns is called. If the new passes aren't executed and the result of that analysis are used to write out the cache it won't contain the new macros information. I believe cljs.analyzer/parse-ns does this. On the next run if it was decided to use the cache, we are missing some information AND won't require the macros. This was true before the changes but since the dependant namespaces would require the macros on their own the issue was somewhat hidden. Now they assume that macros were already loaded, therefore the must require macros when reconstructing from cache. How to do that best in CLJS I don't know since shadow-build does its own caching. The passes could be refactored so they can be called with the cached information as well. Note that this issue was basically introduced by load-macros (CLJS-940), but since that flag is already gone it is now moved to the :load-macros option of parse-ns. The simplest way to address this would be to always run the new passes, basically how it was before CLJS-940.
Hide
Thomas Heller added a comment -

Sorry, my mistake. Fixed.

Show
Thomas Heller added a comment - Sorry, my mistake. Fixed.
Hide
David Nolen added a comment -

The patch has not been updated with these changes.

Show
David Nolen added a comment - The patch has not been updated with these changes.
Hide
Thomas Heller added a comment -

That is why you should probably not write code tired at 3am. Was using infer-tag as a pass instead of infer-type.

I decided to drop the cljs.analyzer.passes namespace and instead moved the passes to cljs.analyzer. The extra namespace introduced a weird circular dependency which cljs.analyzer/parse-ns couldn't quite handle.

The patch now applies cleanly, would probably be nice to have a test that actually tests that it does what its supposed to.

Show
Thomas Heller added a comment - That is why you should probably not write code tired at 3am. Was using infer-tag as a pass instead of infer-type. I decided to drop the cljs.analyzer.passes namespace and instead moved the passes to cljs.analyzer. The extra namespace introduced a weird circular dependency which cljs.analyzer/parse-ns couldn't quite handle. The patch now applies cleanly, would probably be nice to have a test that actually tests that it does what its supposed to.
Hide
Thomas Heller added a comment -

My head hurts, I need to get some sleep.

I cannot figure out why the attached patch does not work, but ./script/test produces errors in parse-ns.

Will get back to it tommorrow.

Exception in thread "main" java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Symbol, compiling:(/Users/zilence/code/oss/clojurescript/bin/../bin/cljsc.clj:18:68)
	at clojure.lang.Compiler.load(Compiler.java:7142)
	at clojure.lang.Compiler.loadFile(Compiler.java:7086)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$script_opt.invoke(main.clj:336)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at clojure.lang.Var.invoke(Var.java:388)
	at clojure.lang.AFn.applyToHelper(AFn.java:160)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Symbol
	at clojure.lang.RT.seqFrom(RT.java:505)
	at clojure.lang.RT.seq(RT.java:486)
	at clojure.core$seq.invoke(core.clj:133)
	at cljs.analyzer$parse_ns$fn__1660.invoke(analyzer.clj:1757)
	at cljs.analyzer$parse_ns.invoke(analyzer.clj:1747)
	at cljs.analyzer$parse_ns.invoke(analyzer.clj:1741)
	at cljs.compiler$compile_root.invoke(compiler.clj:1069)
	at cljs.closure$compile_dir.invoke(closure.clj:358)
	at cljs.closure$eval3084$fn__3085.invoke(closure.clj:398)
	at cljs.closure$eval3020$fn__3021$G__3011__3028.invoke(closure.clj:306)
	at cljs.closure$eval3071$fn__3072.invoke(closure.clj:412)
	at cljs.closure$eval3020$fn__3021$G__3011__3028.invoke(closure.clj:306)
	at cljs.closure$build.invoke(closure.clj:1018)
	at cljs.closure$build.invoke(closure.clj:972)
	at user$eval3279.invoke(cljsc.clj:21)
	at clojure.lang.Compiler.eval(Compiler.java:6703)
	at clojure.lang.Compiler.load(Compiler.java:7130)
	... 9 more
Show
Thomas Heller added a comment - My head hurts, I need to get some sleep. I cannot figure out why the attached patch does not work, but ./script/test produces errors in parse-ns. Will get back to it tommorrow.
Exception in thread "main" java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Symbol, compiling:(/Users/zilence/code/oss/clojurescript/bin/../bin/cljsc.clj:18:68)
	at clojure.lang.Compiler.load(Compiler.java:7142)
	at clojure.lang.Compiler.loadFile(Compiler.java:7086)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$script_opt.invoke(main.clj:336)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at clojure.lang.Var.invoke(Var.java:388)
	at clojure.lang.AFn.applyToHelper(AFn.java:160)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Symbol
	at clojure.lang.RT.seqFrom(RT.java:505)
	at clojure.lang.RT.seq(RT.java:486)
	at clojure.core$seq.invoke(core.clj:133)
	at cljs.analyzer$parse_ns$fn__1660.invoke(analyzer.clj:1757)
	at cljs.analyzer$parse_ns.invoke(analyzer.clj:1747)
	at cljs.analyzer$parse_ns.invoke(analyzer.clj:1741)
	at cljs.compiler$compile_root.invoke(compiler.clj:1069)
	at cljs.closure$compile_dir.invoke(closure.clj:358)
	at cljs.closure$eval3084$fn__3085.invoke(closure.clj:398)
	at cljs.closure$eval3020$fn__3021$G__3011__3028.invoke(closure.clj:306)
	at cljs.closure$eval3071$fn__3072.invoke(closure.clj:412)
	at cljs.closure$eval3020$fn__3021$G__3011__3028.invoke(closure.clj:306)
	at cljs.closure$build.invoke(closure.clj:1018)
	at cljs.closure$build.invoke(closure.clj:972)
	at user$eval3279.invoke(cljsc.clj:21)
	at clojure.lang.Compiler.eval(Compiler.java:6703)
	at clojure.lang.Compiler.load(Compiler.java:7130)
	... 9 more
Hide
Thomas Heller added a comment -

Alright, resolved that macro issue.

I updated [1] so the analyzer passes are initialized correctly and outside cljs.analyzer. The patch would also change the cljs.analyzer/parse 'def since that functionality is moved to cljs.analyzer.passes.

(when (and *analyze-deps* (seq @deps))
      (analyze-deps name @deps env opts))
    #_ (when (and *analyze-deps* (seq uses))
         (check-uses uses env))
    (set! *cljs-ns* name)
    #_ (when *load-macros*
         (load-core)
         (doseq [nsym (concat (vals require-macros) (vals use-macros))]
           (clojure.core/require nsym))
         (when (seq use-macros)
           (check-use-macros use-macros env)))

The question is where do we require cljs.analyzer.passes (must be required somewhere so the default is configured correctly)?

cljs.closure is probably the best bet.

Not a big fan of this alter-var-root but I'm not sure how we could do without. I doubt anyone but me currently uses cljs.analyzer/passes and therefore the default behavior of (or passes [infer-type]) always triggers. Since that would no longer work correctly we need to initialize to another default.

cljs.analyzer/parse-ns would also need to change again given the removal of load-macros. Same functionality can be achieved within the same binding though.

[1] https://github.com/thheller/shadow-build/blob/master/src/clj/shadow/cljs/passes.clj

Show
Thomas Heller added a comment - Alright, resolved that macro issue. I updated [1] so the analyzer passes are initialized correctly and outside cljs.analyzer. The patch would also change the cljs.analyzer/parse 'def since that functionality is moved to cljs.analyzer.passes.
(when (and *analyze-deps* (seq @deps))
      (analyze-deps name @deps env opts))
    #_ (when (and *analyze-deps* (seq uses))
         (check-uses uses env))
    (set! *cljs-ns* name)
    #_ (when *load-macros*
         (load-core)
         (doseq [nsym (concat (vals require-macros) (vals use-macros))]
           (clojure.core/require nsym))
         (when (seq use-macros)
           (check-use-macros use-macros env)))
The question is where do we require cljs.analyzer.passes (must be required somewhere so the default is configured correctly)? cljs.closure is probably the best bet. Not a big fan of this alter-var-root but I'm not sure how we could do without. I doubt anyone but me currently uses cljs.analyzer/passes and therefore the default behavior of (or passes [infer-type]) always triggers. Since that would no longer work correctly we need to initialize to another default. cljs.analyzer/parse-ns would also need to change again given the removal of load-macros. Same functionality can be achieved within the same binding though. [1] https://github.com/thheller/shadow-build/blob/master/src/clj/shadow/cljs/passes.clj
Hide
David Nolen added a comment -

The inlining macros are an exception, it's done that way to break circularity which would otherwise stack overflow the compiler.

Show
David Nolen added a comment - The inlining macros are an exception, it's done that way to break circularity which would otherwise stack overflow the compiler.
Hide
Thomas Heller added a comment -

Yeah, but cljs.core calls all macros directly.

(defn ^boolean identical?
  "Tests if 2 arguments are the same object"
  [x y]
  (cljs.core/identical? x y))

The recently changed defonce vars are the exception here. Although even if I change them to cljs.core/defonce I still can't explain the error.

Show
Thomas Heller added a comment - Yeah, but cljs.core calls all macros directly.
(defn ^boolean identical?
  "Tests if 2 arguments are the same object"
  [x y]
  (cljs.core/identical? x y))
The recently changed defonce vars are the exception here. Although even if I change them to cljs.core/defonce I still can't explain the error.
Hide
David Nolen added a comment -

yes cljs.core and core macros are an exception

Show
David Nolen added a comment - yes cljs.core and core macros are an exception
Hide
Thomas Heller added a comment -

Wait, are other namespaces allowed to use their own macros unqualified?

(ns my-ns
  (:require-macros [my-ns]))

(this-is-a-macro)
;; vs
(my-ns/this-is-a-macro)

I don't think so? The recent defonce seems to be an exception?

Show
Thomas Heller added a comment - Wait, are other namespaces allowed to use their own macros unqualified?
(ns my-ns
  (:require-macros [my-ns]))

(this-is-a-macro)
;; vs
(my-ns/this-is-a-macro)
I don't think so? The recent defonce seems to be an exception?
Hide
Thomas Heller added a comment -

Should stop refering to specific hashes, please see [1].

There is a slight issue with cljs.core itself. There is a bit of code that makes cljs.core macros special. I'm not quite sure why this is, since it would not be required if it did.

(ns cljs.core
  (:require-macros [cljs.core]))

Getting an error [2] on the recently changed def to (defonce print-fn). Haven't exactly figured out why this is yet.

[1] https://github.com/thheller/shadow-build/blob/master/src/clj/shadow/cljs/passes.clj
[2] https://www.refheap.com/61bc05e4b4a74a3b573797721

Show
Thomas Heller added a comment - Should stop refering to specific hashes, please see [1]. There is a slight issue with cljs.core itself. There is a bit of code that makes cljs.core macros special. I'm not quite sure why this is, since it would not be required if it did.
(ns cljs.core
  (:require-macros [cljs.core]))
Getting an error [2] on the recently changed def to (defonce print-fn). Haven't exactly figured out why this is yet. [1] https://github.com/thheller/shadow-build/blob/master/src/clj/shadow/cljs/passes.clj [2] https://www.refheap.com/61bc05e4b4a74a3b573797721
Hide
David Nolen added a comment -

This looks OK to me at the moment.

Show
David Nolen added a comment - This looks OK to me at the moment.
Hide
Thomas Heller added a comment -

I cleaned up the reference in shadow-build [1]. This is how a cljs.analyer.passes could look, the default passes would then be

[cljs.analyzer.passes/load-macros
 cljs.analyzer.passes/infer-macro-require
 cljs.analyzer.passes/infer-macro-use
 cljs.analyzer.passes/check-uses
 cljs.analyzer/infer-type]

This would basically make ana/load-macros obsolete since this can now be controlled via passes. Also addresses CLJS-955.

[1] https://github.com/thheller/shadow-build/blob/220d3bb0bef2cdb5696487b639ca5aaa169c56f2/src/clj/shadow/cljs/passes.clj

Show
Thomas Heller added a comment - I cleaned up the reference in shadow-build [1]. This is how a cljs.analyer.passes could look, the default passes would then be
[cljs.analyzer.passes/load-macros
 cljs.analyzer.passes/infer-macro-require
 cljs.analyzer.passes/infer-macro-use
 cljs.analyzer.passes/check-uses
 cljs.analyzer/infer-type]
This would basically make ana/load-macros obsolete since this can now be controlled via passes. Also addresses CLJS-955. [1] https://github.com/thheller/shadow-build/blob/220d3bb0bef2cdb5696487b639ca5aaa169c56f2/src/clj/shadow/cljs/passes.clj
Hide
Thomas Heller added a comment -

Yay! Coming right up.

One issue though: I thought it would be a good idea to put the passes into a separate file (eg. cljs.analyzer.passes), that is currently not possible due to cljs.analyzer/analyze (it controls the passes defaults) [1]. cljs.analyzer cannot depend on cljs.analyzer.passes since it most likely depends on cljs.analyzer (circular dependency). Technically the passes currently only require the analyzer because of ::ana/namespaces but faking out a circular dependency is not a good idea.

If we remove the (or passes [infer-type]) and instead force opts to contain :passes (or bind passes beforehand) we should be fine.

Although I'm perfectly fine with putting the new passes into cljs.analyzer, just thought it would be cleaner not to.

Also, how should I address the invalid refer warning issue? Since the check-uses runs before the pass it can never find the macro. I was going to make check-uses a pass in itself if thats alright with you.

[1] https://github.com/clojure/clojurescript/blob/2d1e2167f5ab8bd76ac5c8bafd198990cc88d34a/src/clj/cljs/analyzer.clj#L1711

Show
Thomas Heller added a comment - Yay! Coming right up. One issue though: I thought it would be a good idea to put the passes into a separate file (eg. cljs.analyzer.passes), that is currently not possible due to cljs.analyzer/analyze (it controls the passes defaults) [1]. cljs.analyzer cannot depend on cljs.analyzer.passes since it most likely depends on cljs.analyzer (circular dependency). Technically the passes currently only require the analyzer because of ::ana/namespaces but faking out a circular dependency is not a good idea. If we remove the (or passes [infer-type]) and instead force opts to contain :passes (or bind passes beforehand) we should be fine. Although I'm perfectly fine with putting the new passes into cljs.analyzer, just thought it would be cleaner not to. Also, how should I address the invalid refer warning issue? Since the check-uses runs before the pass it can never find the macro. I was going to make check-uses a pass in itself if thats alright with you. [1] https://github.com/clojure/clojurescript/blob/2d1e2167f5ab8bd76ac5c8bafd198990cc88d34a/src/clj/cljs/analyzer.clj#L1711
Hide
David Nolen added a comment -

OK, let's try a patch for this. This is an interesting simplification that unifies Clojure & ClojureScript macro usage.

Show
David Nolen added a comment - OK, let's try a patch for this. This is an interesting simplification that unifies Clojure & ClojureScript macro usage.
Hide
Thomas Heller added a comment -

Yeah, probably. Also played with the idea of just looking for (io/resource (str (ns->path) ".clj")). If that is nil the namespace doesn't have macros, if its not null we require it.

But I like your approach of just using (ns my-ns (:require-macros [my-ns])) more.

Show
Thomas Heller added a comment - Yeah, probably. Also played with the idea of just looking for (io/resource (str (ns->path) ".clj")). If that is nil the namespace doesn't have macros, if its not null we require it. But I like your approach of just using (ns my-ns (:require-macros [my-ns])) more.
Hide
David Nolen added a comment -

I looked a bit at your code. It looks interesting. Definitely not going to do the namespace metadata bit. It seems to me this should work only if the required namespace loaded macros from a Clojure namespace of the same name.

Show
David Nolen added a comment - I looked a bit at your code. It looks interesting. Definitely not going to do the namespace metadata bit. It seems to me this should work only if the required namespace loaded macros from a Clojure namespace of the same name.
Hide
Thomas Heller added a comment -

Is it ok if I leave implementation notes here?

Things that would need addressing should this be implemented:

  • cljs.analyzer/check-uses runs when parsing the ns form, since the macro passes run AFTER the parsing finished the check-uses will warn incorrectly if a :refer is not defined in CLJS but is a macro. Solution: check-uses should probably just become a pass itself which runs after the macro ones. Moving a side-effect out of the parse function is probably well worth it.
  • cljs.analyzer/load-macros would obsolete and the entire functionality it was toggling can be moved to the load-macros pass.
Show
Thomas Heller added a comment - Is it ok if I leave implementation notes here? Things that would need addressing should this be implemented:
  • cljs.analyzer/check-uses runs when parsing the ns form, since the macro passes run AFTER the parsing finished the check-uses will warn incorrectly if a :refer is not defined in CLJS but is a macro. Solution: check-uses should probably just become a pass itself which runs after the macro ones. Moving a side-effect out of the parse function is probably well worth it.
  • cljs.analyzer/load-macros would obsolete and the entire functionality it was toggling can be moved to the load-macros pass.
Hide
David Nolen added a comment -

This is interesting. Will think about it.

Show
David Nolen added a comment - This is interesting. Will think about it.

People

Vote (1)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: