ClojureScript

Stop using Clojure Namespaces entirely for non-macro names/lookups

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • 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?

Activity

Hide
David Nolen added a comment -

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

Show
David Nolen added a comment - This sounds interesting, should probably get Nicola Mometto to weigh in on this.
Hide
Chas Emerick added a comment -

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

Show
Chas Emerick added a comment - Yup, already linked him here on Twitter; will mail him later this week if necessary.
Hide
Chas Emerick added a comment -

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).

Show
Chas Emerick added a comment - 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).
Chas Emerick made changes -
Field Original Value New Value
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:

https://github.com/cemerick/simple-check/blob/cljx/src/cljx/simple_check/generators.cljx

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?
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:

{code}
(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]))
{code}

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?
Hide
Nicola Mometto added a comment -

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)

Show
Nicola Mometto added a comment - 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)
Hide
David Nolen added a comment -

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

Show
David Nolen added a comment - Wow, cool. I would love to see tools.reader support this, what we currently have is a dirty hack.
Hide
Chas Emerick added a comment - - edited

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.

Show
Chas Emerick added a comment - - edited 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.
Hide
Nicola Mometto added a comment -

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

Show
Nicola Mometto added a comment - I pushed the change and deployed a 0.7.10-SNAPSHOT to Sonatype, let me know if you encounter any issue
Hide
Chas Emerick added a comment -

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?
Show
Chas Emerick added a comment - 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?
Chas Emerick made changes -
Attachment CLJS-632.diff [ 12380 ]
Chas Emerick made changes -
Patch Code [ 10001 ]
Hide
Nicola Mometto added a comment -

+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.

Show
Nicola Mometto added a comment - +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.
Hide
Chas Emerick added a comment -

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!

Show
Chas Emerick added a comment - 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!
Hide
Chas Emerick added a comment -

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

Show
Chas Emerick added a comment - Revised patch attached, addressing the issues raised in my previous comment. Also added a couple of alias-resolution sanity tests for fns and macros.
Chas Emerick made changes -
Patch Code [ 10001 ] Code and Test [ 10002 ]
Attachment CLJS-632.diff [ 12390 ]
Chas Emerick made changes -
Attachment CLJS-632.diff [ 12380 ]
David Nolen made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
David Nolen made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: