Jedi: Introducing Refactoring

[davidhalter]

I'm thinking about introducing basic refactoring methods for Jedi. Apart from renaming (which is already possible, there's not really much I know about typical/usual refactoring methods.

The goal would be to add a new module refactoring that stands for itself and returns the changed lines of different files (which is like a unified interface for all refactoring operations). Maybe it should also return changed file names.

So, @dbrgn, @tkf, @jjay or anyone else:

1. Can you tell me what you think are the most basic refactoring operations (I'm speaking of Python, of course. Clearly some refactoring methods in Java are possible but maybe make no sense for Python).
2. Does anyone want to get involved in this? As far as I think it will be really different from the Jedi core and mostly self-explanatory.

[dbrgn]

The two basic refactorings that I want most:

---

Inline/Extract Local Variable

calculation = do_something() + SOMETHING
print(calculation)

becomes

print(do_something() + SOMETHING)

...and vice versa.

Inline/Extract Function/Method

def do_something(x, y):
    z = (x + y) / 2
    print z

do_something(10, 20)

becomes

z = (10 + 20) / 2
print z

...and vice versa.

---

Stuff like "pull up method" and "push down method" should be fairly easy to implement too, but would probably be rarely used as Python code is usually not too much inheritance oriented.

It should be done in a way so that the IDEs can provide useful help tools, like preview, undo etc... I'm not sure how the API should look like. Maybe a Refactoring object similar to a Completion object? Then you could add properties like code_before, code_after, diff etc...

Refactoring across multiple files will be tricky. But doable.

You could use some kind of strategy pattern to design the refactorings. If we can find a common interface that works for all refactorings, we can create a workflow with different "action points" that the IDEs can implement (e.g. ask for a variable/function name, preview, etc...). Then you can put the refactorings in separate modules/functions/objects (strategies) to make this entire thing extendable.

I have to admit though that I don't know the Jedi internals enough to judge what's possible and what is not. Basic refactorings should be possible though.

More links:

• Huge catalog of Java refactorings: http://www.refactoring.com/catalog/
• Refactorings supported by rope: http://rope.sourceforge.net/
• Pycharm feature list: http://www.jetbrains.com/pycharm/features/

[tkf]

My first reaction was "why not Rope"? Rope was not designed for auto-completion so I think it makes sense to develop another library to do that. But rope already has many refactoring methods (although I use just a few of them). I am wondering if that means Jedi is going to be "full stack" Python environment and cover everything what Rope does and more.

Although I don't have a strong opinion about the direction to cover everything in Jedi, I think it makes more sense to focus on the area where Rope does not cover for the early stage of the development. One thing I came up with is to more focusing on helping user to understand code. For example, I made IPython "magic" command to show class inheritance diagram (https://github.com/tkf/ipython-hierarchymagic). It is helpful when reading big source code. But this requires me to evaluate source in IPython. Supporting this in Jedi makes it easier to use. Other useful thing is to show what method in what class is called by super (probably Jedi can already do that by "goto"? I didn't check). Can we use Jedi to create pseudo "call tree" without executing it? Knowing what function may be called from a piece of code will be very useful.

[davidhalter]

Basically I don't intend to write a full-fledged refactoring library. My goal would be a basic library with 5 refactorings or so... Rope offers also many addtional services like undo/repository control/file system control, etc. I don't intend to do anything like this. Renaming is possible for a while now which I think is a good thing. I really only intend to do the refactorings that @dbrgn proposed (maybe not even that).

@tkf Basically the hierarchymagic is possible with Jedi. The question is much more how that would look like. What would be in there and what now (What about properties?) Basically you could just look at all names in a function and start a goto on every name that is being executed (recursively).

I'm really happy to see if other people want to move Jedi in yet another direction. And I think this makes also sense. We could add a utils module for that.

[davidhalter]

Inline/Extract Function/Method

How does that work exactly? I just don't get why 10 and 20 should be parameters, but 2 not.

[dbrgn]

Instead of the function call, the function is inlined and all function parameters are replaced by the values present in the original function call.

[davidhalter]

Oh sorry, I wasn't clear enough, I want to know about vice versa?!

[dbrgn]

Ah, now I get what you were asking... Yeah, extract method isn't as straightforward as inline method.

In IntelliJ IDEA, it works as follows for Java code:

int z = (10 + 20) / 2;
System.out.println("Value of z: " + z);

Here the system doesn't know which variables to extract. Therefore you need to use "extract variable":

int x = 10;
int y = 20;
int z = (x + y) / 2;
System.out.println("Value of z: " + z);

After this you can use "extract method" on the last two lines, which then asks you how which parameters you want to keep and how you want to call them.

So to implement this in vim, you would have to ask the user some questions on how the refactoring should be done. But that's the case for most refactorings, as many times the system can't figure on its own how to call new variables etc...

[davidhalter]

Theres some progress here:

18 files changed, 601 insertions(+), 144 deletions(-) 

Now all in the dev branch. I added a extract/inline/rename method, but it needs still quite some work. So don't use it in your plugins...

[dbrgn]

:sparkles: :sparkles:

[tkf]

It's not refactoring but it would be nice to have "insert import" helper. For example, you are at line 98 and notice that you need to import os. Going to the head of file and come back is cumbersome. Probably Jedi can do this when I insert os.path and os is not in the scope? Ideally, it would be even better if Jedi understands "import group", as recommended in PEP8, like this:

import sys    # stdlib imports
import os     # <- this is inserted

import jedi   # thrid party imoprts

from . import something  # local imports

[dbrgn]

+1, as discussed in davidhalter/jedi-vim#67 :)

[dbrgn]

Is the "extract variable" ready, or not yet? :)

[davidhalter]

Is the "extract variable" ready, or not yet? :)

Yes it is. More or less. But there's no VIM implementation yet. Also I'm not sure how good it works. There are tests, but not too many (also some TODOs were left in the code).

[donaldbraman]

I think simple and elegant refactoring would be a lovely feature. Rope is fine and all, but there's no reason to not have another flavor that isn't quite as massive. An light approach would be very welcome.

[dbrgn]

Any news on this yet? :)

[davidhalter]

I've given up hopes for the refactoring module in its current state. However, we're waiting for #346 to be finished. Once that task is accomplished, it will be way easier to just modify our AST. At the moment we just modify the source code directly, which is very tedious.

[dbrgn]

True, dynamic AST manipulation and code-regeneration would be awesome.

[davidhalter]

Everything we have done in the refactoring module can be rewritten. With the new parser it should be way easier. Just start anew.

[asmeurer]

A little tutorial on how to use the parser would be helpful, or maybe a little toy refactoring example. I've been playing around with it, but mostly by examining the objects interactively. But I'm not sure if I'm using any non-public interfaces or doing things the wrong way.

For instance, what is the correct way to combine multiple ASTs? I've been mutating the children attribute, but I'm not sure if that's correct. For instance, you can easily create ASTs that round-trip in terms of AST -> get_code -> AST because the original AST is not achievable in the grammar, like

In [2]: from jedi.parser import *

In [3]: grammar = load_grammar()

In [15]: p = Parser(grammar, "(x)")

In [20]: i = Parser(grammar, 'f')

In [22]: i.module.children
Out[22]: [Node(simple_stmt, [<Name: f@1,0>, <Whitespace: >]), <Whitespace: >]

In [24]: p.module.children
Out[24]:
[Node(simple_stmt, [Node(atom, [<Operator: (>, <Name: x@1,1>, <Operator: )>]), <Whitespace: >]),
 <Whitespace: >]

In [26]: p.module.children.insert(0, i.module.children[0])

In [27]: p.module.get_code()
Out[27]: 'f(x)'

In [28]: p.module.children
Out[28]:
[Node(simple_stmt, [<Name: f@1,0>, <Whitespace: >]),
 Node(simple_stmt, [Node(atom, [<Operator: (>, <Name: x@1,1>, <Operator: )>]), <Whitespace: >]),
 <Whitespace: >]

In [29]: r = Parser(grammar, 'f(x)')

In [30]: r.module.children
Out[30]:
[Node(simple_stmt, [Node(power, [<Name: f@1,0>, Node(trailer, [<Operator: (>, <Name: x@1,2>, <Operator: )>])]), <Whitespace: >]),
 <Whitespace: >]

Am I doing things how you would do them here? The basic question I want to answer here is, how can I build up and manipulate code entirely from the AST?

[asmeurer]

Aside from the fact that my In [28] uses a Name node next to an atom node instead of a power node (whatever that means), there is the issue of the column number on x not being correct any more. Is that something I need to worry about? get_code seems to have done the right thing, which I'm impressed by, but I haven't played with it enough to know if things will always work.

[davidhalter]

_TL;DR Positions don't matter, use Node.children, Leaf.value and Leaf.prefix to modify an AST._

But I'm not sure if I'm using any non-public interfaces or doing things the wrong way.

You did just fine :) As you can see, there's mostly Node.children that you can play around with.

For instance, what is the correct way to combine multiple ASTs?

I've never thought about this. But it doesn't really matter, the AST can and should be changed however you want. In the end you want a good get_code output, as long as that's correct you're fine.

Am I doing things how you would do them here? The basic question I want to answer here is, how can I build up and manipulate code entirely from the AST?

You're doing it exactly the way I would do it. The only thing that is not going to work with this is that the start/end positions are going to be wrong (start_pos, end_pos). I wouldn't care about this, since after combining, you usually don't need these positions anymore. get_code doesn't use those positions at all. It just uses Leaf.value and Leaf.prefix. That's all.

For example if I wanted to replace a Name node with a certain piece of code, I wouldn't even care about the type, just say name.value = '()' and call get_code after that:

>>> from jedi.parser import Parser, load_grammar
>>> p = Parser(load_grammar(), 'foo = 3')
>>> p.module.children[0].children[0]
<ExprStmt: foo = 3@1,0>
>>> name = p.module.children[0].children[0].children[0]
>>> name
<Name: foo@1,0>
>>> name.value = 'bar[0]'  # name is really not a name anymore, but we don't care.
>>> p.module.get_code()
'bar[0] = 3'

[davidhalter]

Closed in favor of #667 that summarizes and gives ideas on how to do it.