Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #36 resolved. #85

Merged
merged 3 commits into from
May 20, 2022
Merged

Issue #36 resolved. #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8656799
Issue #36 resolved.
monjurmorshed793 Apr 5, 2022
37049be
Code refactored and unwanted break statement removed.
monjurmorshed793 Apr 5, 2022
d9266d8
Formatting tweaks
May 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 50 additions & 10 deletions ...fasoftware/astra/core/refactoring/operations/methods/ChainedMethodInvocationRefactor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import org.alfasoftware.astra.core.utils.ASTOperation;
import org.eclipse.jdt.core.dom.ASTNode;
import org.eclipse.jdt.core.dom.CompilationUnit;
import org.eclipse.jdt.core.dom.Expression;
import org.eclipse.jdt.core.dom.MethodInvocation;
import org.eclipse.jdt.core.dom.rewrite.ASTRewrite;
import org.eclipse.jface.text.BadLocationException;
Expand All @@ -17,19 +18,25 @@
* Similar to a method invocation refactor, this swaps two chained method invocations to a different method.
*
* For example,
*
* <pre>
* # getCurrentFoo().doFooThing()
* </pre>
*
* can be swapped to
*
* <pre>
* # doBarThing()
* </pre>
*
* In that case, the chain to refactor is:
*
* <pre>
* ["org.example.ThingProvider getCurrentFoo", "org.example.Foo doFooThing"]
* </pre>
*
* and the method name should be swapped to:
*
* <pre>
* ["doBarThing"]
* </pre>
Expand All @@ -39,11 +46,13 @@ public class ChainedMethodInvocationRefactor implements ASTOperation {
private List<MethodMatcher> before = new ArrayList<>();
private List<String> after = new ArrayList<>();


public ChainedMethodInvocationRefactor(List<MethodMatcher> before, List<String> after) {
this.before = before;
this.after = after;
}


@Override
public void run(CompilationUnit compilationUnit, ASTNode node, ASTRewrite rewriter)
throws IOException, MalformedTreeException, BadLocationException {
Expand All @@ -56,19 +65,50 @@ public void run(CompilationUnit compilationUnit, ASTNode node, ASTRewrite rewrit
private void handleMethodInvocation(MethodInvocation node, ASTRewrite rewriter) {
// second
if (before.get(before.size() - 1).getMethodName().filter(name -> name.test(node.getName().toString())).isPresent() &&
// wrappedA.get().first()
node.getExpression() != null && node.getExpression() instanceof MethodInvocation) {
// wrappedA.get().first()
node.getExpression() != null && node.getExpression() instanceof MethodInvocation) {
// first
MethodInvocation nextMethodInvocation = (MethodInvocation) node.getExpression();
if (before.get(before.size() - 2).getMethodName().filter(name -> name.test(nextMethodInvocation.getName().toString())).isPresent()) {
// TODO #36 make this looped, so we can handle chains of arbitrary length
MethodInvocation methodInvocation = (MethodInvocation) node.getExpression();
int methodIterator = 2;
while (methodIterator <= before.size()) {
MethodInvocation nextMethodInvocation = methodInvocation;
if (before.get(before.size() - methodIterator).getMethodName()
.filter(name -> name.test(nextMethodInvocation.getName().toString()))
.isPresent()) {
methodIterator += 1;
if (before.size() == 2) {
rewriter.set(node, MethodInvocation.EXPRESSION_PROPERTY, methodInvocation.getExpression(), null);
rewriter.set(node, MethodInvocation.NAME_PROPERTY, node.getAST().newSimpleName(after.get(after.size() - 1)), null);
break;
} else if (methodIterator == before.size()) {
methodInvocation = (MethodInvocation) methodInvocation.getExpression();
MethodInvocation newMethodInvocation = node.getAST().newMethodInvocation();

for (int i = 0; i < after.size(); i++) {
if (i == 0) {
Expression methodInvocationExpression = methodInvocation.getExpression();
methodInvocation.setExpression(null);
newMethodInvocation.setName(newMethodInvocation.getAST().newSimpleName(after.get(i)));
newMethodInvocation.setExpression(methodInvocationExpression);
} else {
MethodInvocation expression = node.getAST().newMethodInvocation();
expression.setName(expression.getAST().newSimpleName(after.get(i)));
expression.setExpression(newMethodInvocation);
newMethodInvocation = expression;
}
}

// change the chain to the "after"
// TODO #36 handle arbitrary "after" lengths
rewriter.set(node, MethodInvocation.EXPRESSION_PROPERTY, nextMethodInvocation.getExpression(), null);
rewriter.set(node, MethodInvocation.NAME_PROPERTY, node.getAST().newSimpleName(after.get(0)), null);
rewriter.set(node, MethodInvocation.EXPRESSION_PROPERTY, newMethodInvocation.getExpression(), null);
rewriter.set(node, MethodInvocation.NAME_PROPERTY, node.getAST().newSimpleName(after.get(after.size() - 1)), null);

break;
}
} else {
break;
}

methodInvocation = (MethodInvocation) methodInvocation.getExpression();
}
}
}
}

48 changes: 23 additions & 25 deletions ...oftware/astra/core/refactoring/methods/methodInvocation/TestMethodInvocationRefactor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ public void testInvocationChangeOutsideClassFullyQualifiedUsingMatcher() throws
)
);
}


/**
* Changes an invocation of a method inherited from a class with a specific name, to another method.
* Method invocations with the same name, but declared on a different class to the one specified,
Expand All @@ -88,6 +88,7 @@ public void testInvocationChangeInheritedFromSuperclass() {
.toNewMethodName("getClass")))));
}


/*
* Uses the JavaPattern framework to change an invocation of a method inherited from a class with a specific name, to another method.
*/
Expand All @@ -103,7 +104,7 @@ public void testInvocationChangeInheritedFromSuperclassMatcher() throws IOExcept
);
}


/**
* Changes an invocation of a method inherited from an interface with a specific name, to another method.
* Method invocations with the same name, but declared on a different class to the one specified,
Expand All @@ -121,8 +122,8 @@ public void testInvocationChangeInheritedFromInterface() {
.to(new Changes()
.toNewMethodName("getClass")))));
}


/**
* Changes an invocation of a method inherited from an interface which extends another interface, to another method.
* Method invocations with the same name, but declared on a different class to the one specified,
Expand All @@ -140,7 +141,7 @@ public void testInvocationChangeInheritedFromExtensionOfInterface() {
.to(new Changes()
.toNewMethodName("getClass")))));
}


/**
* Changes a static method invocation target.
Expand Down Expand Up @@ -268,16 +269,13 @@ public void testInvocationChained() {
))
);
}


/**
* ChainedMethodInvocationRefactor should be able to handle chains of arbitrary length.
* This means we should be able to match on arbitrarily large chains of method calls,
* and replace them with chains of method calls.
*
* Both the matching and replacement of method calls currently stops after two.
*/
@Ignore("Illustrates issue https://github.com/alfasoftware/astra/issues/36")
@Test
public void testInvocationChainedWithLargeMatchingAndReplacementChains() {
assertRefactor(InvocationChainedLargeChainsExample.class,
Expand Down Expand Up @@ -319,7 +317,7 @@ public void testInvocationChainedWithLargeMatchingAndReplacementChains() {
))
);
}


@Test
public void testInvocationChainedWrapped() {
Expand Down Expand Up @@ -388,19 +386,19 @@ public void testInvocationChangeWithTransformMatcher() throws IOException {


@Test
@Ignore("In this case, we find that we can't resolve bindings inside a lambda, where the lambda has any prior unknown types." +
" Here, that means that even though we have the class files for a method invocation, because we're missing the class" +
" files for a type referenced in the lambda, we cannot resolve the method invocation." +
" We need this information for type inference - https://docs.oracle.com/javase/specs/jls/se8/html/jls-18.html" +
" Because we don't have that type information, we don't know the return type, which means we are also missing" +
" the type which could be used as a type parameter, or an argument to a later function - so we can't determine which" +
" of a number of overloaded methods are being called, or if we have met the specific type criteria for a method." +
" The best we could do is match a method which uses 'Object'. In a later release, it's possible that Eclipse JDT Core" +
" may attempt a 'best guess' - but this sounds like it's not planned, and is considered a difficult problem to solve." +
" In this test, the unknown type is 'ASTNode'. If we provided the classpath for org.eclipse.jdt.core, the ASTNode type" +
" would be resolved, the rest of the lambda would be evaluated, the method identified and refactored, and the test would pass." +
" The 'proper' solution for this tool is to run with all the class files provided to the AST - but this makes the tool" +
" take significantly longer to run, due to the extra time taken to generate each file's AST." +
@Ignore("In this case, we find that we can't resolve bindings inside a lambda, where the lambda has any prior unknown types." +
" Here, that means that even though we have the class files for a method invocation, because we're missing the class" +
" files for a type referenced in the lambda, we cannot resolve the method invocation." +
" We need this information for type inference - https://docs.oracle.com/javase/specs/jls/se8/html/jls-18.html" +
" Because we don't have that type information, we don't know the return type, which means we are also missing" +
" the type which could be used as a type parameter, or an argument to a later function - so we can't determine which" +
" of a number of overloaded methods are being called, or if we have met the specific type criteria for a method." +
" The best we could do is match a method which uses 'Object'. In a later release, it's possible that Eclipse JDT Core" +
" may attempt a 'best guess' - but this sounds like it's not planned, and is considered a difficult problem to solve." +
" In this test, the unknown type is 'ASTNode'. If we provided the classpath for org.eclipse.jdt.core, the ASTNode type" +
" would be resolved, the rest of the lambda would be evaluated, the method identified and refactored, and the test would pass." +
" The 'proper' solution for this tool is to run with all the class files provided to the AST - but this makes the tool" +
" take significantly longer to run, due to the extra time taken to generate each file's AST." +
" If 100% accuracy is required, provide all the class files and accept the slow run.")
public void testInvocationInLambdaWithUnknownType() {
assertRefactor(
Expand Down