Bug 313109 - @SuppressWarnings on multiple locals is marked unnecessary if any local is never used
Summary: @SuppressWarnings on multiple locals is marked unnecessary if any local is ne...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.6   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.6 RC2   Edit
Assignee: Olivier Thomann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-17 06:54 EDT by Ayushman Jain CLA
Modified: 2010-05-24 05:01 EDT (History)
4 users (show)

See Also:
srikanth_sankaran: review+
frederic_fusier: review+

Attachments
Proposed fix + regression tests (7.15 KB, patch)
2010-05-17 13:51 EDT, Olivier Thomann CLA 		no flags Details | Diff
Proposed fix + regression tests (7.19 KB, patch)
2010-05-18 12:34 EDT, Olivier Thomann CLA 		no flags Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ayushman Jain CLA 2010-05-17 06:54:34 EDT 
Build Identifier: I20100513-1500

This is FUP of bug 311849.

In the following code

import java.util.*;

class Bug {

    @SuppressWarnings("rawtypes")
    void foo(ArrayList arg) {
        @SuppressWarnings("unchecked")  // bad: unnecessary suppressWarnings
warn
        boolean aa = arg.add(1),  // good: local variable aa never read warning
                bb = arg.add(1);
        if (bb)
            System.out.println("hi");

    }
}

If either aa or bb is not read the suppressWarnings annotation is warned by
the compiler as unnecessary. This is a bogus warning since the annotation
applies for atleast one variable in the declaration series.


Reproducible: Always

Steps to Reproduce:
1.Use the above code
2.See the bogus compiler warning
Comment 1 Markus Keller CLA 2010-05-17 07:05:11 EDT 
This was OK in 3.6M7.
Comment 2 Olivier Thomann CLA 2010-05-17 09:44:34 EDT 
This comes from the fact that the annotation is shared for the two locals. I can take a look to see how we can try to fix it.
Comment 3 Olivier Thomann CLA 2010-05-17 10:24:17 EDT 
Targeting 3.6RC2.
Will be reassessed once the fix is ready.
Comment 4 Olivier Thomann CLA 2010-05-17 12:25:32 EDT 
The problem comes from the fact that in this case we end up duplicating some information inside the suppress warnings data, because outside of the case for the for statement's initializers, the local declaration source end and local declaration source start are identical for both locals.
So we can either fix it by making sure we don't add duplicated information inside the compilation unit suppress warnings data or we modify the grammar to initialize the declaration source end of the local inside for statement's initializers.
I have a fix using the first approach.

The second way to fix it would also need to revert the patch for bug 311849.
Comment 5 Olivier Thomann CLA 2010-05-17 13:51:45 EDT 
Created attachment 168779 [details]
Proposed fix + regression tests
Comment 6 Olivier Thomann CLA 2010-05-17 21:50:19 EDT 
Frédéric, Srikanth, please review.
Comment 7 Srikanth Sankaran CLA 2010-05-18 07:34:38 EDT 
Patch looks good to me.
Comment 8 Frederic Fusier CLA 2010-05-18 08:40:12 EDT 
(In reply to comment #6)
> Frédéric, Srikanth, please review.

Patch looks good to me.

One cosmetic remark, IMO the isAllSet method name looks a little bit ambiguous
as it looks more like an equality between two IrritantSet... Maybe hasSameBits or something similar would be a little bit better?
Comment 9 Olivier Thomann CLA 2010-05-18 10:57:09 EDT 
(In reply to comment #8)
> One cosmetic remark, IMO the isAllSet method name looks a little bit ambiguous
> as it looks more like an equality between two IrritantSet... Maybe hasSameBits
> or something similar would be a little bit better?
ok, I'll rename it.
Please add the +1
Comment 10 Olivier Thomann CLA 2010-05-18 12:34:22 EDT 
Created attachment 168965 [details]
Proposed fix + regression tests

Same patch with renamed method.
Frédéric, please review.
Comment 11 Frederic Fusier CLA 2010-05-18 13:20:33 EDT 
(In reply to comment #10)
> Created an attachment (id=168965) [details]
> Proposed fix + regression tests
> 
> Same patch with renamed method.
> Frédéric, please review.

OK for me: +1
Comment 12 Olivier Thomann CLA 2010-05-18 14:19:32 EDT 
Released for 3.6RC2.
Regression tests added in:
org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test290
org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test291
org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test292
Comment 13 Jay Arthanareeswaran CLA 2010-05-24 05:01:06 EDT 
Verified for 3.6RC2 using build  I20100520-1744.