Bugzilla – Full Text Bug Listing
| Summary: | [1.7] Incorrect line numbers in stack trace with try with resources | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Deepak Azad <deepakazad> | ||||||
| Component: | Core | Assignee: | Srikanth Sankaran <srikanth_sankaran> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | Olivier_Thomann, srikanth_sankaran | ||||||
| Version: | 3.7 | ||||||||
| Target Milestone: | 3.7.1 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
Another example
----------------------------------------------------------------------
package q;
public class TryWithResources1 {
public static void main(String[] args) throws Throwable {
try (Test t1 = new Test(false)) {
try (Test t2 = new Test(true)) { //5
} //6
} catch (Exception e) {
e.printStackTrace();
}
}
}
class Test implements AutoCloseable {
private boolean throwException;
Test(boolean throwException) {
this.throwException = throwException;
}
@Override
public void close() throws Exception {
if (throwException)
throw new Exception();
}
}
---------------------------------------------------------------------
Eclipse gives this stack trace
java.lang.Exception
at q.Test.close(TryWithResources1.java:21)
at q.TryWithResources1.main(TryWithResources1.java:5) // the line number should be 6
I remember consciously leaving it as it is since in some tests it was more reasonable than what javac was reporting. I'll take a second look. (In reply to comment #2) > I remember consciously leaving it as it is since in some > tests it was more reasonable than what javac was reporting. Here is a test case where eclipse's behavior is more intuitive than javac. We show line 5 as the invocation site of close() call while javac shows line 10. I think line 5 sheds more light on what is going on in the program. public class X { public static void main(String[] args) throws Throwable { while (true) try (Test t = new Test()) { break; // line 5 } // line 10 } } class Test implements AutoCloseable { @Override public void close() throws Exception { System.out.println("closing"); throw new Exception(); } } For the test case reported at comment# 0 I agree javac behavior is more reasonable. Patch will follow shortly. (In reply to comment #0) > With javac > the exception happens on the catch, which I think is more correct. The exception cannot be attributed to the catch. It happens at the end of the try block. You can easily verify this by inserting a few blank lines between the } and the catch block. (In reply to comment #4) > (In reply to comment #0) > > With javac > > the exception happens on the catch, which I think is more correct. > > The exception cannot be attributed to the catch. It happens at the end > of the try block. You can easily verify this by inserting a few blank > lines between the } and the catch block. Yup, I agree. (In reply to comment #3) > Here is a test case where eclipse's behavior is more intuitive > than javac. We show line 5 as the invocation site of close() > call while javac shows line 10. I think line 5 sheds more light > on what is going on in the program. > > public class X { > public static void main(String[] args) throws Throwable { > while (true) > try (Test t = new Test()) { > break; // line 5 > > > > > } // line 10 > } > } > > class Test implements AutoCloseable { > @Override > public void close() throws Exception { > System.out.println("closing"); > throw new Exception(); > } > } > > For the test case reported at comment# 0 I agree javac behavior is > more reasonable. > > Patch will follow shortly. I disagree here. The exception does not happen on the 'break', it happens after the try block (in a 'hidden' finally block). Hence showing line 10 is more accurate. In the example from comment 0 I had just made the last statement of the try block span more than 1 line to make the problem stand out a bit more :) Created attachment 194048 [details]
Patch under consideration
(In reply to comment #5) > I disagree here. The exception does not happen on the 'break', it happens after > the try block (in a 'hidden' finally block). Hence showing line 10 is more > accurate. In the example from comment 0 I had just made the last statement of > the try block span more than 1 line to make the problem stand out a bit more :) I'll leave this case as it is - the synthetic finally block is inlined into the break/continue/return site and I feel the stack trace better educates the user the origins of the exception. Other cases are fixed by the patch. (In reply to comment #7) > I'll leave this case as it is - the synthetic finally block is inlined into > the break/continue/return site and I feel the stack trace better educates > the user the origins of the exception. Other cases are fixed by the patch. I see, you want the last statement that was executed in the block to be mentioned in the stack trace and in case of break/continue/return the last statement could be in the middle of the block. Hence, in this case the line containing the first 'break' is mentioned -------------------------------------------------------------------- public class X { public static void main(String[] args) throws Throwable { int i = 10; while (true) try (Test t = new Test()) { if (i == 10) break; else break; } } } class Test implements AutoCloseable { @Override public void close() throws Exception { System.out.println("closing"); throw new Exception(); } } -------------------------------------------------------------------- and in this case the line containing the closing brace for the try block is mentioned (because something() throws an exception, hence return statement does not complete execution) -------------------------------------------------------------------- public class X { public static void main(String[] args) throws Throwable { foo(); } private static int foo() throws Throwable { int i = 10; while (true) try (Test t = new Test()) { if (i == 10) return something(); else return something(); } } private static int something() throws Throwable { throw new IllegalArgumentException(); } } class Test implements AutoCloseable { @Override public void close() throws Exception { System.out.println("closing"); throw new Exception(); } } -------------------------------------------------------------------- Is this understanding correct ? (In reply to comment #8) > (In reply to comment #7) > > I'll leave this case as it is - the synthetic finally block is inlined into > > the break/continue/return site and I feel the stack trace better educates > > the user the origins of the exception. Other cases are fixed by the patch. > I see, you want the last statement that was executed in the block to be > mentioned in the stack trace and in case of break/continue/return the last > statement could be in the middle of the block. The code that needs to be generated for a break/continue/return is dependent on the context and where that code has to be contain synthetic constructs that have no source correlation, the compiler has some wiggle room on how/where the byte code is charged. In the current scheme of code generation, the AutoCloseable.close calls get emitted right at the break/continue/return site without there being a control transfer of some form to a hidden block. As a side effect, this has the "beneficial" effect of showing some more details about the close method came to be invoked. This does differ from how the stack trace looks for the non-resource case. Now, I am in two minds whether to leave it as is or to go for consistency at the expense of arguably better clarify in context. What do you think ? (In reply to comment #9) > In the current scheme of code generation, the AutoCloseable.close > calls get emitted right at the break/continue/return site without > there being a control transfer of some form to a hidden block. Isn't this an implementation detail? Wouldn't most developers imagine that there is a control transfer to a hidden finally block ? Created attachment 194068 [details] Proposed patch (In reply to comment #10) > (In reply to comment #9) > > In the current scheme of code generation, the AutoCloseable.close > > calls get emitted right at the break/continue/return site without > > there being a control transfer of some form to a hidden block. > > Isn't this an implementation detail? Wouldn't most developers imagine that > there is a control transfer to a hidden finally block ? May be. This patch fixes even the break/continue/return cases to be consistent with javac and eclipse non-resources case. Released latest patch into BETA_JAVA7 branch. Thanks Deepak. Let us know if something else is broken. Verified using Java 7 feature patch. |
-------------------------------------------------------------------- public class TryWithResources1 { public static void main(String[] args) throws Throwable { try (Test t = new Test()) { for (int i = 0; i < 10; i++) { int a = 10; System.out.println(i); a++; a *= 10; System.out.println(a); } } catch (Exception e) { e.printStackTrace(); } } } class Test implements AutoCloseable { @Override public void close() throws Exception { System.out.println("closing"); throw new Exception(); } } ------------------------------------------------------------------ When I compile this with Eclipse and then run I get java.lang.Exception at Test.close(TryWithResources1.java:21) at TryWithResources1.main(TryWithResources1.java:4) On the other hand when I compile this with javac and then run I get java.lang.Exception at Test.close(TryWithResources1.java:21) at TryWithResources1.main(TryWithResources1.java:11) Notice the line numbers 4 and 11 in the two stack traces. According to eclipse the exception happened on the for loop, which is wrong (I think it marks the exception at the beginning of the last statement in the try block). With javac the exception happens on the catch, which I think is more correct.