Community
Participate
Working Groups
Inspired by the bug pattern from bug 349121 I propose to add a warning to cases where try-with-resources should perhaps be used but is not. I suggest the following pattern: - assignment or initialization of local variable - assigned value is a subtype of Closeable IMHO this could speed up adoption of this very useful new feature, pin-pointing potentially dangerous code. That warning would trigger some false positives, so a) it should be suppressable b) some more heuristics could be useful to exclude unlikely candidates. E.g., a typically false positive would be a method that returns the Closeable to its client but stores intermediate results in a local variable. This method would *not* be responsible for closing. The heuristic could be as simple as "if the method returns a Closeable" and ideally "if the assigned value can flow into a method return" (not sure if the compiler can be made to detect that). A corner case where the warning would be useful but is not captured by the initial pattern: getInputStream().read(bytes); This can be described as: a method call returning Closeable is used as the receiver for another method call. Ideally, this analysis would be directed by some ownership model :) but as Java doesn't support the concept of ownership this would have to be approximated by other analyses, but I'm confident that these can be made precise enough to be really helpful. Sorry if this has been discussed before, but I couldn't find a bug yet.
(In reply to comment #0) > - assignment or initialization of local variable > - assigned value is a subtype of Closeable Stephan, are you referring to java.io.Closeable or java.lang.AutoCloseable ? I have been thinking of providing a quick assist and clean-up to modify code to use try with resources. But the pattern that I had in mind was - a variable of type java.lang.AutoClosable which is closed explicitly in a finally block. The code modification could remove the explicit close with a try with resources block. However, the example on your blog (http://blog.objectteams.org/2011/06/a-use-case-for-java-7/) has a nice example where JDT could probably help. But I think the warning we should suggest in this case should be "Potential resource leak: Resource is never closed". In pre 1.7 world we look for variables of type java.io.Closeable to create this warning, and the quick fix can add an explicit close in a finally block. In 1.7 we look for variables of type java.lang.AutoCloseable to create this warning, and the quick fix can use try with resources.
(In reply to comment #1) > (In reply to comment #0) > > - assignment or initialization of local variable > > - assigned value is a subtype of Closeable > Stephan, are you referring to java.io.Closeable or java.lang.AutoCloseable ? Oops, to be honest when I wrote that I didn't think of the difference. > I have been thinking of providing a quick assist and clean-up to modify code to > use try with resources. But the pattern that I had in mind was - a variable of > type java.lang.AutoClosable which is closed explicitly in a finally block. The > code modification could remove the explicit close with a try with resources > block. That's fair. This would be a semantics preserving cleanup ... What about explicit closing not in a finally block? > However, the example on your blog > (http://blog.objectteams.org/2011/06/a-use-case-for-java-7/) has a nice example > where JDT could probably help. ... this is a real "fix", right? :) > But I think the warning we should suggest in > this case should be "Potential resource leak: Resource is never closed". I agree. > In pre 1.7 world we look for variables of type java.io.Closeable to create this > warning, and the quick fix can add an explicit close in a finally block. In 1.7 > we look for variables of type java.lang.AutoCloseable to create this warning, > and the quick fix can use try with resources. Sounds good. I'll take this bug (for the warning), which will also be a nice subject to familiarize myself with the Java7 branch.
(In reply to comment #2) > > I have been thinking of providing a quick assist and clean-up to modify code to > > use try with resources. But the pattern that I had in mind was - a variable of > > type java.lang.AutoClosable which is closed explicitly in a finally block. The > > code modification could remove the explicit close with a try with resources > > block. > > That's fair. This would be a semantics preserving cleanup ... > What about explicit closing not in a finally block? Good point! Captured this in bug 349390.
I'm a bit confused by comment 0. Are the warnings being raised on a method call? Or on resource declarations such as "FileReader f = new FileReader("")", where f has not been explicitly closed? I would expect the latter to be the case, but then the heuristics discussed in comment 0 won't really apply.
(In reply to comment #4) > I'm a bit confused by comment 0. Are the warnings being raised on a method > call? Or on resource declarations such as "FileReader f = new FileReader("")", > where f has not been explicitly closed? I would expect the latter to be the > case, but then the heuristics discussed in comment 0 won't really apply. You're right. Sorry, my plans are still a moving target :) I think I'll tackle this in the following steps: a) detect expression of (sub)type of Closeable or AutoCloseable [easy in theory - watch out for impact on performance] b) detect if inside the resources part of try-with-resources c) investigate special flow analysis from a) to close() would be cool to have but needs investigation before committing to it If c) seems not viable => stick to the weaker analysis: "is not captured in try-with-resources" which says nothing about close, except that close is not automatic per t-w-r.
Created attachment 198503 [details] sketch of impl and some tests Here is an early draft of how this could work. Contained are some infrastructure changes that we should probably discuss before I continue and elaborate all the details: - to determine expressions of subtype of AutoCloseable I added eager propagation of type information in ClassScope.connectSuperclass(), connectSuperInterfaces() and BinaryTypeBinding.superclass() and superInterfaces(). - that info is stored in a new field ReferenceBinding.typeBits, which could in the future include similar information for other well-known types, too. - I introduced the concept of a FakedTrackingVariable: a special local variable declaration whose sole purpose is for special flow analysis. Here faked null info is used to encode interesting events during the flow. This infra structure is applied as follows: LocalVariableBinding may store a "closeTracker" that keeps track if the variable has a value of type AutoCloseable and whether this object has been closed (potentially, definitely). At certain points the null status of the tracking variables (if any) is evaluated for reporting: end of block, end of method, return statement. As the tests demonstrate this sketch can already detect a number of interesting situations. Issues not yet handled by the patch include: - cope with several tracking variables within the same scope (need to assign distinct ids) - cope with re-assigning to the same variable - consider exceptions and try-catch-finally - make new diagnostics configurable (suppressable?) - improve wording of warning messages - extend solution to Closeable if compliance < 1.7 - copy typeBits for generic types (e.g., DirectoryStream<T>). Note: I used null-info rather than def-assign-analysis because the former is/can be more precise because it's not limited by the JLS :)
Olivier, do you want to comment on the infrastructure changes outlined in the previous comment?
I think it is worth investigating it.
Stephan, a question.. The warning will come iff a variable of type AutoCloseable or Closeable is created but is not closed a) either via twr or explicitly b) via twr I assume the answer is (a), no ?
(In reply to comment #9) > Stephan, a question.. > > The warning will come iff a variable of type AutoCloseable or Closeable is > created but is not closed > a) either via twr or explicitly > b) via twr > > I assume the answer is (a), no ? I do consider both ways of closing. In fact the warning currently (and a bit clumsily) states: "Value of type AutoCloseable is not closed neither explicitly nor using a try-with-resources" Are you asking for a marker that can be used for a quick assist that offers to convert explicit closing to using twr? The analysis should be able to provide that, too, if we agree on a pattern for that warning, like - local variable of type AutoCloseable - explicit invocation of "close" - what if close is inside a conditional? - what if close is (not) within finally? I suppose, if any path exists where *no* close occurs the stronger warning ("not closed on all paths") should be issued. Only if the code is already safe, a style-warning should be given suggesting to use twr. Does this answer your question?
(In reply to comment #10) > In fact the warning currently (and a bit clumsily) states: > > "Value of type AutoCloseable is not closed neither explicitly nor using a > try-with-resources" This is good, it is the right thing to do. I just wanted to confirm with you that you have the same in mind :) > Are you asking for a marker that can be used for a quick assist that offers > to convert explicit closing to using twr? I don't think that we need a warning for this. A cleanup and/or quick assist will be sufficient.
> > Are you asking for a marker that can be used for a quick assist that offers > > to convert explicit closing to using twr? > I don't think that we need a warning for this. A cleanup and/or quick assist > will be sufficient. The problem is that the correct analysis is not trivial, and the compiler will implement it anyway. So I think it would be good to have a separate warning from the compiler (off by default), but we shouldn't add this to the Errors/Warnings page. JDT UI would just use it internally in the clean up / quick assist.
(In reply to comment #12) > > > Are you asking for a marker that can be used for a quick assist that offers > > > to convert explicit closing to using twr? > > I don't think that we need a warning for this. A cleanup and/or quick assist > > will be sufficient. > > The problem is that the correct analysis is not trivial, and the compiler will > implement it anyway. So I think it would be good to have a separate warning > from the compiler (off by default), but we shouldn't add this to the > Errors/Warnings page. JDT UI would just use it internally in the clean up / > quick assist. If it helps I can propose such (internal) warning in my next patch.
Stephan, are you targeting this for 3.7.1 or 3.8 ? Note that for this to make it in 3.7.1 it needs to be implemented by sometime next week.
(In reply to comment #14) > Stephan, are you targeting this for 3.7.1 or 3.8 ? Note that for this to make > it in 3.7.1 it needs to be implemented by sometime next week. Really? I wasn't aware of any deadlines that soon, but it'd be cool if we'd make it for 3.7.1. I'll attach a new patch in a minute, so that you can start making experiments with the quick fixes and assists. While we're at it: what's the policy/deadlines for adding new API (like new problem IDs in JavaCore) for SR1? And: how long to actual feature freeze / code freeze for 3.7.1? When targeting 3.7.1 should the analysis be constrained to AutoCloseable? We could still add similar analysis for Closeable (for 1.6-) to 3.8.
Created attachment 199192 [details] snapshot v0.2 Some progress: + New warning if a resource is definitely closed by an explicit call to close() - subject to conversion to using t-w-r (as promised in comment 13). + Also handles re-assignments, checking that both values before and after the assignment are closed properly. + Make new diagnostics configurable (support in CompilerOptions and JavaCore, no @SuppressWarnings - yet). 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. + More robust initialization of typeBits. For BinaryTypeBinding this requires to propagate calls to superclass() and superInterfaces() up the hierarchy. This *could* have a small impact on performance, but this is the least expensive solution I could find so far. + Handle multiple local variables within the same method. + Handle ParameterizedTypeBinding and subclasses. Remaining TODOs from my list: - discuss @SuppressWarnings - polish wording of messages and javadoc (JavaCore) - extend solution to Closeable if compliance < 1.7 - consider precise analysis with exceptions and (normal) try-catch-finally - discuss transfer of responsibility when passing the resource as an argument in a method call Is this patch sufficient for working on the UI side / related bugs?
patch v0.2 causes some regressions in TryWithResourcesStatementTest (bogus warnings). I'm currently working on fixing those.
Created attachment 199216 [details] patch v0.4 with regressions fixed This patch passes all of tests.compiler.regression Work done: - adjust 2 tests in TryWithResourcesStatementTest (expect 1 more warning) - avoid secondary error when re-assigning a resource in t-w-r - implement initialization strategies for typeBits for all relevant subclasses of ReferenceBinding - categorize new problems and adjust CompilerInvocationTests & BatchCompilerTests Time for an initial review, anyone?
Stephan, I get a few conflicts when I try to apply this patch. Can you please merge with BETA_JAVA7 and create a new patch.
Created attachment 199235 [details] patch v0.5 Patch with latest updates from CVS. Also: respect two kinds of nesting: - nested blocks (test055h) analyze for each local var at the end of its declaring block - methods in local types (test055g) if resource is closed in a method of a local type treat as potentially closed, since we don't analyze if that method is actually invoked on all paths
Thanks Stephan! I tried the patch and it looks promising. One question, if a resource is passed to another method it is still marked as not closed. e.g. FileReader reader1 = new FileReader("file1"); try { int ch; bar(reader1); while ((ch = reader1.read()) != -1) { System.out.println(ch); } } finally { reader1.close(); } So the assumption you make is that the called method will not close the resource. The warning message does not indicate that it could be potentially closed. Is the problem id different in this case ?
(In reply to comment #21) > Thanks Stephan! I tried the patch and it looks promising. > > One question, if a resource is passed to another method it is still marked as > not closed. e.g. > > FileReader reader1 = new FileReader("file1"); > try { > int ch; > bar(reader1); > while ((ch = reader1.read()) != -1) { > System.out.println(ch); > } > } finally { > reader1.close(); > } > > So the assumption you make is that the called method will not close the > resource. The warning message does not indicate that it could be potentially > closed. Is the problem id different in this case ? This issue is still on my TODO list :) I'm planning to mark reader1 as potentially closed, just as you expected. Should be easy to implement, so I first focused on harder problems ...
Created attachment 199358 [details] patch v0.6 New in this patch: + Includes exceptional exits in the analysis. QUESTION: should we use a different message like "Resource of type AutoCloseable is not closed if exception {0} is thrown" ? + Mark resource as potentially closed if passed as an argument to another method. Remaining issues: - for exception analysis I introduced a method on UnconditionalFlowInfo that doesn't yet handle extraBits. - should the new warnings be suppresseable, e.g., with token "resource"? - polish wording (error messages and JavaCore javadoc). Almost done ...
Marking my goal to get this included in 3.7.1.
Created attachment 199378 [details] patch v0.7 Consolidate analysis in the presence of exceptions and loops: + Handle more than 64 variables (i.e., respect FlowInfo.extraBits) + Consider break and exception exits from all kinds of flow context and improve specific logic to merge null info + fix a quirk in existing query methods of UnconditionalFlowInfo: Was checking extraBits[0] for null although only cells 2-5 are relevant here. Without this the new impl. would cause NPE.
Thanx Stephan. I(or someone from the team) will try and review the patch sometime next week. I guess apart from testing an review, its very imp. to get performance numbers since the fix tries to do some kind of static analysis which may be expensive in complex cases. We will have to make a call based on all that about whether the fix goes in 3.7.1 or 3.8 (the patch looks fairly complex and may need some cycles to get reviewed properly - I'm not saying though, that this won't make it to 3.7.1, but may be a possibility. 3.7.1 is maintenance branch so nothing goes in without review). Did you also try to host jdt.core plugin or your object teams plugin with the compiler patched with this fix? That will make sure there are no stray exceptions.
Created attachment 199385 [details] patch v0.8 (In reply to comment #26) > Thanx Stephan. I(or someone from the team) will try and review the patch > sometime next week. Great. > I guess apart from testing an review, its very imp. to get > performance numbers since the fix tries to do some kind of static analysis > which may be expensive in complex cases. Indeed. I had performance in mind when developing this analysis, but we'll only know for sure once we have numbers. Who'll run the performance tests and report number back here? (I don't have access to the DB). > We will have to make a call based on > all that about whether the fix goes in 3.7.1 or 3.8 (the patch looks fairly > complex and may need some cycles to get reviewed properly - I'm not going to downplay the complexity :) When reviewing, please let me know if the code is unclear, I don't want to waste your cycles decyphering cryptic code. > Did you also try to host jdt.core plugin or your object teams plugin with the > compiler patched with this fix? That will make sure there are no stray > exceptions. I made some self hosting experiments with org.eclipse.osgi in the runtime workspace, which showed a number of false positives. This is what the latest patch (v0.8) improves to reduce false positives: + include (Qualified)AllocationExpressions in analysis: passing a resource means callee might close the resource + exclude values passed as argument into the current method, caller remains responsible + don't complain if a resource is actually null at the given point (return, re-assignment) + recognize if assignment/initialization failed due to an exception (order of processing in {LocalDeclaration/Assignment}.analyseCode()) There're a few more of those and after that I'll populate the runtime workspace with more projects to check for any new exceptions/bugs.
Created attachment 199402 [details] patch v0.9 Looking at some more false positives in real life it turns out that precise analysis in the context of multiple (nested?) try statements is far from trivial using the existing null analysis. I tried hard to distinguish the situations of fr1 and fr2 in FileReader fr1 = new FileReader(someFile); try { FileReader fr2 = new FileReader(someFile); fr1.read(buf); fr2.read(buf); fr2.close(); } finally { fr1.close(); } With patch v0.8 it would report that fr2 would not be closed on all paths (due to possible exception before close). However, that patch would give a false positive in a nested try: try { FileReader fr3 = new FileReader(someFile); try { fr3.read(buf); } finally { fr3.close(); } } catch (IOException e) { } saying that fr1 is not closed on all paths. This happens because the outer try statement assumes that its try block can be exited at any point. I think its more important to avoid such false positives and rather live without the distinction regarding fr1 and fr2. With the new patch it will now assume that fr2 is properly closed and only suggest to use t-w-r instead of explicitly closing (i.e., we just get a weaker warning). With this change patch v0.9 is actually a bit simpler than v0.8, but some situations involving multiple try statements (even sequentially) need still more investigation. Current status when compiling org.eclipse.osgi: - 7 messages about (definitely) missing call to close() All seem good, compiler cannot know if/why these patterns are safe - 76 warnings that close() may be missing on some path Most of these look good, yet a few of these still look suspicious, I'll take another look at those.
Created attachment 199435 [details] patch v0.9.1 (In reply to comment #28) > With this change patch v0.9 is actually a bit simpler than v0.8, but some > situations involving multiple try statements (even sequentially) need still > more investigation. Latest patch fixes this issue: don't overeagerly report a problem when seeing pot.nonnull (=potentially closed), without seeing pot.null. > Current status when compiling org.eclipse.osgi: > - 7 messages about (definitely) missing call to close() > All seem good, compiler cannot know if/why these patterns are safe > - 76 warnings that close() may be missing on some path > Most of these look good, yet a few of these still look suspicious, > I'll take another look at those. Those 76 warnings are now down to 65. test055o() documents the last false positive I could find. Investigating.
Created attachment 199443 [details] patch v0.9.2 (In reply to comment #29) > test055o() documents the last false positive I could find. Investigating. fixed by special analysis of if-else: if resource is closed on one branch and known to be null on the other consider it definitely closed in the merged flow. > > Current status when compiling org.eclipse.osgi: > > - 7 messages about (definitely) missing call to close() > > All seem good, compiler cannot know if/why these patterns are safe > > - 76 warnings that close() may be missing on some path > > Most of these look good, yet a few of these still look suspicious, > > I'll take another look at those. > > Those 76 warnings are now down to 65. Down to 41 warnings, could not find any false positives. Also applied analysis to JDT/Core itself: no exceptions, a number of new errors/warnings, which all look interesting. => RELEASE CANDIDATE :) (sauf polish: wording, suppress warnings token ...).
Markus/Deepak, can you please look at the error messages and see if they're ok. Also, Stephan, is the new analysis being done like null analysis, where we only report the errors/warning based on the configured options but still do the whole analysis anyway? Or does the analysis only come into action when the relevant options are set to error/warning? I would say we should do the latter. Also, the defaults should be ignore for all options, unless you feel one of those will have the slightest possible overhead. I don't think its a good idea to hit the compiler's performance, even by 10-15% because of any new analysis. Adding Satyam to cc list for performance numbers.
(In reply to comment #31) > Also, Stephan, is the new analysis being done like null analysis, where we only > report the errors/warning based on the configured options but still do the > whole analysis anyway? Or does the analysis only come into action when the > relevant options are set to error/warning? I would say we should do the latter. A quick theoretical answer regarding performance: I have not added any up-front checks for avoiding certain branches (yet). The initial trigger is when the compiler detects an expression of type AutoCloseable (inside a local declaration or an assignment etc.). That's why I avoid using queries like isCompatibleWith() but eagerly record subtypes of AutoCloseable, so checking for AutoCloseable is really cheap. If no AutoCloseables are found there *should* be *no* noticeable performance impact. If numbers show a degradation I can certainly add checks whether the analysis is enabled at all. > Also, the defaults should be ignore for all options, unless you feel one of > those will have the slightest possible overhead. I don't think its a good idea > to hit the compiler's performance, even by 10-15% because of any new analysis. I see no reason why the overhead should be anything close to a two-digit percentage (unless perhaps *all* your variables have type AutoCloseable :) Actually I do hope that performance will give no reason to disable any of these warnings, but let's see ... > Adding Satyam to cc list for performance numbers. I'm looking forward to those numbers, since without them all we can do is speculate :) The patch v0.9.2 should be a reasonable basis for performance tests.
Let's move this to 3.8 as JDT/UI cannot use it extensively for 3.7.1. This is something we want to get for 3.8, but since the cut for string externalization is this week, it is too short to polish them and make sure they are the ones we want in the end. Stephan, please keep the patch alive for the 3.8 stream. As soon as 3.8 is open for development again, this will be the good time to release it.
OK, two things before I suspend work on this issue: Satyam, can you please provide performance data for the latest patch? I'd love to know whether the patch needs considerable reworking. For the externalized strings / warning messages the patch contains: 884 = Value of type AutoCloseable is not closed on all paths. 885 = Instance '{0}' of type AutoCloseable may be unclosed at this point. 886 = Value of type AutoCloseable is not closed neither explicitly nor using a try-with-resources. 887 = Instance '{0}' of type AutoCloseable is not closed at this point. 888 = Instance '{0}' should be managed by try-with-resource. In an email I proposed an alternative set of messages: 884 = Potentially leaking resource '{0}': is not closed on all paths. 885 = Potentially leaking resource '{0}': may not be closed at this location. 886 = Leaking resource '{0}': is never closed. 887 = Leaking resource '{0}': is not closed at this location. 888 = Resource '{0}' should be managed by try-with-resource. The placeholder always represents the name of a local variable. Comments welcome.
(In reply to comment #34) > Satyam, can you please provide performance data for the latest patch? > I'd love to know whether the patch needs considerable reworking. Yes, we need numbers now. > 884 = Potentially leaking resource '{0}': is not closed on all paths. > 885 = Potentially leaking resource '{0}': may not be closed at this location. > 886 = Leaking resource '{0}': is never closed. > 887 = Leaking resource '{0}': is not closed at this location. > 888 = Resource '{0}' should be managed by try-with-resource. I would merge the first two into: The resource ''{0}'' might not have been closed. Something similar to the error message for uninitialized local variables. If you want the ' to be preserved, I think you need to double them as there is substitution in the message with the {0}. Thanks for the work. You can be sure this goes into 3.8. This is a very valuable analysis.
(In reply to comment #35) > > 884 = Potentially leaking resource '{0}': is not closed on all paths. > > 885 = Potentially leaking resource '{0}': may not be closed at this location. > > 886 = Leaking resource '{0}': is never closed. > > 887 = Leaking resource '{0}': is not closed at this location. > > 888 = Resource '{0}' should be managed by try-with-resource. > I would merge the first two into: > The resource ''{0}'' might not have been closed. Thanks for the comment. Is that message clear for the following example: FileReader fr = new FileReader(file); if (foo) fr.close(); if (bar) return; // <- warning here: "The resource 'fr' might not have been closed" fr.close(); Or a simpler example with a definite problem: FileReader fr = new FileReader(file); if (bar) return; // <- warning here: "The resource 'fr' has not been closed" fr.close(); Or: FileReader fr = new FileReader(file); fr = new FileReader(file2); // <- warning: "The resource 'fr' has not been closed" Message 884 gives a warning for all paths starting at the declaration of a local variable (warning pointing to the declaration), whereas 885 is given at early returns and re-assignment, when a reference to a resource is forgotten without closing; this message points to the return/re-assign to give a more precise hint. > Something similar to the error message for uninitialized local variables. If > you want the ' to be preserved, I think you need to double them as there is > substitution in the message with the {0}. I didn't think about that, but interestingly the ' is actually preserved as it is. Looking through messages.properties I could actually see some inconsistencies, maybe both forms are supported alike?
(In reply to comment #34) > Satyam, can you please provide performance data for the latest patch? > I'd love to know whether the patch needs considerable reworking. > I am in the process of getting the performance numbers. I should get you the numbers by tomorrow.
(In reply to comment #37) > (In reply to comment #34) > > > Satyam, can you please provide performance data for the latest patch? > > I'd love to know whether the patch needs considerable reworking. > > > I am in the process of getting the performance numbers. I should get you the > numbers by tomorrow. With 1.4 projects, I don't see any noticeable regressions - If any, it should be less than 2%, which I will try to validate and update.
(In reply to comment #38) > With 1.4 projects, I don't see any noticeable regressions - If any, it should > be less than 2%, which I will try to validate and update. Cool, thanks!
Created attachment 199812 [details] patch v0.9.2b same patch as before, updated to latest from CVS
Ayush, do you want me to update the patch for your review, or is the latest patch OK for that purpose? I could also add a new suppress warnings token while updating (should that be "resource"?)
Created attachment 201450 [details] patch updated to HEAD Here's the patch updated to HEAD and with copyright updates. In addition to cosmetic changes (for easier reviewing :) ) I've included: + handling of a new suppress warnings token "resource" (+ a test) + updated error/warning messages to mention "resource" (see also comment 34 ff.) + avoid bogus import of Assert (not available for batch compiler) + safety for UnresolvedReferenceBinding + new test using a type variable with AutoCloseable bound.
I was going to summarize the strategies in the patch, here you go: Eager determination of subtypes of AutoCloseable: See TypeIds.BitAutoCloseable and ReferenceBinding.typeBits / hasTypeBit(int) Initialization for SourceTypeBinding: - during ClassScope.connectSuperclass() / connectSuperInterfaces() Initialization for BinaryTypeBinding: - on-demand during BinaryTypeBinding.superclass() / superInterfaces() also triggert from hasTypeBit(int). This means slightly more eager resolving of supertypes The other trick is the introduction of FakedTrackingVariable, see the class comment. Most changes throughout the compiler.ast package deal with updating the "nullness" state for a faked tracking variable, notably: - Assignment / LocalDeclaration: mark fresh assignment of a resource by setting the corresponding null status to "definite null". - MessageSend: mark invocations of "close()" by setting the corresponding null status to "definitely non-null". - arguments in a message send / ctor call: mark as "potentially non-null", saying: the method/ctor receiving this resource as its argument might actually close it, responsibility is unclear - ReturnStatement: when returning a resource mark as pot-non-null (might be closed by the caller receiving this return value). - TryStatement: make special note of resources in t-w-r - IfStatement: explicit merging of close-information from then+else branches Reporting happens at these events: - Assignment: a previously assigned resource might leak/remain unclosed - Block,MethodDeclaration: at ending of block report unclosed resources NOTE: this might be missing in ConstructorDeclaration(?) - ReturnStatement: resources not-closed at this location Next, some new methods in the compiler.flow package plus BlockScope are introduced to compute specific analysis results like *after* a given flow context. UnconditionalFlowInfo fixes an array index weirdness which otherwise causes NPE with the patch. Finally a summary of new fields: ReferenceBinding.typeBit (see above) LocalVariableBinding.closeTracker (nullable): link from the real variable to the FakedTrackingVariable back reference is FakedTrackingVariable.originalBinding (non-null) MethodScope.trackVarCount: number of tracking variables in this method (including nesting) for computing a valid fresh variable position "back reference" is FakedTrackingVariable.methodScope BlockScope.trackingVariables: list of all FakedTrackingVariables representing resources declared directly in this scope (for reporting at the end of the scope). HTH
Created attachment 202016 [details] updated patch for HEAD Had to once again re-sync the patch
(In reply to comment #44) > Created attachment 202016 [details] [diff] > updated patch for HEAD > > Had to once again re-sync the patch Oops. A stray buildnotes file also got into the patch. That should be excluded.
Just a few things I found at a first glance: - Currently, in BlackScope.registerTrackingVariable(..) , suppose there are 3 variables in the method. So analysisIndex = 3, and index of last variable = 2 (starts at 0). But now the first tracking variable's index will be 4. Index 3 doesnt have anything then. While this shouldnt cause any problem, just making sure this is intended. - It'll be good to test the behaviour with dead code in the picture viz. FileReader fr = ... if (true) return; fr.close(); - Also this case (when one of the if-else branch is evaluated to a DEAD_END): FileReader fr = ... int b; fr.close(); if (b == 1) { fr.open(); return; } else { sysout(..) } return; // Should not complain about fr
Deepak/Markus, here's something I and Stephan discussed on which you could comment maybe We currently have a warning "Resource '{0}' should be managed by try-with-resource." This is given for all Autocloseable resources that are definitely closed by the user but those that don't occur inside a TWR. However, when we know that the user is in 1.7 compliance and that the resource is auto-closeable, don't we know by default that its best to use TWR? So maybe a warning "Autocloseable Resource '{0}' should be managed by try-with-resource". on all resources that are of type autocloseable and that have been declared outside a TWR statement also looks good . What do you think?
(In reply to comment #47) > Deepak/Markus, here's something I and Stephan discussed on which you could > comment maybe My suggestion is: let the compiler identify the problem (e.g., "Potentially leaking resource '{0}': is not closed on all paths"), and let the UI (quickfixes) make suitable suggestions for improvement (like: "Manage resource '{0}' with try-with-resources"). The warning which Ayush quoted is my fallback, if for normal flows everything is already OK, but could still be improved for safety and readability. Or should compiler and quickfixes speak more the same language? What's your say?
Created attachment 202189 [details] Patch v0.9.4 When I tried to explain the story behind ExceptionHandlingFlowContext.improveNullInfoForExceptionExits(..) I found that this wasn't quite working as intended. The relevant test is test056l(), which has two close() calls in a finally block. Without special treatment the exceptions that can be thrown from those close() calls will shed doubt on the resources' status and warnings will be flagged against both, which I consider undesirable. In the attached patch I took a new approach by passing a flag signaling if recordHandlingException(..) is called for the first exception that may occur in a finally block. If so, and if the location is actually a close() call on a resource I override the corresponding bit, indicating that the close() call should be considered successful despite the exception. However, for subsequent close() calls the doubt from the previous exception is actually relevant. I also took the liberty of changing the bit constants in CompilerOptions to avoid conflicts with bug 186342. Issues mentioned in comment 46 are not yet addressed.
Created attachment 202721 [details] Patch v0.9.5 New patch is updated to head again an resolves these issues: (In reply to comment #46) > Just a few things I found at a first glance: > - Currently, in BlackScope.registerTrackingVariable(..) , suppose there > are 3 variables in the method. So analysisIndex = 3, and index of last > variable = 2 (starts at 0). But now the first tracking variable's index > will be 4. Index 3 doesnt have anything then. While this shouldnt cause any > problem, just making sure this is intended. fixed. > - It'll be good to test the behaviour with dead code in the picture viz. > FileReader fr = ... > if (true) > return; > fr.close(); This is test056q() and required the following changes: - check for problems also when entering dead code (Statement.complainIfUnreachable(..)) - had to extend that signature with a FlowContext - at a dead end nullStatus(..) is too shy for the task at hand -> need to temporarily reset reachability info to get precise null info (FlowContext.getNullStatusAfter(..)) - defer actual error reporting so the strongest error can win - record issues using FakedTrackingVariable.recordErrorLocation(..) - issues are prioritized in BlockScope.checkUnclosedCloseables(..) > - Also this case (when one of the if-else branch is evaluated to a DEAD_END): > FileReader fr = ... > int b; > fr.close(); > if (b == 1) { > fr.open(); > return; > } else { > sysout(..) > } > return; // Should not complain about fr I derived test056r() from this, pls. check if it matches your intention. Required this change to pass: - *inside* dead code do not report any problems (was reporting at the first return statement)
Move to M3 to give time for Ayushman to review it after his vacations.
I've spent more time reviewing the patch and I didnt quite find anything else major. Just a few very minor points: - MethodScope.trackVarCount has not been initialized. Safer if done so. - In markPotentialClosing, flowInfo.copy is used to create another flowInfo which is a copy of the original one, but I think flowInfo.unconditionalCopy should be used instead because copy() does not guarantee that the original flowInfo will not have side effects. Can you cross-check this? - We can think about initializing the ArrayList of FakedTrackingVariable with some initial capacity instead of default 10. Each scope has its own array list. So a try inside a for loop inside a block in a method will actually have 40 wasted memory slots if no Autocloseables are used.
(In reply to comment #52) > I've spent more time reviewing the patch thanks! > - MethodScope.trackVarCount has not been initialized. Safer if done so. sure, will do so. > - We can think about initializing the ArrayList of FakedTrackingVariable with > some initial capacity instead of default 10. Sure. > Each scope has its own array list. No, the list is allocated lazily at the first call to registerTrackingVariable(), but still using initial capacity of - say - 3 shouldn't hurt. > - In markPotentialClosing, flowInfo.copy is used to create another flowInfo > which is a copy of the original one, but I think flowInfo.unconditionalCopy > should be used instead because copy() does not guarantee that the original > flowInfo will not have side effects. Can you cross-check this? That's the tricky part. I must admit I don't fully understand the consequences of using unconditionalCopy vs. copy. What side effects do you mean? If we have an UnconditionalFlowInfo, both methods actually do the same. In order to perform experiments I tried to feed a ConditionalFlowInfo into that method and I got the impression, that by design it should not be possible, as the vast majority of call chains that could produce a ConditionalFlowInfo soon after call unconditionalInits(). Bug 358007 is an example where calls to unconditionalInits() are missing, by mistake I believe. A few more exceptions exist, like in ArrayReference.analyseCode, and SynchronizedStatement.analyseCode. After some unsuccessful attempts here is a twisted test program that will trigger the point you are raising: import java.io.File; import java.io.FileReader; import java.io.IOException; public class X { void foo(boolean b) throws IOException { File file = new File("somefile"); FileReader fileReader = new FileReader(file); synchronized (b ? this : new X()) { new ReadDelegator(fileReader); // <- here } } class ReadDelegator { ReadDelegator(FileReader reader) { } } } But then I didn't know what difference to look for when using copy() vs. unconditionalCopy(). Can you help me here?
> But then I didn't know what difference to look for when using > copy() vs. unconditionalCopy(). > > Can you help me here? Thanks for checking this Stephan. Actually, I raised this point because I remembered having fixed some bug (couldn't find it) which arose specifically because copy() had been used and if you have a code such as FlowInfo newInfo = oldInfo.copy(); and you modify newInfo, oldInfo runs the risk of being changed. But I guess if you've debugged and checked thats not the case we don't need to worry. I'll also be on the lookout for this problem.
Markus, can you please review the UI part (error messages, defaults, doc, etc.)? Thanks! (Also look at comment 47 and comment 48)
Created attachment 203866 [details] patch v0.9.6: polish Some minor improvements after more comments from Ayush: If a resource is already managed by t-w-r we were wastefully tracking the closing of this resource. Fixed by removing the tracker binding once we find out that a resource is indeed managed by t-w-r. test056s() has been added for this issue. If a resource variable is nulled before the end of the block we did not detect missing closing. Fixed by respecting the previous nullStatus only when doing locations-specific checks (i.e., checking at a return statement). test056g() has been updated to check this issue. Patch also includes the changes promised in comment 53.
I would reword the warning messages a little 885 = Potential resource leak: '{0}' may not be closed 886 = Potential resource leak: '{0}' may not be closed at this location 887 = Resource leak: '{0}' is never closed 888 = Resource leak: '{0}' is not closed at this location 889 = Resource '{0}' should be managed by try-with-resource Consider the following 2 cases - Here we know for sure that the resource *is not* closed on all paths, as we have examined all paths completely. -------------------------------------------------------------------------- void foo34(boolean b) throws Exception { FileReader reader = new FileReader("file"); if (b) { reader.close(); } } -------------------------------------------------------------------------- However, in the following case the resource may or may not have been closed on all paths, as we do not examine "close(reader)" invocation. Hence it is incorrect to say 'Potentially leaking resource 'reader': *is not* closed on all paths' in this case. -------------------------------------------------------------------------- void foo35(boolean b) throws Exception { FileReader reader = new FileReader("file"); if (b) { close(reader); } else { close(reader); } } private void close(FileReader reader) throws IOException { reader.close(); } -------------------------------------------------------------------------- We can possibly split 885 in two different warnings. 885a = Potential resource leak: '{0}' is not closed on all paths. 885b = Potential resource leak: '{0}' may not be closed on all paths. But this may be a overkill and also a little confusing. "885 = Potential resource leak: '{0}' may not be closed" is probably sufficient. You also get an incorrect resource is never closed warning in the following case ------------------------------------------------------------------------------- void foo31() throws Exception { FileReader reader = new FileReader("file"); //warning if (reader != null) { reader.close(); } else { } } ------------------------------------------------------------------------------- (In reply to comment #55) > Markus, can you please review the UI part (error messages, defaults, doc, > etc.)? Thanks! (Also look at comment 47 and comment 48) I think the current approach is good. For example, in the following example a "Resource '{0}' should be managed by try-with-resource" warning does not make much sense. ---------------------------------------------------------------- FileReader foo81() throws Exception { FileReader reader = new FileReader("file1"); return reader; } ---------------------------------------------------------------- Rest looks good to me.
Created attachment 203961 [details] patch v0.9.8 This patch combines fixes in response to comment 57 and after discussions with Ayush. (In reply to comment #57) > I would reword the warning messages a little > 885 = Potential resource leak: '{0}' may not be closed + Done. I agree with your argumentation. > You also get an incorrect resource is never closed warning in the following > case > ------------------------------------------------------------------------------- > void foo31() throws Exception { > FileReader reader = new FileReader("file"); //warning > > if (reader != null) { > reader.close(); > } else { > > } > } Good catch! + Added new test test056t() and fixed. (Background: 9 out of 10 callers of Statement.complainIfUnreachable(..) pass a scope which the corresponding ast node owns, thus the call marks the end of a block and analysis must be performed. 1 caller (IfStatement) passes the *enclosing* scope without marking an exit from that scope, thus analysis must be avoided here. Fixed by a new boolean parameter). > Rest looks good to me. Thanks! + Patch fixes a case where an error could be reported at position 0 due to bogus reuse of a ProblemReporter instance. Also, the new test test056i2() showed that my analysis of exceptions could still be tricked. We discussed two strategies regarding exceptions: (1) Aggressively report any exception that may be thrown at a point where a resource is not closed. => This would be very many locations. (2) Do not directly warn when on exceptions, consider resource as closed if close() occurs under normal flows. Only emit warning "... should be managed by t-w-r" which will lead to the desired exception safety. + We decided for (2) in order to avoid floods of false positives. This resulted in a simplification of the implementation (mostly in package flow). A few test results have been updated accordingly. From all I can see this patch resolves all issues mentioned in reviews. I'm planning to release it when the git repository becomes available.
Maybe I was not completely clear earlier. The following messages are a bit awkward. 885 = Potentially leaking resource '{0}': may not be closed 886 = Potentially leaking resource '{0}': may not be closed at this location 887 = Leaking resource '{0}': is never closed 888 = Leaking resource '{0}': is not closed at this location I would reword them as follows - (In reply to comment #57) > 885 = Potential resource leak: '{0}' may not be closed > 886 = Potential resource leak: '{0}' may not be closed at this location > 887 = Resource leak: '{0}' is never closed > 888 = Resource leak: '{0}' is not closed at this location > 889 = Resource '{0}' should be managed by try-with-resource (These are similar to other existing messages, for example, messages 451-458 for null pointer issues) Just realized this case, should we worry about resource creation? For example, consider the following case where a shared resource is being used in foo(), certainly the resource should not be closed in foo(). Maybe the warning in this case should be 'Potential resource leak' instead of 'Resource leak'. --------------------------------------------------------------------------- void foo() throws Exception { FileReader reader1 = getSharedReader(); try { int ch; while ((ch = reader1.read()) != -1) { System.out.println(ch); } } finally { } } FileReader getSharedReader() throws Exception { if(fileReader==null) fileReader = new FileReader("file1"); return fileReader; } ----------------------------------------------------------------------------
A case of missing warning - the first reassignment inside a twr block is not flagged --------------------------------------------------------------------------- void foo() throws Exception { FileReader reader1 = new FileReader("file1"); FileReader reader2 = new FileReader("file2"); reader2 = reader1;// warning try (FileReader reader3 = new FileReader("file3")) { int ch; while ((ch = reader2.read()) != -1) { System.out.println(ch); reader1.read(); } reader2 = reader1;// NO WARNING! reader2 = reader1;// warning } finally { if (reader2 != null) { reader2.close(); } else { System.out.println(); } } } --------------------------------------------------------------------------- (In reply to comment #16) > - extend solution to Closeable if compliance < 1.7 Stephan, this has not been implemented yet? We should probably implement the feature for compliance < 1.7 early in the cycle as it will be used more widely and hence the feature can be tested thoroughly for 3.8/4.2.
(In reply to comment #59) > Maybe I was not completely clear earlier. Sorry, I just focused at the one change you explained, didn't notice the other differences. Will adopt your variants before committing. > Just realized this case, should we worry about resource creation? So the difference would be: stronger warning only when rhs is an AllocationExpression or QualifiedAllocationExpression? All other cases say "potential"? I wouldn't want to do deep analysis of, e.g.,: fr = (path == null ? new FileReader(defaultPath) : new FileReader(path)); I.e., not seeing an AllocationExpression we would say "potential" also here. Is that OK? I don't know what is the more common use of methods here: fr = getSharedReader(); or fr = createFreshReader(); I could argue that for the case of a shared reader you do have a resource leak: the resource stays open beyond the scope where it is read from, and no part of the code seems to take responsibility for closing. This is different from the cases where a resource is passed as an argument to another method or returned from the current method: in both cases we see control flows that could reasonable lead to closing the resource: Reader foo() { Reader r = new FileReader("file"); read(r); return r; } Method read(Reader) could reasonably close the reader as could do the caller of foo(). In the case of shared resources we have no indication which part of the code might close the resource (unless it is closed at creation :)) Thus I'm leaning towards leaving things as they are, but I could also live with a shallow syntactic check: allocation: strong warning, else: potential.
(In reply to comment #60) > A case of missing warning - the first reassignment inside a twr block is not > flagged > --------------------------------------------------------------------------- > void foo() throws Exception { > FileReader reader1 = new FileReader("file1"); > FileReader reader2 = new FileReader("file2"); > reader2 = reader1;// warning > try (FileReader reader3 = new FileReader("file3")) { > int ch; > while ((ch = reader2.read()) != -1) { > System.out.println(ch); > reader1.read(); > } > reader2 = reader1;// NO WARNING! > reader2 = reader1;// warning > } finally { > if (reader2 != null) { > reader2.close(); > } else { > System.out.println(); > } > } > } > --------------------------------------------------------------------------- Here normal twr analysis has introduced a bug into null analysis. See bug 358827 which is independent of this bug. I.e., the above example should automatically work OK once bug 358827 is fixed.
(In reply to comment #60) > (In reply to comment #16) > > - extend solution to Closeable if compliance < 1.7 > Stephan, this has not been implemented yet? We should probably implement the > feature for compliance < 1.7 early in the cycle as it will be used more widely > and hence the feature can be tested thoroughly for 3.8/4.2. Thanks for mentioning. This indeed fell through the cracks, but that's trivial to add. Despite my wording from comment 16 I'd make analysis based on Closeable independent of the compliance level. Any objections? In case someone was going to ask: this will not generate duplicate warnings even though in 1.7 Closeable extends AutoCloseable :)
Created attachment 203975 [details] patch v0.9.9 (In reply to comment #57) > I would reword the warning messages a little > 885 = Potential resource leak: '{0}' may not be closed > 886 = Potential resource leak: '{0}' may not be closed at this location > 887 = Resource leak: '{0}' is never closed > 888 = Resource leak: '{0}' is not closed at this location > 889 = Resource '{0}' should be managed by try-with-resource Adopted. Sorry again for missing parts of the suggestion. (In reply to comment #60) > A case of missing warning - the first reassignment inside a twr block is not > flagged Added disabled test _test056u(), awaiting fix in bug 358827. (In reply to comment #60) > (In reply to comment #16) > > - extend solution to Closeable if compliance < 1.7 > Stephan, this has not been implemented yet? We should probably implement the > feature for compliance < 1.7 early in the cycle as it will be used more widely > and hence the feature can be tested thoroughly for 3.8/4.2. Fixed, new tests are FlowAnalysisTest.testCloseable[12](0).
(In reply to comment #61) > Thus I'm leaning towards leaving things as they are, but I could also live > with a shallow syntactic check: allocation: strong warning, else: potential. Yeah, the analysis of rhs expression could get tricky. Let's leave things as is for now, we can revisit this case if it proves problematic in real life. Javadoc of the following 2 options should mention Closeable as well JavaCore.COMPILER_PB_UNCLOSED_CLOSEABLE JavaCore.COMPILER_PB_POTENTIALLY_UNCLOSED_CLOSEABLE Something like... " * <p>When enabled, the compiler will issue an error or a warning if * a local variable holds a value of type <code>java.lang.AutoCloseable</code> (compliance>=1.7) * or a value of type <code>java.io.Closeable</code> (compliance<=1.6) and if " The javadoc of JavaCore.COMPILER_PB_EXPLICITLY_CLOSED_AUTOCLOSEABLE says " * <p>Note that this option is not intended to be surfaced in the UI, as it is intended * only for internal use for computing quick assists / cleanups. " However, I think this warning is useful to be shown to users. The default value of ignore ensures that not everyone will see this warning. I also notice comment 12, but I am not sure how JDT/UI will use a disabled warning and hence a non-existent marker in quick assists.
(In reply to comment #65) > Javadoc of the following 2 options should mention Closeable as well > JavaCore.COMPILER_PB_UNCLOSED_CLOSEABLE > JavaCore.COMPILER_PB_POTENTIALLY_UNCLOSED_CLOSEABLE Sure, thanks. > Something like... > " > * <p>When enabled, the compiler will issue an error or a warning if > * a local variable holds a value of type > <code>java.lang.AutoCloseable</code> (compliance>=1.7) > * or a value of type <code>java.io.Closeable</code> (compliance<=1.6) and > if > " Do you really mean we should disable analysis based on Closeable in 1.6-? (see comment 63) > The javadoc of JavaCore.COMPILER_PB_EXPLICITLY_CLOSED_AUTOCLOSEABLE says > " > * <p>Note that this option is not intended to be surfaced in the UI, as it is > intended > * only for internal use for computing quick assists / cleanups. > " > However, I think this warning is useful to be shown to users. The default value > of ignore ensures that not everyone will see this warning. I also notice > comment 12, but I am not sure how JDT/UI will use a disabled warning and hence > a non-existent marker in quick assists. I agree.
(In reply to comment #66) > Do you really mean we should disable analysis based on Closeable in 1.6-? > (see comment 63) In 1.7, every Closeable is also an AutoCloseable. Hence do we need to do something special for Closeable in 1.7 ?
Changing reviewer to Deepak, since he's already looked at the doc.
(In reply to comment #67) > (In reply to comment #66) > > Do you really mean we should disable analysis based on Closeable in 1.6-? > > (see comment 63) > > In 1.7, every Closeable is also an AutoCloseable. Hence do we need to do > something special for Closeable in 1.7 ? You're right. So there's no observable difference if we do (not) respect Closeable in 1.7. In the implementation I don't bother checking the compliance level when we find a subtype of Closeable (for simplicity). Do you still want to mention the compliance in the javadoc: " or a value of type <code>java.io.Closeable</code> (compliance<=1.6) " ? It's not wrong, just a question of which version is less confusing. Thanks
(In reply to comment #69) > Do you still want to mention the compliance in the javadoc: > " or a value of type <code>java.io.Closeable</code> (compliance<=1.6) " > ? > It's not wrong, just a question of which version is less confusing. I find the version with compliance less confusing :) I am trying the patch with jdt.core and core.resources - found a couple more bugs so far. No warning comes in the following case --------------------------------------------------------------------- boolean foo1() throws Exception { FileReader reader = new FileReader("file"); try { int ch; while ((ch = reader.read()) != -1) { System.out.println(ch); reader.read(); } if (ch > 10) { return true; } return false; } finally { } } --------------------------------------------------------------------- In the following case there are warnings at the two return statements. However there should simply be a single warning at the resource declaration. --------------------------------------------------------------------- boolean foo1() throws Exception { try { FileReader reader = new FileReader("file"); int ch; while ((ch = reader.read()) != -1) { System.out.println(ch); reader.read(); } if (ch > 10) { return true; //warning } return false; //warning } finally { } } --------------------------------------------------------------------- To simplify things further, consider the following 2 methods. The warning in foo31() is good, but the warning in foo32() isn't great. In foo32() the there should be a warning on the declaration of reader that reader is never closed. --------------------------------------------------------------------- void foo31(boolean b) throws Exception { FileReader reader = new FileReader("file"); if (b) { reader.close(); } else { return; //warning } } void foo32(boolean b) throws Exception { FileReader reader = new FileReader("file"); return; //warning } ---------------------------------------------------------------------
(In reply to comment #70) > I find the version with compliance less confusing :) so be it :) > I am trying the patch with jdt.core and core.resources - found a couple more > bugs so far. > No warning comes in the following case That looks like a bug indeed. I'll check once I'm at my other machine. > --------------------------------------------------------------------- > In the following case there are warnings at the two return statements. However > there should simply be a single warning at the resource declaration. > --------------------------------------------------------------------- > [...] > --------------------------------------------------------------------- > To simplify things further, consider the following 2 methods. The warning in > foo31() is good, but the warning in foo32() isn't great. In foo32() the there > should be a warning on the declaration of reader that reader is never closed. > --------------------------------------------------------------------- Both these look like I should just improve the priorities of different warning kinds. Normally, when I analyse the return statement I don't yet know if any close() calls follow further down. Luckily I already have the infastructure for delayed reporting.
Stephan, can you also take a look at the following 3 warnings in core.resources. (These warnings look imperfect to me but I am unable to get a simplified code snippet for these) Resource leak: 'input' is never closed ProjectPreferences.java /org.eclipse.core.resources/src/org/eclipse/core/internal/resources line 591 Resource leak: 'o1' is never closed SaveManager.java /org.eclipse.core.resources/src/org/eclipse/core/internal/resources line 1575 Resource leak: 'o1' is never closed SaveManager.java /org.eclipse.core.resources/src/org/eclipse/core/internal/resources line 1685
Created attachment 204041 [details] patch v1.0.0 Changes included in this patch: 1. handle more cases involving methods of local classes: + method of local type may pass resource to outside code -> see test056y() 2. on early return check also those resources of all enclosing block scopes -> see test056v(), test056w() 3. better prioritization of warnings: + if no close() is found this is more relevant than any early exit + for this we now explicitly record more bits in FakedTrackingVariable (globalClosingState, merged with previous flag closedInNestedMethod) -> test056x() 4. pass the tracking status along assignments to avoid false positives when a resource is closed using an alias -> test056z() + refactored some: moved code into class FakedTrackingVariable + had to adjust one expected test result in NullReferenceTests where a Closeable (LineNumberReader) is indeed leaked. This resolves the following issues: (In reply to comment #70) > I am trying the patch with jdt.core and core.resources - found a couple more > bugs so far. > No warning comes in the following case > --------------------------------------------------------------------- > boolean foo1() throws Exception { > FileReader reader = new FileReader("file"); > try { > int ch; > while ((ch = reader.read()) != -1) { > System.out.println(ch); > reader.read(); > } > if (ch > 10) { > return true; > } > return false; > } finally { > } > } > --------------------------------------------------------------------- At the return statements I failed to check resources of enclosing scopes. At the method end we have a DEAD_END and hence not enough info to give any warnings. Fixed by item (2) above. > In the following case there are warnings at the two return statements. However > there should simply be a single warning at the resource declaration. > --------------------------------------------------------------------- > boolean foo1() throws Exception { > try { > FileReader reader = new FileReader("file"); > int ch; > while ((ch = reader.read()) != -1) { > System.out.println(ch); > reader.read(); > } > if (ch > 10) { > return true; //warning > } > return false; //warning > } finally { > } > } > --------------------------------------------------------------------- Again at the method end we have a DEAD_END, so no place were we detected "never closed". Fixed by item (3) above. > To simplify things further, consider the following 2 methods. The warning in > foo31() is good, but the warning in foo32() isn't great. In foo32() the there > should be a warning on the declaration of reader that reader is never closed. > --------------------------------------------------------------------- > void foo31(boolean b) throws Exception { > FileReader reader = new FileReader("file"); > > if (b) { > reader.close(); > } else { > return; //warning > } > } > > void foo32(boolean b) throws Exception { > FileReader reader = new FileReader("file"); > return; //warning > } > --------------------------------------------------------------------- Same as above. (In reply to comment #72) > Stephan, can you also take a look at the following 3 warnings in > core.resources. (These warnings look imperfect to me but I am unable to get a > simplified code snippet for these) > > Resource leak: 'input' is never closed ProjectPreferences.java > /org.eclipse.core.resources/src/org/eclipse/core/internal/resources line 591 Methods of a local type pass the resource to outside code, one of which actually closes it. Resolved by item (1) above. > Resource leak: 'o1' is never closed SaveManager.java > /org.eclipse.core.resources/src/org/eclipse/core/internal/resources line > 1575 The resource is first assigned to 'o1' than to the final 'markersOutput'. During that assignment we lost significant information. Fixed by item (4) above. > Resource leak: 'o1' is never closed SaveManager.java > /org.eclipse.core.resources/src/org/eclipse/core/internal/resources line > 1685 Dito. Thanks for all the pointers to non-optimal warnings! It looks like I'm running out of letters (test056a-z()) and version numbers (patches v0.1 - v1.0.0) :) I guess further requests for improvements can now go into separate bugs like bug 358903 (thanks for opening that one), OK? @Deepak, does "patch v1.0.0" get your +1?
(In reply to comment #73) > Created attachment 204041 [details] [diff] > patch v1.0.0 The patch has paths containing a/ and b/ - "a/org.eclipse.jdt.core", "b/org.eclipse.jdt.core". I had to remove these to apply the patch. Once I did that I see that the patch does not contain the new class - FakedTrackingVariable. With Git you have to make sure that the new files have been added/committed, e.g. unadded/unstaged files are not selected by default in the commit dialog. Stephan, can you please create a new patch :)
Created attachment 204066 [details] patch v1.0.0a (In reply to comment #74) > (In reply to comment #73) > > Created attachment 204041 [details] [details] [diff] > > patch v1.0.0 > > The patch has paths containing a/ and b/ - "a/org.eclipse.jdt.core", > "b/org.eclipse.jdt.core". I had to remove these to apply the patch. I guess this is expected, see http://wiki.eclipse.org/Platform-releng/Git_Workflows#Apply_a_patch > Once I did that I see that the patch does not contain the new class - > FakedTrackingVariable. With Git you have to make sure that the new files have > been added/committed, e.g. unadded/unstaged files are not selected by default > in the commit dialog. #@%&*!! > Stephan, can you please create a new patch :) Let's see, if this one works better..
(In reply to comment #75) > I guess this is expected, see > http://wiki.eclipse.org/Platform-releng/Git_Workflows#Apply_a_patch Ha! I did not see the 'Ignore leading path name segments' checkbox. I can apply the patch now :) Something went wrong in this patch as I see several duplicate warning messages. With the previous patch I had 100 warnings in jdt.core, but with the latest patch I get 140. e.g. The following is listed 5 times in problems view Resource 'reader' should be managed by try-with-resource JavaModelManager.java /org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core line 3168 (In reply to comment #73) > > Resource leak: 'o1' is never closed SaveManager.java > > /org.eclipse.core.resources/src/org/eclipse/core/internal/resources line > > 1575 > > The resource is first assigned to 'o1' than to the final 'markersOutput'. > During that assignment we lost significant information. > Fixed by item (4) above. > > > Resource leak: 'o1' is never closed SaveManager.java > > /org.eclipse.core.resources/src/org/eclipse/core/internal/resources line > > 1685 > > Dito. The warnings at these 2 locations are reported twice. A minor issue, the new warning in these 2 cases is.. "Resource 'o1' should be managed by try-with-resource SaveManager.java /org.eclipse.core.resources/src/org/eclipse/core/internal/resources line 1575" ... it took me a few seconds to understand the warning. o1 is assigned to markersOutput and close() is called on markersOutput, as I did not see a close() call on o1 I was wondering for a while why should this me managed by twr. Consider the following simplified snippet ----------------------------------------------------------------------------- void foo32(FileReader param) throws Exception { FileReader reader = new FileReader("file"); //warning FileReader reader1= reader; reader1.close(); FileReader reader2= param; //warning reader2.close(); } ----------------------------------------------------------------------------- - reader1 and reader2 are similar, but there is no warning on reader1. - close() is never called on reader, yet the warning suggests to manage this via twr. => I am leaning towards moving the warning on reader to reader1, for the sake of clarity and consistency with reader2 case. Thanks for the fast responses to review comments! The patch is almost there :)
(In reply to comment #76) > Consider the following simplified snippet > ----------------------------------------------------------------------------- > void foo32(FileReader param) throws Exception { > FileReader reader = new FileReader("file"); //warning > > FileReader reader1= reader; > reader1.close(); > > FileReader reader2= param; //warning > reader2.close(); > } > ----------------------------------------------------------------------------- > - reader1 and reader2 are similar, but there is no warning on reader1. > - close() is never called on reader, yet the warning suggests to manage this > via twr. > => I am leaning towards moving the warning on reader to reader1, for the sake > of clarity and consistency with reader2 case. The warning on reader is intended. I'm really in favor of t-w-r blocks that brace the full life-cycle of a resource. Consider this scenario after conversion to t-w-r: FileReader reader = new FileReader("file"); //warning if (b) throw new RuntimeException(); try (FileReader reader1= reader) { // reading } How safe is that? :) For reader2 we simply can't apply the same strategy because we cannot embed param in a t-w-r.
(In reply to comment #76) > I can apply the patch now :) good > Something went wrong in this patch as I see several duplicate warning messages. > With the previous patch I had 100 warnings in jdt.core, but with the latest > patch I get 140. > e.g. > The following is listed 5 times in problems view > Resource 'reader' should be managed by try-with-resource > JavaModelManager.java > /org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core line 3168 I have no idea what happened. I just applied the patch to a clean workspace (had to resolve two conflicts w/ bug 358827), launched a runtime workbench, imported org.eclipse.jdt.core, selected a JRE 1.7 (so we have a type Closeable to begin with) and set compliance back to 1.4 (to avoid tons of warnings re generics). After that dance I see 8 warnings re resource leaks. None of them are duplicates. Please re-check your workspace, or tell my how you got into the bad state.
(In reply to comment #78) > I have no idea what happened. I just applied the patch to a clean > workspace (had to resolve two conflicts w/ bug 358827), launched a > runtime workbench, imported org.eclipse.jdt.core, selected a JRE 1.7 > (so we have a type Closeable to begin with) and set compliance back to > 1.4 (to avoid tons of warnings re generics). > > After that dance I see 8 warnings re resource leaks. None of them > are duplicates. > Please re-check your workspace, or tell my how you got into the bad state. The duplicates are of type "Resource 'reader' should be managed by try-with-resource", which you will see only in 1.7 compliance. Hence, I am using 1.7 JRE and 1.7 compliance, and to focus only on the new warnings I have set all other warnings to 'ignore' in 'org.eclipse.jdt.core.prefs' via a simple find-replace.
Created attachment 204108 [details] patch v1.0.0b (In reply to comment #76) > The following is listed 5 times in problems view > Resource 'reader' should be managed by try-with-resource > JavaModelManager.java > /org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core line 3168 I can see now. This was probably an artifact of this change from comment 73: > 2. on early return check also those resources of all enclosing block scopes Multiple nested scopes delegate up to the same enclosing block scope. In one specific scenario each of them would raise the same warning over and over. Fixed in the attached patch (incl. new tests). Please ignore the change in CompilationUnit, which is a product of git and me not yet speaking the same language (it is not part of the commit, yet creating a patch from the commit includes this change, hm??).
(In reply to comment #80) > Please ignore the change in CompilationUnit, which is a product of git > and me not yet speaking the same language (it is not part of the commit, > yet creating a patch from the commit includes this change, hm??). I think this is because git works with a set of linear changes. So inside a git patch you have all the changes since the last time you synch'd. I committed a typo fix inside CompilationUnit. As long as you don't do a pull from the git repo, you will see that change. Try the command line git tool and type "git status". It should tell you that you missed one commit from the remote repo.
The number of warnings in jdt.core and core.resources with patch v0.9.9 and patch v1.0.0b is different, which made me investigate the differences. I think there is only one problem in patch v1.0.0b (it is not a regression over v0.9.9, things are actually much worse in v0.9.9) In the following snippet there is a single warning on the declaration -"Resource leak: 'reader2' is never closed". If the indicated line is uncommented there are two warnings on the return statements - "Potential resource leak: 'reader2' may not be closed at this location". However there should be one warning on the resource declaration - "Potential resource leak: 'reader2' may not be closed", as uncommenting the line should only change the warning from 'Resource leak' to "Potential Resource Leak' ------------------------------------------------------------------------------ void foo11() throws Exception { FileReader reader2 = new FileReader("file2"); try { int ch; while ((ch = reader2.read()) != -1) { System.out.println(ch); reader1.read(); } if (ch < 0) return; //warning else if (ch > 100) return; //warning } finally { //processReader(reader2); //uncomment this line } } void processReader(FileReader reader) { } ------------------------------------------------------------------------------ An improvement that I see in v1.0.0b over 0.9.9, not sure if you have already added a test case for this. A few "Resource 'reader' should be managed by try-with-resource" warnings were missed in v0.9.9 because of return statements, but this works correctly in v1.0.0b e.g. Resource 'in' should be managed by try-with-resource Util.java /org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/util line 1863 Simplified snippet ------------------------------------------------------------------------------ void foo111() throws Exception { FileReader reader = new FileReader("file2"); try { int ch; while ((ch = reader.read()) != -1) { System.out.println(ch); reader1.read(); } return; // results in a missing warning in v0.9.9 } finally { if (reader != null) { reader.close(); } } } ------------------------------------------------------------------------------
(In reply to comment #82) > In the following snippet there is a single warning on the declaration > -"Resource leak: 'reader2' is never closed". > > If the indicated line is uncommented there are two warnings on the return > statements - "Potential resource leak: 'reader2' may not be closed at this > location" [...] > > ------------------------------------------------------------------------------ > void foo11() throws Exception { > FileReader reader2 = new FileReader("file2"); > try { > int ch; > while ((ch = reader2.read()) != -1) { > System.out.println(ch); > reader1.read(); > } > if (ch < 0) > return; //warning > else if (ch > 100) > return; //warning > } finally { > //processReader(reader2); //uncomment this line > } > } > > void processReader(FileReader reader) { > > } > ------------------------------------------------------------------------------ The behavior is in line with the priorities defined in the code: - warnings at return statements are considered more specific (higher prio) than just saying "may not be closed". Perhaps we hadn't yet looked at an example where a return statement occurs before the resource is passed to outside code. The difference to v0.9.9 comes from the fact that in v0.9.9 return statements in some situations of scope nesting were not detected as causing a leak. > [...] However there should be one warning on the resource declaration - > "Potential resource leak: 'reader2' may not be closed", as uncommenting the > line should only change the warning from 'Resource leak' to "Potential Resource > Leak' I disagree. Without the line we have (1) early exits causing leak -> not closed at this location (2) end of method still has no close() -> never closed. Here (2) is stronger then (1) so we report "never closed". Adding the call to processReader() changes (2) to: (2b) end of method has doubts about leaking or not Now I consider (1) as stronger/more specific than (2b). I'd prefer leaving this as it is, because I think the specific warnings are helpful, and because I'm not sure if further adjusting the system of priorities won't create surprises for other examples - this is already getting a tad complex. > An improvement that I see in v1.0.0b over 0.9.9, not sure if you have already > added a test case for this. > > A few "Resource 'reader' should be managed by try-with-resource" warnings were > missed in v0.9.9 because of return statements, but this works correctly in > v1.0.0b I have one or two similar tests, adding this one, too, won't hurt. Please assume it's done :)
(In reply to comment #64) > Added disabled test _test056u(), awaiting fix in bug 358827. Just as a reminder for myself: this test should be enabled before release as bug 358827 is already fixed.
(In reply to comment #83) > this is already getting a tad complex. :) (In reply to comment #83) > I'd prefer leaving this as it is I thought a bit more on how the quick fixes would look for the warnings in question, and based on that I think it ok to leave things as is. Looks like we are done and the patch can be released. Thanks Stephan!
I applied last pending polish and released everything as commit 1bf30b93f1c2f17c02d0cecfa43e877f00d01800 for 3.8 M3. Thanks for all the great feedback!
Post scriptum regarding assignments: The implementation uses a simple analysis of data flows across assignments so that in cases like this: FileReader r1 = new FileReader("file"); FileReader r2 = r1; r2.close(); we can recognize that r1 is indeed closed (since comment 73). I should mention that this is not a full data flow analysis that can cope with arbitrary re-assignments. In test056u() this minimal analysis already gives more precise results than Deepak and me were expecting, but the messages can get confusing if the code is messy. If confusing messages are actually seen in real code with multiple re- assignments of resources (which I haven't seen yet) we might open a new bug to investigate more precise data flow analysis, but from today's perspective I'd say that would be overkill.
Verified for 3.8M3 using build N20111022-2000.
