Rename refactoring

[expez]

I think the most valuable refactoring we haven't tackled yet is that of renaming stuff. Does anyone have any thoughts on how this should work? What happens if you try to analyze source that is invalid? I'm thinking it might be easy to get into a situation where we can find 99% of the occurrences of an identifier but miss one or two due to syntax errors in the file.

We've previously talked a bit about how we're going to tackle undo when we're doing refactoring in the middlewere and this might be difficult for this feature. I propose we just do like ternjs does: drop undo and instead just let the user rename it once again to undo the operation. When you rename something using tern it will give just prompt your for the new name instead of letting you edit inline with multiple cursors or whatever.

@AlexBaranosky any interest in continuing your explorations?

[benedekfazekas]

I kinda started this work on the find-symbol branch. Meaning that my idea is that all refactor-nrepl needs to do is find occurrences of symbols and return their exact locations. then the client can generate a buffer with the result, or open the files for the user or in fact rename them.

on not valid source: for really broken stuff AST building should fail. if we really want to make sure we might want to eval the code while building the AST that should surely filter out broken code (I don't know if you ever used eastwood: but when you lint your code with it it defo fails if you have not compiling code as it evals whilest linting.)

[benedekfazekas]

find-symbol is basically done. need to do loads of testing I guess (will add some tests here too).

todo:

• [x] implement integration tests using the clojure client for find symbol
• [x] add rename symbol to clojure client
• [x] add some integration tests for rename symbol
• [x] implement emacs integartion in clj-refactor.el

open question:

• what to do with macros?

[expez]

Great job! I can take on clj-refactor.el part this week. Have you decided on an interface, e.g. what will be returned to the clients?

I haven't looked at all at the result of running a parse; what are the challenges in regards to macros? Just getting functions and symbols (fn args and let bindings etc) renaming would be a huge win on it's own, so this is far from a deal breaker in my eyes.

[benedekfazekas]

that would be great although this is not thoroughly tested yet. interface is basically [line-number end-line-number column-number end-line-number fn-name absolute-path] see here.

you get back one msg from the middleware/hit.

macros: they are basically expanded when the AST is built so if you look for an macro in the project that won't be found but if you look for a function which is a part of an expanded macro you will find it... bit confusing tbh.

i think with this you can find/rename defs and defns (that is what I tested so far anyway ;) )

[expez]

How do you work on this locally? I just built the jar and used localrepo to install it, with mr anderson, but this is kind of cumbersome when you're used to working with a repl :)

When I try to use find-symbol it just times out now. I'm guessing that means it crashed trying to do its thing.

Did you mean to write end-column-number instead of end-line-number a second time?

So for example if I have this file:

(ns test)

(defn greet [name]
  (println "Hello, " name))

then calling find-symbol with cursor somewhere inside greet will return: [[3 3 7 11 "greet" "/some/path/test.clj"]]?

[benedekfazekas]

so working with mranderson:

• you can do the usual repl thing only caveat that you should open and edit the source files in target/srcdeps/refactor_nrepl/ dir or alternatively
• just don't build the project with mranderson for local development work. if your sample project does not clash with refactor-nrepl on deps you are good to go with vanilla

end-column-number: yeah, you are right. typo.

do you have the latest find-symbol? (just pushed couple of hours ago: which basically fixed a timeout problem which did result in nothing returned sometimes).

with the latest clj client from the repl:

user> (require '[refactor-nrepl.client :as refactor])
contemplate => nil
user> (refactor/find-symbol :ns 'fe.utils.bench :name "bench-channel")
/Users/benedekf/projects/clj_fe/src/fe/endpoints/api/feeds.clj [ 58 ] :       (fe.utils.bench/bench-channel channel-uri)
/Users/benedekf/projects/clj_fe/src/fe/endpoints/api/feeds.clj [ 66 ] :       (fe.utils.bench/bench-channel channel-url)
/Users/benedekf/projects/clj_fe/src/fe/endpoints/channel.clj [ 49 ] :         (fe.utils.bench/bench-channel (:shortName channel))
/Users/benedekf/projects/clj_fe/src/fe/endpoints/football.clj [ 77 ] :     (fe.utils.bench/bench-channel (:shortName channel))
/Users/benedekf/projects/clj_fe/src/fe/endpoints/topics.clj [ 75 ] :      (fe.utils.bench/bench-channel (:shortName channel))
/Users/benedekf/projects/clj_fe/src/fe/endpoints/weather.clj [ 51 ] :        (fe.utils.bench/bench-channel (:shortName channel))
/Users/benedekf/projects/clj_fe/src/fe/utils/bench.clj [ 40 ] :  (defn bench-channel [short-name]
  (fn [start]
    (bench-to-riemann start {:channel short-name} "channel")))
contemplate => (nil nil nil nil nil nil nil)
user> 

does this help?

[benedekfazekas]

btw for emacs i have a nice clickable search buffer in mind (like the grep buffer basically ;) )

[expez]

error in process sentinel: Could not start nREPL server: Error loading refactor-nrepl.refactor: java.io.FileNotFoundException: Could not locate clojure/tools/analyzer__init.class or clojure/tools/analyzer.clj on classpath: , compiling:(refactor_nrepl/analyzer.clj:1:1)

after doing:

lein install && lein localrepo coords target/refactor-nrepl-0.2.0-SNAPSHOT.jar | xargs lein localrepo install

with nothing but cider and refactor-nrepl in my profiles.clj in a project where the only dep is clojure 1.6.

When I try to load refactor.clj into a regular repl I get an exception that walk is already defined in analyzer.ast. The offending form in refactor.clj is this dep: [deps.toolsanalyzerjvm.v0v5v4.deps.toolsanalyzer.v0v5v2.clojure.tools.analyzer.ast :refer :all]

I don't think the emacs part is going to be very difficult, so it might make sense to wait until you've just pushed a working 0.2-SNAPSHOT to clojars :p

[benedekfazekas]

perhaps the mrandersoned and not mrandersoned versions are mixed on your classpath?!

anyways i uploaded a 0.2.0-SNASPSHOT to clojars built from the find-symbol branch

[expez]

(defun cljr-find-symbol ()
  (interactive)
  (cljr--assert-middleware)
  (let* ((symbol-name (thing-at-point 'symbol))
         (nrepl-sync-request-timeout 20))
    (message "%s" (nrepl-send-sync-request (nrepl-send-sync-request
                                            (list "op" "refactor"
                                                  "refactor-fn" "find-symbol"
                                                  "name" symbol-name))))))

;; => nrepl-send-sync-request: Sync nREPL request timed out (op refactor refactor-fn find-symbol name foo id 20)

I just tried the new snapshot and it's still just timing out.

I'm using this in a project with only clojure as a dep (though with my full profiles.clj), and in a file with this content:

(defn foo
  "I don't do a whole lot." []
  (println "Hello, World!"))

(foo)

with the cursor on the call to foo.

[benedekfazekas]

you need to send either the ns where foo is or the whole namespace as string with a ns declaration in it so the middleware can figure out the ns itself; see this line. I never tested what happens if you try to search for a not qualified symbol tbh.

[expez]

The code is here. Probably still a few rough edges but through the wonders of compilation-mode you can now M-n and M-p between matches like in the regular compilation/grep buffer.

I didn't have as much time today as I had hoped, so I really appreciate your patience in helping me get started.

Hopefully find time to implement the renaming shortly :)

[benedekfazekas]

wow! lovely! sorry for the halfbaked code on refactor-nrepl side. just putting together some tests atm.

[bbatsov]

Guys, something like this should be part of cider itself I think. We've been discussing doing something similar to what swank-clojure had here. This functionality is obviously needed for the rename refactoring, but it's pretty useful on its own for thing like "find usages".

[benedekfazekas]

fair point @bbatsov but then do you want to build AST for stuff in cider?! AST makes this thing possible really... and for this search you practically need to build one for all the clj files you wanna search in in the project...

you may be interested how this just committed integration test is done too (although that needs a client implemented in clojure too)

[bbatsov]

fair point @bbatsov but then do you want to build AST for stuff in cider?! AST makes this thing possible really... and for this search you practically need to build one for all the clj files you wanna search in in the project...

Yeah, I'm well aware of this. I wonder how fast is this? Probably pretty fast for reasonably sized projects. SLIME's approach to the problem was pretty flawed - it basically grepped the source code of all loaded functions.

[benedekfazekas]

relatively quick: have a go with it on some of your projects (see comments above how to use the clojure client from the repl). we could in theory also cache ASTs in the future.

[expez]

If I try to find occurrences for get in the project (that is a fn that isn't defined in the project, but in clojure core) I get nil back. Is that expected? I was wondering if I had to check for symbols that we 'own' vs symbols that are imported from libraries when doing the renaming, but it seems you're already doing that?

[benedekfazekas]

well i suppose i do it 'implicitly' as in the request you are sending me the ns too so i will look for your.ns/get which won't be found...

that said you can look for invocations of any fns: see remove debug fns which basically does that

that said i might have overcomplicated the look for invocations so possibly a more generic use of find-symbol could be used there too... (meaning: you have a point here saying that find-symbols should in fact find extraproject symbols too...)

[expez]

Yeah, I think maybe we should split this in two:

1. rename the find-symbol op to something else, and we'll keep that as is. It's perfect when we want to rename stuff, as we only want a list of symbols that are our own.
2. Extract something more generic from remove debug fns, which returns the project-wide usage of any symbol (external symbols included).

[benedekfazekas]

yup makes sense. filtering on invocations still has its place only impl perhaps can be made simpler based on find-symbol. have u seen the integration test i pushed here using the clojure client?

[expez]

yeah I saw it, looks good. Only change I'd suggest is to just make a test project under test/resources already and use for testing. You might argue that this hurts readability, but embedding code as strings and creating directories as part of test setup is having much the same effect :)

[benedekfazekas]

fair point. my motivation to use a temp dir was to easily get a clean sheet for the individual tests. but perhaps the two things can be combined. will give it a try

[expez]

I just noticed that I have to hit C-c C-k to load the file into the repl before find-symbol starts working, how come? What does cider's evaluation of the file have to do with our middlewere?

[benedekfazekas]

well... most cases (not all) u need ur code loaded into the repl so a correct AST can be built. see the load-string lines in the integration test too.

as we (usually) don't eval the code while building the AST. this is really my ongoing dilemma: eval or not eval.

[benedekfazekas]

tbh i use a variation of the reloaded workflow so in practice i always run refresh and load all my code

[expez]

Implementation of rename-symbol

I use the reloaded workflow myself, so this won't bother me much, but this is a bit awkward. Should we take it upon ourselves to load the project code into the repl? I suppose one benefit of this is that the user will notice when the project is in a bad state. I mean, if half the files don't compile, our AST and subsequent renaming isn't going to be great.

[benedekfazekas]

wow nice!! will have a play with it tmrw.

this is a more far fetching issue with tools.analyzer really. see the readme of eastwood for example which warns u that ur code will be evaled beware of side effects.

i guess for now we want to fail gracefully if ast is missing or not very usable and detail in the readme that code needs to be loaded. for longer term we need to find a nice way to solve it or bite the bullet as eastwood...

[expez]

Already found a bug, unfortunately. If you rename a symbol from file where it's defined it will also rename in the files where it's used, but if you try to rename from the file where it's used nothing will be renamed.

[benedekfazekas]

u need to pass the right ns. i suppose that is the reason...

[expez]

Aha, I thought what was wanted was the ns that the client is currently looking at (i.e. cljr--current-namespace), but you need the ns where the symbol is defined? If I instead of ns use ns-string (and just pass the entire file to the middlewere) will the midddlewere then take care of everything?

[benedekfazekas]

ns where symbol defined: correct. middleware taking care of everything: not at the moment but the whole ns-string should be enough input to figure it out

[benedekfazekas]

thinking about above (eg looking for a symbol from a namespace where it is used (not defined)): cider already provides this functionality and in version 0.7.0

I wonder if the elisp client should use this functionality in cider-nrepl to figure out what ns to pass to refactor-nrepl's find-symbol. why would we reinvent this when we clearly depend on cider anyway?

[benedekfazekas]

I merged find-symbol branch. inclined to close this if you @expez agree with previous comment: eg use cider functionality to figure out ns.

[bbatsov]

I wonder if the elisp client should use this functionality in cider-nrepl to figure out what ns to pass to refactor-nrepl's find-symbol. why would we reinvent this when we clearly depend on cider anyway?

Reusing the existing functionality seems like the right decision to me.

[expez]

@benedekfazekas sure, relying on cider makes good sense to me. But there are two outstanding issues related to this feature:

• What do we do about macros?
• What do we do about code evaluation?

[expez]

@benedekfazekas have you thought any more about this? I'm leaning in the direction of doing what eastwood does: warn the user about side-effects and load the code.

As far as macros disappearing when the AST is built, the simplest thing that could possibly work is this:

1. Call cider-var-info to find the origin of the symbol at point, if it is a macro.
2. Grep the project for symbols with that name.
3. Call cider-var-info on each occurrence to find out if it refers to the same place as identified in 1)

Granted this is super naive, but it would be a lot better than nothing imo.

Btw, I haven't tried this, but does find-symbol extend to test code now? I guess it does, but only if the user manually loads everything into the repl?

[expez]

(defn foo [e]
  (println e))

I tried renaming e here but find-symbol returns no occurrences.

[benedekfazekas]

sorry for late answer, very busy week here...

• e i don't think it is a symbol in the sense of a fn (defn) or def or the reference of it.
• using tools.analyzer's analyze+eval by default i was struggling with this for long. if @magnars and @bbatsov agree we can do that way but i am simply concerned that this would be a very undesirable sideeffect of a development tool. You can always say that certain functions will only work in their full capability if you load your code. this is true for cider as well (if your code is not loaded very basic functionality just won't work for that code)
• macros: sounds interesting. need to think about this a bit more. let me play with this over the weekend and come back with something.

[expez]

I'm just thinking the rename refactoring is going to be super annoying to use for anyone not using the reloaded workflow. They'd either have to go through the entire project to load every file before using the refactoring, or just accept that some occurrences won't be found. And what about test files? Even if you're using the reloaded workflow you'd have to manually load all your tests when renaming.

We have to be able to rename local variables, like the e above. Can you extend find-symbol to return such occurrences? I agree that it doesn't make much sense to call clj-find-symbol on such a symbol, but if someone does that then whatever.

[benedekfazekas]

i think tests are searched (as long as they are loaded of course). so in my environment they are searched too always.

we can always go and trust our users too: eg. offer an option to analyze+eval or just analyze and then it is their choice.

i think local variables renaming is absolutely a valid usecase but a different one.

[expez]

Why don't you want to extend find-symbol to return results for locally defined variables / functions? This makes perfect sense to me, the user is asking "where is this symbol defined?" and we answer "it's introduced in this let form and used here and here" or "that's a function parameter and it's used here and here".

If you don't want to include these in the result set of find-symbol then the clients have to have additional logic related to renaming of symbols: it first has to call the middleware to find out if the symbol is a local variable or not, and then either call find-symbol or find-local-symbol when it makes no difference, in terms of renaming the occurrences, if they are defined locally or not.

Unless I'm missing something this seems like unnecessary steps for the client and unnecessary implementation work in the middleware.

Are you on IRC btw? if so, you should join the #clojure-emacs channel on the freenode network :)

[expez]

Should we rename the cljr-find-symbol to cljr-find-usages? I think this name is more in line with what IDE users expect. It also conveys that we're looking for where the symbol is used, whereas find-symbol is somewhat ambiguous in that it can also mean 'find where this symbol is defined'.

[benedekfazekas]

:+1:

side note: it also finds where it is defined ;)

[benedekfazekas]

i would only rename this in the clients (elisp and clojure) but leave untouched in the refactor-nrepl backend

[benedekfazekas]

rename is done find-symbols->find-usages in the clients

[narendraj9]

@benedekfazekas

I am trying to understand what node->var is doing. Why do we need to check :class? For :op :catch type AST nodes, :class is a map. Sometimes, this map is so huge that I get Java Heap Out of Memory exception while doing a join

Can you please help me understand what the intention is with using :class?