<< Back to previous view

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

Status: Closed
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: 0
Labels: None

Attachments: File patch-cljs-455.diff     File 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.

Comment by Jonas Enlund [ 03/Feb/13 10:25 PM ]

The patch revert455 removes the warnings on js/Foo.bar forms.

Comment by David Nolen [ 04/Feb/13 10:40 AM ]

Patch applied to master

Comment by David Nolen [ 20/Feb/13 8:35 AM ]

closing this one

Generated at Wed Oct 22 04:53:49 CDT 2014 using JIRA 4.4#649-r158309.