Community
Participate
Working Groups
Build ID: M20080911-1700 At present, some methods in APT classes throw UnsupportedOperationException. It makes implementing APT processors harder than necessary. - TypesImpl#isSubtype throws UOE for array types and reference types. - VariableElementImpl#getEnclosingElement throws UOE for PARAMETER elements.
*** Bug 258915 has been marked as a duplicate of this bug. ***
Opened bug 259248 and 259249 for individual methods.
Assigning to myself for tracking - will close when the cloned bugs are fixed.
Created attachment 121644 [details] Draft patch (for discussion) Attaching a patch from Olivier, for discussion. I suspect this patch is too invasive to get into 3.4.2, but I thought I would ask since it is affecting a user. (This bug was cloned into two others, but since Olivier's patch addresses both it seems best to put the patch here.) I have two concerns about this patch: 1. The patch adds a backpointer to the enclosing method for each parameter binding. APT needs to be able to retrieve the enclosing method having only a binding that corresponds to the parameter of the method. In this draft patch, the backpointer is added to the LocalVariableBinding. It is initialized only for APT, but it takes space for every parameter. So this is not ideal. Is it okay to do that? An alternative might be to subclass the LocalVariableBinding and create an instance of this class based on the current LocalVariableBinding. 2. Is it correct that the erasure of a non-reference type is itself? Does that correctly handle array types? I'm looking at http://java.sun.com/docs/books/jls/third_edition/html/typesValues.html#4.6 and would expect it to return a type mirror for an array of the erasure of the leaf type.
Created attachment 121657 [details] Proposed fix New patch for erasure(), getEnclosingElement() and isSubType(). That patch should address the two concerns from comment 4.
Note that erasure() was not directly requested to be fixed, but since it is also missing implementation, it might be a good idea to get it right.
For me, when applied to 3.4 maintenance branch, this does not pass the tests in o.e.jdt.compiler.apt.tests.AllTests. It fails on ModelTests.testElementWithEclipseCompiler, examining the declaration "public void methodDvoid(DEnum dEnum1) {}". When it tries to get the enclosing element of dEnum1, the declaringScope of the LocalVariableBinding representing dEnum1 is null, instead of pointing to the method as expected. In ExecutableElementImpl.getParameters(), line 137, the methodDeclaration we get does have a MethodScope; but in its argument list, the LocalVariableBinding's declaredScope is null, instead of pointing back to the MethodScope as I would expect. As a side note, we'll also need to update the copyright dates of any files we touch, since it's now 2009. I can take care of that, of course.
I'll double check again.
Created attachment 122269 [details] New proposal Walter, Please try this one. Let me know if it works for you. I don't use the scope anymore, but I create an instance of a subclass of LocalVariableBinding to hold to the enclosing method. Like this there is no impact on the compiler, and the impact should be minimal for apt. Let me know what you think.
I think I also handle the binary case. Which was not the case with the previous patch.
Created attachment 122305 [details] Last proposal I forgot one case in the previous proposal. This should now be fine.
Created attachment 122307 [details] Same fix + one more regression test Same patch as before with one more regression test for binary methods.
With that patch, the tests all pass for me. I was a little concerned about putting org.eclipse.jdt.internal.compiler.lookup types into the org.eclipse.jdt.compiler.apt plug-in because it adds confusion, but since this plug-in is actually just a fragment that separates the JSR269 types (many of which require Java 6) from the rest of the compiler I think I can convince myself it's okay. If the types were in the compiler core plug-in, they would never get used or referenced except by the APT fragment, so that is probably even worse. Vladimir, does this patch work for you?
Created attachment 122310 [details] Corresponding patch for HEAD With a slightly improved regression test for binary methods.
Tests pass in HEAD. Committed to HEAD for 3.5M5. I will request PMC approval to apply to 3.4.x.
We missed one last case when the argument has no binding because there is a compile error. I'll update the patch for 3.4.x. HEAD should be fixed now.
Hi, Olivier. I am wondering whether the problem you found this morning means that maybe there are still some other problems lurking in the Visitor. It would be bad to introduce NPEs into the Visitor - I can deal with UOEs in particular API methods, because I can just tell people that those aren't implemented yet, but if we get NPEs in the Visitor it means that APT is broken depending on the code being processed, even if the processor doesn't call any particular methods. How confident do you feel in the final version of the code? Should we pull back, and not try to get the changes into 3.4.2? (Reopening the bug for discussion, till we resolve this.)
Hi, I am double-checking a last fix. Before we used to process method and arguments only when the argument had annotations. So the fix was not good enough in error cases (compile errors, where some bindings (method or argument are null)). I am working on it.
Created attachment 122438 [details] New patch that handles error cases
That patch is already released in HEAD.
Requesting PMC approval to put this into 3.4.2. I have reviewed Olivier's patch and tested it manually as well as with the automated tests, and it seems to cope with the error conditions (defective and unexpected code) that I threw at it. Olivier is confident that the patch does not expose any new failures, that is, the AST visitor is not going to fail in situations where before it would have succeeded. I therefore retract my concerns. Thanks!
+1 for 3.4.2
(In reply to comment #13) > > Vladimir, does this patch work for you? > Walter, this patch seems to work for me. Thanks a lot!
(In reply to comment #21) > I have reviewed Olivier's patch ... I therefore retract my > concerns. > Thanks, Walter. I appreciate your diligence.
Fixed in 3.4.2.
