Bug 328481 - [quick fix] Adjust quick fixes for unused variable/field/param to improved problem detection
Summary: [quick fix] Adjust quick fixes for unused variable/field/param to improved pr...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.7   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 3.7 M3   Edit
Assignee: Deepak Azad CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-22 10:44 EDT by Dani Megert CLA
Modified: 2010-10-28 02:07 EDT (History)
3 users (show)

See Also:
markus.kell.r: review+


Attachments
fix + tests (8.09 KB, patch)
2010-10-27 09:15 EDT, Deepak Azad CLA
no flags Details | Diff
fix + tests (13.16 KB, patch)
2010-10-27 10:55 EDT, Deepak Azad CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2010-10-22 10:44:06 EDT
We need to adjust our quick fixes for unused variable/field/param to the improved problem detection, see bug 185682.

This means we not only remove the declaration but also the usages of the variables.
Comment 1 Stephan Herrmann CLA 2010-10-23 06:37:16 EDT
Aside from extending the quickfixes' action, test 
LocalCorrectionsQuickFixTest.testUnusedVariables7() obviously needs
adjusting, since this comment:
  buf.append("        e+=\"\";\n");//makes e an used variable
is no longer true. For an easy fix I suggest changing to s.t. like
  buf.append("        e= e.toUpperCase();\n");//makes e an used variable
(in all three contents strings).
Comment 2 Dani Megert CLA 2010-10-23 06:39:15 EDT
(In reply to comment #1)
> Aside from extending the quickfixes' action, test 
> LocalCorrectionsQuickFixTest.testUnusedVariables7() obviously needs
> adjusting, since this comment:
>   buf.append("        e+=\"\";\n");//makes e an used variable
> is no longer true. For an easy fix I suggest changing to s.t. like
>   buf.append("        e= e.toUpperCase();\n");//makes e an used variable
> (in all three contents strings).

Yep, I'm about to fix the JUnit failures in the latest build.
Comment 3 Deepak Azad CLA 2010-10-27 09:15:18 EDT
Created attachment 181829 [details]
fix + tests

(In reply to comment #0)
> This means we not only remove the declaration but also the usages of the
> variables.
Patch fixes these.
Comment 4 Markus Keller CLA 2010-10-27 10:10:07 EDT
The fix goes into the right direction, but there are some cases that are not handled correctly:

    void foo(boolean b) {
        for (int i= 0; ; ) {
            if (b)
                i++;
            System.out.println("hi");
            i-= 18;
        }
    }
    
    private int fField;
    void bar() {
        new Snippet().fField++;
    }

See RemoveUnusedMemberOperation#removeVariableWithInitializer(..) for existing code that solves such problems when processing assignments.
Comment 5 Markus Keller CLA 2010-10-27 10:19:45 EDT
>         new Snippet().fField++;

Sorry, that's also not handled correctly for assignments, e.g.:
        new Snippet().fField= 12;

Please fix if easily possible, otherwise make a new bug for such problems.
Comment 6 Deepak Azad CLA 2010-10-27 10:55:26 EDT
Created attachment 181841 [details]
fix + tests

Fixes this case as well
>         for (int i= 0; ; ) {
>             if (b)
>                 i++;
>             System.out.println("hi");
>             i-= 18;
>         }

Will file a new bug for 
>     private int fField;
>     void bar() {
>         new Snippet().fField++;
>     }
Comment 7 Markus Keller CLA 2010-10-27 10:59:23 EDT
(In reply to comment #6)
+1 for M3.
Comment 8 Deepak Azad CLA 2010-10-27 11:05:45 EDT
Fixed in HEAD.

> Will file a new bug for 
> >     private int fField;
> >     void bar() {
> >         new Snippet().fField++;
> >     }
Filed Bug 328839.
Comment 9 Deepak Azad CLA 2010-10-28 02:07:33 EDT
Verified in I20101027-1800