ClojureScript

Warn on invalid js forms

Details

  • Type: Enhancement Enhancement
  • Status: Resolved Resolved
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • 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.

Attachments

  1. File
    patch-cljs-455.diff
    12/Jan/13 6:21 AM
    4 kB
    Jonas Enlund
  2. File
    revert-455.diff
    03/Feb/13 10:23 PM
    4 kB
    Jonas Enlund

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jonas Enlund added a comment -

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

Show
Jonas Enlund added a comment - Mailing list discussion here: https://groups.google.com/d/topic/clojure/rtmPtmpo4qA/discussion
Hide
Permalink
Jonas Enlund added a comment -

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

Show
Jonas Enlund added a comment - patch-cljs-455.diff adds a warning message and updates cljs.core and twitterbuzz to use correct js/ syntax.
Jonas Enlund made changes -
Field Original Value New Value
Patch Code [ 10001 ]
Attachment patch-cljs-455.diff [ 11804 ]
David Nolen made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
Hide
Permalink
Jonas Enlund added a comment -

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

Show
Jonas Enlund added a comment - 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
Hide
Permalink
David Nolen added a comment -

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.

Show
David Nolen added a comment - 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.
David Nolen made changes -
Status Resolved [ 5 ] Reopened [ 4 ]
Resolution Completed [ 1 ]
Jonas Enlund made changes -
Attachment revert-455.diff [ 11830 ]
Hide
Permalink
Jonas Enlund added a comment -

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

Show
Jonas Enlund added a comment - The patch revert455 removes the warnings on js/Foo.bar forms.
Hide
Permalink
David Nolen added a comment -

Patch applied to master

Show
David Nolen added a comment - Patch applied to master
Hide
Permalink
David Nolen added a comment -

closing this one

Show
David Nolen added a comment - closing this one
David Nolen made changes -
Resolution Completed [ 1 ]
Status Reopened [ 4 ] Resolved [ 5 ]

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.