review 1: Change local variable name refactoring #1005

pvojtechovsky · 2016-11-24T21:57:04Z

This code is not tested and may be even wrong somewhere. I will work on it, because I will need such refactoring in my project.

Just have a short look at class Refactoring, please. Whether you want such methods in Spoon or it is out of it's scope.

What about checking of validity of such refactoring requests? For example if I have

class A and B in package P and I decide to rename class A to B

then the class B will be twice in the model. Do you have some preferences how to report such invalid requests?
A) to throw exception
B) to call some listener and silently skip the operation
C) ?
I am just sure that in all cases the validity should be checked before the model is modified.

monperrus · 2016-11-24T22:04:03Z

src/main/java/spoon/refactoring/Refactoring.java

 			}
 		});
+	}
+
+	public static void changeVariableName(CtVariable<?> variable, String name) {


monperrus · 2016-11-24T22:04:13Z

Just have a short look at class Refactoring, please.

that's a very useful contribution

Do you have some preferences how to report such invalid requests?

an exception.

we also need strong tests.

pvojtechovsky · 2016-11-28T20:05:44Z

we also need strong tests.

I like the idea of usage of complex thirdparty library (like guava), with good jUnit tests, as a test environment.

The test would look like this:

rename all local variables to v1, v2, ... vN.
rename all method parameters to v1,v2,...vN
rename all fields to to v1,v2,...vN
rename all classes to v1,v2...vN
The renaming should detect conflict if name vX is already used in current context and throw exception, then test will try name v(X+1) as long as v(X+Y) is usable name.
Then we print the refactored model and run the refactored tests on it too. If it pass, then it is OK
... but may be it is too complex task, so it will never pass :-) ... we will see

I came with this idea, hoping that I can avoid writing small jUnit test for each refactoring feature. But now I see these small tests are useful when detecting where is the problem in the code.

So the best will be combination of small tests at beginning and at the end that thirparty library refactoring test, which founds the most tricky things.

monperrus · 2016-11-29T08:16:08Z

So the best will be combination of small tests at beginning and at the end that thirparty library refactoring test, which founds the most tricky things.

I agree. For the complex ones, we run them through `build.sh`.

pvojtechovsky · 2017-01-12T20:45:43Z

The refactoring request validity checks are missing.

These checks can be quite tricky. For example rename of local variable should check

if there is another local variable with same name defined in visibility scope of that local variable
if there is method parameter with same name in current method
if there is catch parameter with same name in current catch block

Do you think that we should continue with refactoring functions, which are implemented as static methods of one class? I have feeling like it would be better to have one class for each refactoring. The API might be like this:

interface CtRefactoring {
  boolean isValid();
  void refactor() throws SpoonInvalidRequest;
}

And sometime there are probably some refactorings which cannot be done without temporary inconsistency in the model, so then we should be able to switch OFF the validation check.

I have implemented nothing yet, so I welcome your suggestions. It is easy to accept any design if there is no code yet. ;-)

monperrus · 2017-01-13T08:16:15Z

Do you think that we should continue with refactoring functions, which are implemented as static methods of one class?

Spoon's design has always been very much OO, So I really support the introduction of OO for refactoring.

pvojtechovsky · 2017-01-17T21:52:00Z

It is POC of Refactoring class, which does rename of local variable.
Usage:

CtLocalVariable target = ...
ChangeLocalVariableName rename = new ChangeLocalVariableName();
rename.setTarget(target);
rename.setNewName("someName");
if(rename.getIssues().size()>0) {
 ... handle conflicts, etc...
}
rename.refactor();

I will add fluent API or a constructor of course. But please give me feedback mainly to

Refactor interface
Issue interface
VariableScopeQuery and VariableReferenceQuery - the nearly finished queries, which can be used for many purposes - they will be used by other future refactorings too ...
anything else, what seems to be relevant from your point of view

Please ignore other classes, which comes from other PRs or are obsolete ... I will clean it after your feedback about API

monperrus · 2017-01-18T19:40:44Z

Hi Pavel,

This is very interesting but this already contains a lot of different things to be discussed, they may be split in several PRs (hence in several separate discussions, one after the other).

According to your long-term plan, what do you prefer to discuss and merge first? maybe VariableScopeQuery?

pvojtechovsky · 2017-01-18T20:02:10Z

Hi Martin,

I am aware that there is a lot of things, many baby steps, some of them may be in wrong direction... I develop it this way as a prototype to see what code/structures I need to implement refactoring algorithms in short, efficient and maintainable way. And note, I do not need VariableScopeQuery or VariableReferenceQuery. I need refactorings, which are based on these queries. So I started with refactorings and as a side effect there are VariableScopeQuery and VariableReferenceQuery.

first topic for discussion: VariableScopeQuery?

It is good topic for discussion, because it is one of the core components of that refactorings. So yes, let's discuss it first. I am just not sure if it makes sense to make a PR only for that, because VariableScopeQuery is not much usable on it's own. The better is combination with VariableReferenceQuery. These two are tightly coupled. If you agree then we can move discussion to #1114, which contains only these two queries.

monperrus · 2017-01-18T20:29:18Z

If you agree then we can move discussion to #1114, which contains only these two queries.

OK

pvojtechovsky · 2017-02-07T21:07:30Z

During implementation of local variable rename refactoring, class ChangeLocalVariableName, I have found that I need to extend CtQuery. I need:

to know when query enter/exit an element during filterChildren on any query step level
to be able to skip processing of children of current element
to be able to skip processing of siblings and children of current elements

I will prepare another PR for that soon. Do you have any preference/ideas how to do it? You can influence the solution before it is implemented - so less effort is needed.

If you want to see context where I need it, then have a look at ChangeLocalVariableName#detectNameConflicts of this PR. There are comments which explains the situation.
This PR depends on PR #1154, so look there for classes, which are missing here.

monperrus · 2017-02-08T20:24:08Z

Sounds interesting.

Do you have any preference/ideas how to do it?

not really. I look forward to your solution for this.

pvojtechovsky · 2017-02-16T21:46:11Z

I am testing ChangeLocalVariableName refactoring method using ChangeVariableNameTest. OK, there are still some todos, but I stuck on one thing. The testing algorithm is like this:

take one method from VariableRename class and search for TryRename annotation.
For each found annotation try to rename assigned variable to all new names defined in TryRename annotation. The prefix "-" in the annotation, means that rename to this name should be not possible.
If the rename is possible and passes then the test does following extra steps (see ChangeVariableNameTest#printModelAndTestConsistency)
print the refactored model
compile refactored model
create new class loader and load the class from the refactored model
create instance of that refactored class = call constructor, whose implementation will run all the methods of that class and will check that all assertions are still passing.

The problem is that step 7) does not work. I have no idea why class loader loads some other implementation of the class ...
... it must be so, because otherwise the assertions should fail, because the rename of "var1" to "var2" in this method

	void nestedClassMethodWithShadowVar() {
		@TryRename({"var2", "var3"})
		int var1 = 2; //<--------- the "var1" is renamed to "var2"
		new Runnable() {
			@TryRename({"var1", "var3"})
			int var2 = 3;
			@Override
			public void run() {
				@TryRename({"-var1", "var2"})
				int var3 = 1;
				assertTrue(var1 == 2); //<-----------this reference is renamed too
				//this var1 shadows above defined var1. It can be renamed
				@TryRename({"var2", "-var3"})
				int var1 = 4;
				assertTrue(var1 == 4);
				assertTrue(var2 == 3);
				assertTrue(var3 == 1);
			}
		}.run();
		assertTrue(var1 == 2);
	}

by mistake passes (and it should not pass), and pretty printer produces this valid java code:

    void nestedClassMethodWithShadowVar() {
        @spoon.test.refactoring.testclasses.TryRename(value = { "var2" , "var3" })
        int var2 = 2; //<!----------- is renamed to "var2" - OK
        new java.lang.Runnable() {
            @spoon.test.refactoring.testclasses.TryRename(value = { "var1" , "var3" })
            int var2 = 3;

            @java.lang.Override
            public void run() {
                @spoon.test.refactoring.testclasses.TryRename(value = { "-var1" , "var2" })
                int var3 = 1;
                org.junit.Assert.assertTrue((var2 == 2)); //<-------- is renamed to var2, 
//but the value should be 3, because it is no more local variable reference, but it is now the reference to the class field, 
//which shadows origin local variable! So it should fail here!!!
                @spoon.test.refactoring.testclasses.TryRename(value = { "var2" , "-var3" })
                int var1 = 4;
                org.junit.Assert.assertTrue((var1 == 4));
                org.junit.Assert.assertTrue(((var2) == 3));
                org.junit.Assert.assertTrue((var3 == 1));
            }
        }.run();
        org.junit.Assert.assertTrue((var2 == 2));
    }

but execution of that code does not fail on AssertionError, but it should fail :-(

Any idea why my test class loader does not load classes compiled from refactored code?

tdurieux · 2017-02-16T23:02:59Z

I could be wrong (I did not read all your changes).
It seams that you compile the original files not the model : https://github.com/INRIA/spoon/pull/1005/files#diff-617f2b7a84783b10a9bd2ed77f0fe739R135

Change: SpoonModelBuilder.InputType.FILES to SpoonModelBuilder.InputType.CTTYPES

pvojtechovsky · 2017-02-18T14:40:07Z

@tdurieux thanks! It really compiled origin not refactored sources. Now the test fails as expected :-)

This PR is still not finished. There are several bugs ... working on it.

spoon-bot · 2017-03-15T19:31:34Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170315.164827-83

New API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-SNAPSHOT

Detected changes: 8.

Change 1

Name	Element
Old	class spoon.reflect.visitor.filter.CatchVariableReferenceFunction
New	class spoon.reflect.visitor.filter.CatchVariableReferenceFunction
Code	java.class.nonFinalClassInheritsFromNewClass
Description	Non-final class now inherits from 'spoon.reflect.visitor.filter.LocalVariableReferenceFunction'.
Breaking	source: potentially_breaking, binary: potentially_breaking

Change 1

Name	Element
Old	class spoon.reflect.visitor.filter.CatchVariableReferenceFunction
New	class spoon.reflect.visitor.filter.CatchVariableReferenceFunction
Code	java.class.noLongerImplementsInterface
Description	Class no longer implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.code.CtCatchVariable<?>>'.
Breaking	source: breaking, binary: breaking

Change 2

Name	Element
Old	class spoon.reflect.visitor.filter.FieldReferenceFunction
New	class spoon.reflect.visitor.filter.FieldReferenceFunction
Code	java.class.noLongerImplementsInterface
Description	Class no longer implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtField<?>>'.
Breaking	source: breaking, binary: breaking

Change 2

Name	Element
Old	class spoon.reflect.visitor.filter.FieldReferenceFunction
New	class spoon.reflect.visitor.filter.FieldReferenceFunction
Code	java.class.nowImplementsInterface
Description	Class now implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	source: non_breaking, binary: non_breaking

Change 2

Name	Element
Old	class spoon.reflect.visitor.filter.FieldReferenceFunction
New	class spoon.reflect.visitor.filter.FieldReferenceFunction
Code	java.class.superTypeTypeParametersChanged
Description	Super type's type parameters changed from 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtField<?>>' to 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	source: potentially_breaking, binary: potentially_breaking

Change 3

Name	Element
Old	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
New	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
Code	java.class.noLongerImplementsInterface
Description	Class no longer implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.code.CtLocalVariable<?>>'.
Breaking	source: breaking, binary: breaking

Change 3

Name	Element
Old	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
New	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
Code	java.class.nowImplementsInterface
Description	Class now implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	source: non_breaking, binary: non_breaking

Change 3

Name	Element
Old	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
New	class spoon.reflect.visitor.filter.LocalVariableReferenceFunction
Code	java.class.superTypeTypeParametersChanged
Description	Super type's type parameters changed from 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.code.CtLocalVariable<?>>' to 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	source: potentially_breaking, binary: potentially_breaking

Change 4

Name	Element
Old	class spoon.reflect.visitor.filter.ParameterReferenceFunction
New	class spoon.reflect.visitor.filter.ParameterReferenceFunction
Code	java.class.nonFinalClassInheritsFromNewClass
Description	Non-final class now inherits from 'spoon.reflect.visitor.filter.LocalVariableReferenceFunction'.
Breaking	source: potentially_breaking, binary: potentially_breaking

Change 4

Name	Element
Old	class spoon.reflect.visitor.filter.ParameterReferenceFunction
New	class spoon.reflect.visitor.filter.ParameterReferenceFunction
Code	java.class.noLongerImplementsInterface
Description	Class no longer implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtParameter<?>>'.
Breaking	source: breaking, binary: breaking

Change 5

Name	Element
Old	class spoon.reflect.visitor.filter.VariableReferenceFunction
New	class spoon.reflect.visitor.filter.VariableReferenceFunction
Code	java.class.noLongerImplementsInterface
Description	Class no longer implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtVariable<?>>'.
Breaking	source: breaking, binary: breaking

Change 5

Name	Element
Old	class spoon.reflect.visitor.filter.VariableReferenceFunction
New	class spoon.reflect.visitor.filter.VariableReferenceFunction
Code	java.class.nowImplementsInterface
Description	Class now implements interface 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	source: non_breaking, binary: non_breaking

Change 5

Name	Element
Old	class spoon.reflect.visitor.filter.VariableReferenceFunction
New	class spoon.reflect.visitor.filter.VariableReferenceFunction
Code	java.class.superTypeTypeParametersChanged
Description	Super type's type parameters changed from 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtVariable<?>>' to 'spoon.reflect.visitor.chain.CtConsumableFunction<spoon.reflect.declaration.CtElement>'.
Breaking	source: potentially_breaking, binary: potentially_breaking

Change 6

Name	Element
Old	parameter void spoon.reflect.visitor.filter.FieldReferenceFunction::apply(===spoon.reflect.declaration.CtField<?>===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
New	parameter void spoon.reflect.visitor.filter.FieldReferenceFunction::apply(===spoon.reflect.declaration.CtElement===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
Code	java.method.parameterTypeChanged
Description	The type of the parameter changed from 'spoon.reflect.declaration.CtField<?>' to 'spoon.reflect.declaration.CtElement'.
Breaking	source: potentially_breaking, binary: breaking

Change 7

Name	Element
Old	parameter void spoon.reflect.visitor.filter.LocalVariableReferenceFunction::apply(===spoon.reflect.code.CtLocalVariable<?>===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
New	parameter void spoon.reflect.visitor.filter.LocalVariableReferenceFunction::apply(===spoon.reflect.declaration.CtElement===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
Code	java.method.parameterTypeChanged
Description	The type of the parameter changed from 'spoon.reflect.code.CtLocalVariable<?>' to 'spoon.reflect.declaration.CtElement'.
Breaking	source: potentially_breaking, binary: breaking

Change 8

Name	Element
Old	parameter void spoon.reflect.visitor.filter.VariableReferenceFunction::apply(===spoon.reflect.declaration.CtVariable<?>===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
New	parameter void spoon.reflect.visitor.filter.VariableReferenceFunction::apply(===spoon.reflect.declaration.CtElement===, spoon.reflect.visitor.chain.CtConsumer<java.lang.Object>)
Code	java.method.parameterTypeChanged
Description	The type of the parameter changed from 'spoon.reflect.declaration.CtVariable<?>' to 'spoon.reflect.declaration.CtElement'.
Breaking	source: potentially_breaking, binary: breaking

monperrus · 2017-03-17T09:07:40Z

rebase to have a cleaner diff?

pvojtechovsky · 2017-03-17T09:12:07Z

Here you are :-)

monperrus

Thanks for this brand-new feature, which looks promising.

I've tried to understand the design and usage from the tests and the interface documentation.

Here is a first set of comments.

monperrus · 2017-03-18T20:03:39Z

src/test/java/spoon/test/refactoring/ChangeVariableNameTest.java

+
+	@Test
+	public void testModelConsistency() throws Throwable {
+		new VariableRename();


does this have side effects?

It has no side effects, but it throws an exception if the tested model is inconsistent. It is similar to VariableReferencesModelTest, where I first tested model consistency by call of constructor, which internally called all other methods of model to check if they contain consistent assertions. Then you came with good idea to switch it to jUnit test class too. So VariableRename is very similar to that, but in this case I cannot easily switch it to jUnit test class, because I am testing model consistency before and after each model refactoring test action.

monperrus · 2017-03-18T20:05:24Z

src/test/java/spoon/test/refactoring/ChangeVariableNameTest.java

+
+	@Test
+	public void testRefactorWithoutTarget() throws Exception {
+


contract: a name refactoring where the given variable name does not exist results in a SpoonException

monperrus · 2017-03-18T20:07:54Z

src/test/java/spoon/test/refactoring/ChangeVariableNameTest.java

+	}
+
+	@Test
+	public void testRenameLocalVariableToInvalidName() throws Exception {


merge all exception tests (incl testRefactorWithoutTarget) in one single test

monperrus · 2017-03-18T20:11:02Z

src/test/java/spoon/test/refactoring/testclasses/VariableRename.java

+
+import static org.junit.Assert.*;
+
+public class VariableRename


better name? such as RenameTestSubject?

monperrus · 2017-03-18T20:11:18Z

src/test/java/spoon/test/refactoring/testclasses/VariableRename.java

+	public VariableRename() throws Throwable
+	{
+		int local1 = 0;
+		//call all not private methods of this class automatically, to check assertions


interesting. would be event better in a separate clear method.
new RenameTestSubject().callAllMethods()

monperrus · 2017-03-18T20:11:44Z

src/test/java/spoon/test/refactoring/ChangeVariableNameTest.java

+	public void testRenameLocalVariableToSameName() throws Exception {
+
+		ChangeLocalVariableName refactor = new ChangeLocalVariableName();
+		refactor.setTarget(local1Var);


local1var: what is this?

tests that are self-contained and that can be understood by just looking at the method (without scrolling) are easier to understand an maintain in the long term.

monperrus · 2017-03-18T20:11:50Z

src/test/java/spoon/test/refactoring/ChangeVariableNameTest.java

+
+	@Test
+	public void testRenameLocalVariableToUsedName() throws Exception {
+


what's the contract tested here?

it is the main test method (...bad named...), which uses spoon model of VariableRename class. It looks for each CtVariable and it's CtAnnotation and tries to rename that variable to the name defined by annotation.
If the annotation name is prefixed with "-", then that refactoring should fail.
If the annotation name is not prefixed, then that refactoring should pass.

monperrus · 2017-03-18T20:14:51Z

src/main/java/spoon/refactoring/Issue.java

+ */
+package spoon.refactoring;
+
+public interface Issue {


rename to RefactoringIssue? If it's an issue, what about using a RefactoringException directly?

Do you mean to throw exception directly instead of collecting of issues and then throw an exception?
I would like to support use case where list of issues is better then one exception on the first issue.
The idea is like this:

create an refactoring

call Refactoring#getIssues()

if there are some issues, then solve them automatically, by applying other refactorings

call Refactoring#getIssues() again

if there are still some issues then skip this refactoring

if there are no issues, then process refactoring.

But it is no problem to remove issues for now and add them later with some mature inheritance hierarchy of issues. So KISS wins here.

monperrus · 2017-03-18T20:16:08Z

src/main/java/spoon/refactoring/Refactor.java

+/**
+ *
+ */
+public interface Refactor {


interface and methods must be documented. Can you give a usage snippet? I don't really see here what can be done.

monperrus · 2017-03-18T20:16:49Z

src/main/java/spoon/refactoring/AbstractRenameRefactor.java

+		target.setSimpleName(newName);
+	}
+
+	protected abstract void forEachReference(CtConsumer<CtReference> consumer);


javadoc? contract for the implementors?

pvojtechovsky · 2017-03-19T08:56:28Z

Your ides/change suggestions were implemented. Please review

monperrus

a second set of comments.

I'm really impressed by the test :-)

monperrus · 2017-03-20T20:26:56Z

src/main/java/spoon/refactoring/Refactor.java

+ * r.refactor();
+ * </pre>
+ */
+public interface Refactor {


rename to Refactoring?

add method setTarget(CtElement) to really show in the interface that one always refactors something?

rename to Refactoring

but there is already class spoon.refactoring.Refactoring, so ?

add method setTarget(CtElement)

Does all possible refactorings accepts exactly one CtElement? Does all possible refactoring have input called target? I am not sure. I vote to let setTarget method defined on level of more specific refactoring class

monperrus · 2017-03-20T20:29:33Z

src/main/java/spoon/refactoring/AbstractRenameRefactor.java

+import spoon.SpoonException;
+import spoon.reflect.declaration.CtNamedElement;
+
+public abstract class AbstractRenameRefactor<T extends CtNamedElement> implements Refactor {


currently we have only one subclass. what other subclasses do you envision?

RenameTypeRefactor, RenameParameterRefactor, RenameFieldRefactor, RenameMethodRefactor, ...

monperrus · 2017-03-20T20:34:30Z

src/test/java/spoon/test/refactoring/RenameLocalVariableRefactorTest.java

+		}
+	}
+
+	private void printModelAndTestConsistency(String refactoringDescription) {


is an assert-like method, it should be named accordingly assert*, eg assertCorrectModel

monperrus · 2017-03-20T20:37:07Z

src/test/java/spoon/test/refactoring/RenameLocalVariableRefactorTest.java

+	 * If it behaves different then expected, then this test fails
+	 */
+	@Test
+	public void testRenameAllLocalVariablesOfRenameTestSubject() throws Exception {


this test is super powerful, it's beautiful.

However, I would suggest to add before a simpler test, which does one simple refactoring, it would show a small and clear snippet on how to use RenameLocalVariableRefactor, and it would help to understand this one.

by the way, it's in essence a parametrized test, where RenameLocalVariableRefactorTestSubject contains the test parameters. Really clever. It would be even more explicit and maintainable written as @Parametrized.

Do you have experience with parametrized test? Me not. I can learn it, but if you would have time to rewrite this test to parametrized, I might focus on resolving of type parameters. ;-)

monperrus · 2017-03-20T20:42:31Z

src/main/java/spoon/refactoring/RenameLocalVariableRefactor.java

+ * }
+ * </pre>
+ */
+public class RenameLocalVariableRefactor extends AbstractRenameRefactor<CtLocalVariable<?>> {


add a static method in Refactoring (eg Refactoring#renameLocalVariable(CtNamedElement target, String newName)) to help discoverability and usage of this class?

good idea. I will do so tomorrow evening.

monperrus · 2017-03-21T07:54:23Z

but there is already class |spoon.refactoring.Refactoring|, so ?

right. Then "CtRefactoring"?

monperrus · 2017-03-21T07:55:18Z

Does all possible refactorings accepts exactly one CtElement?

correct, setTarget(CtElement...) may be a better option.

I vote to let |setTarget| method defined on level of more specific refactoring class

The problem with that is that the interface cannot be understood without knowing concrete implementations, which is an issue

monperrus · 2017-03-21T07:58:20Z

Do you have experience with parametrized test? Me not. I can learn it, but if you would have time to rewrite this test to parametrized

No problem, we keep it like this. I wanted to point to an interesting thing...

pvojtechovsky · 2017-03-21T18:13:57Z

CtRefactor#setTarget

I would prefer to keep CtRefactor as it is. I can imagine refactoring methods, which has no target at all. And how would rename refactoring work on array of target elements?

The problem with that is that the interface cannot be understood without knowing concrete
implementations, which is an issue

But CtRefactor is very abstract. It is more like marker interface, which collects all the future refactoring implementations. I guess the only think which can be "understood" here is that "client can run refactoring after s/he configures it by refactoring specific methods"

I understand that Spoon is interface based, so I have added new interface, where setTarget fits better. What about this one?

public interface CtRenameRefactoring<T extends CtNamedElement> extends CtRefactoring {
	CtRenameRefactoring<T> setTarget(T target);
	CtRenameRefactoring<T> setNewName(String newName);
}

add Refactoring#renameLocalVariable ...

I have added

public static void changeLocalVariableName(CtLocalVariable<?> localVariable, String name)

whose name and parameters fits more to the existing method

public static void changeTypeName(final CtType<?> type, String name)

What about rename of RenameLocalVariableRefactor to
A) CtRenameLocalVariableRefactor
B) CtRenameLocalVariableRefactoring' because it is based on CtRefactoring` interface now.

spoon-bot · 2017-03-21T18:27:46Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170321.120037-10

New API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-SNAPSHOT

Detected changes: 1.

Change 1

Name	Element
Old	method java.util.List<java.io.File> spoon.reflect.cu.CompilationUnit::getBinaryFiles()
New	none
Code	java.method.removed
Description	Method was removed.
Breaking	source: breaking, binary: breaking

spoon-bot · 2017-03-21T19:29:30Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170321.120037-10

New API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-SNAPSHOT

Detected changes: 1.

Change 1

Name	Element
Old	method java.util.List<java.io.File> spoon.reflect.cu.CompilationUnit::getBinaryFiles()
New	none
Code	java.method.removed
Description	Method was removed.
Breaking	source: breaking, binary: breaking

monperrus · 2017-03-21T21:12:24Z

src/main/java/spoon/refactoring/Refactoring.java

+	 * @throws RefactoringException when rename to newName would cause model inconsistency, like ambiguity, shadowing of other variables, etc.
+	 */
+	public static void changeLocalVariableName(CtLocalVariable<?> localVariable, String name) throws RefactoringException {
+		new RenameLocalVariableRefactor().setTarget(localVariable).setNewName(name).refactor();


beautiful line

monperrus · 2017-03-21T21:15:22Z

I have added public static void changeLocalVariableName(CtLocalVariable<?> localVariable, String name)

thanks, rename parameter name to newName?

I guess the only think which can be "understood" here is that "client can run refactoring after s/he configures it by refactoring specific methods" I would prefer to keep CtRefactoring as it is.

I'm not convinced, I prefer strong and consistent interfaces, but it's no reason to argue about this for now :-)

|Good idea, ||CtRenameLocalVariableRefactoring is perfect. ||Let's move |AbstractRenameRefactor <https://github.com/INRIA/spoon/pull/1005/files#diff-4ae10ecea7772cfbf5caccd21ff3b831>to AbstractRenameRefactoring also

spoon-bot · 2017-03-22T17:57:56Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-20170321.234528-11

New API: fr.inria.gforge.spoon:spoon-core:jar:5.7.0-SNAPSHOT

Detected changes: 1.

Change 1

Name	Element
Old	method java.util.List<java.io.File> spoon.reflect.cu.CompilationUnit::getBinaryFiles()
New	none
Code	java.method.removed
Description	Method was removed.
Breaking	source: breaking, binary: breaking

monperrus · 2017-03-22T20:35:09Z

thanks!

pvojtechovsky · 2017-03-22T20:51:15Z

You are welcome ;-)

monperrus reviewed Nov 24, 2016

View reviewed changes

pvojtechovsky force-pushed the variableRenameRefactoring branch from d8f30aa to 02a8082 Compare November 27, 2016 20:16

monperrus changed the title ~~New refactoring methods - rename of CtVariable based elements~~ WIP New refactoring methods - rename of CtVariable based elements Jan 11, 2017

monperrus changed the title ~~WIP New refactoring methods - rename of CtVariable based elements~~ WIP[test missing] New refactoring methods - rename of CtVariable based elements Jan 11, 2017

pvojtechovsky mentioned this pull request Jan 12, 2017

review: feature: VariableReferenceFunction, VariableScopeFunction #1114

Merged

pvojtechovsky force-pushed the variableRenameRefactoring branch from 02a8082 to aa5badd Compare January 17, 2017 21:39

pvojtechovsky changed the title ~~WIP[test missing] New refactoring methods - rename of CtVariable based elements~~ POC Change local variable name refactoring Jan 17, 2017

pvojtechovsky force-pushed the variableRenameRefactoring branch from aa5badd to 54c734b Compare January 18, 2017 19:26

pvojtechovsky force-pushed the variableRenameRefactoring branch from 54c734b to f5e67a3 Compare January 30, 2017 22:37

pvojtechovsky changed the title ~~POC Change local variable name refactoring~~ WiP: Change local variable name refactoring Jan 30, 2017

pvojtechovsky force-pushed the variableRenameRefactoring branch 2 times, most recently from 20db5d7 to 9857e30 Compare February 7, 2017 20:55

pvojtechovsky mentioned this pull request Feb 9, 2017

review: introduction of CtScannerFunction #1180

Merged

pvojtechovsky force-pushed the variableRenameRefactoring branch from 9857e30 to 0c7d49d Compare February 16, 2017 21:22

pvojtechovsky mentioned this pull request Feb 17, 2017

CtTypeReferenceImpl#canAccess does not properly handle sub-interface of another interface #1188

Closed

pvojtechovsky mentioned this pull request Feb 18, 2017

bug: Wrong printed field reference after rename of variable to the same name #1190

Closed

pvojtechovsky changed the title ~~review 2: Change local variable name refactoring~~ review 1: Change local variable name refactoring Mar 17, 2017

feature: change LocalVariable name

69b5996

pvojtechovsky force-pushed the variableRenameRefactoring branch from 7516392 to 69b5996 Compare March 17, 2017 09:11

monperrus requested changes Mar 18, 2017

View reviewed changes

feature: RenameLocalVariableRefactor

85135df

added license header and comment to the new RefactoringException

89b613a

monperrus reviewed Mar 20, 2017

View reviewed changes

monperrus approved these changes Mar 20, 2017

View reviewed changes

new interface CtRenameRefactoring, class and method renaming

d31543a

improve javadoc

9888ff4

monperrus reviewed Mar 21, 2017

View reviewed changes

rename RenameLocalVariableRefactor*->CtRenameLocalVariableRefactoring*

32587f3

monperrus merged commit ae72ad6 into INRIA:master Mar 22, 2017

pvojtechovsky deleted the variableRenameRefactoring branch March 22, 2017 20:51

surli mentioned this pull request Jun 1, 2017

Release: Spoon-5.7.0 incoming! #1328

Closed

henry-lp mentioned this pull request Jul 15, 2019

Doc: test commit eclipse-repairnator/repairnator#866

Closed


		@Test
		public void testRefactorWithoutTarget() throws Exception {


		import static org.junit.Assert.*;

		public class VariableRename


		@Test
		public void testRenameLocalVariableToUsedName() throws Exception {

review 1: Change local variable name refactoring #1005

review 1: Change local variable name refactoring #1005

Conversation

pvojtechovsky commented Nov 24, 2016

Choose a reason for hiding this comment

monperrus commented Nov 24, 2016

pvojtechovsky commented Nov 28, 2016

monperrus commented Nov 29, 2016 via email

pvojtechovsky commented Jan 12, 2017

monperrus commented Jan 13, 2017 via email

pvojtechovsky commented Jan 17, 2017 • edited Loading

monperrus commented Jan 18, 2017

pvojtechovsky commented Jan 18, 2017

monperrus commented Jan 18, 2017

pvojtechovsky commented Feb 7, 2017

monperrus commented Feb 8, 2017 via email

pvojtechovsky commented Feb 16, 2017

tdurieux commented Feb 16, 2017 • edited Loading

pvojtechovsky commented Feb 18, 2017

spoon-bot commented Mar 15, 2017

Revapi Analysis results

Change 1

Change 1

Change 2

Change 2

Change 2

Change 3

Change 3

Change 3

Change 4

Change 4

Change 5

Change 5

Change 5

Change 6

Change 7

Change 8

monperrus commented Mar 17, 2017

pvojtechovsky commented Mar 17, 2017

monperrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvojtechovsky commented Mar 19, 2017

monperrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monperrus commented Mar 21, 2017 via email

monperrus commented Mar 21, 2017 via email

monperrus commented Mar 21, 2017 via email

pvojtechovsky commented Mar 21, 2017

spoon-bot commented Mar 21, 2017

Revapi Analysis results

Change 1

spoon-bot commented Mar 21, 2017

Revapi Analysis results

Change 1

Choose a reason for hiding this comment

pvojtechovsky commented Jan 17, 2017 •

edited

Loading

tdurieux commented Feb 16, 2017 •

edited

Loading