This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
This issue tracks implementation of Inline Method/Variable refactoring.
Created attachment 81240 [details] Proposed patch
ralphbenjamin: Thank you for contributing. I cannot find any implementation of InlineAction.java, InlineRefactoring.java, InlineRefactoringPanel.java, InlineRefactoringUI.java and InlineRefactoringPlugin.java in your patch. It seems to contain only the integration part.
You can find guide how to contribute a patch at http://www.netbeans.org/community/contribute/patches.html.
Oops. sorry. I will look up the code and attach it again. I did switch to a new macbook, so could take some time to find it.
Created attachment 86543 [details] Implementation of Inline Temp and Inline Field
Thank you Ralph. Here are some comments. Specification: JP1) the spec describes that allowed elements are methods, static final fields or local variables. I have found that you support whatever field and variables. Are you going to implement support for methods too? JP2) I am not sure whether refactoring of not final fields is safe. You can easily change semantics of the refactored code. JP3) if no usage is found the refactoring should stop. Now it removes the declaration. InlineRefactoring: JP4) replace Tree with TreePathHandle. JavaSource API does not allow to touch Tree or Element instances outside of a java source's task. It would be necessary to mention that requirement in javadoc but it would be inconsistent with other refactorings. JP5) setName/getName - are these methods really necessary as API? InlineRefactoringPlugin: JP6) replace CancellableTask with Task. It does not make sense in user action tasks as those are never canceled. JP7) Tree.Kind and ElementKind are enums, you can use == instead of equals InlineRefactoringUI and others: JP8) new MessageFormat(NbBundle.getMessage(InnerToOuterAction... is not necessary. NbBundle.getMessage(InnerToOuterAction.class, "DSC_Inline", refactoring.getName()) does the same. JP9) refactoring.getName() may be null in InlineRefactoringUI.getDescription() InlineTransformer: JP10) replaceUsageIfMatch: use generics please for Iterator iter = workingCopy.getElementUtilities().getMembers... Could you attach test cases that are covered by your implementation please? I would like to add them as a functional or unit tests. I have tried several cases that fails: private int f = 3; void f() { f++; int a = ++f; int b = a * 3; f(a); } after inlining of 'f' I get void f() { 3++; // syntax and semantic error int b = ++f * 3; f(++f); // semantic error } which is something else. Or arrays private int[] fa = null; void f(int x) { fa[0] = 1; int[] a = fa; } after inlining of 'fa' I get void f(int x) { null[0] = 1; int[] a = null; }
Ralph, do you think you will have time to address/answer Jan Pokorsky's suggestions/questions? Thanks a lot!
Created attachment 87160 [details] changes per Jans comments
JP1) the spec describes that allowed elements are methods, static final fields or local variables. I have found that you support whatever field and variables. Are you going to implement support for methods too? I want to implement Inline Method too, but first wanted to finish Inline Temp and Inline Field. JP2) I am not sure whether refactoring of not final fields is safe. You can easily change semantics of the refactored code. When using an automated refactoring, you should be able to do the exact opposite later. Inlining of not final fields is safe most of the time, for the others there should be checks. Do you have a test case for this? JP3) if no usage is found the refactoring should stop. Now it removes the declaration. Changed InlineRefactoring: JP4) replace Tree with TreePathHandle. JavaSource API does not allow to touch Tree or Element instances outside of a java source's task. It would be necessary to mention that requirement in javadoc but it would be inconsistent with other refactorings. Changed JP5) setName/getName - are these methods really necessary as API? Changed the name to elementName. I think this is the best place for the elementName. InlineRefactoringPlugin: JP6) replace CancellableTask with Task. It does not make sense in user action tasks as those are never canceled. Changed JP7) Tree.Kind and ElementKind are enums, you can use == instead of equals Changed InlineRefactoringUI and others: JP8) new MessageFormat(NbBundle.getMessage(InnerToOuterAction... is not necessary. NbBundle.getMessage(InnerToOuterAction.class, "DSC_Inline", refactoring.getName()) does the same. Changed JP9) refactoring.getName() may be null in InlineRefactoringUI.getDescription() Can you give me some more info on this one? InlineTransformer: JP10) replaceUsageIfMatch: use generics please for Iterator iter = workingCopy.getElementUtilities().getMembers... Changed Could you attach test cases that are covered by your implementation please? I would like to add them as a functional or unit tests. I have tried several cases that fails: I fixed the failing cases by fixing the check for multiple assignments. Also added a check for operator precedence.
Honzo, now it's your turn I guess. ;-) Thanks a lot!
Please do not remove my user name I do not get notification about new comments then. Thanks! We have already discussed next steps by email together with Ralph. I am proposing to create a new module in some contrib repository that would be published via the autoupdate server until the feature is ready and stable. Later it could be integrated into the refactoring.java module. ad JP2) cases when the field is updated in different methods. Not final fields usually hold some state which is lost with the inlining thats the case of most not final fields IMO. But if there are proper checks then it would work for remaining cases. ad JP5) I would remove set/getElementName from API at all. It is useless for clients as it is used just by your UI or plugin implementation. The same seems to apply to set/getElementBody. ad JP9) there is a potential NPE in InlineRefactoringUI.geDescription as it is not guaranteed that InlineRefactoring.elementName is initialized by the plugin as it is wrote now. Otherwise the patch looks fine. Some cases still fail. Use the previous example private int f = 3; void f() { f++; int a = ++f; int b = a * 3; f(a); } and inline 'a'. You get void f() { f++; int b = ++f * 3; f((++f)); // redundant parenthesis + semantic error, you end up with f == 6 instead of expected f == 5 } Or the array example private int[] fa = new int[]{0}; void f(int x) { fa[0] = 1; int[] a = fa; ma(fa); } void ma(int[] a) { fa[0] = 3; } after inlining 'fa' you get void f(int x) { new int[]{0}[0] = 1; // error; this should be removed int[] a = new int[]{0}; // error; new int[]{1} expected ma(new int[]{0}); // error; another array created } void ma(int[] a) { new int[]{0}[0] = 3; // error; this should stop refactoring; wrong declaration }
*** Bug 74580 has been marked as a duplicate of this bug. ***
*** Bug 108759 has been marked as a duplicate of this bug. ***
*** Bug 126168 has been marked as a duplicate of this bug. ***
Excuse me, is there any progress here with Inline refactoring since 2009?
Changeset: 6619a82e64d0 Author: Ralph Benjamin Ruijs <ralphbenjamin@netbeans.org> Date: 2011-05-23 11:17 Message: Issue #57545 - Inline Method/Variable Refactoring
Integrated into 'main-golden', will be available in build *201105240400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main/rev/6619a82e64d0 User: Ralph Benjamin Ruijs <ralphbenjamin@netbeans.org> Log: Issue #57545 - Inline Method/Variable Refactoring
So, has this RFE been implemented by Ralph or not? Honzo, have you approved the final patch to go in? Why is this issue open and why is it marked as planned for 7.1?
Yes. The feature is already implemented by Ralph. I did the review and approved this patch. The feature was planned for 7.1 and now I'm closing is at implemented.
Thanks for your explanation and resolving the issue.