Community
Participate
Working Groups
In fact Extract Method, could find simple aliasing example : <pre> class A { private Object o; void foo() { Object m = o; call(m, this.o); } } </pre> If we call Extract Method on "this.o" by replacing it with "getO", it does not recognize that "this.o" and "o" are very simple aliases. In fact, even "m" is an alias of "this.o" so we could add a check box to find all the aliases +-------------------------------------------------------------+ | Method name : |___________________________________________| | | | | Access modifier : O public O protected O default O private | | _ | | |_| Add thrown runtime exceptions to method signature | | _ | | |_| Generate Javadoc comment | | _ | | |v| Replace XXX duplicate code fragement | | _ | | |_| Replace aliases | | | +-------------------------------------------------------------+ and the result could be in one shot <pre> class A { private Object o; void foo() { Object m = getO(); call(getO(), getO()); } Object getO() { return this.o; } } </pre>
Nice suggestion. But given the fact that M9 is our last milestone this has to wait for > 3.0.
reopen for 3.1
Mohamed, please don't reopen bugs marked as resolved later. We reopen them when we plan fixing them for a certain milestone.
Markus, I think this is something worth to reopen. I don't think the case with m is the goal but at least common aliases like these could be handled: package p; class A { int x; void f() { // from doSomething(x); // to doSomething(this.x); // should be replaced doSomething(A.this.x); // should be replaced } private void doSomething(int x) { } }
I agree. Full alias handling is too hard, but the case with this.x and A.this.x is not even aliasing but just another way to reference the same variable.
Benny, please have a look if this cold be done without complicating the duplicates finding code severely.
Hello, I tested comparing SimpleName and FieldAccess locally in my workspace and it worked. I think using the following code for the Matcher in org.eclipse.jdt.internal.corext.refactoring.code.SnippetFinder should work. Could you please give it a try? Thanks, Jean-Noel private class Matcher extends ASTMatcher { @Override public boolean match(SimpleName candidate, Object s) { if (s instanceof SimpleName) return match(candidate, (SimpleName) s); } else if (s instanceof FieldAccess) { return match(candidate, (FieldAccess) s); } return false; } public boolean match(SimpleName candidate, FieldAccess snippet) { IBinding cb= candidate.resolveBinding(); IBinding sb= snippet.resolveFieldBinding(); if (cb == null || sb == null) return false; return Bindings.equals(cb, sb); } public boolean match(SimpleName candidate, SimpleName snippet) { if (candidate.isDeclaration() != snippet.isDeclaration()) return false; IBinding cb= candidate.resolveBinding(); IBinding sb= snippet.resolveBinding(); if (cb == null || sb == null) return false; IVariableBinding vcb= ASTNodes.getVariableBinding(candidate); IVariableBinding vsb= ASTNodes.getVariableBinding(snippet); if (vcb == null || vsb == null) return Bindings.equals(cb, sb); if (!vcb.isField() && !vsb.isField() && Bindings.equals(vcb.getType(), vsb.getType())) { SimpleName mapped= fMatch.getMappedName(vsb); if (mapped != null) { IVariableBinding mappedBinding= ASTNodes.getVariableBinding(mapped); if (!Bindings.equals(vcb, mappedBinding)) return false; } fMatch.addLocal(vsb, candidate); return true; } return Bindings.equals(cb, sb); } }
(In reply to comment #7) I forgot to say: sorry if I did not not follow proper formatting rules. Also, I was wondering about this code, do we need to cater for IVariableBindings and put them in a Match object?
> I forgot to say: sorry if I did not not follow proper formatting rules. Formatting is fine, but syntax errors are not ;-) While this code maybe fixes one specific situation, it doesn't look complete. We'd need test cases that confirm that it e.g. - also works the other way 'round (if you extract "o") - doesn't find wrong matches if the FieldAccess' expression is not "this"
(In reply to comment #9) Oopsie, I just saw the error now. Sorry about that. What format do you expect for the tests? Can you point out some examples to me on GitHub?
Tests are here in git://git.eclipse.org/gitroot/jdt/eclipse.jdt.ui.git : /org.eclipse.jdt.ui.tests.refactoring/test%20cases/org/eclipse/jdt/ui/tests/refactoring/ExtractMethodTests.java Test data is in /org.eclipse.jdt.ui.tests.refactoring/resources/ExtractMethodWorkSpace/ExtractMethodTests/
(In reply to comment #11) Ok, this is a bit tricky due to how the code is built. - When extracting code with SimpleNames like "o", it looks easy enough to ensure "this.o" and "A.this.o" would be extracted too. I could not test it or even ensure no regression was introduced (see below). However I have written tests that ensure what must be extracted is extracted and what must not be extracted is not extracted. - When extracting code with QualifiedName/FieldAccess like "this.o", the FlowAnalyzer does not classify them as local variables and subsequently, the ExtractMethodAnalyzer does not offer to replace the QualifiedName/FieldAccess with method arguments. I have reworked a bit the Matcher's code, but I have several issues: - I did not manage to set up everything to compile without errors in Eclipse for org.eclipse.jdt.ui and org.eclipse.jdt.ui.tests.refactoring - Subsequently, I cannot make the tests pass locally. Is there an article/wiki page describing how to do it properly? It would be nice if I managed to set this up, I could be a bit more autonomous :)
(In reply to comment #12) > (In reply to comment #11) > Is there an article/wiki page describing how to do it properly? > It would be nice if I managed to set this up, I could be a bit more > autonomous :) Maybe this helps: http://wiki.eclipse.org/JDT_UI/How_to_Contribute
(In reply to comment #13) I tried to do this, it helped a bit, but I still need to manually add all the external JARs and projects into the build path of all these projects. When I got no compile errors, I ran ExtractMethodTests but got: "Exception occurred executing command line. Cannot run program "C:\Program Files\Java\jre7\bin\javaw.exe" (in directory "C:\Users\jnrouvignac\workspace\jdt\eclipse.jdt.ui\org.eclipse.jdt.ui.tests.refactoring"): CreateProcess error=206, The filename or extension is too long" I tried to remove the unnecessary external JARs as much as I could, but started hitting compile errors again. After I was done with reducing the classpath, I was still getting the "CreateProcess error=206, The filename or extension is too long" error. My contributing experience has been a bit miserable so far. Please tell me I am doing something wrong and there is a better way to do it.
(In reply to comment #14) > (In reply to comment #13) > I tried to do this, it helped a bit, but I still need to manually add all > the external JARs and projects into the build path of all these projects. Mmh, no, that is not needed. Just import the required test projects from Git and PDE will resolve the stuff. What "external JARs" are you talking about?
(In reply to comment #15) Thanks Dani for coming back to me. So it should not work like that. I am adding the Eclipse Juno JARs as "External JARs" into each projects where classes cannot be recognized by the compiler. This is not an enjoyable process :( I am using Eclipse Juno SR1, and I can see I hit bug 327193 :( . I am now trying with Eclipse 4.3M3, but I am now hitting another issue: java.lang.IllegalStateException: Workspace is closed. at org.eclipse.core.resources.ResourcesPlugin.getWorkspace(ResourcesPlugin.java:399) at org.eclipse.jdt.internal.ui.util.CoreUtility.setAutoBuilding(CoreUtility.java:205) at org.eclipse.jdt.ui.tests.refactoring.infra.AbstractRefactoringTestSetup.setUp(AbstractRefactoringTestSetup.java:40) at org.eclipse.jdt.ui.tests.refactoring.RefactoringTestSetup.setUp(RefactoringTestSetup.java:67) at org.eclipse.jdt.ui.tests.refactoring.ExtractMethodTestSetup.setUp(ExtractMethodTestSetup.java:50) What version are you using for development? Without this working, I cannot test that my code is working properly with the test suite. Is it ok if I send you a patch with the tests and the code I changed?
(In reply to comment #16) > (In reply to comment #15) > > Thanks Dani for coming back to me. So it should not work like that. > > I am adding the Eclipse Juno JARs as "External JARs" into each projects > where classes cannot be recognized by the compiler. This is not an enjoyable > process :( Indeed and it is wrong. Simply import the project from Git. > I am using Eclipse Juno SR1, and I can see I hit bug 327193 :( . I am now > trying with Eclipse 4.3M3, but I am now hitting another issue: > java.lang.IllegalStateException: Workspace is closed. > at > org.eclipse.core.resources.ResourcesPlugin.getWorkspace(ResourcesPlugin.java: > 399) > at > org.eclipse.jdt.internal.ui.util.CoreUtility.setAutoBuilding(CoreUtility. > java:205) > at > org.eclipse.jdt.ui.tests.refactoring.infra.AbstractRefactoringTestSetup. > setUp(AbstractRefactoringTestSetup.java:40) > at > org.eclipse.jdt.ui.tests.refactoring.RefactoringTestSetup. > setUp(RefactoringTestSetup.java:67) > at > org.eclipse.jdt.ui.tests.refactoring.ExtractMethodTestSetup. > setUp(ExtractMethodTestSetup.java:50) You probably didn't start the test as a JUnit Plug-in Test. > What version are you using for development? I run on the latest I-builds with source from Git master branch. > Is it ok if I send you a patch with the tests and the code I changed? Without them having passed? Preferably not ;-).
Created attachment 224492 [details] Code fix and test cases (In reply to comment #17) I finally found what I was missing: Setting up the API baseline. After that it worked very well. Thank you very much Dani. I am now in a position to submit a patch with several test cases. Please let me know what you think of it.
Created attachment 224493 [details] Code fix and test cases included I just noticed I forgot to include the test cases. Sorry about that, I am new to Git.
Hi, did anybody have a look at this patch? Can I have some feedback?
(In reply to comment #20) > Hi, did anybody have a look at this patch? Can I have some feedback? We're low on resources. There will be feedback but it just takes time.
(In reply to comment #21) > (In reply to comment #20) > > Hi, did anybody have a look at this patch? Can I have some feedback? > > We're low on resources. There will be feedback but it just takes time. Hi Dani, Thank you for answering. I have seen several emails about this resourcing problem on the mailing list. It is very sad JDT does not have the resources this project deserves. I am sending a last reminder about this patch. Is there still no badwidth at all to review this patch currently? Do you have an expected timeframe when this review will happen or when the resourcing problem will end? In all cases, I'll just wait for the review from now on. Thank you for your patience and effort. Jean-Noel
(In reply to comment #19) All the tests have compile errors. We're mainly focusing on compiling code, so the tests should reflect that. Note that you can create an Eclipse Application launch configuration with location "${workspace_loc:org.eclipse.jdt.ui.tests.refactoring/resources/ExtractMethodWorkSpace}". Then you can launch this workspace, choose File > Import > Existing projects, and import the test project into the runtime workspace. The tests should go to the "duplicates_*" package (not "validSelection_*"). The SnippetFinder.Matcher grew from a simple linear matcher into a huge beast I don't really understand any more. And that means, I can't release it like this. The various "isSame" methods should get names that explain what "sameness" they are looking for (one is comparing identifiers, and I think another one is trying to find out whether the arguments belong to the same "this" object). We also need some comments that explain why the implementation looks like that and why this is a correct and complete solution. You should also have another look at the places where you deal with TypeDeclaration nodes. I'm pretty sure you should use AbstractTypeDeclaration instead.
(In reply to comment #23) > (In reply to comment #19) > All the tests have compile errors. We're mainly focusing on compiling code, > so the tests should reflect that. Sorry about that, I'll resubmit them. BTW, some existing tests also have compile errors. I'll submit a patch for them too. > The tests should go to the "duplicates_*" package (not "validSelection_*"). OK. > The various "isSame" methods should get names that explain what > "sameness" they are looking for (one is comparing identifiers, and I think > another one is trying to find out whether the arguments belong to the same > "this" object). What about isSameName(), isAlias() or isSimpleAlias() (as per the bug summary)? > We also need some comments that explain why the > implementation looks like that and why this is a correct and complete > solution. I think it is best to use the existing SnippetFinder.Matcher class because it provides all the matches for all the other cases (and there are a lot of them) and it allow us to only tweak the places we need to (SimpleName, QualifiedName and FieldAccess). However I agree it does not look as pretty as it used too. Do you want to move this type to its own file? Or do you want to push some methods to utility classes? Or do you have better suggestions? > You should also have another look at the places where you deal with > TypeDeclaration nodes. I'm pretty sure you should use > AbstractTypeDeclaration instead. You're right. I'll do it.
Created attachment 227226 [details] Eclipse JDT UI: fix compilation errors for existing tests
