<< Back to previous view

[CLJS-2051] Add end-line and end-column to analyzer AST Created: 25/May/17  Updated: 26/May/17

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

Type: Enhancement Priority: Minor
Reporter: Julien Fantin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: analyzer

Attachments: Text File CLJS-2051.patch    

 Description   

Some existing Clojure tooling [1] currently built on top of tools.analyzer.jvm, depend on having end-line and end-column on an AST node.

This data is currently missing from the ClojureScript analyzer, which prevents these tools from being ported to ClojureScript [2]

[1] https://github.com/clojure-emacs/refactor-nrepl
[2] https://github.com/clojure-emacs/refactor-nrepl/issues/195#issuecomment-303910871



 Comments   
Comment by Julien Fantin [ 25/May/17 11:24 PM ]

Here is a patch that adds end-line and end-column and tries to standardize how that data is obtained from the env.

Comment by António Nuno Monteiro [ 26/May/17 12:04 AM ]

I think this is already being tackled in CLJS-1461, which goal is to achieve full compatibility with the tools.analyzer AST.

Comment by David Nolen [ 26/May/17 7:57 AM ]

CLJS-1461 is a big project and we're not sure how long it will take. In the meantime I don't see a problem with incremental steps in that direction if we get the patches.

Comment by David Nolen [ 26/May/17 1:14 PM ]

This patch looks fine but it would be nice to get some feedback that in fact source mapping is not affected.

Comment by Julien Fantin [ 26/May/17 4:16 PM ]

Unfortunately our main project depends on an older ClojureScript version so I couldn't test this on our main codebase. Are there specific things you'd watch out for?

Comment by David Nolen [ 26/May/17 4:25 PM ]

Julien, no need for you to test this, trying to get some outside help here





[CLJS-2037] Throw if overwriting alias in current namespace Created: 14/May/17  Updated: 05/Jul/17  Resolved: 05/Jul/17

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

Type: Defect Priority: Minor
Reporter: António Nuno Monteiro Assignee: António Nuno Monteiro
Resolution: Completed Votes: 0
Labels: analyzer

Attachments: Text File CLJS-2037.patch    
Patch: Code and Test
Approval: Accepted

 Description   

We should make this behavior the same as Clojure, where requiring a different namespace under and existent alias throws



 Comments   
Comment by David Nolen [ 15/May/17 7:59 AM ]

Go for it.

Comment by António Nuno Monteiro [ 05/Jul/17 12:18 AM ]

Patch attached with fix and test

Comment by David Nolen [ 05/Jul/17 6:54 AM ]

Does the patch need a rebase? I can't get it to apply cleanly here.

Comment by David Nolen [ 05/Jul/17 10:28 AM ]

fixed https://github.com/clojure/clojurescript/commit/84e38b638172b954f090f9bd11da74d3d76afb6a





[CLJS-2010] refer-clojure :rename throws on valid invocations Created: 18/Apr/17  Updated: 28/Apr/17  Resolved: 28/Apr/17

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

Type: Defect Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: analyzer

Attachments: Text File CLJS-2010.patch    
Patch: Code and Test

 Comments   
Comment by David Nolen [ 28/Apr/17 4:08 PM ]

https://github.com/clojure/clojurescript/commit/9dafe7d87a4b4a387a82fce9e3a0d63bbe565e4f





[CLJS-1059] Simple interface wanted to convert cljs forms to js Created: 22/Feb/15  Updated: 31/Jan/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 0.0-3211
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Stuart Mitchell Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: analyzer, compiler


 Description   

In our project (a clojurescript debugger) we want to convert cljs forms or a sequence of forms into javascript so that they can be executed in the javascript console.

We would like something similar to closure/compile-form-seq (https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/closure.clj#L308)

However, we need to supply, the namespace requires and locals in an env like this

{:ns {:name "test.core" :requires {(quote gstring) (quote goog.string)}} :locals {}}

This code seems to do what we want.

(defn compile-form-seq
    \"Compile a sequence of forms to a JavaScript source string.\"
    [forms env]
    (env/ensure
    (compiler/with-core-cljs nil
      (fn []
        (with-out-str
            (doseq [form forms]
              (compiler/emit (analyzer/analyze env form))))))))

I am not sure why I need env/ensure.

Would you be able to patch compile-form-seq to provide the needed interface, or suggest what we should be doing.

Thanks
Stu



 Comments   
Comment by Mike Thompson [ 22/Feb/15 10:09 PM ]

Just to be clear:
1. when our debugger is at a breakpoint,
2. the user can type in an expression at the repl
3. in response, our debugger has to compile the user-typed-in expression to javascript (and then execute it, showing a result)
4. taking into account any local bindings. <---- this is the key bit.

To satisfy point 4, our tool extracts all the 'locals' from the current call-frame, and then supplies all these local bindings in env/locals, so the compiler doesn't stick a namespace on the front of them.

For example, if there was a local binding for 'x' in the callstack, and the user's repl-entered-expression involves 'x', then we want the compiler to leave the symbol 'x' alone and to not put some namespace on the front of it. In the final javascript, it must still be 'x', not 'some.namespace.x'

Our method to achieve this is to put 'x' into env/locals when compiling – and it all works. Except, with the recent changes this has become more of a challenge. Hence this ticket asking for a way to pass in env.

Comment by Thomas Heller [ 23/Feb/15 3:19 AM ]

You could wrap the user expression in an fn, that would allow you to skip messing with the locals. The REPL basically does the same trick for *1,*2,...

(fn [x]
  ~user-expression-here)
Comment by David Nolen [ 29/Apr/15 7:21 AM ]

Seems like something useful to add to a cljs.compiler.api namespace.





[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 Tue Jul 25 05:40:17 CDT 2017 using JIRA 4.4#649-r158309.