Community
Participate
Working Groups
This is a generalization of bug 349326 comment 60: This works: Object foo2() { String o = null; try { o = o.toUpperCase(); // warn here: o is null } finally { o = "OK"; } return o; } but this doesn't: Object foo2() throws FileNotFoundException, IOException { String o = null; try (FileReader fr = new FileReader("f1")) { o = o.toUpperCase(); } finally { o = "OK"; } return o; } Initial analysis: the call to handlingContext.checkExceptionHandlers(..) from TryStatement.java:275 causes that the flow info of the finally block is erroneously merged into the main flowInfo. (line number is with bug 349326 applied).
Another find: TryStatement.analyseCode contains two similar calls to checkExceptionHandlers. The first in l.135 passes true to isExceptionOnAutoClose, the second in l.261 (head) lacks this param. @Srikanth, it seems you wrote some of this. Do you recall if the difference is by intention? Adding the flag to the second call doesn't fix the issue but based on this flag we could easily avoid the bogus update of the flowInfo.
Created attachment 203976 [details] test and proposed fix Test case and proposed fix.
Srikanth, do you want to check if this collides with your intentions inside TryStatement.analyseCode(..)? Thanks.
(In reply to comment #1) > Another find: TryStatement.analyseCode contains two similar calls to > checkExceptionHandlers. The first in l.135 passes true to > isExceptionOnAutoClose, the second in l.261 (head) lacks this param. > > @Srikanth, it seems you wrote some of this. Do you recall if the > difference is by intention? This code was written by Ayush to fix the bug 348705, yes this difference is by intention to issue slightly different error messages. > Adding the flag to the second call doesn't fix the issue but based > on this flag we could easily avoid the bogus update of the flowInfo. After reviewing the bug 348705, you should be able to conclude whether you can safely piggy back your fix on top of this without causing any side effects, I would think.
(In reply to comment #4) > After reviewing the bug 348705, you should be able to conclude whether > you can safely piggy back your fix on top of this without causing any > side effects, I would think. (In reply to comment #3) > Srikanth, do you want to check if this collides with your intentions > inside TryStatement.analyseCode(..)? > Thanks. Sorry, I just saw this patch which already attempts to piggy back on top of this extra parameter Ayush introduced. As you can see against the backdrop of the context for bug 348705, the proposed patch would mischaracterize the exception as the one raised by automatic close call when it is not, and so would be a no-go.
(In reply to comment #5) > (In reply to comment #4) > > > After reviewing the bug 348705, you should be able to conclude whether > > you can safely piggy back your fix on top of this without causing any > > side effects, I would think. > > > (In reply to comment #3) > > Srikanth, do you want to check if this collides with your intentions > > inside TryStatement.analyseCode(..)? > > Thanks. > > Sorry, I just saw this patch which already attempts to piggy back on > top of this extra parameter Ayush introduced. As you can see against > the backdrop of the context for bug 348705, the proposed patch would > mischaracterize the exception as the one raised by automatic close > call when it is not, and so would be a no-go. Do you have a test case that would fail with the patch? compiler.regression.TestAll seem to be happy with the patch. The location where I added the "true" flag is almost identical to the one we already have, it concerns AutoCloseable resources, too, just for the case where we also have a finally block. BTW: try adding a finally block to TryWithResourcesStatementTest.test053() first you get a AIOOBE (an obvious typo in the code) and after fixing that you get the wrong error message. Which can then be fixed by the patch I proposed here.
(In reply to comment #6) > Do you have a test case that would fail with the patch? > compiler.regression.TestAll seem to be happy with the patch. > The location where I added the "true" flag is almost identical to the > one we already have, it concerns AutoCloseable resources, too, > just for the case where we also have a finally block. Sorry I didn't look closely the first time. So instead of mislabelling, your patch indeed would complete the fix for bug 348705. > BTW: try adding a finally block to TryWithResourcesStatementTest.test053() > first you get a AIOOBE (an obvious typo in the code) and after fixing > that you get the wrong error message. Which can then be fixed by the > patch I proposed here. Yes, the patch does look good for me. Ayush, do you have any comments ?
(In reply to comment #7) > Ayush, do you have any comments ? +1 from me too
I'd suggest to backport the fix for the AIOOBE mentioned in comment 6 to 3.7.2 (typo i vs. j). Do we want to separate that out, or should actually the whole issue go into 3.7.2, too?
(In reply to comment #9) > I'd suggest to backport the fix for the AIOOBE mentioned in comment 6 > to 3.7.2 (typo i vs. j). > Do we want to separate that out, or should actually the whole issue go > into 3.7.2, too? +1 for 3.7.2 for the full fix, given that it is small and well contained and is obvious oversight.
Created attachment 204079 [details] full patch Patch fixing both issues (with tests), to be released for HEAD.
Released into HEAD for 3.8 M3.
Created attachment 204124 [details] same patch for R3_7_maintenance I tried the same tests in 3.7 maintenance and could observe the same three bugs: - AIOOBE - wrong error re exception from t-w-r - missing null error All are fixed by this (small) patch.
+1 for 3.7.2
Stephan, will you release your patch to 3.7 maintenance or should I do it ?
(In reply to comment #15) > Stephan, will you release your patch to 3.7 maintenance or should I do it ? Done: Released commit 1d6277040a15d6a361edbfc4d3f5e370c8d71128 for R3_7_maintenance
Verified for 3.8 M3 using build id: N20111022-2000
Leave state as resolved and clear white board status for 3.8 M3.
*** Bug 367879 has been marked as a duplicate of this bug. ***
Verified for 3.7.2RC2 using build M20120118-0800
