<< Back to previous view

[CLJS-1507] Implicit macro loading: macro var inference in :refer Created: 09/Dec/15  Updated: 24/Jun/16  Resolved: 24/Jun/16

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.7.145
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 2
Labels: None

Attachments: Text File CLJS-1507-partial.patch    


Background: When employing ns, if a namespace is required or used, and that namespace itself requires or uses macros from its own namespace, then the macros will be implicitly required or used using the same specifications.

This ticket asks for a mild extension to the above, automatically inferring when referred symbols are macro vars in two cases:

  1. :require / :refer
  2. :use / :only

Here is a concrete example illustrating the cases:

Assume src/foo/core.clj contains a single macro:

(ns foo.core)

(defmacro unless [pred a b]
  `(if (not ~pred) ~a ~b))

and src/foo/core.cljs requires that macro namespace:

(ns foo.core
  (:require-macros foo.core))

Without loss of generality, assume we do the following in a REPL.

$ java -cp cljs.jar:src clojure.main -m cljs.repl.node
ClojureScript Node.js REPL server listening on 49522
To quit, type: :cljs/quit
cljs.user=> (require '[foo.core :refer [unless]])
;; Currently get an error

Currently, the code assumes that unless is a non-macro var and a "Referred var foo.core/unless does not exist" diagnostic is emitted. To satisfy this ticket, the code should consult the foo.core macro namespace for the existence of an unless macro, and refer it as if

(require-macros '[foo.core :refer [unless]])
had been issued.

Likewise, this should work for the :use / :only dual form: When

(ns bar.core (:use [foo.core :only [unless]]))
is issued.

Additionally, the same behavior should be ensured to work in bootstrapped mode.

Comment by Thomas Heller [ 10/Dec/15 4:26 AM ]

Great to see you pick this up.

My patch for CLJS-948 included an implementation for this if you need a reference, although some things have changed since then and the patch won't apply cleanly. Still the basic idea is still the same.

Comment by Thomas Heller [ 10/Dec/15 4:33 AM ]

Again, just for reference:

Is the full implementation in use by shadow-build ever since the :refer part was rejected from CLJS-948, that is also where my patch initially came from.

Comment by Mike Fikes [ 11/Dec/15 8:33 PM ]

Attaching a partial patch in case it leads to interesting commentary.

This patch was written prior to my knowledge of Thomas' prior work. I don't have an opinion yet on the values of either approach: This one differs simply by accident of being written independently.

This patch works by transforming things in the sugared domain, thus taking something like

(:require [foo.core :refer [bar unless]])


(:require [foo.core :refer [bar] :refer-macros [unless]])

This patch is incomplete because it determines whether a symbol is a macro by attempting to find its macroexpander, and thus encounters the need for the foo.core namespace to have been analyzed, which is not guaranteed.

But, this patch will work if you artificially (say, in a REPL), load foo.core by itself first. Evidently Thomas' approach encountered a similar issue, but he resolved it (presumably by forcing the needed analysis to occur).

Comment by Thomas Heller [ 12/Dec/15 4:09 AM ]

Mike: I had two separate goals with my implementation. In shadow-build I have a discovery phase that inspects all files and parses the ns form in case of .cljs files. I wanted this phase to be completely free of side-effects in regards to the compiler state. I did not want to touch the analyzer env or load macros since I do not know whether the discovered namespace is going to be used later. I only extract the basic requires/provides to build the dependency graphs via :main (forced by shadow-build). Actual compilation always happens in full dependency order and macros are loaded when needed with all checks. The discovery skips some basic checks it cannot know about yet, ie. it checks for self-require but doesn't load macros yet.

The approach chosen by parse-ns in cljs.analyzer is to capture the compiler env and reset it after parsing the ns, your approach moves side-effects into the parsing again which may become a problem later since it cannot reset {{require}}ing a Clojure ns.

Whether or not this is a problem I cannot say, it just didn't agree with my goals. YMMV.

Comment by Mike Fikes [ 12/Dec/15 7:35 AM ]

Thanks @thheller, I'll see if I can come up with a patch that implements the inference in ns-side-effects.

Comment by Mike Fikes [ 30/Mar/16 11:18 PM ]

It's been a while since I've dedicated any time to this one. Unassigned from me in case anyone else wants to take it up.

Comment by David Nolen [ 24/Jun/16 6:03 PM ]

fixed https://github.com/clojure/clojurescript/commit/783001a6786f8dca4a13fdeadc995716903b07f9

[CLJS-1692] Autoalias clojure.* to exisiting cljs.* namespaces if possible Created: 24/Jun/16  Updated: 24/Jun/16  Resolved: 24/Jun/16

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.76
Fix Version/s: Next

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Comment by David Nolen [ 24/Jun/16 3:04 PM ]

fixed https://github.com/clojure/clojurescript/commit/23632baa35f86de8866dede624545bc0cdf4a2bb

Generated at Wed Jun 29 02:26:33 CDT 2016 using JIRA 4.4#649-r158309.