Bugzilla – Full Text Bug Listing
| Summary: | [extract method] Extract method with 'replace additional occurrences' does not analyze additional occurrences | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Johannes Mueller <quirksquarks> | ||||
| Component: | UI | Assignee: | Noopur Gupta <noopur_gupta> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | daniel_megert, markus.kell.r, noopur_gupta, ruediger.herrmann, smikkelsen | ||||
| Version: | 3.8.1 | ||||||
| Target Milestone: | 4.7 M7 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| See Also: |
https://git.eclipse.org/r/87869 https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=005865cacb326191f6eb048893d793742f289683 |
||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
Hi, It's been some time. Can you reproduce the bug or is there something wrong with my description? Please ask if I should provide additional information. I can reproduce the bug with Eclipse 3.8.1 also. The extracted method should return the variable "idx", like it does when we add the statement "System.out.println(idx);" after second for loop. *** Bug 488942 has been marked as a duplicate of this bug. *** It has been a year, and there seems to be no visible progress. With the bug, the feature is no longer a refactoring in the sense that it maintains the original functionality. Moreover, it can introduce Bugs in production code. Guessing that this is a valid reason, I set the importance to major. It would be safer to disable the feature or at least give a fair warning about potential problems until the issue is solved. *** Bug 507105 has been marked as a duplicate of this bug. *** Created attachment 265525 [details]
Test Class
In the attached test class, extract first three lines of code in each method. The return value is expected to be different in each case and will cause issues with other invocations of the extracted method.
The problem is that we perform the flow analysis on the selected code only and the return value is defined based on that.
The fix should perform the analysis separately on all the duplicate code snippets also and compare their return values. In the case of different return values (not return types - see c() and d() in attached test class), "replace additional occurrences" should be disabled.
New Gerrit change created: https://git.eclipse.org/r/87869 (In reply to Eclipse Genie from comment #7) > New Gerrit change created: https://git.eclipse.org/r/87869 Uploaded WIP patch set 1, which identifies invalid duplicates differing in the return value, based on the return value node's location in the parent. It results in four test failures in ExtractMethodTests (test957, test 983, test984, test987) which seem to be valid failures. So the approach to identify invalid duplicates should be corrected. Markus, please have a look and provide suggestions on how to proceed. You're pretty close. To fix test957, the algorithm just needs to detect the case where the variables that are to be returned are declared outside of the snippet/duplicate to be extracted. In that case, the duplicate finder should already ensure that it's OK to extract the duplicate. I've updated the Gerrit with that change (code is still WIP-quality and may need cleanup). The other three test failures reveal a bug in the ExtractMethodAnalyzer. Analysis for test983: Extracting the "x" in statement "x[i]= 1;" should be allowed, since x is not the left hand of the assignment. It's *in* the left hand of an assignment, but that's OK. It doesn't make sense to only allow extracting x if it is found as a duplicate while extracting another reference to x. For the example in comment 0, it would actually be nice to have the refactoring find the duplicate code. If you add another System.out.println(idx); after to for-loop to extract, or if you extract the first for-loop, the refactoring already works fine and uses the return value of the extracted method to update the idx variable. a() and b() in the snippet from comment 6 are the same case. Implementing that is a bit trickier and should probably left as a separate enhancement request: In case the extracted snippet returns void but a structural duplicate returns a value, we could create an extracted method that returns that value. That would also mean the return type of the extracted method would have to change depending on whether duplicates should be extracted or not. However, you should fix the other way 'round: In comment 0, you can extract the first for-loop, and the second one is correctly detected as duplicate. This is broken in the Gerrit both with and without my changes. I've added some code style suggestions in the Gerrit. (In reply to Markus Keller from comment #9) > You're pretty close. To fix test957, the algorithm just needs to detect the > case where the variables that are to be returned are declared outside of the > snippet/duplicate to be extracted. In that case, the duplicate finder should > already ensure that it's OK to extract the duplicate. I've updated the > Gerrit with that change (code is still WIP-quality and may need cleanup). Thanks, Markus. > The other three test failures reveal a bug in the ExtractMethodAnalyzer. > Analysis for test983: Extracting the "x" in statement "x[i]= 1;" should be > allowed, since x is not the left hand of the assignment. It's *in* the left > hand of an assignment, but that's OK. It doesn't make sense to only allow > extracting x if it is found as a duplicate while extracting another > reference to x. I have changed the code in ExtractMethodAnalyzer to fix the issues causing these three test failures. Also made sure that the fixes for the other bugs which had code changes in the same area are still valid. Listing those bugs here for reference: bug 203260, bug 264606, bug 287374, bug 366281. > For the example in comment 0, it would actually be nice to have the > refactoring find the duplicate code. If you add another > System.out.println(idx); after to for-loop to extract, or if you extract the > first for-loop, the refactoring already works fine and uses the return value > of the extracted method to update the idx variable. a() and b() in the > snippet from comment 6 are the same case. > > Implementing that is a bit trickier and should probably left as a separate > enhancement request: In case the extracted snippet returns void but a > structural duplicate returns a value, we could create an extracted method > that returns that value. That would also mean the return type of the > extracted method would have to change depending on whether duplicates should > be extracted or not. I will create a separate enhancement request for this. > However, you should fix the other way 'round: In comment 0, you can extract > the first for-loop, and the second one is correctly detected as duplicate. > This is broken in the Gerrit both with and without my changes. This is still pending. I will have a look. Also, need to add tests to the patch. > I've added some code style suggestions in the Gerrit. Thanks, I have taken care of them in patch set 4. Gerrit change https://git.eclipse.org/r/87869 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=005865cacb326191f6eb048893d793742f289683 (In reply to Noopur Gupta from comment #10) > I will create a separate enhancement request for this. Created bug 516372. > > However, you should fix the other way 'round: In comment 0, you can extract > > the first for-loop, and the second one is correctly detected as duplicate. > > This is broken in the Gerrit both with and without my changes. > > This is still pending. I will have a look. > > Also, need to add tests to the patch. Done. (In reply to Eclipse Genie from comment #11) > Gerrit change https://git.eclipse.org/r/87869 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=005865cacb326191f6eb048893d793742f289683 Discussed with Dani and released for M7. |
Extract method produces an incorrect result. If you extract the marked for-loop in the following class, the program changes. But as it is a refactoring it should stay the same. public class Test { public static void main(String[] args) { for(int i=0; i<1; i++) { int idx = 0; for (int j = 0; j < 3; j++) { idx++; System.out.println(idx); } // for-loop to extract: for (int j = 0; j < 3; j++) { idx++; System.out.println(idx); } } } }