Bug 361331 - Allow to filter certain types from 'Unused object allocation' warning
Summary: Allow to filter certain types from 'Unused object allocation' warning
Status: NEW
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.8   Edit
Hardware: PC Windows 7
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Ayushman Jain CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-19 01:44 EDT by Deepak Azad CLA
Modified: 2014-08-17 10:42 EDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Deepak Azad CLA 2011-10-19 01:44:27 EDT
We have the following pattern in a number of places in Eclipse SDK. SWT widgets are often created to create a separator or a gap in the UI.
     new Label(buttonComp, SWT.NONE); // separator

PDE has few occurrences of the following pattern, where an object is created to perform validation. 
	private boolean verifySiteUrl(IFeature feature, String value) {
		try {
			new URL(value);//WARN:The allocated object is never used
		} catch (MalformedURLException e) {
			return false;
		}
		return true;
	}

In bug 358903 we are thinking of a solution which involves hand-crafted list of types to filter certain warnings. 

Can we do something similar here?
- Create a list of predefined types e.g. SWT widgets, whose unallocated objects get a 'Potential unused object allocation' warning instead of 'Unused object allocation' warning
- Allow users to add or remove types from this list
Comment 1 Ayushman Jain CLA 2011-10-19 02:41:57 EDT
Can't the user just use a @SuppressWarnings("unused") at these locations? 

Also how will you use the whitelist? If you add all allocations of say, URL or Label, then even objects that are actually redundantly initialized will be uncaught by the warning. So there's a tradeoff in doing the whitelist.
Comment 2 Deepak Azad CLA 2011-10-19 03:00:10 EDT
(In reply to comment #1)
> Can't the user just use a @SuppressWarnings("unused") at these locations? 
You can, but then you have to do this for each and every warning and the source is cluttered with a bunch of avoidable @SuppressWarnings. Essentially the same reasoning as bug 358903.

> Also how will you use the whitelist? If you add all allocations of say, URL or
> Label, then even objects that are actually redundantly initialized will be
> uncaught by the warning. So there's a tradeoff in doing the whitelist.
I am only asking for the severity of the warning to be lowered to 'Potential' for these filtered types, this would allow a user to turn off the warning for several chosen types in one shot.

Currently the option is not enabled in jdt.ui because it produces too many false positives of the type described in comment 0. I checked in other projects as well  e.g. pde.ui, team.ui and there also there are several instances of such false positives. Hence, I only see a benefit in reducing these false positives by creating a whitelist.
Comment 3 Markus Keller CLA 2011-10-19 10:16:17 EDT
The internal whitelist discussed in bug 358903 was only for a few common cases in the JRE. If we start to create general whitelists, then this would have to be made configurable. We can't give a good reason for adding SWT examples, but not other non-JRE libraries.

Adding a configuration UI makes the stuff even more complicated. If we add this, then the UI needs to be very lightweight, e.g.:
- a quick fix "Ignore problem for all references to 'fully.qualified.constructor.Name(ParameterTypes...)'"
- on the Errors/Warnings page, add a link "(_exceptions_):" behind the "Unused object allocation" label
Comment 4 Olivier Thomann CLA 2011-10-19 10:27:48 EDT
We should also make sure this is usable by the command line compiler.
Comment 5 Deepak Azad CLA 2011-10-19 10:59:21 EDT
(In reply to comment #3)
> We can't give a good reason for adding SWT examples, but
> not other non-JRE libraries.
You mean only selected types from JRE should be added to a predefined list?
Comment 6 Stephan Herrmann CLA 2011-10-19 17:20:18 EDT
(In reply to comment #3)
> ... If we start to create general whitelists, then this would have to
> be made configurable. ... 
> Adding a configuration UI makes the stuff even more complicated. If we add
> this, then the UI needs to be very lightweight, e.g.:
> - a quick fix "Ignore problem for all references to
> 'fully.qualified.constructor.Name(ParameterTypes...)'"
> - on the Errors/Warnings page, add a link "(_exceptions_):" behind the "Unused
> object allocation" label

This sounds similar to what I had in mind writing bug 331651 comment 6
(see item B(2)).

Perhaps we could create a general theme here, s.t., like "the learning IDE":
Like a spell-checker with the option to add new words to a local dictionary.

Just: in our case we need more information:
- which API element?
- what semantics do we want the IDE to learn?

I don't have a cooked proposal, just thinking aloud.
Comment 7 Stephan Herrmann CLA 2011-10-19 17:33:25 EDT
(In reply to comment #4)
> We should also make sure this is usable by the command line compiler.

Do you see a problem feeding this into the compiler using
 -properties: <file>
?
Comment 8 Olivier Thomann CLA 2011-10-19 18:26:11 EDT
(In reply to comment #7)
> (In reply to comment #4)
> > We should also make sure this is usable by the command line compiler.
> 
> Do you see a problem feeding this into the compiler using
>  -properties: <file>
> ?
Certainly not, but it has to be convenient enough to be easily used.
Comment 9 Markus Keller CLA 2011-10-20 06:36:47 EDT
(In reply to comment #3)
> You mean only selected types from JRE should be added to a predefined list?

No, I mean we cannot have a *hardcoded* list that includes items from SWT but not from <insert-most-important-example-of-library-XYZ>.


However, the more I think of this, the more it feels like featurism. If it's really a problem for a project, they can always have a static helper somewhere (e.g. SWTUtil.newLabel()) and add @SuppressWarnings("unused") there.

For both mentioned cases (Label, URL), there are situations where the warning is helpful, so we wouldn't even put those into the exclusion list by default.
Comment 10 Deepak Azad CLA 2011-10-20 09:11:06 EDT
(In reply to comment #9)
> However, the more I think of this, the more it feels like featurism. If it's
> really a problem for a project, they can always have a static helper somewhere
> (e.g. SWTUtil.newLabel()) and add @SuppressWarnings("unused") there.

That could possibly work!

Currently, in both jdt.ui and jdt.core 'Unused object allocations' is disabled for some reason.(If we do not use our compiler options, I wonder who will...) I assume the reason so far has been too many uninteresting warnings. Now that we have a possible solution can we enable the warning and try the solution?

If the solution suggested by Markus works nicely for us, we can close this bug as WONTFIX.
Comment 11 Deepak Azad CLA 2011-10-20 09:19:51 EDT
In case someone is looking for hard numbers -

jdt.ui has a total of 43 unused object warnings, 11 or about 25% are on instances of org.eclipse.swt.widgets.Label, which are not interesting practically. Rest are sort of distributed between various types - MenuItem, FocusHandler etc. For several of these types/constructors there are 3-4 (uninteresting) warnings.
Comment 12 Ayushman Jain CLA 2011-10-20 09:26:36 EDT
(In reply to comment #10)
> (In reply to comment #9)
> > However, the more I think of this, the more it feels like featurism. If it's
> > really a problem for a project, they can always have a static helper somewhere
> > (e.g. SWTUtil.newLabel()) and add @SuppressWarnings("unused") there.
> 

This is a better solution I think. For jdt.core we get only 3 warnings - all of them on constructor of InputStreamReader. All these can be suppressed as the object is being allocated to check whether an encoding is valid. It is trivial to fix this on Markus' suggestion - we'll just have a checkEncoding utility method and put a @SW there.
Comment 13 Stephan Herrmann CLA 2011-10-20 10:18:17 EDT
(In reply to comment #9)
> ... If it's really a problem for a project, they can always have a static
> helper somewhere (e.g. SWTUtil.newLabel()) and add
> @SuppressWarnings("unused") there.

I agree. 

(In reply to comment #12)
> [...] It is trivial to fix this on Markus' suggestion - we'll just have a
> checkEncoding utility method and put a @SW there.

Yes, so this would not only suppress the warning but also document *why* the
object was created in the first place - by giving a suitable name to the 
helper method.

But, are we ready to use annotations in JDT/Core?


Once we have the experience of enabling the warning in our projects,
would that be the time to reconsider the default (currently: ignore)?