Bugzilla – Full Text Bug Listing
| Summary: | [1.7][compiler][enh] Open issues in try with resources implementation | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Srikanth Sankaran <srikanth_sankaran> | ||||||||||||||
| Component: | Core | Assignee: | Srikanth Sankaran <srikanth_sankaran> | ||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||
| Severity: | normal | ||||||||||||||||
| Priority: | P3 | CC: | amj87.iitr, daniel_megert, deepakazad, markus.kell.r, Olivier_Thomann, satyam.kandula | ||||||||||||||
| Version: | 3.7 | ||||||||||||||||
| Target Milestone: | 3.7.1 | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Windows XP | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 190016 [details] Patch - part 1 Since this bug is an umbrella one for the whole feature, I'll be releasing the patches in a phased manner rather than making a big bang release at the end. > There are several issues with the implementation as it stands now. > > - New constructs are NOT rejected at source level < 1.7 > - NPE from > org.eclipse.jdt.internal.compiler.lookup.BlockScope.maxShiftedOffset() > since the finally block's shifted scopes are not populated > correctly with the (synthetic) scopes created to manage the > resources. These are fixed in the current patch has been released to the BETA_JAVA7 branch Also see that the following program fails to compile
with eclipse (we get a bogus error : "The local variable y
may not have been initialized")
public class X {
public static void main(String [] args) {
try (Y y = new Y()) {
if (y == null) {}
}
}
}
class Y implements AutoCloseable {
public void close() {
}
}
Created attachment 190034 [details] Patch - part 2 This patch : - Improves the flow analysis for try with resources. The missing "Unhandled exception" error mentioned in comment#0 is fixed. - The bogus "The local variable y may not have been initialized" error mentioned in comment#2 is fixed. - Eliminates the compiler ASTNode TryStatementWithResources folding the code into its super type TryStatement itself. (The DOM/ASTNode node of the same name is retained) - Adds several more tests. I will test the data flow and control flow analysis some more before venturing to generate code. In preliminary testing code complete, code select, search seem to work alright with the new construct. Another control flow issue here: the following program compiles fine
with eclipse, while javac 7b130 rightly complains:
X.java:3: unreported exception XXException; must be caught or declared to be thr
own
try (X x = new X(); Y y = new Y(); Z z = new Z()) {
^
1 error
See however that there should be two more errors which are NOT reported
by javac regarding YYException and ZZException being neither caught nor
declared to be thrown (propagated.) These do show up if the problem regarding
XXException is addressed.
public class X implements AutoCloseable {
public static void main(String [] args) {
try (X x = new X(); Y y = new Y(); Z z = new Z()) {
} catch (XException x) {
} catch (YException y) {
} catch (ZException z) {
} finally {
}
}
public X() throws XException {
throw new XException();
}
public void close() throws XXException {
throw new XXException();
}
}
class Y implements AutoCloseable {
public Y() throws YException {
throw new YException();
}
public void close() throws YYException {
throw new YYException();
}
}
class Z implements AutoCloseable {
public Z() throws ZException {
throw new ZException();
}
public void close() throws ZZException {
throw new ZZException();
}
}
class XException extends Exception {}
class XXException extends Exception {}
class YException extends Exception {}
class YYException extends Exception {}
class ZException extends Exception {}
class ZZException extends Exception {}
Created attachment 190131 [details] Patch - part 3 Released this patch in the BETA_JAVA7 branch which fixed the problem mentioned in comment #5 Basic code generation in place.
I am able to compile the following program and run it
successfully and the program behavior is as expected:
public class X implements AutoCloseable {
public static void main(String [] args) throws Exception {
try (X x = new X(); Y y = new Y()) {
System.out.println("Body");
} catch (Exception e) {
e.printStackTrace();
}
}
public X() {
System.out.println("X CTOR");
}
public void close() {
System.out.println("X DTOR");
}
}
class Y implements AutoCloseable {
public Y() {
System.out.println("Y CTOR");
}
public void close() {
System.out.println("Y DTOR");
}
}
Amazingly, this program ran the very first time. Absolutely, the very
first time !
I was bracing for a long battle with verification problems, exceptions
and such, it was a huge surprise that it ran the first time.
I am sure, lots of interesting problems will surface as I test more.
Stay tuned.
We also compile and run the following program correctly:
//-------------
public class X implements AutoCloseable {
public static void main(String [] args) throws Exception {
try (X x = new X(); Y y = new Y()) {
System.out.println("Body");
throw new Exception("Body");
} catch (Exception e) {
e.printStackTrace(System.out);
Throwable [] suppressed = e.getSuppressed();
for (int i = 0; i < suppressed.length; i++) {
System.out.println("Suppressed:" + suppressed[i]);
}
} finally {
int finallyVar = 10;
System.out.println(finallyVar);
}
}
public X() {
System.out.println("X CTOR");
}
public void close() throws Exception {
System.out.println("X Close");
throw new Exception("X Close");
}
}
class Y implements AutoCloseable {
public Y() {
System.out.println("Y CTOR");
}
public void close() throws Exception {
System.out.println("Y Close");
throw new Exception("Y Close");
}
}
//---------------------------
However, when I attempt to run the same test as a junit, there are
some problems. The exception thrown by the body is the try block
is directly caught by (the programmer visible) catch block with
there being no suppressed exceptions. If I run the produced class
file from outside using 7b130, everything is OK.
The VM used by org.eclipse.jdt.core.tests.util.TestVerifier.verifyClassFiles(String, String, String, String, String[], String[], String[]) for some reason
doesn't like the class file produced.
Under investigation.
This is the translation being used:
A construct of the form:
try (X x1 = new X(); X x2 = new X(); X x3 = new X()) {
x1.doSomething();
x2.doSomething();
x3.doSomething();
} catch (Exception x) {
x.printStackTrace();
}
is treated to be:
try {
Exception _primaryException = null;
try {
X x1 = new X();
try {
X x2 = new X();
try {
X x3 = new X();
try {
x1.doSomething();
x2.doSomething();
x3.doSomething();
} catch (Exception e) {
if (_primaryException == null) {
_primaryException = e;
} else if (_primaryException != e) {
_primaryException.addSuppressed(e);
}
throw _primaryException;
} finally {
x3.close();
}
} catch (Exception e) {
if (_primaryException == null) {
_primaryException = e;
} else if (_primaryException != e) {
_primaryException.addSuppressed(e);
}
throw _primaryException;
} finally {
x2.close();
}
} catch (Exception e) {
if (_primaryException == null) {
_primaryException = e;
} else if (_primaryException != e) {
_primaryException.addSuppressed(e);
}
throw _primaryException;
} finally {
x1.close();
}
} catch (Exception e) {
if (_primaryException == null) {
_primaryException = e;
} else if (_primaryException != e) {
_primaryException.addSuppressed(e);
}
throw _primaryException;
}
} catch (Exception e) {
e.printStackTrace();
}
}
(In reply to comment #9) > This is the translation being used: This is slightly suboptimal, will be improved upon. Created attachment 190659 [details]
Patch - Part 4
This patch adds code generation support. Passes all JDT/Core tests.
Known problems:
- No support for automated resource closure on break, continue,
or return causing control to leave the try block.
- Some mystery around why the same test case that works in the
IDE alright would produce a different behavior inside the
junit frame work.
- Has seen only minimal testing so far, while it should not be
very brittle, there could be several issues.
- Code generated could be improved upon.
(In reply to comment #8) > We also compile and run the following program correctly: [...] > However, when I attempt to run the same test as a junit, there are > some problems. The exception thrown by the body is the try block > is directly caught by (the programmer visible) catch block with > there being no suppressed exceptions. If I run the produced class > file from outside using 7b130, everything is OK. This is a bug in JDK 7b130, which has nothing to do with try with resources per se. Running the following junit also shows the problem: //-------- public void test027() { this.runConformTest( new String[] { "X.java", "public class X {\n" + " public static void main(String [] args) throws Exception {\n" + " try {\n" + " throw new Exception(\"Body\");\n" + " } catch (Exception e) {\n" + " System.err.println(\"Before stack trace printing\");\n" + " e.printStackTrace();\n" + " System.err.println(\"After stack trace printing\");\n" + " } finally {\n" + " System.err.println(\"In finally block\");\n" + " }\n" + " }\n" + "}\n" + "\n" }, ""); } //--------------- Basically the call to e.printStack() appears to crash (doesn't like some of the eclipse reflective invocation related frames.) It doesn't appear to result in an exception as can be deduced by the fact that the finally block doesn't get executed at all. The output of running the above junit abruptly stops with: "Before stack trace printing\n" + "java.lang.Exception: Body\n" + " at X.main(X.java:4)\n" + " at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n" + " at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)\n" + " at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)\n" + " at java.lang.reflect.Method.invoke(Method.java:613)\n" + " at" (In reply to comment #11) > - Some mystery around why the same test case that works in the > IDE alright would produce a different behavior inside the > junit frame work. We should be able to workaround this issue by not calling printStackTrace from within the junit. *** Bug 338881 has been marked as a duplicate of this bug. *** Created attachment 190744 [details] Patch - Part 5 >(In reply to comment #11) > Created attachment 190659 [details] > Patch - Part 4 > Known problems: > > - No support for automated resource closure on break, continue, > or return causing control to leave the try block. Fixed. > - Some mystery around why the same test case that works in the > IDE alright would produce a different behavior inside the > junit frame work. Not an eclipse issue, JDK7 bug, worked around. > - Code generated could be improved upon. For the inner most catch block, we now simply emit catch (Throwable e) { throw (_primaryException = e); } Released this patch into BETA_JAVA7 branch. Created attachment 190751 [details]
Patch - Part 6
This patch cleans up some redundancy/code duplication
in the parser.
With this, I believe the work on the compiler is complete.
Now onto testing, fixing, code review, spec review ...
For the record, this translation supercedes the one in comment# 9. I tried what I thought were some really cool low level tricks to produce some really tight code, but these require some unorthodox code patterns (such as falling deliberately from a try block into a catch block etc) that send the bytecode verifier into a total tail spin (even though the code sequence is perfectly valid), so will settle for straightforward sequences instead. A construct of the form: try (X x1 = new X(); X x2 = new X(); X x3 = new X()) { x1.doSomething(); x2.doSomething(); x3.doSomething(); } catch (Exception x) { x.printStackTrace(); } is treated to be: try { Throwable _primaryException = null; try { X x1 = new X(); try { X x2 = new X(); try { X x3 = new X(); try { x1.doSomething(); x2.doSomething(); x3.doSomething(); } catch (Throwable e) { _primaryException = e; throw _primaryException; } finally { x3.close(); } } catch (Throwable e) { if (_primaryException == null) { _primaryException = e; } else if (_primaryException != e) { _primaryException.addSuppressed(e); } throw _primaryException; } finally { x2.close(); } } catch (Throwable e) { if (_primaryException == null) { _primaryException = e; } else if (_primaryException != e) { _primaryException.addSuppressed(e); } throw _primaryException; } finally { x1.close(); } } catch (Throwable e) { if (_primaryException == null) { _primaryException = e; } else if (_primaryException != e) { _primaryException.addSuppressed(e); } throw _primaryException; } } catch (Exception e) { e.printStackTrace(); } } Added several more negative tests and runtime behavior conformance tests. Thus far, I see only two differences in behavior between javac 7b131 and the BETA_JAVA7 branch. (1) javac does not accept a trailing semicolon in the resource specification. (2) Even when the resource is null, javac attempts to invoke close(). On both counts, eclipse behavior is the correct one. Resolving current bug as FIXED. Will use separate follow up defects to track issues discovered afresh or if the spec changes. (In reply to comment #17) > (1) javac does not accept a trailing semicolon in the resource > specification. Yes, this is correct and will be fixed into future version of javac. > (2) Even when the resource is null, javac attempts to invoke > close(). A null check is required before calling the close() method. So a null resource should not cause a NPE. > On both counts, eclipse behavior is the correct one. I think so. Verified. I opened bug 350579 to track the runtime issue with the code in comment 5. |
This enhancement is to track the try with resources implementation. There are several issues with the implementation as it stands now. - New constructs are NOT rejected at source level < 1.7 - NPE from org.eclipse.jdt.internal.compiler.lookup.BlockScope.maxShiftedOffset() since the finally block's shifted scopes are not populated correctly with the (synthetic) scopes created to manage the resources. - Exception analysis incomplete: As a result the following program compiles without any error while it should fail with a unhandled exception error: //-----------------------------------------8<------------------------- public class X { public static void main(String [] args) { try (Y y = new Y();) { System.out.println("Try block\n"); } finally { System.out.println("Finally block\n"); } } } class Y implements AutoCloseable { public Y() throws WeirdException { throw new WeirdException(); } public void close() { System.out.println("Closing resource\n"); } } class WeirdException extends Throwable {} //-----------------------------------------8<------------------------- - Code generation is not in place. - Other issues as and when they are discovered...