Community
Participate
Working Groups
20080314 The changes in bug 196200 resulted in more recovered bindings to be shown in a AST created for bindings. Although this is a unspecified area, the additional bindings required us to update some code and tests. It seems also other components like APT and Dali (see bug 196200 comment 46) were affected. For compatibility reasons, the additional bindings should only show up when the user enabled 'recovered bindings'. Missing binary types are a special case. They have been available for a longer time already but didn't seem to produce any troubles as they are rare and and also contain a package declaration.
I am going to change the Dali code to enable 'recovered bindings' since we can get more out of the JDT model this way. I am also doing this so that our functionality doesn't change again when this bug is fixed. With 'recovered bindings' enabled what are the cases where org.eclipse.jdt.core.dom.Type.resolveBinding() or org.eclipse.jdt.core.dom.Expression.resolveTypeBinding() would return null?
Created attachment 96876 [details] Proposed fix
Created attachment 96885 [details] Proposed fix One compile error didn't show up in the dom tests before. New patch with changes only in the tests.
Created attachment 96890 [details] fix for failing APT tests With that patch, two org.eclipse.jdt.apt.tests (Java 5 APT) tests fail, because a MethodBinding that used to have a non-null return type now has a null return type. The failing tests are MirrorDeclarationTests.testUnresolvableDeclarations0 and ..1. It is easy to fix the failing tests, by changing the expected return value in ASTBasedMirrorDeclarationProcessorFactory; I'll attach a patch. See also org.eclipse.jdt.apt.core.internal.declaration.MethodDeclarationImpl.getReturnType(). However, this failure shows that even with the patch, JDT's behavior with "resolveBindings=false" is still slightly different than it used to be before all the binding recovery work was done. In this case JDT seems to be returning less information than it used to, which I think is more of a problem than returning extra information. Although I have a fix for the tests, I would have to do more work to figure out what the right fix would be for APT to still produce a return type in the case where JDT doesn't give it to us. Honestly I'm not sure why it doesn't work already; we should be using an ASTBasedMethodDeclarationImpl instead of the plain MethodDeclarationImpl that we are using; but I will need to look deeper to figure out why that isn't happening.
(In reply to comment #4) > However, this failure shows that even with the patch, JDT's behavior with > "resolveBindings=false" is still slightly different than it used to be before > all the binding recovery work was done. In this case JDT seems to be returning > less information than it used to, which I think is more of a problem than > returning extra information. This is right. Before when a binding was not found and was coming from binaries, we would still return a binding. Now we are more consistent with the binding recovery mode. If the binding is not found, then we return null in all cases regardless of the cause. > Although I have a fix for the tests, I would have to do more work to figure out > what the right fix would be for APT to still produce a return type in the case > where JDT doesn't give it to us. Honestly I'm not sure why it doesn't work > already; we should be using an ASTBasedMethodDeclarationImpl instead of the > plain MethodDeclarationImpl that we are using; but I will need to look deeper > to figure out why that isn't happening. I think APT should always use the mode with the binding recovery on.
(In reply to comment #5) > I think APT should always use the mode with the binding recovery on. For the Java 5 implementation, I suspect this might be a big change. The Java 5 implementation has always had bindingResolution=false; it works by falling back to the AST when it can't get something from the Java Model. (It does not work all that well, and avoiding the ugliness and complexity of all that code was one big reason why we did it differently in the Java 6 implementation.) I will give it a try tonight and see what happens if I change it, but it makes me a bit nervous.
I ran all JDT/UI tests and didn't any failures related to this change. Martin, Walter, is it ok to release this for tomorrow's integration build ?
Ok for me!
Walter, let me know when I can release.
Sorry, I was not able to check email or bugs again till now. I guess it is too late for what I say to make any difference, but anyway: okay to release, as long as you release the APT fix also. Otherwise the APT tests will fail.
Created attachment 96972 [details] Set recovered bindings to 'true' in Java 5 APT I tried setting recoveredBindings to 'true' whenever we create an ASTParser in Java 5 APT, and to my surprise, it seems to work fine and introduce no problems, and it fixes the tests that were failing with Olivier's patch. With this change, no test fixes are required. I think this is worth trying. It still feels a little risky to check this in right away. I am willing to take the gamble right now if we need to, but if we can wait a few days I will try to find time to write some more thorough tests.
Walter, I'll release the code for JDT/Core and I let you take care of releasing the code for APT. Let me know if you want me to do it. Released for 3.4M7. Updated existing regression tests.
I've released the APT changes as well, for 3.4M7.
Verified for 3.4M7 using I20080429-0100
