[#CLJS-455] Warn on invalid js forms

<< Back to previous view

[CLJS-455] Warn on invalid js forms Created: 12/Jan/13 Updated: 20/Feb/13 Resolved: 20/Feb/13
Status:	Resolved
Project:	ClojureScript
Component/s:	None
Affects Version/s:	None
Fix Version/s:	None

Type:

Enhancement

Priority:

Minor

Reporter:

Jonas Enlund

Assignee:

Jonas Enlund

Resolution:

Completed

Votes:

Labels:

None

Attachments:

patch-cljs-455.diff

revert-455.diff

Patch:

Code

Description

The ClojureScript compiler happily accepts forms like js/Math.MAX_NUMBER and (js/Math.ceil 3.14) which is not valid Clojure code. The correct way to write these expressions in ClojureScript is (.-MAX_NUMBER js/Math) and (.ceil js/Math 3.14).

The ClojureScript analyzer should at least emit a warning when these bad forms are encountered. Preferably compilation should fail but that would probably break lots of existing code.

Comments

Comment by Jonas Enlund [ 12/Jan/13 6:13 AM ]

Mailing list discussion here: https://groups.google.com/d/topic/clojure/rtmPtmpo4qA/discussion

Comment by Jonas Enlund [ 12/Jan/13 6:21 AM ]

patch-cljs-455.diff adds a warning message and updates cljs.core and twitterbuzz to use correct js/ syntax.

Comment by David Nolen [ 13/Jan/13 10:44 AM ]

Fixed, http://github.com/clojure/clojurescript/commit/b2df2c2a69a4f1c9be8d698b74122c98b1e22491

Comment by Jonas Enlund [ 02/Feb/13 3:28 PM ]

I'm having second thoughts about this. Looking at the commit when the js namespace was introduced[1] you can see that, for example (goog.global.Math/exp x) was changed to (js/Math.exp x). I'm thinking maybe the js magic namespace is meant to support these kinds of calls?

Another issue I ran into was when using a js library (paper.js) and I was supposed to translate

var x = new paper.Path()

into ClojureScript. I could not find any way to achieve this except

(let [x (js/paper.Path.)]
  ...)

There is an assertion[2] in the analyzer that prohibits the following alternative:

(let [x (new (.-Path js/paper))]
  ...)

Removing the assertion (and the call to 'resolve-existing-var') the above expression seems to work just fine.

[1] https://github.com/clojure/clojurescript/commit/954e8529b1ec814f40af77d6035f90e93a9126ea
[2] https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L528

Comment by David Nolen [ 03/Feb/13 3:12 PM ]

This patch creates issues around using constructors provided by libraries outside ClojureScript and GClosure. Also from Rich's original commit support js prefix, it seems like JS style access after the / was actually intended.

[CLJS-455] Warn on invalid js forms Created: 12/Jan/13 Updated: 20/Feb/13 Resolved: 20/Feb/13