Bug 57545 - Inline Method/Variable Refactoring
Inline Method/Variable Refactoring
Status: RESOLVED FIXED
Product: java
Classification: Unclassified
Component: Refactoring
6.x
All All
: P2 with 10 votes (vote)
: 7.2
Assigned To: Ralph Ruijs
issues@java
http://refactoring.netbeans.org/refac...
et_25
: PLAN
: 74580 108759 126168 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-04-06 14:27 UTC by Daniel Prusa
Modified: 2011-05-26 07:04 UTC (History)
4 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Proposed patch (11.44 KB, text/plain)
2009-04-29 21:09 UTC, Ralph Ruijs
Details
Implementation of Inline Temp and Inline Field (50.98 KB, text/plain)
2009-08-23 23:25 UTC, Ralph Ruijs
Details
changes per Jans comments (61.59 KB, text/plain)
2009-09-05 18:12 UTC, Ralph Ruijs
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Prusa 2005-04-06 14:27:51 UTC
This issue tracks implementation of Inline Method/Variable refactoring.
Comment 1 Ralph Ruijs 2009-04-29 21:09:37 UTC
Created attachment 81240 [details]
Proposed patch
Comment 2 Jan Pokorsky 2009-08-19 16:01:08 UTC
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.
Comment 3 Jan Pokorsky 2009-08-19 16:11:43 UTC
You can find guide how to contribute a patch at http://www.netbeans.org/community/contribute/patches.html.
Comment 4 Ralph Ruijs 2009-08-19 22:39:19 UTC
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.
Comment 5 Ralph Ruijs 2009-08-23 23:25:19 UTC
Created attachment 86543 [details]
Implementation of Inline Temp and Inline Field
Comment 6 Jan Pokorsky 2009-08-24 16:36:12 UTC
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;
    }
Comment 7 Jiri Kovalsky 2009-08-31 10:28:02 UTC
Ralph, do you think you will have time to address/answer Jan Pokorsky's suggestions/questions? Thanks a lot!
Comment 8 Ralph Ruijs 2009-09-05 18:12:40 UTC
Created attachment 87160 [details]
changes per Jans comments
Comment 9 Ralph Ruijs 2009-09-05 19:24:40 UTC
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.
Comment 10 Jiri Kovalsky 2009-09-10 08:42:46 UTC
Honzo, now it's your turn I guess. ;-) Thanks a lot!
Comment 11 Jan Pokorsky 2009-09-10 13:11:47 UTC
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
    }
Comment 12 Jan Pokorsky 2009-12-02 07:19:21 UTC
*** Bug 74580 has been marked as a duplicate of this bug. ***
Comment 13 Jan Pokorsky 2009-12-02 07:31:11 UTC
*** Bug 108759 has been marked as a duplicate of this bug. ***
Comment 14 catweasle 2010-11-10 13:16:17 UTC
*** Bug 126168 has been marked as a duplicate of this bug. ***
Comment 15 electriq 2011-05-05 08:50:42 UTC
Excuse me, is there any progress here with Inline refactoring since 2009?
Comment 16 Ralph Ruijs 2011-05-23 09:35:00 UTC
Changeset: 6619a82e64d0
Author:    Ralph Benjamin Ruijs <ralphbenjamin@netbeans.org>
Date:      2011-05-23 11:17
Message:   
Issue #57545 - Inline Method/Variable Refactoring
Comment 17 Quality Engineering 2011-05-24 10:34:19 UTC
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
Comment 18 Jiri Kovalsky 2011-05-25 17:11:00 UTC
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?
Comment 19 Jan Becicka 2011-05-26 06:02:34 UTC
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.
Comment 20 Jiri Kovalsky 2011-05-26 07:04:39 UTC
Thanks for your explanation and resolving the issue.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo