[
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 Lars,
Thanks for the link. It was really helpful!
It's been a while since I have had time to look more into this.
But now I have a suggested fix and pushed it to Gerrit
(https://git.eclipse.org/r/140148) and have assigned you as reviewer
as you asked.
I included a test for the fixed bug and all tests where green before I
uploaded the fix.
I discovered that all the white spaces had been changed in one of the
files (one of the automated tests in Gerrit complained about the size
of the changes), so I have tried to fix this and pushed an updated
version up.
I suspect it is eclipse that caused this somehow, although I am unsure
what triggered it.
There are two different bugs that describes the problem:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=285554
https://bugs.eclipse.org/bugs/show_bug.cgi?id=488942
I suppose one of them should be closed as duplicate. Should I do
something to them, or will that be handled by a committer?
Best regards,
Dan N. Christensen
On Mon, 25 Feb 2019 at 09:37, Lars Vogel <lars.vogel@xxxxxxxxxxx> wrote:
>
> 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
> _______________________________________________
> 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
