Community
Participate
Working Groups
If a variable of type java.lang.AutoClosable is closed explicitly in a finally block, we can suggest a quick assist to remove the explicit close with a try with resources block. I suppose the quick assist can also be provided if the close call is not in a finally block.
The quick assist would also be interesting if the resource is currently never closed. The try-with-resources would just end after the last reference to the resource. In case there's already an enclosing try statement, we should offer 2 quick fixes: - Add resource 'varName' to surrounding try - Surround resource 'varName' with try-with-resources
Created attachment 199229 [details] fix + tests v0.9 I think we can provide this quick assist even without bug 349326. We can probably modify/improve this once bug 349326 is fixed, but we need not depend on it. Note: You should also apply patch from bug 351170. - Quick assist is proposed on a resource declaration - If the resource is passed to another method or returned then the quick assist is not proposed - There are still some formatting issues with the quick assist, I am investigating those. - TODO: I am not sure on how to deal with multiple close() calls. Markus, give this patch a try, it will be good if we can have this for 3.7.1.
Created attachment 199231 [details] fix + tests v0.91 This patch behaves a bit better with multiple close() calls.
Created attachment 199247 [details] fix + tests v0.92 Now the quick assist is also not offered if the resource is reassigned. I think this patch should be able to handle a large majority of cases.
- Formatting needs more work. With the patch from bug 351170 comment 10 I get everything on one line. - You need to take care of VariableDeclarationStatements that declare multiple variables: int m1() throws IOException { FileInputStream fis = new FileInputStream("/tmp/bla"), fis2 = new FileInputStream("/tmp/bla2"); return fis.available() + fis2.available(); } - The scope of the try statement sometimes needs to be larger, when applying quick fix to 'fis' here: int m2() throws IOException { if ("abc".length() == 3) { FileInputStream fis = new FileInputStream("/tmp/bla"); FileInputStream fis2 = new FileInputStream("/tmp/bla2"); int a1 = fis.available(); return a1 + fis2.available(); } return 1; } Until we have a proper analysis to find the necessary scope, I think we should just enclose the rest of the block that contains the variable declaration in the TryStatement. - Apply to 'fis' here (big surprise, simplifies code a lot!): int m3() { if ("abc".length() == 3) { try { FileInputStream fis = new FileInputStream("/tmp/bla"); try { FileInputStream fis2 = fis; int a1 = fis.available(); return a1 + fis2.available(); } catch (IOException e) { e.printStackTrace(); } finally { fis.close(); } } catch (FileNotFoundException e) { e.printStackTrace(); } catch (IOException e) { e.printStackTrace(); } } return 1; } - Looking closer at examples where the close() call is not just a method invocation in the finally block, I think it's too dangerous to offer the quick assist there. v0.92 deletes too much, but even the code that stay changes semantics. The order in which resources are closed can be important. We should just offer the quick assist when the close method is not called or when it is called as the last statement of the finally block. Example 4: int m4() throws IOException { if ("abc".length() == 3) { FileInputStream fis = new FileInputStream("/tmp/bla"); try { return fis.available(); } finally { System.out.println("closing..."); if (fis.available() > 0) fis.close(); else { fis.read(); } fis.close(); System.out.println("closed..."); } } return 1; } - "Add resource 'varName' to surrounding try" has a wrong name here (we add to the following try): int m5() throws IOException { FileInputStream fis = new FileInputStream("/tmp/bla"); try { return fis.available(); } finally { fis.close(); } }
This may or may not overlap with comment 5 (we were typing simultaneously). I have a few questions from a brief look at the patch (haven't tried it): The handling of close() inside "if" looks very specific to a given coding pattern. What happens to: if (rc != null) rc.close(); else log("we have no resource!"); From reading the code it looks like you'll remove the whole if statement?? How do you determine the range end for the new t-w-r? E.g.: if (foo) { FileReader fr = getReader(); if (fr != null) { fr.read(); fr.close(); log("all is well"); } log("let's move on"); } I would expect: if (foo) { try (FileReader fr = getReader()) { if (fr != null) { fr.read(); fr.close(); log("all is well"); } } // <- this is the tricky part log("let's move on"); } Would it help if the internal warning COMPILER_PB_EXPLICITLY_CLOSED_AUTOCLOSEABLE be reported with a source range covering the minimal block to be surrounded by the new t-w-r? Or is that exact range easy enough to be computed in UI code? When bug 349326 comes into play, do you still want/need your own analysis of when the resource is passed as an argument to a method call?
Created attachment 199415 [details] fix + tests v0.93 (In reply to comment #5) > - Formatting needs more work. With the patch from bug 351170 comment 10 I get > everything on one line. Yes, the formatting needs work when a new try statement is created, I will open a bug for this. When a resource is added to an existing try statement the formatting is good now. > - The scope of the try statement sometimes needs to be larger, when applying > quick fix to 'fis' here: > > int m2() throws IOException { > if ("abc".length() == 3) { > FileInputStream fis = new FileInputStream("/tmp/bla"); > FileInputStream fis2 = new FileInputStream("/tmp/bla2"); > int a1 = fis.available(); > return a1 + fis2.available(); > } > return 1; > } > > Until we have a proper analysis to find the necessary scope, I think we should > just enclose the rest of the block that contains the variable declaration in > the TryStatement. You refer to the fact that in the result a1 and fis2 are declared inside the try block but referred outside it ? SurroundWithTryCatchRefactoring handles this problem, we can try something similar here (and if possible also reuse the code) > - Apply to 'fis' here (big surprise, simplifies code a lot!): Good catch! I was removing the if statement incorrectly :) This if fixed in the attached patch. > - Looking closer at examples where the close() call is not just a method > invocation in the finally block, I think it's too dangerous to offer the quick > assist there. v0.92 deletes too much, but even the code that stay changes > semantics. The order in which resources are closed can be important. > > We should just offer the quick assist when the close method is not called or > when it is called as the last statement of the finally block. > > Example 4: > > int m4() throws IOException { > if ("abc".length() == 3) { > FileInputStream fis = new FileInputStream("/tmp/bla"); > try { > return fis.available(); > } finally { > System.out.println("closing..."); > if (fis.available() > 0) > fis.close(); > else { > fis.read(); > } > fis.close(); > System.out.println("closed..."); > } > } > return 1; > } In the new patch I preserve the contents of the else block and still remove all close() calls. Is that good enough ? Or do you want the if statement itself to be preserved ? Also how often can we expect such cases in real life ? I am assuming that such if else sequences are rare.
(In reply to comment #6) > The handling of close() inside "if" looks very specific to a given > coding pattern. What happens to: > if (rc != null) > rc.close(); > else > log("we have no resource!"); > From reading the code it looks like you'll remove the whole if statement?? See comment 7. > How do you determine the range end for the new t-w-r? E.g.: > if (foo) { > FileReader fr = getReader(); > if (fr != null) { > fr.read(); > fr.close(); > log("all is well"); > } > log("let's move on"); > } > I would expect: > if (foo) { > try (FileReader fr = getReader()) { > if (fr != null) { > fr.read(); > fr.close(); > log("all is well"); > } > } // <- this is the tricky part > log("let's move on"); > } In this case, you get what you expect. I simply find the last reference of the resource and enclose everything till that point in twr. > Would it help if the internal warning > COMPILER_PB_EXPLICITLY_CLOSED_AUTOCLOSEABLE > be reported with a source range covering the minimal block to be > surrounded by the new t-w-r? Or is that exact range easy enough > to be computed in UI code? I will still have to compute the source range in case the resource is not closed :) > When bug 349326 comes into play, do you still want/need your own analysis > of when the resource is passed as an argument to a method call? Once bug 349326 is fixed, the analysis done in UI can be reduced.
Now that the resource leak warnings are there we will provide quick fixes. Once the quick fixes are in place we may consider implementing a simple quick assist "Move initialization into try-with-resource statement", e.g. for folks who have the warnings set to ignore.
