<< Back to previous view

[CLJS-922] Warning message for non-dynamic vars is incomplete Created: 23/Dec/14  Updated: 06/Jan/15  Resolved: 06/Jan/15

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

Type: Defect Priority: Trivial
Reporter: Michael Griffiths Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: analyzer, cljs

Attachments: Text File CLJS-922-001.patch     Text File CLJS-922-002.patch     PDF File Michael Griffiths Clojure CA.pdf    

 Description   

e.g.

user> (do 
        (cljs.analyzer/analyze nil '((ns dynamic-warning-test-ns)
                                     (def a nil)
                                     (binding [a nil] nil)))
        nil)
WARNING:  not declared ^:dynamic at line 4 
;; => nil

The name of the var is missing at the start of the warning message.



 Comments   
Comment by Michael Griffiths [ 23/Dec/14 3:58 PM ]

Patch attached. Evaluating the above form with patch applied now results in:

WARNING: dynamic-warning-test-ns/a not declared ^:dynamic at line 4

Maybe it would be fine to just use the name of the var rather than the namespace-qualified name - let me know if you want the patch updated.

Comment by David Nolen [ 24/Dec/14 7:52 AM ]

Patch looks good, have you submitted a CA? Thanks.

Comment by Mike Fikes [ 24/Dec/14 8:09 AM ]

I wonder if the fix for CLJS-921 will affect this patch. (I'm thinking that the patch relied on the :name meta containing the fully qualified name, but that with CLJS-921 the :ns would also need to be consulted—as is done in the doc printing impl.)

Comment by David Nolen [ 24/Dec/14 8:16 AM ]

The patch is at the level of the compiler not the runtime so it should be unaffected by CLJS-921.

Comment by Michael Griffiths [ 24/Dec/14 11:29 AM ]

Yeah, I signed an electronic CA the same day I submitted the patch - I'm not sure how often the list of contributors is updated on clojure.org but should be on there soon. Find attached regardless.

Thanks!

Comment by Michael Griffiths [ 05/Jan/15 5:17 AM ]

I'm now listed on http://clojure.org/contributing.

Comment by David Nolen [ 05/Jan/15 9:32 AM ]

The patch needs to be rebased to master.

Comment by Michael Griffiths [ 06/Jan/15 9:54 AM ]

Of course. Rebased patch attached.

Comment by David Nolen [ 06/Jan/15 1:41 PM ]

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





[CLJS-540] Switch to tools.reader for cljs.analyzer/forms-seq Created: 15/Jul/13  Updated: 29/Jul/13  Resolved: 29/Jul/13

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

Type: Enhancement Priority: Major
Reporter: Sean Grove Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: analyzer, cljs, reader, sourcemap
Environment:

CLJS master


Attachments: Text File tools_reader_with_passing_tests.patch    
Patch: Code

 Description   

Switches cljs.analyzer to use tools.reader, so we can get more accurate column location for symbols, prepping for more fleshed-out source maps.



 Comments   
Comment by David Nolen [ 15/Jul/13 10:43 AM ]

Excellent, we need two more things in this patch, can you update bootstrap.sh and the POM file? Thanks.

Comment by Sean Grove [ 15/Jul/13 11:00 AM ]

Updated with POM information and bootstrap update

Comment by Sean Grove [ 22/Jul/13 2:20 PM ]

CA's been processed and I'm listed on the contributing page, so shouldn't be blocked by that any longer.

Comment by David Nolen [ 27/Jul/13 2:09 PM ]

I tried applying this patch and rerunning the bootstrap script. This works but when I try to run script/test I get an error about the tools.reader not being on the classpath. Even trying to require tools.reader at via the repl doesn't work for me.

Comment by David Nolen [ 29/Jul/13 11:14 AM ]

fixed http://github.com/clojure/clojurescript/commit/9f010ff5d4a122b0f1dc93905647f309cc45c699





Generated at Mon Jan 26 04:26:21 CST 2015 using JIRA 4.4#649-r158309.