[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [jdt-dev] Is this the right way to fix [extract method] bug?
|
Hi Dan,
thanks for looking into this and also for your dedication to free and
open software.
I'm one of the Eclipse developers but I'm not a JDT committer so I
will only comment on the general approach not at your code suggestion
IMHO the best way of sharing your work and getting feedback from the
committers is by providing a Gerrit change request. This will also
allow the committers to look at your suggestion in the context of the
existing code and will automatically run all existing tests.
So please convert your analysis into a Gerrit code review. Ffor
details on the setup please see
https://www.vogella.com/tutorials/EclipsePlatformDevelopment/article.html#exericse-eclipse-user-creation-and-gerrit-server-configuration
The setup is relatively easy and typically only takes 10 minutes. If
you face issues with the Gerrit setup please feel free to reach out to
me via private email.
Once you created the Gerrit please add me as reviewer so that I can do
a quick initial check if the change looks correct.Adding a new test
for the new supported scenario is typically helpful for the
committers, if that is possible for you.
Best regards, Lars
On Sun, Feb 24, 2019 at 3:10 PM Dan N. Christensen <dan@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> I currently use Eclipse as a Java IDE at work and have run into a
> problem with the extract method refactoring not always giving the
> result I would expect.
>
> More specifically I ran into a variant of:
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=285554
> where extracting a statement from inside a nested loop do not properly
> account for side effects, thereby causing changed functionality.
>
> I had a discussion with a couple of coworkers at work when I hit this
> bug, and they all said that they preferred to use IntelliJ for
> refactoring work, as they had experienced the Eclipse refactoring
> tools as too unreliable and that the IntelliJ refactorings works much
> better.
> I am however not that keen on switching to a proprietary solution, so
> I have decided to try to figure out how to fix this bug as a hobby
> project. This is my first venture into eclipse plugin programming. If
> it becomes a success I might try to take on some more bug-fixing in
> the refactoring code.
>
> Currently I have gotten to the point where I think I know why it
> fails. But before I venture further into trying to fix the bug, I
> would like to hear if someone experienced with the refactoring code
> can verify that I am on the right track.
>
> The problem is exemplified as to extract the innermost statement
> 'list.add(a++);' in the following method:
>
> void foo(){
> ArrayList<Integer> list = new ArrayList<Integer>();
> for (Integer var: list) {
> int a = 0;
> for (int count = 0; count < 10; count++) {
> list.add(a++);
> }
> }
> System.out.println(list.toString());
> }
>
> If there is only one loop the extracted method will (correctly) return
> the modified value of the 'a' parameter and assign it to 'a' in the
> inner loop.
> In the above example however, the extracted method returns void and
> the side effect (incrementing 'a') is lost.
>
> I have narrowed the differences between the working and not working
> case down to the result of the InputFlowAnalyzer as called from
> ExtractMethodAnalyzer.computeOutput().
> In the nested loop case, where it fails, it does not properly account
> for the read of the local variable in the inner loop, which leads to
> the (wrong) conclusion it is not necessary to return the modified
> value from the extracted method.
>
> So the problem is that the InputFlowAnalyzer class does not pick up
> all variable uses from nested loops.
>
> This in turn seems to be due to the way the analysis of loops is
> separated out into a separate AST visitor called
> LoopReentranceVisitor.
> When InputFlowAnalyzer visits the first (outermost) loop it creates an
> instance of the LoopReentranceVisitor:
>
> public boolean visit(ForStatement node) {
> createLoopReentranceVisitor(node);
> return super.visit(node);
> }
>
> private void createLoopReentranceVisitor(ASTNode node) {
> if (fLoopReentranceVisitor == null && fDoLoopReentrance &&
> fSelection.coveredBy(node))
> fLoopReentranceVisitor= new
> LoopReentranceVisitor(fFlowContext, fSelection, node);
> }
>
> When visiting the nested loop the above method does nothing as we
> already have a fLoopReentranceVisitor.
> When we are finished traversing the nested loop we also do nothing in
> endVisit() as this is not the outermost loop
> (fLoopReentranceVisitor.getLoopNode() != node):
>
> public void endVisit(ForStatement node) {
> super.endVisit(node);
> handleLoopReentrance(node);
> }
>
> private void handleLoopReentrance(ASTNode node) {
> if (fLoopReentranceVisitor == null ||
> fLoopReentranceVisitor.getLoopNode() != node)
> return;
>
> fLoopReentranceVisitor.process(node);
> GenericSequentialFlowInfo info= createSequential();
> info.merge(getFlowInfo(node), fFlowContext);
> info.merge(fLoopReentranceVisitor.getFlowInfo(node), fFlowContext);
> setFlowInfo(node, info);
> }
>
> When we then finish the traversal of the outermost loop the
> handleLoopReentrance() function will make the fLoopReentranceVisitor
> traverse the loop too. This visitor now traverses the loops, handling
> both the innermost and the outermost loops and attaches the flow info
> results to the respective loop nodes.
> But there is nothing that propagates/merges this result from the
> nested loop node upwards in LoopReentranceVisitor, so that part of the
> analysis is lost. Only the analysis of the outermost loop is merged
> with the result of InputFlowAnalyzer's traversal of the sub-tree (in
> handleLoopReentrance above) and is propagated upwards by
> InputFlowAnalyzer when finishing visiting the parent nodes.
>
> I tried (as an experiment to verify my understanding) to remove the
> "fLoopReentranceVisitor.getLoopNode() != node" part of the
> conditioning in handleLoopReentrance() so that the
> fLoopReentranceVisitor got called for both the inner and the outer
> loops and the info also merged while InputFlowAnalyzer is visiting the
> nested loop. The result was that InputFlowAnalyzer propagated the
> results upwards and this did in fact fix the bug, although it is not
> the right solution, as it causes us to traverse the tree too many
> times.
>
> The problem is with all the loop constructs (for, enhanced-for, do and
> while) as they are all handled the same way.
>
> The main problem with the current solution as I see it, is that we
> traverse the outermost loop twice with two different visitors, which
> prevents the first visitor from picking up the results from the second
> visitors handling of sub-nodes (nested loops) as the first visitor has
> already finished the traversal of these.
>
> I think the best solution is to move the analysis of the loops into
> InputFlowAnalyzer and drop the separate LoopReentranceVisitor so the
> outermost loop is only traversed once. Then we simply need to set a
> reentry flag, while traversing the outer loop body, so we know we are
> in a nested situation when handling inner loop(s).
>
> This also makes the code easier to understand, as all node types will
> be handled at the same level, instead of some being handled in a
> separate visitor class. And it removes the complexities of having to
> understand how the two traversals work together.
>
> Do you agree in my analysis and do you agree in my proposed way to
> solve the problem, or would you rather have it addressed in a
> different way?
>
> Best regards,
> Dan N. Christensen
> _______________________________________________
> jdt-dev mailing list
> jdt-dev@xxxxxxxxxxx
> To change your delivery options, retrieve your password, or unsubscribe from this list, visit
> https://www.eclipse.org/mailman/listinfo/jdt-dev
--
Eclipse Platform project co-lead
CEO vogella GmbH
Haindaalwisch 17a, 22395 Hamburg
Amtsgericht Hamburg: HRB 127058
Geschäftsführer: Lars Vogel, Jennifer Nerlich de Vogel
USt-IdNr.: DE284122352
Fax (040) 5247 6322, Email: lars.vogel@xxxxxxxxxxx, Web: http://www.vogella.com
