Bug 158154 - Braces when rewriting IfTree
Braces when rewriting IfTree
Status: RESOLVED FIXED
Product: java
Classification: Unclassified
Component: Source
6.x
All All
: P3 (vote)
: 8.0
Assigned To: Svata Dedic
issues@java
: NETFIX, PATCH_AVAILABLE
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-08 14:27 UTC by tronicek
Modified: 2013-11-01 14:08 UTC (History)
5 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
test case (3.14 KB, text/plain)
2009-05-16 20:51 UTC, fommil
Details
Correct test patch (3.20 KB, text/plain)
2009-05-22 12:17 UTC, Rastislav Komara
Details
test case and proposed fix (3.85 KB, text/plain)
2009-05-23 16:57 UTC, fommil
Details
Diff file with test case and proposed fix (18.66 KB, patch)
2012-09-26 01:51 UTC, vezzoni
Details | Diff
Proposed fix with its test case and rewriting of unit tests (101.68 KB, patch)
2012-11-05 07:35 UTC, vezzoni
Details | Diff
test case and proposed fix (7.04 KB, patch)
2013-03-17 05:14 UTC, vezzoni
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tronicek 2009-02-08 14:27:43 UTC
I am rewriting an IfTree using these statements:

            IfTree original = (IfTree) node;
            modified = make.If(
                    make.Parenthesized(
                        make.Unary(Kind.LOGICAL_COMPLEMENT, original.getCondition())),
                    original.getElseStatement(), null);
            System.out.println("original: " + node);
            System.out.println("modified: " + modified);
            wc.rewrite(node, modified);

For input:

    void m1(boolean b) {
        if (b) ;
        else
            System.out.println("hi");
        if (b) ;
        else
            System.out.println("hi");
    }

I get the output:

    void m1(boolean b) {
        if (!(b)) System.out.println("hi");
        if (!(b)) {
            System.out.println("hi");
        }
    }

That is, braces are added in the second case but they should not be. Even if you decided to add braces (which
is not a good idea in my opinion), it should be consistent. That is, either add or not to add in all cases.

The log:

original: if (b) ; else System.out.println("hi");
modified: if (!(b)) System.out.println("hi");
original: if (b) ; else System.out.println("hi");
modified: if (!(b)) System.out.println("hi");
Comment 1 Rastislav Komara 2009-02-16 10:39:52 UTC
.
Comment 2 fommil 2009-05-16 20:03:54 UTC
I'd like to NetFIX [1] this bug. Is it possible? [1] http://wiki.netbeans.org/NetFIX
Comment 3 fommil 2009-05-16 20:51:08 UTC
Created attachment 82246 [details]
test case
Comment 4 fommil 2009-05-16 20:52:19 UTC
I think this is INVALID. @see attached test case, which passes for me.
Comment 5 Rastislav Komara 2009-05-22 12:16:26 UTC
I consider this issue valid. Your patch for test case does not reflect this issue. I'm adding patch to test which covers
this issue.
Comment 6 Rastislav Komara 2009-05-22 12:17:35 UTC
Created attachment 82629 [details]
Correct test patch
Comment 7 fommil 2009-05-22 12:25:04 UTC
aah! I see the mistake now.
Comment 8 fommil 2009-05-23 16:29:28 UTC
Ok, looks like CasualDiff is looking at these two actions as DELETE, DELETE, INSERT, INSERT rather than MODIFY, MODIFY. The problem is therefore in the code 
that decides what to do, rather than the code that does it... if that makes sense.
Comment 9 fommil 2009-05-23 16:57:25 UTC
Created attachment 82693 [details]
test case and proposed fix
Comment 10 fommil 2009-05-23 17:03:22 UTC
@moonko, I think this bug has actually shed light on something quite fundamentally broken in the diff code.

The Measure.MEMBER was requiring that the Kind and pos of a JTree both be the same before they were considered ALMOST_THE_SAME, whereas 
THE_SAME_KIND was being ignored. I've updated the code to behave more like ARGUMENT. This might have far reaching consequences... so I urge you to run 
all test cases (which is too much for my laptop) to ensure this doesn't inadvertently reveal bugs elsewhere.

An alternative solution could be to find out why the replacement IfTrees have negative pos.

PS: the expected golden is slightly different than in your test case. The whitespace makes sense if you think about it.
Comment 11 fommil 2009-07-03 13:49:19 UTC
Update on status, please?
Comment 12 fommil 2009-07-03 13:56:05 UTC
Update on status, please?
Comment 13 Jan Lahoda 2009-08-20 10:00:24 UTC
Reassigning all moonko's java/source bugs to myself.
Comment 14 Jiri Kovalsky 2009-10-14 13:44:05 UTC
Any update on this Honzo? Thanks!
Comment 15 Jan Lahoda 2009-10-19 14:42:40 UTC
Hm, the patch which directly modifies the MEMBER metric seems to break too many codegen tests. I tried to introduce new
metric, STATEMENT, used in diffBlock. The codegen tests pass with this metric, but still several java.hints tests fails.
In some cases, the new behavior is probably OK (and so the golden output should be changed, e.g. IntroduceHintTest). In
at least one case, however, the output is incorrect - a comment is missing in the output
(AddParameterOrLocalFixTest.testAddLocalVariableWithComments). One of my working patches to issue #170213 was solving a
similar case - it conflicted with some java.hints test too, IIRC.
Comment 16 Jiri Kovalsky 2009-12-03 09:01:20 UTC
OK, so do you propose Honzo that Sam modifies the patch and ask for another review then?
Comment 17 vezzoni 2012-09-21 01:32:39 UTC
Any update of this issue? Can I give a try to fix it?
Comment 18 Jiri Kovalsky 2012-09-21 08:23:42 UTC
Yes, I think you can. Just make sure to address Jan Lahoda's comments.
Comment 19 fommil 2012-09-21 08:38:55 UTC
by all means vezzoni, I am no longer actively contributing to Netbeans – I hope you appreciate the difficulty of this particular bug ;-)
Comment 20 vezzoni 2012-09-21 14:59:21 UTC
ok Jika!

fommil, it's a pity you are no longer contributing with netbeans. I really hope that you come back in the future.

how are you doing Jan? I would like to NetFIX this issue, is it possible?
Comment 21 vezzoni 2012-09-26 01:51:11 UTC
Created attachment 124917 [details]
Diff file with test case and proposed fix

Affected files on Java.Source project:

* org.netbeans.api.java.source.gen.IfTest
  #test158154OneIf()
  #test158154TwoIfs()
  #test158154SeveralIfs()

* org.netbeans.modules.java.source.save.CasualDiff:2893
  #diffList(
      List<? extends JCTree>, 
      List<? extends JCTree>, 
      int, PositionEstimator, Comparator<JCTree>, VeryPretty)
Comment 22 Jan Lahoda 2012-09-26 19:23:12 UTC
Vezzoni, thanks for the patch. Are codegen tests passing with it?
(cd java.source; ant -Dtest.config=generator test)

Moreover, it would be good to check if java.hints tests still pass:
(cd java.hints; ant test)

I can run the tests, but won't likely have time for that in next week or so.
Comment 23 vezzoni 2012-09-26 19:47:57 UTC
Jan, I did not run those executions. but, I did run commit-validation on java.source, as Jirka has taught me.

I'm still learning these tasks.

I'll run those both executions tonight (GMT-3 :o) to ensure the patch.

Thank you for the tip. I'll comment here with the result.
Comment 24 Jiri Kovalsky 2012-10-31 13:01:27 UTC
Roberto, how did the Java Hints test run? Was it okay so that we can review your patch? Thanks for any update.
Comment 25 vezzoni 2012-10-31 13:25:25 UTC
It's broken Jirka, I am still working on that.

The big problem is: the IfTree (CasualDiff) is building different output statements for the same input. 
I can fix this behaviour, but I'll break a lot of JUnit tests. I'm stuck with that.

how many time I still have to deliver this?
Comment 26 vezzoni 2012-11-05 07:35:50 UTC
Created attachment 127113 [details]
Proposed fix with its test case and rewriting of unit tests

Proposed fix with its test case and rewriting of unit tests.

the "ant -Dtest.config=generator test" at java.source was executed with success.
the "ant test -Dignore.random.failures=true" at java.hints was executed with success.
Comment 27 Jan Lahoda 2012-11-12 17:23:18 UTC
Roberto,

As I read the patch, it basically prints the if statements anew. I am sorry, but this is not going to be accepted - it goes against the "smallest changes possible" idea.

Looking into this much deeper, the least incorrect solution may the fommil's one (it is actually enough to change MEMBER to REAL_MEMBER in diffBlock). Some java.hints tests that fail after this change actually produce better code in the updated version, but some don't produce necessarily better variant, e.g. MagicSurroundWithTryCatchFixTest.test204165b. But that may be acceptable if that happens only in rare unrealistic cases.
Comment 28 vezzoni 2013-02-23 19:48:58 UTC
Ok Jan! Understood.

Can I keep going through this bug?
Comment 29 vezzoni 2013-03-17 05:14:48 UTC
Created attachment 132706 [details]
test case and proposed fix
Comment 30 Jiri Kovalsky 2013-03-18 19:11:15 UTC
Honzo, can you please evaluate this?
Comment 31 Svata Dedic 2013-10-23 16:47:21 UTC
(In reply to vezzoni from comment #29)
> Created attachment 132706 [details]
> test case and proposed fix

I evaluated the fix and changed it a little; the fix works OK for replacing several consecutive statements of the same type. However if the last statement from the replaced range was of a different kind, the fix would fail to recognize the DELETE-INSERT pair as a replacement.

The basic idea remains - thanks for providing the patch !
Comment 32 Svata Dedic 2013-11-01 13:57:29 UTC
Fixed in changeset http://hg.netbeans.org/jet-main/rev/1aa96c213370
Comment 33 Svata Dedic 2013-11-01 13:58:55 UTC
Oops. Wrong changeset. Should be http://hg.netbeans.org/jet-main/rev/7214900c79dd
Comment 34 vezzoni 2013-11-01 14:08:34 UTC
(In reply to Svata Dedic from comment #31)

Thank you very much for your time for this evaluation, Svata!
I'm glad I was helpful.


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