<< Back to previous view

[CLJS-632] Stop using Clojure Namespaces entirely for non-macro names/lookups Created: 22/Oct/13  Updated: 24/Oct/13  Resolved: 24/Oct/13

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

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File CLJS-632.diff    
Patch: Code and Test

 Description   

AFAICT, the ClojureScript compiler uses clojure.lang.Namespace in only a couple of situations now:

1. during macroexpansion
2. when reading:

  • to bind *ns* to *cljs-ns*
  • to set aliases (and their referents) for resolving namespaced keywords/symbols

#1 is unavoidable and unproblematic. #2(b) can cause issues at dev-time though, especially when using portable libraries. For example:

(ns simple-check.generators
  (:require #+clj [clojure.core :as lang]
            #+cljs [cljs.core :as lang]
            [cemerick.pprng :as pprng]
            clojure.string)
  (:refer-clojure :exclude [int vector list hash-map map keyword
                            char boolean byte bytes sequence]))

The differing lang alias there will always cause an error for the second compiler that attempts to load the file.

A similar problem would also affect any multitenant ClojureScript compilation environment that didn't use classloaders to provide the compiler with its own static clojure.lang.Namespace environment.

A plan:

1. Enhance tools.reader to provide a dynamically rebindable *namespaces* var (hopefully to contain a map equivalent in structure to the one being used by the analyzer already?).
2. Change the analyzer to:

  • bind that var to the current value of namespaces when reading
  • modify the namespaces map as necessary to account for aliases, etc.

Does this sound sane?



 Comments   
Comment by David Nolen [ 22/Oct/13 9:26 AM ]

This sounds interesting, should probably get Nicola Mometto to weigh in on this.

Comment by Chas Emerick [ 22/Oct/13 9:37 AM ]

Yup, already linked him here on Twitter; will mail him later this week if necessary.

Comment by Chas Emerick [ 22/Oct/13 9:40 AM ]

Added an inline example, since I'm working around the problem in the originally-referenced project/branch (https://github.com/cemerick/simple-check/blob/cljx/src/cljx/simple_check/generators.cljx).

Comment by Nicola Mometto [ 22/Oct/13 10:54 AM ]

In its early days, tools.reader (formerly "blind") had a alias-map dynamic var that if bound, would be used to retrieve aliases instead of (ns-aliases ns).

I removed that feature when moving to contrib as I changed my mind about it and thought it was a useless feature – looks like I was wrong.

Would bringing back such feature be a good-enough fix for this, or do you have a better approach in mind?

(relevant commit https://github.com/clojure/tools.reader/commit/e74817c765c2bc7924d703fea5c7ac73357e5080#diff-56d12a080274e797b5d0948a73249)

Comment by David Nolen [ 22/Oct/13 10:59 AM ]

Wow, cool. I would love to see tools.reader support this, what we currently have is a dirty hack.

Comment by Chas Emerick [ 22/Oct/13 11:02 AM ]

Yeah, that looks just about right! @Nicola, if you push a snapshot restoring that functionality, I'll take a crack at modifying the analyzer appropriately.

Comment by Nicola Mometto [ 22/Oct/13 11:15 AM ]

I pushed the change and deployed a 0.7.10-SNAPSHOT to Sonatype, let me know if you encounter any issue

Comment by Chas Emerick [ 22/Oct/13 11:56 PM ]

Attached is a proposed patch. It requires the latest snapshot of tools.reader, the latest here.

All tests pass here, and REPL interaction works as expected.

I have some remaining concerns:

  • The :requires-macros slot in each namespaces sub-map in the analyzer was previously being constructed separately from all other requires/uses data stored in that map. I've commented out that expression, on line 809; the patch uses the reduction applied to all other requires/uses data, yet all tests pass. Perhaps that separate expression was simply vestigial, but I wanted to call it out.
  • Related, the "misspelling" of :requires-macros is worrying. I've retained the existing keyword, but perhaps it would be smart to fix that?
  • The REPL was still using the Clojure reader; I've changed that so that namespaced keywords and symbols used on the REPL work properly. That's less a concern than an FYI; this is a bit of unavoidable scope creep.
  • forms-seq has always been used to read more than just files (e.g. URLs provided by the REPL), yet the input argument is provided directly as the "file-name" for the reader's use. I haven't yet seen where ClojureScript uses this (perhaps it's only referred in certain errors thrown by the reader?), but it seems like something that should change, perhaps so that `forms-seq` takes the input's name as a separate arg. If that's reasonable, what would be the "right" name for *in*, provided by the REPL?
  • There are a number of entry points into the analyzer. I believe I covered all of them (each must establish a non-root binding for *alias-map* so the ns parse multimethod can set! new values into it), but perhaps I missed one?
Comment by Nicola Mometto [ 23/Oct/13 9:05 PM ]

+1 for renaming :requires-macros

A minor issue with your patch: I don't see the need from switching away from slurp to PBR+io/reader, slurp should handle Files/URLs/Readers just as fine.

Regarding "file-name" you are correct, it's only used by tools.reader to provide source info on the errors thrown; currently I make no promise whether it will be a String rather than a File or a URL, but I guess if we really want we can still call .getPath on Files/URLs if we want to always have it as a String.

In the case of REPL use, I think it should be either "NO_SOURCE_FILE" like Clojure, or nil

I'll release a 0.7.10 release in a couple of days so that cljs can depend on it.

Comment by Chas Emerick [ 23/Oct/13 9:34 PM ]

Going away from slurp was necessary because the REPL is now just sitting on the return of forms-seq; of course, (slurp *in*) wouldn't be very effective in that kind of context since it's strict.

Thanks for your help with this, Nicola!

Comment by Chas Emerick [ 24/Oct/13 5:29 PM ]

Revised patch attached, addressing the issues raised in my previous comment. Also added a couple of alias-resolution sanity tests for fns and macros.

Comment by David Nolen [ 24/Oct/13 5:40 PM ]

fixed https://github.com/clojure/clojurescript/commit/d643d73095662afc49cc3227a6f09c7428be3568

Generated at Tue Nov 25 16:14:07 CST 2014 using JIRA 4.4#649-r158309.