Community
Participate
Working Groups
Extracted method should be declared static if extracted expression is also used in another static method. Steps to reproduce: 1. In the example below invoke "Extract Method" on "shared()" expression in non-static method (nsm). 2. New method will not be declared as static. 3. There will be a compilation error because non-static method is invoked from static context. Note: Originally the bug was encountered when extracting "getTestConstructor(theClass)" on line 136 in junit.framework.TestSuite, JUnit commit e8b91fa9f797dfe16aff66ed6ad5d6104e5133fe. public class ExtractMethodBug { public static void sm() { // Compilation error will be on the next line shared(); } public void nsm() { // Invoke Extract Method refactoring on the "shared()" expression shared(); } public static void shared() { } }
Created attachment 236358 [details] Proposed fix This patch aims to fix two scenarios 1. Matching expression is found in static method 2. Matching expression is found in super or constructor invocation Also added tests for both the scenarios This contribution complies with http://www.eclipse.org/legal/CoO.php
Created attachment 236371 [details] updated patch I have updated original patch so that the new method is static (based on the duplicates) only if the user selects to replace the duplicates. Also updating the method declaration in preview dialog based on if replace duplicates is selected. This contribution complies with http://www.eclipse.org/legal/CoO.php
Here are the review comments: 1) Always pull changes from Git before creating a patch. The latest patch has a compiler error as it refers to SnippetFinder#isMethodBody(). 2) The patch contains incomplete/part of the fix for bug 394030(in ExtractMethodTests). Take care not to mix changes of different bugs in the same patch. However it is fine to submit a single and complete patch for multiple bugs. 3) There is a utility method ASTResolving #isInStaticContext(ASTNode), make use of this method in SnippetFinder #isForceStatic(). This method will take care of both the scenario you have pointed out in comment #1. Also take a look at JDTUIHelperClasses.java to see the different utility classes available for each purpose. 4) SnippetFinder#isForceStatic() can be better named. e.g. isNodeInStaticContext 5) ExtractMethodRefactoring#forceStatic(), better to exit as soon as a node is found which is in static context. In that case no need to use the boolean variable 'forceStatic'. 6) SnippetFinder #isForceStatic() can be better named. e.g. isNodeInStaticContext 7) Add your name in the contributors list in ExtractMethodRefactoring and SnippetFinder along with the bug details. 8) Changes in ExtractMethodInputPage looks unnecessary. The preview page is updated even without those changes. Also Copyright year is not 'Copyright (c) 2000, 2013, 2012 IBM', instead it should be 'Copyright (c) 2000, 2013 IBM' 9) Add the duplicate tests after test987. e.g test988 and test989
Created attachment 236414 [details] updated patch Using ASTResolving #isInStaticContext(ASTNode)in SnippetFinder. This is definitely a better way to do it. Thanks for the pointer to JDTUIHelperClasses.java. Changes to ExtractMethodInputPage are required to refresh the method signature on preview dialog in case the user selects or de-selects replace duplicates check box. If the user de-selects to replace duplicate which are in static context, we will not create a static method, but the preview dialog will still display it with a static signature.
The above contribution complies with http://www.eclipse.org/legal/CoO.php
(In reply to Samrat Dhillon from comment #4) > Created attachment 236414 [details] [diff] > updated patch > Changes to ExtractMethodInputPage are required to > refresh the method signature on preview dialog in case the user selects or > de-selects replace duplicates check box. If the user de-selects to replace > duplicate which are in static context, we will not create a static method, > but the preview dialog will still display it with a static signature. You are right. Though the actual preview in the second page will be updated, the method signature preview in the first page will not be updated without the change in ExtractMethodInputPage. Released the fix as: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=c34f9c7e211fcf5f1713cdd569cce4fed8a485e2
This should also be handled for "default methods" in Java 8. See bug 406786 comment#3.
This bug did not consider any of the new features in Java 8, hence we did not consider default methods in Interface. I will cherry pick this patch to mmathew/BETA_JAVA8 and provide a fix and upload it as part of bug 406786.
