Community
Participate
Working Groups
See also bug 349326. The following 6 warnings in jdt.core are theoretically correct, but practically not important as no OS resource is leaked (the resources are StringReader, StringWriter etc). Is there a way we can filter these? Resource leak: 'javaReader' is never closed Scribe.java /org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter line 2354 Resource leak: 'print' is not closed at this location ClassFileReader.java /org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/classfmt line 67 Resource leak: 'print' is not closed at this location ClassFileReader.java /org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/classfmt line 1181 Resource leak: 'reader' is never closed Main.java /org.eclipse.jdt.core/batch/org/eclipse/jdt/internal/compiler/batch line 1794 Resource leak: 'reader' is never closed Scribe.java /org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter line 2325 Resource leak: 'writer' is never closed ProblemReporter.java /org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/problem line 1114 Thanks Markus for pointing these out!
For Closeable classes from the JRE System Libraries, a good indication that close() is not really relevant is the absence of a finalize() method. But that's not really a reliable predicate in general, and it's hard to track in practice when resources are wrapped in other resources like this: LineNumberReader resources = new LineNumberReader(new StringReader(string));
In practice if a large proportion of warnings are unimportant from a practical standpoint and we do not have a way to suppress them, we can also consider changing the default of 'Resource leak' warning to ignore.
(In reply to comment #2) > In practice if a large proportion of warnings are unimportant from a practical > standpoint and we do not have a way to suppress them, we can also consider > changing the default of 'Resource leak' warning to ignore. I think that should only be the very last resort because that would probably mean that the vast majority of users will never learn about the warning. I see two options to suppress warnings: @SuppressWarnings("resource") r.close(); :) I'd say in Eclipse code I expect the large proportion of the warnings to be already fixed sometime during the last 10 years. If a handful of unimportant warnings remain this looks quite normal to me. Regarding the classification important/unimportant: should we for a start just use a white list of JDK classes that are known not to use external resources (StringReader/Writer etc.)? That would only leave us with the issue of instance nesting, but it would be something I could start from.
(In reply to comment #3) > (In reply to comment #2) > > In practice if a large proportion of warnings are unimportant from a practical > > standpoint and we do not have a way to suppress them, we can also consider > > changing the default of 'Resource leak' warning to ignore. > > I think that should only be the very last resort because that would probably > mean that the vast majority of users will never learn about the warning. Same feeling for me. I find this warning useful. It can definitely help to find resources leaks. > @SuppressWarnings("resource") yes, we should add this suppress warnings token. Adding a while list might worth being investigated.
(In reply to comment #4) > > @SuppressWarnings("resource") > yes, we should add this suppress warnings token. It's already part of the patch in bug 349326 :) Related question: would we also want to support a corresponding token for the batch compiler? I started to think about his when I noticed that TestVerifier/VerifyTests raises the new warning and one way to avoid clutter in the test output might be to pass -warn:-resource when compiling VerifyTests.java? Should I file a new bug for that?
(In reply to comment #3) > Regarding the classification important/unimportant: should we for > a start just use a white list of JDK classes that are known not to > use external resources (StringReader/Writer etc.)? > That would only leave us with the issue of instance nesting, > but it would be something I could start from. Let's take an example (org.eclipse.jdt.internal.compiler.batch.Main: line 1794) LineNumberReader reader = new LineNumberReader(new StringReader(new String(Util.getFileCharContent(new File(arg.substring(1)), null)))); This example is fairly involved and even in this case wouldn't it be simpler to just check if any type used on the rhs has a finalize method or not? If no finalize method if found, it is probably safe to infer that there is no OS resource involved. Preparing a list would be error prone, and I assume even there the basis of inclusion or exclusion of a type would be the presence or absence of finalize(). Hence, why not simply do on the fly checks?
Of course abstract classes like java.io.InputStream and java.io.OutputStream do not have finalize methods.
(In reply to comment #6) > Preparing a list would be error prone, and I assume even there the basis of > inclusion or exclusion of a type would be the presence or absence of > finalize(). Hence, why not simply do on the fly checks? No, the list would have to be hand-checked and would only include the common cases from the JRE libraries that are safe because they only process a given string or array. I really meant comment 1 exactly as I wrote it. The finalize() method is just a pattern used in the JRE, but I'm sure there are many third-party resource classes that don't have a finalize() method. AFAICS, even the java.nio.* classes don't have finalizers. Actually only java.io.FileInput/OutputStream and java.util.zip.ZipFile do use finalize().
I suggest to use 2 white lists, actually: Group 1: classes that wrap another Closeable Group 2: (leaf) classes that are known not to allocate any OS resources Thus we can safely state that any expression new Group1_A(new Group1_B(value_from_group_2)) does not require tracking by our analysis. A white list is not a complete solution but in this case I think it's the best balance between avoiding false positives and effort (development & runtime).
(In reply to comment #9) > I suggest to use 2 white lists, actually: Sounds good.
I don't know how easy or difficult is the complete solution, but as a first step we should be able to eliminate warnings on the following pattern Group1_Type resource= new Group1_Type(); There are a few of these in jdt.ui which involve one of StringReader,StringWriter, ByteArrayOutputStream, ByteArrayInputStream.
(In reply to comment #11) > Group1_Type resource= new Group1_Type(); I meant Group2 :) Group2_Type resource= new Group2_Type();
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.
(In reply to comment #13) > Could we put this and the dependent bugs on the plan for M5? I'll assess feasibility once the null annotations work is fully done for M4 > 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. BTW: how can I see which builder is used (for which build)?
Please set the default for the unclosed resource back to "warning" since it has been changed to "ignore" via bug 365566. Only changes in IrritantSet and BatchCompilerTest will need to be backed out. Rest everything is ok.
Created attachment 208899 [details] Tests & fix v0.1 Here's a first take at implementing the two white lists and using these during analysis. While the implementation still has a few FIXMEs, I'd appreciate if someone could already have a look at these: - tests: do you agree with the expected results? - the concrete white lists (in TypeConstants)
(In reply to comment #16) > - the concrete white lists (in TypeConstants) JAVA_IO_RESOURCE_FREE_CLOSEABLES - This list looks OK. However, JAVA_IO_WRAPPER_CLOSEABLES is missing quite a few types e.g. LineNumberReader, DataInputStream, DataInputStream, FilterReader, ObjectOutputStream, ObjectInputStream etc. (I am wondering whether it is safe to say that if constructors for a 'resource type' in java.io package accept as parameter one of the type from JAVA_IO_RESOURCE_FREE_CLOSEABLES then all those 'resource types' belong in JAVA_IO_WRAPPER_CLOSEABLES. Maybe we can use this logic in code instead of enumerating all types in JAVA_IO_WRAPPER_CLOSEABLES?) > - tests: do you agree with the expected results? I have not tried the patch yet.
(In reply to comment #17) > [...] > However, JAVA_IO_WRAPPER_CLOSEABLES is missing quite a few types e.g. > LineNumberReader, DataInputStream, DataInputStream, FilterReader, > ObjectOutputStream, ObjectInputStream etc. Thanks, yep that's quite a list ... > (I am wondering whether it is safe to say that if constructors for a 'resource > type' in java.io package accept as parameter one of the type from > JAVA_IO_RESOURCE_FREE_CLOSEABLES then all those 'resource types' belong in > JAVA_IO_WRAPPER_CLOSEABLES. Maybe we can use this logic in code instead of > enumerating all types in JAVA_IO_WRAPPER_CLOSEABLES?) a) the parameter need not be a RESOURCE_FREE_CLOSEABLE, any Closeable indicates a potential wrapper. b) we cannot prove that this logic is safe. Hence I'd feel more comfortable with applying the logic offline, checking each match manually and storing the result in a manifest white list. > > - tests: do you agree with the expected results? > I have not tried the patch yet. I'd hoped in a TDD spirit we could agree on the expected test results before the patch is finalized and tried :)
Created attachment 209082 [details] Tests & fix v0.5 Updates: Handle more combinations like directly nested allocation vs. storing chained closeables in a local variable, vs. assignment-as-argument etc. (see the tests). More complete handling if arbitrary points in the chain are (potentially) closed. No longer treat BitWrapperCloseable and BitResourceFreeCloseable as inheritable properties. See FileReader for an example: It's a subclass of a wrapper (InputStreamReader), but hides this fact from clients, i.e., FileReader is solely responsible for the underlying resource. CAVEAT: things *could* get messy when a resource is obtained by calling a method returning InputStreamReader which could actually return a FileReader. Analysis would think it's a wrapper, but it actually holds an OS resource. However, since we are shy to report against resources obtained from another method this should be tolerable I think. I'm now down to one FIXME in the code. I haven't yet added much to the white lists. Note that the current patch also covers bug 360908 and bug 361073.
Created attachment 209107 [details] Tests & fix v0.6 Please don't use the previous patch it caused a regression when trying to compile expressions of this shape resource = new Wrapper(resource); From this I created a recursive link structure resulting in stack overflow due to infinite recursion. This pattern is now recognized. Normally, the pattern would also raise a warning that the previous value of resource would leak. I've updated the implementation to remain silent in this case because the responsibility for resource remains the same with just one more level of wrappers. Also warnings are now correctly raised in this kind of code: resource1 = ... resource2 = new Wrapper(resource1 = resource3); In order to detect that the old value of resource1 may leak, we need to capture the null-info before the outer assignment analyzes its rhs. With this patch and with resource leak warning enabled I'm down to 4 warnings in JDT/Core: Resource leak: 'file' is never closed Util.java /org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/util line 977 Java Problem --> will be handled in bug 362332 Resource leak: 'reader' is never closed Scribe.java /org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter line 2325 Java Problem --> I see no obvious reason why this code should be OK :-/ Resource leak: 'javaReader' is never closed Scribe.java /org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter line 2354 Java Problem Resource leak: 'jarFile' is never closed CheckDebugAttributes.java /org.eclipse.jdt.core/antadapter/org/eclipse/jdt/core line 60 Java Problem --> these readers are JDT/Core's own resource wrappers, similar to those in java.io, but we wouldn't want to put our own classes on the white list :-/
(In reply to comment #20) > Created attachment 209107 [details] > Tests & fix v0.6 This patch causes an NPE in UnconditionalFlowInfo because it calls markAs..() methods with a possibly null argument. See bug 368027 comment 0
(In reply to comment #21) > (In reply to comment #20) > > Created attachment 209107 [details] > > Tests & fix v0.6 > > This patch causes an NPE in UnconditionalFlowInfo because it calls markAs..() > methods with a possibly null argument. See bug 368027 comment 0 Wow, that's bad. As I'm not in my JDT/Core-dev-environment currently, I can only give a theoretical answer from the sources: I checked before that a regular FakedTrackingVariable has a non-null binding (unless the ctor bails out with an exception). However, the lately added marker instance SELF_WRAP doesn't follow this rule. Most likely, this signals that there are more exuction paths that need to respect the "res = new Wrapper(res);" pattern. I'll investigate later today. @Deepak: any chance you could associate the NPE to which plugin you were compiling? If not: is there in easy way to setup a workspace with full SDK sources for testing? (e.g., a .psf?)
(In reply to comment #22) > @Deepak: any chance you could associate the NPE to which plugin you > were compiling? If not: is there in easy way to setup a workspace > with full SDK sources for testing? (e.g., a .psf?) The NPE happened with several plugins e.g. o.e.jdt.launching. It also happens with reconcile operations. So I am not sure what exactly causes it, but I am able to reproduce it in my workspace. I was trying to look at the number of warnings in the SDK (see comment 13). My setup is - Start Eclipse (I started with M4) in a fresh workspace - File > Import > Plug-ins and Fragments - Select 'Projects with Source folders' - 'Add All' - Finish Compilation takes a while but the steps are simple enough.
(In reply to comment #20) > --> these readers are JDT/Core's own resource wrappers, similar to > those in java.io, but we wouldn't want to put our own classes on the > white list :-/ In Platform Text there is at least one instance where a StringReader is wrapped in org.eclipse.jface.internal.text.html.HTML2TextReader, which is a candidate for JAVA_IO_WRAPPER_CLOSEABLES. I also came across org.eclipse.jetty.util.ByteArrayISO8859Writer, which is a candidate for JAVA_IO_RESOURCE_FREE_CLOSEABLES. So my question is are we sure that we want to hardcode the white lists? It might be better to create preferences with appropriate defaults. No?
(In reply to comment #24) > So my question is are we sure that we want to hardcode the white lists? It > might be better to create preferences with appropriate defaults. No? I prefer this. Coincidentally, I'm fighting for the same in bug 365437. :)
(In reply to comment #23) > I was trying to look at the number of warnings in the SDK (see comment 13). My > setup is > - Start Eclipse (I started with M4) in a fresh workspace > - File > Import > Plug-ins and Fragments > - Select 'Projects with Source folders' > - 'Add All' > - Finish > Compilation takes a while but the steps are simple enough. Thanks, that looks simple indeed, and works pretty well (after I mounted another disk for the required 1GB :) However, I can't quite believe the result of the operation: About a dozen projects has compile errors of all sorts!! Some of them *could* be due to the fact that I don't have a Foundation-1.1 JRE. (Any ideas where I could get one for linux??) After removing those projects next I saw over 13000 warnings!! Is the SDK code really that bad?? Only after disabling for the entire workspace problems regarding generics and restricted/discouraged access, I'm now down to 150 warnings so I can start investigating the remaining resource warnings....
(In reply to comment #25) > (In reply to comment #24) > > So my question is are we sure that we want to hardcode the white lists? It > > might be better to create preferences with appropriate defaults. No? > > I prefer this. Coincidentally, I'm fighting for the same in bug 365437. :) We shouldn't pollute the preference UI for 1% corner cases. If we really need this configurable then we might consider adding it in JDT Core with reasonable defaults but don't surface it in the UI. That way power users can still change it in the prefs file directly.
(In reply to comment #27) > (In reply to comment #25) > > (In reply to comment #24) > > > So my question is are we sure that we want to hardcode the white lists? It > > > might be better to create preferences with appropriate defaults. No? > > > > I prefer this. Coincidentally, I'm fighting for the same in bug 365437. :) > > We shouldn't pollute the preference UI for 1% corner cases. If we really need > this configurable then we might consider adding it in JDT Core with reasonable > defaults but don't surface it in the UI. That way power users can still change > it in the prefs file directly. I agree that normal users shouldn't see such an option in the UI. It's indeed a corner case and it would take quite a bit of talking to explain when exactly a Closeable type should be classified as a wrapper or as resource-free. Conceptually, I would even argue that this shouldn't be a configuration option for the compiler but an annotation by which library developers mark their classes, so everybody using the library can automatically benefit from that classification. To keep this bug focused I suggest to leave it as hard coded lists for now and wait how desperately users would want to fine tune this.
Oops: my comments don't match to the reported problems for these: (In reply to comment #20) > With this patch and with resource leak warning enabled I'm down to > 4 warnings in JDT/Core: > > Resource leak: 'reader' is never closed Scribe.java > /org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter line 2325 > Java Problem > Resource leak: 'javaReader' is never closed Scribe.java > /org.eclipse.jdt.core/formatter/org/eclipse/jdt/internal/formatter line 2354 > Java Problem > Resource leak: 'jarFile' is never closed CheckDebugAttributes.java > /org.eclipse.jdt.core/antadapter/org/eclipse/jdt/core line 60 Java > Problem The two occurrences in Scribe are OK (custom resource wrappers). But the occurrences in CheckDebugAttributes looks like a real bug to me! We read a ZipFile (using entries()) and that's all we do with it! However, I noticed the issue only while accidentally compiling against a 1.7 JRE where ZipFile implements Closeable, which is not the case in older JRE's.
Created attachment 209169 [details] Tests & fix v0.8 Wow, precise analysis of various structures of nested resource instances across various syntactic options turns out to be pretty complex. The good news is: the new patch also covers bug 360908, bug 361073, bug 362331 and bug 362332. Specifically bug 362331 required a change in implementation strategy (to be documented there) which had some impact also on this bug. So it's good to address both issues in one patch rather than going back and forth. Ah, and yes, the strategy change also resolved the NPE from bug 368027 (the special instance SELF_WRAP, which indeed was the culprit, is no longer needed). When compiling the full SDK (almost full, see comment 26) with this patch I'm down at 31 resource warnings (after restoring the intended default). Out of these 2 or 3 could still be avoided by smarter analysis. Several warnings demonstrate an inherent limitation: the analysis cannot cope well with variables assigned from various sources on different branches, since we have no representation for values, only variables and their status (wrt def / null / closed). I've added java.* classes to the white lists as they occurred in the sources, but lists are still not complete.
Created attachment 209179 [details] Tests & Fix v0.9 Mostly cleanup, including copyright. Values read from a field are now excluded from analysis.
More candidate types for inclusion in the wrapper white list (in addition to the ones contained in the patch): java.io: DataInputStream, PushbackInputStream, SequenceInputStream, StringBufferInputStream, DataOutputStream, PrintStream, PushbackReader. OutputStreamWriter NOT: PipedInputStream, PipedOutputStream, PipedReader, PipedWriter java.util.zip: GZIPInputStream, InflaterInputStream, DeflaterInputStream, CheckedInputStream, ZipInputStream, JarInputStream, GZIPOutputStream, InflaterOutputStream, DeflaterOutputStream, CheckedOutputStream, ZipOutputStream, JarOutputStream java.security: DigestInputStream, DigestOutputStream java.beans: XMLEncoder, XMLDecoder javax.sound.sampled: AudioInputStream
Satyam, could you please review the latest patch from comment#31 bearing in mind comment# 32 ? TIA.
(In reply to comment #26) > However, I can't quite believe the result of the operation: > About a dozen projects has compile errors of all sorts!! Some of them > *could* be due to the fact that I don't have a Foundation-1.1 JRE. > (Any ideas where I could get one for linux??) Yup, I ignored these projects. > After removing those projects next I saw over 13000 warnings!! > Is the SDK code really that bad?? > Only after disabling for the entire workspace problems regarding > generics and restricted/discouraged access, I'm now down to 150 warnings > so I can start investigating the remaining resource warnings.... In the problems view I use 'Group by > Java Problem Type' and then focus on 'Potential Programming Problems' group. (In reply to comment #27) > We shouldn't pollute the preference UI for 1% corner cases. If we really need > this configurable then we might consider adding it in JDT Core with reasonable > defaults but don't surface it in the UI. That way power users can still change > it in the prefs file directly. I am OK with not exposing the options in the UI. One example of a power user would be our Eclipse SDK build. (In reply to comment #28) > To keep this bug focused I suggest to leave it as hard coded lists > for now and wait how desperately users would want to fine tune this. Sure, we can look at the number of warnings left in the SDK after this bug is fixed and then take a call.
(In reply to comment #30) > When compiling the full SDK (almost full, see comment 26) with this patch > I'm down at 31 resource warnings (after restoring the intended default). > Out of these 2 or 3 could still be avoided by smarter analysis. > Several warnings demonstrate an inherent limitation: the analysis cannot > cope well with variables assigned from various sources on different > branches, since we have no representation for values, only variables and > their status (wrt def / null / closed). With patch v0.9 I see 61 warnings, but that is probably because I have not deleted any projects and my workspace includes org.apache plugins as well. In any case the patch works well, and most of the warnings look valid to me. I also no longer see the NPE from comment 21.
Some explanations as background for the review: At the input of the resource analysis we use the bitset ReferenceBinding.typeBits which was introduced in bug 349326 with the 2 bits BitAutoCloseable and BitCloseable (from TypeIds). To these, this bug adds BitWrapperCloseable and BitResourceFreeCloseable to mark members of our two new white lists. Note that the former 2 bits are inheritable, whereas the latter two are not (see comment 19). The main part of the analysis uses the concept of a FakedTrackingVariable, also introduced in bug 349326: when an AutoCloseable value is assigned to a local variable we add this tracking variable for which we record nullness information with these interpretations: NULL: not-closed, NON_NULL: closed; plus the corresponding POTENTIALLY_... values. Originally, tracking variables were associated only to local variables. With this patch we also have the option to temporarily store a tracking variable for an AllocationExpression in order to detect patterns like: new Wrapper(new InnerResources()); // not relevant if inner is resource-free and new Resource().read(); // not closed new Resource().close(); // OK For any chained resources (new Wrapper(new Inner())) we now link the tracking variables using FTV.innerTracker so that we can detect whether an underlying resource (the innermost) is closed directly or indirectly via its wrapper chain. An especially tricky situation is this: res = new Wrapper(res); As before we need to analyze the kind of wrapper chain against our white lists, but also we must distinguish this from res1 = new Wrapper(res2); In the latter case the assignment forgets the resource previously stored in res1, and if that was not closed we have a leak. By contrast the former assignment creates a matryoshka where no resource is forgotten. This is a challenge for the implementation because during AST traversal we have no single location that sees both applications of 'res'. To solve this, method FTV.preConnectTrackerAcrossAssignment(..) pushes the LHS of the assignment into any AllocationExpression on the RHS, to be picked up when analyzing the allocation. Another complication is caused by statements like res = new Wrapper( res2 = new Inner()) This means we have to unwrap any assignments (and similarly: casts) in the position of an argument to a resource-type allocation expression. Associating a tracking variable to an allocation expression requires additional care, because now we can have tracking variables with no local variable binding associated (in field originalBinding), which previously would never occur. Finally I'm introducing a distinction of expression kinds from which we obtain a resource: - when obtaining a resource from a message send, we are uncertain which side is responsible for closing -> mark as potentially closed. similarly for an array reference. - when obtaining a resource from a field read, assume that the resource has a longer life cycle than the current method execution -> pretend it is properly closed. Most of this logic is implemented in class FakedTrackingVariable. A few changes result from moving methods from BlockScope into FTV. The interesting new points of interaction are in - Assignment: trigger analysis: check against white list and for wrappers drill into the first argument - LocalDeclaration & Assignment: trigger the left-to-right push mentioned above - then do normal analysis of RHS - then collect information right-to-left. For more detailed description please see the comments in the code - or ask me :)
Created attachment 209232 [details] Tests & fix v0.9.5 Same patch with extended white lists from comment 32 (StringBufferInputStream was wrongly listed as a wrapper, is actually resource-free). When compiling the SDK with this patch I only see 25 resource warnings.
Created attachment 209234 [details] Tests & fix v0.9.6 Previous patch introduced a copy&paste error, detected by NegativeCodeSnippetTest.
(In reply to comment #36) Thanks for the detailed explanation. I have started the review and will give you any comments I have periodically. To start with, what should happen for the subtypes of thewhite list types? For eg: ##### import java.io.StringReader; import java.io.IOException; public class X { void foo() throws IOException { StringReader string = new SReader("content"); System.out.println(string.read()); } public static void main(String[] args) throws IOException { new X().foo(); } } class SReader extends StringReader { public SReader(String arg0) { super(arg0); } } ##### SReader extends StringReader in the white list without overriding close. I believe resource leaks should not be reported even for this.
(In reply to comment #39) > (In reply to comment #36) > Thanks for the detailed explanation. I have started the review and will give > you any comments I have periodically. To start with, what should happen for the > subtypes of thewhite list types? > For eg: > ##### > import java.io.StringReader; > import java.io.IOException; > public class X { > void foo() throws IOException { > StringReader string = new SReader("content"); > System.out.println(string.read()); > } > public static void main(String[] args) throws IOException { > new X().foo(); > } > } > class SReader extends StringReader { > public SReader(String arg0) { > super(arg0); > } > } > ##### > SReader extends StringReader in the white list without overriding close. I > believe resource leaks should not be reported even for this. My implementation assumes nothing about such subtypes. This encodes the policy that we know for sure only for the exact types we've checked for inclusion in the white list. Your heuristic to inherit from a resource-free Closeable and not override close is a good one, but I decided to not implement *any* heuristic of this kind. I'm not sure how relevant your case is. If relevant we could already file a further RFE. Otherwise we might wait if it is more than an extreme corner case in practice?
(In reply to comment #40) We could track the problem in a separate bug, but as there is no user defined white list, these types of filters could help reducing some more false positives.
(In reply to comment #41) > I'm not sure how relevant your case is. If relevant we could already file > a further RFE. Otherwise we might wait if it is more than an extreme > corner case in practice? I could see subtypes of ByteArrayInputStream in my workspace.
(In reply to comment #40) > Your heuristic to inherit from a resource-free Closeable and not override > close is a good one, but I decided to not implement *any* heuristic of > this kind. I vote for waiting on this. - I have not seen too many practical cases for this. - If we decide to make the white lists configurable by the user, then the small number of such scenarios can be taken care of there. Stephan, you may also want to start a topic branch for this (and similar bugs) which involve a number of review cycles. Note that the branch name convention to be followed is "<committerid>/branchname", see bug 362363.
I haven't got hold of the changes as yet. I am looking at the old code also to understand. So, here are some minor comments which includes old code too. Hope that's ok. 1. SingleNameReference.java - no change but there is a copyright year change. 2. I believe ReferenceBinding#applyCloseableWhiteLists should be optimized for OTHER_WRAPPER_CLOSEABLES. The way I see this is that all the types which are in the list are returned quickly than the ones which are not! All the types will be searched for atleast in the OTHER_WRAPPER_CLOSEABLES. I believe this could be improved. - Should the following case report a resource warning void foo( InputStream input) throws IOException { FileInputStream input1 = (FileInputStream)input; System.out.println(input1.read()); input.close(); } 3. Method names could be better: - FakedTrackingVariable#isAutoCloseable() now looks for closeable too. - FakedTrackingVariable#analyseCloseableAllocationArgument(...) should probably be find...(...) 4. Formatting: I found two formatting styles a different from much of the JDT/Core code. I should tell that there are places which don't follow this style, but ... :) - if statements with only one statements are either written as if(condition) statement or if(condition) { statement } and not if (condition) statement They are many places I see in this code which uses the latter style. -Methods in a class are generally sorted. I don't see this in FakedTrackingVariable. If you change the formatting now, diff will again become difficult and hence I would recommend not changing it now, but please keep in mind for the new types you create. 5.Whenever new tracking variable is added through BlockScope#registerTrackingVariable, the MethodScope#analysisIndex is not getting added. As of now, there is no impact as this happens only during analysis and analysisIndex is updated only during resolving, but isn't it a good idea to update that too?
> 2. I believe ReferenceBinding#applyCloseableWhiteLists should be optimized for > OTHER_WRAPPER_CLOSEABLES. The way I see this is that all the types which are in > the list are returned quickly than the ones which are not! All the types will > be searched for atleast in the OTHER_WRAPPER_CLOSEABLES. I believe this could > be improved. Please ignore this, I see that this is getting called only when it extends closeable or Autocloseable and hence this is OK.
(comment crossed with your latest, submitting as-is anyway) (In reply to comment #44) > I haven't got hold of the changes as yet. I am looking at the old code also to > understand. So, here are some minor comments which includes old code too. Hope > that's ok. Sure, just ask. > 1. SingleNameReference.java - no change but there is a copyright year change. oops, seems like I undid some intermediate changes up-to a clean state and then the update-copyright-tool re-added its change on save :-/ I'll revert that. > 2. I believe ReferenceBinding#applyCloseableWhiteLists should be optimized for > OTHER_WRAPPER_CLOSEABLES. The way I see this is that all the types which are in > the list are returned quickly than the ones which are not! All the types will > be searched for atleast in the OTHER_WRAPPER_CLOSEABLES. I believe this could > be improved. You're right. How much effort we put into optimizing this algorithm depends on how many types we have in OTHER_WRAPPER_CLOSEABLES. Initially I had ONE, I'd say for the current list of 5 it is still acceptable (note that only closeable types will reach here). Should the list grow we should certainly do some smarter lookup. > - Should the following case report a resource warning > void foo( InputStream input) throws IOException { > FileInputStream input1 = (FileInputStream)input; > System.out.println(input1.read()); > input.close(); > } Good point. Yes, we want to identify input and input1 (one branch in my implementation still misses to unwrap the cast expression). This will be easily fixed. Secondly, there's some interaction with input being a parameter. I had hoped we could avoid tracking variables for parameters (assuming the current method is not responsible for closing), but your case shows we need a tracking variable but start with a status indicating that no warnings should be raised. It seems REPORTED would be a suitable mark here. > 3. Method names could be better: > - FakedTrackingVariable#isAutoCloseable() now looks for closeable too. I kept confusing myself by looking how Closeable extends AutoCloseable, but only in 1.7 of course, so right, I guess isAnyCloseable() should be better. > - FakedTrackingVariable#analyseCloseableAllocationArgument(...) should > probably be find...(...) If we accept that the "AllocationArgument" part can remain implicit by way of the method arguments 3 & 4, we can say "findCloseTracker". And then I'll make that method private so we're sure its only used in a context where we know what a closeTracker is :) > 4. Formatting: I found two formatting styles a different from much of the > JDT/Core code. I should tell that there are places which don't follow this > style, but ... :) > - if statements with only one statements are either written as > if(condition) statement or > if(condition) { > statement > } > and not > if (condition) > statement > They are many places I see in this code which uses the latter style. I thought I had seen sufficiently many occurrences of the latter style to conclude that this is accepted JDT/Core style :) Just looking at FakedTrackingVariable, strictly adhering to the second style will add almost 30 lines of insignificant code, which IMHO don't add for the readability but just make me scroll more. Now, if the rest of the team feels uneasy about the two-lines style, I can certainly avoid it (after having expressed my dissent :)) Should I? Will we reformat existing occurrences of this style when we touch such code? > -Methods in a class are generally sorted. I don't see this in > FakedTrackingVariable. If you change the formatting now, diff will again become > difficult and hence I would recommend not changing it now, but please keep in > mind for the new types you create. I'm aware of this and yes I know I'm a bit lazy here, sorry. OTOH, while coding it is much more important for me to keep related methods next to each other. Sometimes I try to reconcile the alphabetic and the semantical order by choosing names with common prefixes. Actually the name analyseCloseableAllocationArgument is an example for that. So how do you guys cope with this conflict? > 5.Whenever new tracking variable is added through > BlockScope#registerTrackingVariable, the MethodScope#analysisIndex is not > getting added. As of now, there is no impact as this happens only during > analysis and analysisIndex is updated only during resolving, but isn't it a > good idea to update that too? OK, I can say: outerMethodScope.analysisIndex += (outerMethodScope.trackVarCount++); return outerMethodScope.analysisIndex; or even better: return outerMethodScope.analysisIndex++; That means, we can actually avoid the field trackVarCount, nice!
Created attachment 209276 [details] incremental changes This patch captures the changes I made in reaction to comment 44. > > 1. SingleNameReference.java - no change but there is a copyright year change. After this got me into git-trouble, I think I've resolved this now in my local branch. > > - Should the following case report a resource warning > > void foo( InputStream input) throws IOException { > > FileInputStream input1 = (FileInputStream)input; > > System.out.println(input1.read()); > > input.close(); > > } test (plus varaints thereof) added as test063d() > Good point. Yes, we want to identify input and input1 (one branch in my > implementation still misses to unwrap the cast expression). This will be > easily fixed. Done, see FTV#getCloseTrackingVariable() > Secondly, there's some interaction with input being a > parameter. I had hoped we could avoid tracking variables for parameters > (assuming the current method is not responsible for closing), but your > case shows we need a tracking variable but start with a status > indicating that no warnings should be raised. It seems REPORTED would > be a suitable mark here. To avoid confusion I introduced a new bit OBTAINED_FROM_OUTSIDE and consistently use this for values obtained as method argument or from a field read. Otherwise I resolved the issue as described. > > 3. Method names could be better: > > - FakedTrackingVariable#isAutoCloseable() now looks for closeable too. > > I kept confusing myself by looking how Closeable extends AutoCloseable, but > only in 1.7 of course, so right, I guess isAnyCloseable() should be better. Done. > > - FakedTrackingVariable#analyseCloseableAllocationArgument(...) should > > probably be find...(...) > > If we accept that the "AllocationArgument" part can remain implicit by way > of the method arguments 3 & 4, we can say "findCloseTracker". And then I'll > make that method private so we're sure its only used in a context where we > know what a closeTracker is :) Done. > > 5.Whenever new tracking variable is added through > > BlockScope#registerTrackingVariable, the MethodScope#analysisIndex is not > > getting added. As of now, there is no impact as this happens only during > > analysis and analysisIndex is updated only during resolving, but isn't it a > > good idea to update that too? > > OK, I can say: > outerMethodScope.analysisIndex += (outerMethodScope.trackVarCount++); > return outerMethodScope.analysisIndex; > or even better: > return outerMethodScope.analysisIndex++; > That means, we can actually avoid the field trackVarCount, nice! Done (as per the last and cleanest option). I also added a test resembling an example found in the p2 sources: test61m. By means of the one relevant change in handleResourceAssignment() we can also improve here (was: "is never closed" and has been softened to "may not be closed"). For the remaining warning in this test: Detecting that a method call actually creates a wrapper might be nice but is beyond the scope of this bug, I'd say.
(In reply to comment #46) > > 4. Formatting: I found two formatting styles a different from much of the > Now, if the rest of the team feels uneasy about the two-lines style, > I can certainly avoid it (after having expressed my dissent :)) > Should I? In future, yes please.
Here are some more comments, again all minor. I am not done yet! - Tests should probably be at a different place than in TryWithResourcesTest as these run only on Java7. - isAutoCloseable() is redundant in preConnectTrackerAcrossAssignment: There is a place from Assignment this gets called even if null, so may be a non-null check could be good enough! - Only the first argument of an allocation expression is checked for the wrapped logic. Is this because we have seen these pattern ony in the first arguments? - If the type is in the whitelist but if the argument is not closeable, the closeable is not being tracked. Is there a reason? I didn't look at the complete whitelist completley, but found one case with PrintWriter void foo() throws FileNotFoundException { PrintWriter input1 = new PrintWriter("abc.txt"); input1.write(1); } I am fine to err in this side, but just wanted to bring this to your attention - this.trackingVariables.size() could be a variable in BlockScope#checkUnclosedCloseables(...)
Some more.. - Return statement is not being tracked for wrapped. An incorrect error is getting reported. InputStream foo() throws IOException { File file = new File("somefil"); FileInputStream fileStream = new FileInputStream(file); BufferedInputStream bis = new BufferedInputStream(fileStream); return fileStream; } - Isn't the check for BitWrapperCloseable in markPassedToOutside better done at the call site? It doesn't make a difference, IMHO that is better. - WRAPPED flag not being part of flowinfo is not able to take into account of scopes.Take the example of this. bis in the given example is not really closed, but not reported. void foo() throws IOException { File file = new File("somefil"); FileInputStream fileStream = new FileInputStream(file); BufferedInputStream bis = new BufferedInputStream(fileStream); if (bar()) { BufferedInputStream doubleWrap = new BufferedInputStream(bis); doubleWrap.close(); } } boolean bar() { return false; }
(In reply to comment #49) > Here are some more comments, again all minor. I am not done yet! > - Tests should probably be at a different place than in TryWithResourcesTest as > these run only on Java7. Yes (except that a few would then need to explicitly check compliance, because they expect messages like "Resource \'input\' should be managed by try-with-resource\n" which is only flagged at 1.7). Any suggestion for a better place? Maybe FlowAnalysisTests? > - isAutoCloseable() is redundant in preConnectTrackerAcrossAssignment: There is > a place from Assignment this gets called even if null, so may be a non-null > check could be good enough! You mean "rhs.resolvedType != TypeBinding.NULL", right? I can do that, which means I should document the precondition of this method, because otherwise this check will look pretty strange. > - Only the first argument of an allocation expression is checked for the > wrapped logic. Is this because we have seen these pattern ony in the first > arguments? Exactly. > - If the type is in the whitelist but if the argument is not closeable, the > closeable is not being tracked. Is there a reason? I didn't look at the > complete whitelist completley, but found one case with PrintWriter > void foo() throws FileNotFoundException { > PrintWriter input1 = new PrintWriter("abc.txt"); > input1.write(1); > } > I am fine to err in this side, but just wanted to bring this to your attention Good catch. When I wrote that code I mark it with: // remove unnecessary attempts (wrapper has no relevant inner) but your example proves that I should rather treat it as a normal resource, not as a wrapper then. > - this.trackingVariables.size() could be a variable in > BlockScope#checkUnclosedCloseables(...) sure.
(In reply to comment #50) > Some more.. > - Return statement is not being tracked for wrapped. An incorrect error is > getting reported. > InputStream foo() throws IOException { > File file = new File("somefil"); > FileInputStream fileStream = new FileInputStream(file); > BufferedInputStream bis = new BufferedInputStream(fileStream); > return fileStream; > } Interesting pattern :) Let's see what I can do.. no promise yet. > - Isn't the check for BitWrapperCloseable in markPassedToOutside better done at > the call site? It doesn't make a difference, IMHO that is better. Are you saying this, because only one call site actually passes an allocatedType? I was trying to keep the call sites clean, but since it is only one I can push this out again. > - WRAPPED flag not being part of flowinfo is not able to take into account of > scopes.Take the example of this. bis in the given example is not really closed, > but not reported. > void foo() throws IOException { > File file = new File("somefil"); > FileInputStream fileStream = new FileInputStream(file); > BufferedInputStream bis = new BufferedInputStream(fileStream); > if (bar()) { > BufferedInputStream doubleWrap = new BufferedInputStream(bis); > doubleWrap.close(); > } > } > boolean bar() { > return false; > } Yes, some parts of the analysis are not aware of branching. I mentioned in previous comments that complex patterns of assigning resources to locals on only some branches etc. can not be captured by my approach. I'll have to think if I can push the limits for this example. I don't think extending FlowInfo for such corner cases is a good idea. OTOH, a false negative is a pity. I should probably let markClose() propagate info to inner resources, and then avoid checking WRAPPED, but that alone doesn't do the trick. Let's see ...
Here are some more comments: Stephan, we can open separate bugs if necessary. - In twr, the resource is being removed from tracking. Shouldn't the right thing to do is mark as closed. Right now, it doesn't seem to be a problem, but if you fix the 'PrinteWriter' example, this may become important. - may be out of scope of this bug but I get a 'potential resource warning' for this. Is this expected? void foo() throws IOException { Object input1 = bar(); } private FileInputStream bar() { return null; } - AllocationExpression's closeTracker is not getting null'd. Is there a way, these can be null'd. - Getting a wrong error for class Z { FileInputStream input1; public void foo() throws IOException { FileInputStream input = input1; input = new FileInputStream("adfafd"); input.close(); } }
(In reply to comment #51) > Any suggestion for a better place? Maybe FlowAnalysisTests? I think a separate file is better. > You mean "rhs.resolvedType != TypeBinding.NULL", right? > I can do that, which means I should document the precondition of this > method, because otherwise this check will look pretty strange. Yes, you are right.
(In reply to comment #52) > Are you saying this, because only one call site actually passes an > allocatedType? I was trying to keep the call sites clean, but since it > is only one I can push this out again. Yes, that is the main reason, but I thought that made more sense. I understand the call sites could be clean, but some how I felt doing the check out could be cleaner. The decision is yours :).
Created attachment 209373 [details] incremental changes 2 This incremental patch includes the following changes: (In reply to comment #49) > - Tests should probably be at a different place than in TryWithResourcesTest as > these run only on Java7. I created a new class ResourceLeakTests into which I moved all of test056* and test06[1-3]*. For some of the tests this required specifying different expectations (specif. the t-w-r warning), and some only work in 1.5+ etc. While doing the move I also performed a white space cleanup. While this creates some change noise I hope this is acceptable for tests. (I'll attach just the new test file in a minute for your convenience). > - isAutoCloseable() is redundant in preConnectTrackerAcrossAssignment: There is > a place from Assignment this gets called even if null, so may be a non-null > check could be good enough! Done. > - If the type is in the whitelist but if the argument is not closeable, the > closeable is not being tracked. Is there a reason? I didn't look at the > complete whitelist completley, but found one case with PrintWriter > void foo() throws FileNotFoundException { > PrintWriter input1 = new PrintWriter("abc.txt"); > input1.write(1); > } > I am fine to err in this side, but just wanted to bring this to your attention Added (failing) test061n() -> need more work. > - this.trackingVariables.size() could be a variable in > BlockScope#checkUnclosedCloseables(...) done. (In reply to comment #50) > - Isn't the check for BitWrapperCloseable in markPassedToOutside better done at > the call site? It doesn't make a difference, IMHO that is better. done
Created attachment 209374 [details] ResourceLeakTests.java new test file.
(In reply to comment #53) > Here are some more comments: Stephan, we can open separate bugs if necessary. > - In twr, the resource is being removed from tracking. Shouldn't the right > thing to do is mark as closed. Right now, it doesn't seem to be a problem, but > if you fix the 'PrinteWriter' example, this may become important. to be determined once I work on the remaining issues. > - may be out of scope of this bug but I get a 'potential resource warning' for > this. Is this expected? > void foo() throws IOException { > Object input1 = bar(); > } > private FileInputStream bar() { > return null; > } This works exactly as intended. Just rename "bar" to "createInput" and you would most likely complain if the compiler doesn't complain :) > - AllocationExpression's closeTracker is not getting null'd. Is there a way, > these can be null'd. Not an easy one. In case the alloc is embedded in an assignment it gets collected, but "in the event it does NOT get collected by an assignment" is not a good trigger. I could remove in on the next pass (generateCode), but that wouldn't help much I'd say. OTOH, it doesn't seem to hurt, right? > - Getting a wrong error for > class Z { > FileInputStream input1; > public void foo() throws IOException { > FileInputStream input = input1; > input = new FileInputStream("adfafd"); > input.close(); > } > } I get Resource leak: \'input\' is not closed at this location, should be only potential since obtained from a field. I'll take a look - should be easy. For now I will focus on those issues where a warning is missing: comment 49 and comment 50 second example. We shouldn't filter too much!
Created attachment 209384 [details] incremental changes 3 (In reply to comment #58) > (In reply to comment #53) > > - Getting a wrong error for > > class Z { > > FileInputStream input1; > > public void foo() throws IOException { > > FileInputStream input = input1; > > input = new FileInputStream("adfafd"); > > input.close(); > > } > > } > > I get > Resource leak: \'input\' is not closed at this location, > should be only potential since obtained from a field. Correction: should indeed be no warning. > I'll take a look - should be easy. Indeed, fix is simple (attached).
Created attachment 209409 [details] incremental change 4 Hm, maybe indeed we should have taken some of this to a new bug, and maybe I should have pushed this to a public feature branch. Should I still do that? This delivery fixes comment 50 part 2 by completely getting rid of the flag WRAPPER. This flag implemented a heuristic to avoid irrelevant warnings against wrapper resources, but this part, too, is now computed with more precision. This required some more propagating information up and down the links: FTV#markClose() pushes info into the chain of innerTrackers. reportError(..) has more fine grained control to avoid redundant reporting without letting a more severe issue be masked by a minor one. Finally, during BlockScope#checkUnclosedCloseables() I need to ensure that track variables are iterated in correct order: outer resources before their inners, so we can mark inners as reported when we report against an outer. This requires a back link FTV#outerTracker (inverse of FTV#innerTracker) and changed the loop structure from a for-loop to picking elements from a set until it's empty (sorry this spoils the loop optimization we recently did). After fixing the new test061o(), the existing test061f() was broken and it showed that we had been lucky on that one. Now I was faced with fact that try-finally statements are actually analyzed finally-first! InputStream stream = null; BufferedReader reader = null; try { stream = url.openStream(); reader = new BufferedReader(new InputStreamReader(stream)); } finally { reader.close(); } At the close() call we want to propagate the close-info into the inner resources incl. 'stream'. HOWEVER, when looking at the close call we don't even know about the wrapper-chain because the try block hasn't been analyzed yet! So 'stream' was again flagged as leaking. The full solution would be to chain all resource information through FlowContext and subclasses and postpone the whole issue to Eclipse 3.9+. That would be investing to much effort to a minor issue. To shortcut this I let FTV#analyseCloseableAllocation(..) inject info from the finally block back into newly created wrappers. Now, if the try block re-configures the resources (re-assignments, creating more wrappers) this approach will produce imprecise results. But I don't really want to go any further. Other changes in the patch became necessary in order to fix intermediate regressions, but those fixes are essentially straight-forward.
I haven't finished the last patch yet - I will try to finish this latter today. Please feel free to take out any issue as another bug. For now, I have only one comment for the code: - AllocationExpression#analyseCode: Pulling the hasTypeBit() check outside the for loop could be nice.
Here are couple of failures I found now 1. Getting an error for this void foo() throws IOException { File file = new File("somefil"); FileInputStream fileStream = new FileInputStream(file); BufferedInputStream bis = new BufferedInputStream(fileStream); bis = null; fileStream.close(); } 2. Minor error: Error is being reported at the first line, it would be good to have it in the third line. void foo( FileInputStream input) throws IOException { FileInputStream input1 = new FileInputStream("abc"); input1.close(); input1 = new FileInputStream("abc"); }
(In reply to comment #61) > Please feel free to take out any issue as another bug. The version I have in my workspace looks pretty promising, so I think all examples listed in this bug can be covered by one big patch. I'll post another update shortly. I've filed bug 368546 to track remaining false positives.
(In reply to comment #62) > Here are couple of failures I found now > [..] > 2. Minor error: Error is being reported at the first line, it would be good to > have it in the third line. > void foo( FileInputStream input) throws IOException { > FileInputStream input1 = new FileInputStream("abc"); > input1.close(); > input1 = new FileInputStream("abc"); > } I agree, it would be good, but it's not possible with the current approach: we report against a re-assignment only if the old value was not closed, however, the old input1 is closed properly. When we conclude that a resource is *never* closed we always report it against the first location where a resource was assigned to a given variable. We simply don't have the information which assignment brings the leaking instance (could be many potential locations in a branching structure!). Sorry -> wontfix.
Created attachment 209457 [details] incremental changes 5 This patch completes the changes started in comment 60. That strategy change broke a few things and analysis has become more aggressive, so some issues surfaced that previously were hidden. Changes included in this update: Passing more information up-and-down like - FTV#globalClosingState from finally-block into try-block - FTV#markPassedToOutside also loops into inners Carefully construct the order in which BlockScope#checkUnclosedCloseables processes tracking vars: - process wrappers before their inners - respect several situations of wrappers and inners defined in different scopes See FTV#pickVarForReporting for details. The effect is that now we can safely suppress any reports against inners if their wrappers have been closed, without needing the bogus bit WRAPPER that was removed in incremental change 4. Better evaluate the bits REPORTED_POTENTIAL_LEAK, REPORTED_DEFINITIVE_LEAK - at the same level (each invocation of BlockScope#checkUnclosedCloseables) avoid duplicate reporting of wrappers and inners - accumulate these flags from invocations to FTV#reportError - but don't let errors against an error location hide errors against other locations - reset this information at the end for a fresh start at the next level Includes a few more tests modelled from real world bits in the Eclipse SDK.
Created attachment 209460 [details] incremental changes 6 (In reply to comment #62) > Here are couple of failures I found now > 1. Getting an error for this > void foo() throws IOException { > File file = new File("somefil"); > FileInputStream fileStream = new FileInputStream(file); > BufferedInputStream bis = new BufferedInputStream(fileStream); > bis = null; > fileStream.close(); > } Just when I was going to say "no" to this request I found a fairly simple way to disassemble a wrapper chain when the wrapper is being dropped. Two caveats: - this slightly changed the reporting in test063a, but I think that's OK - the trick only works in a flat structure - if the assignment happens in a different scope than the definition of the resource, then I can do nothing, because I cannot create a "conditional wrapper chain" :-/
Created attachment 209464 [details] incremental change 7 (In reply to comment #49) > - If the type is in the whitelist but if the argument is not closeable, the > closeable is not being tracked. Is there a reason? I didn't look at the > complete whitelist completley, but found one case with PrintWriter > void foo() throws FileNotFoundException { > PrintWriter input1 = new PrintWriter("abc.txt"); > input1.write(1); > } > I am fine to err in this side, but just wanted to bring this to your attention > - this.trackingVariables.size() could be a variable in > BlockScope#checkUnclosedCloseables(...) This is the last one I planned to address (test061n failed until now) Wrapper allocations without a proper inner resource are now treated like normal resources. I'm currently re-running all tests, will scan all comments whether anything got forgotten and will post my results together with a single flattened patch.
(In reply to comment #53) > Here are some more comments: Stephan, we can open separate bugs if necessary. > - In twr, the resource is being removed from tracking. Shouldn't the right > thing to do is mark as closed. Right now, it doesn't seem to be a problem, but > if you fix the 'PrinteWriter' example, this may become important. Interestingly this basic example is correctly silent: PrintWriter writer = new PrintWriter(\"filename\"); try (BufferedWriter bw = new BufferedWriter(writer)) { bw.write(1); } But a more involved variant is missing a warning: PrintWriter writer = new PrintWriter("filename"); if (b) { try (BufferedWriter bw = new BufferedWriter(writer)) { bw.write(1); } } Will add this as a disabled test case and add a note to bug 368546.
Created attachment 209472 [details] incremental changes 8 From collecting all left-over items here's my final update for now: (In reply to comment #61) > - AllocationExpression#analyseCode: Pulling the hasTypeBit() check outside the > for loop could be nice. Done. (In reply to comment #50) > - Return statement is not being tracked for wrapped. An incorrect error is > getting reported. > InputStream foo() throws IOException { > File file = new File("somefil"); > FileInputStream fileStream = new FileInputStream(file); > BufferedInputStream bis = new BufferedInputStream(fileStream); > return fileStream; > } I was removing the tracker for 'fileStream' from the scope, but this left 'bis' without information about this return statement. The fix is in 2 steps: - change ReturnStatement to saying markPassedToOutside(..) - add check to BlockScope#checkUnclosedCloseables whether a tracker being analyzed equals the return value, considering also inners. If so, mark as reported to also prevent inners from being flagged. I had to slightly adjust one expected result in NullReferenceTests. The previous version was actually better, but the current outcome is a direct result of weak flow information coming out of a try statement. Not much I can do. Added tests - a forgotten test from coment 66 - tests from comment 68 - test for the above example (from comment 50)
Created attachment 209482 [details] Tests & fix v1.0 After final editorial changes and another full test run this is my release candidate for now. Functionally this is equivalent to the sum of all incremental changes. Thanks, Satyam, for all your comments so far. I think with our joined desire for perfection we morphed a set of heuristics into a pretty sharp analysis :) I hope my set of incremental changes wasn't too much a moving target for your review. Let me know whether you see the need for further changes.
(In reply to comment #70) > Created attachment 209482 [details] > Tests & fix v1.0 > > After final editorial changes and another full test run this is my > release candidate for now. Functionally this is equivalent to the sum > of all incremental changes. Satyam, I agree with the plan to treat this patch as the release candidate. Unless your further review and verification uncovers ship stopper issues, we'll address them via follow up defects post M5. Stephan may be offline Monday & Tuesday. So you may have to release it on his behalf - Thanks!
Stephan, I just had a customary look now and will try to complete the review tomorrow. I saw one typo as of now. In BlockScope#checkUnclosedCloseables():1059 I believe it should be trackingVariables.get(i) instead of trackingVariables.get(0).
(In reply to comment #72) > Stephan, > I just had a customary look now and will try to complete the review tomorrow. I > saw one typo as of now. > In BlockScope#checkUnclosedCloseables():1059 > I believe it should be trackingVariables.get(i) instead of > trackingVariables.get(0). Of coures, well spotted, eagle-eye! [Shouldn't the compiler be able to detect such stupid typos? :) ]
Couple of minor comments - FakedTrackingVariable#findCloseTracker(): allocation is not used. - May be a good idea to add a small comment in TWRTest that test056xx has been moved to new test. With this, I conclude the review :). +1 for the patch. Please release the patch if you can. If you are not able to today, I can release it tomorrow. Stephan, Thanks very much for your explanations and for taking care of all the comments very quickly.
After applying final polish (see comment 74) I've pushed commit 8d45cb26fc5ad244f93e8632d761d46ad4a120cf which resolves this issue.
(In reply to comment #75) > After applying final polish (see comment 74) I've pushed > commit 8d45cb26fc5ad244f93e8632d761d46ad4a120cf > which resolves this issue. I want to add one point. This patch doesn't change the default for this problem and I think it is better to be left like that for this release. We could change to a warning in 3.8M1.
(In reply to comment #76) > (In reply to comment #75) > > After applying final polish (see comment 74) I've pushed > > commit 8d45cb26fc5ad244f93e8632d761d46ad4a120cf > > which resolves this issue. > I want to add one point. This patch doesn't change the default for this problem > and I think it is better to be left like that for this release. We could change > to a warning in 3.8M1. I was going to ask about this. I'd propose to await progress in bug 368546. If I can resolve that bug during milestone X, maybe after positive experience we can change the default early during milestone X+1? We should certainly coordinate with Kim to clarify which version will be used to build the SDK at what point in time. Doing the change for 3.8M1 is a good one :)
> We should certainly coordinate with Kim to > clarify which version will be used to build the SDK at what point in time. Kim normally switches to the latest milestone compiler after the milestone. At any rate, the switch should never happen close to a milestone because the switch can - result in new warnings/errors being detected - different bytecode for unchanged source which causes comparator test failures
(In reply to comment #77) > > and I think it is better to be left like that for this release. We could change > > to a warning in 3.8M1. > Doing the change for 3.8M1 is a good one :) Ofcourse you caught me :(, but for the record, I meant 3.9M1.
Stephan, do you know what is the percentage of false positives now? If it is even 20-25% I would toggle the default in the beginning of M6. In any case I think we should aim latest for M7 to toggle the default. M7 is supposed to be the 'polish' milestone, we can nudge people to fix resource leaks during that time.
(In reply to comment #79) > Ofcourse you caught me :(, but for the record, I meant 3.9M1. +1
Verified using build I20120122-2000
