Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 370639

Summary:	[compiler][resource] restore the default for resource leak warnings
Product:	[Eclipse Project] JDT	Reporter:	Stephan Herrmann <stephan.herrmann>
Component:	Core	Assignee:	Stephan Herrmann <stephan.herrmann>
Status:	VERIFIED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	amj87.iitr, daniel_megert, deepakazad, markus.kell.r, satyam.kandula, srikanth_sankaran
Version:	3.8
Target Milestone:	3.8 M6
Hardware:	Other
OS:	Linux
Whiteboard:

Description Stephan Herrmann

2012-02-04 13:24:16 EST

In bug 349326 comment 16 I argued:
> I would normally propose the following defaults:
> - UnclosedCloseable: error
> - PotentiallyUnclosedCloseable: warning
> - ExplicitlyClosedAutoCloseable: ignore (internal use only)
>
> However, for 3.7.1 these should perhaps be defined more shyly:
> - UnclosedCloseable: warning
> - PotentiallyUnclosedCloseable: ignore
>  and by the time for 3.8 we should have sufficient experience to re-assess.

The patch was released for 3.8 M3 with the weaker set of defaults:
 - UnclosedCloseable: warning
 - all others: ignore

During the discussion on bug 358903 Markus disabled the warning as per bug 
365566:

> (In reply to bug 358903 comment #13)
> > Could we put this and the dependent bugs on the plan for M5?
> > 
> > The builder has been updated for N20111201-2000, so this will create almost 100
> > compiler warnings in the SDK -- and from a quick glance, I'd say about 30% are
> > practically unimportant.
> 
> We should disable the warning for M4 to avoid generating useless work. After
> bug 358903 is fixed, the default should go back to "warning".

Meanwhile bug 358903 has been fixed including its successor bug 368546. The results by far exceed what Markus expected in the above comment: when compiling the entire Eclipse SDK I only see 8 reports of unclosed resources. All other diagnostics are either detected as irrelevant or ranked as potential.

Therefor I propose to restore the default for UnclosedCloseable to warning.

I think we should do this within the Juno cycle, because after the Juno release many developers will unknowingly store the default in there jdt.core.prefs file, likely to be persisted in source repositories etc. After that only a selected fraction of JDT users would ever notice the option, even if we choose to change the default for 3.9.

In order to dispel remaining fears about bugs in the implementation I offer to add more early checks to the code. This way, should the analysis trigger a bug in the compiler a user can still work around the bug by disabling the warning.

Comment 1 Deepak Azad

2012-02-04 21:39:30 EST

(In reply to comment #0)
> I think we should do this within the Juno cycle
+1

Comment 2 Ayushman Jain

2012-02-06 00:39:10 EST

M6 looks good, if everyone is on the same page

Comment 3 Srikanth Sankaran

2012-02-06 05:21:06 EST

(In reply to comment #0)

> Meanwhile bug 358903 has been fixed including its successor bug 368546.

I think this feature has seen a good deal of scrutiny in terms of code
reviews, white box and black box testing, verification etc. Thanks to
Deepak, Satyam, Ayush and Markus for all the support and of course to
Stephan for very prompt follow ups to issues raised.

> In order to dispel remaining fears about bugs in the implementation I offer to
> add more early checks to the code. This way, should the analysis trigger a bug
> in the compiler a user can still work around the bug by disabling the warning.

Code implementing an optional feature should ideally be under a flag so 
there is an effective escape hatch. Obviously we need to be judicious in 
this principle - we don't want to cause excess code clutter by checking 
for option being turned on all over the place. We also need to bear in 
the cost of these checks. The author of the feature can figure what 
constitutes a reasonable measure of defensive coding and implement it 
that way. 

In that spirit, Appreciate your offer to look into this. Thanks.
With that concern behind us, M6 looks good as the inflection point.

Comment 4 Dani Megert

2012-02-06 07:54:08 EST

I set them to 'Error' in my big source workspace which results in 3 errors for resource leaks and 63 errors for potential resource leaks.

The errors for resource leaks are either correct or an appearance of bug 370702.

For the potential resource leaks, those I sanity checked could be potential ones but in fact aren't. Also, most of them are in jdt.ui i.e. other components aren't affected heavily.

==> OK to change the default for 'UnclosedCloseable' to 'warning'

==>

- UnclosedCloseable: warning
- PotentiallyUnclosedCloseable: ignore
- ExplicitlyClosedAutoCloseable: ignore

Comment 5 Srikanth Sankaran

2012-02-08 07:34:48 EST

(In reply to comment #3)

> ... We also need to bear in 
> the cost of these checks.

The fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=370182 makes
looking up compiler options very cheap from various scopes. Code that
retrieves options from a scope instance would automatically stand to
benefit.

Comment 6 Stephan Herrmann

2012-02-14 08:07:33 EST

Resolved for 3.8 M6 via commit e67d1dc6830648c83fdb0e0b83b59925424638d8

When checking the documentation I found that JavaCore was still at the previous state => no change required.
OTOH, in jdt.doc.user ref-preferences-errors-warnings.htm these options were still lacking, fixed via commit d9b7b2fe0e0e034d4f95826842f5e776ae60eb7e
(w3c validator said OK)

Additionally, I added the mentioned checks to avoid the analysis when all these options are set to ignore. I double checked this with the following result:
When compiling the Eclipse SDK with all resource warnings disabled, no instance of FakedTrackingVariable was ever created, which is the center piece in this analysis.

Comment 7 Srikanth Sankaran

2012-03-12 04:49:46 EDT

Verified for 3.8 M6 using Build id: I20120306-0800