Community
Participate
Working Groups
3.2 The AST has the rule that a binding is either 100% correct or null. In some situations we loose quite some information due to that. E.g. in bug 149088 in 'c.add' we don't know that 'c' is a variable if 'List' is not imported yet. protected String foo() { List c = new ArrayList(); c.add(null); return c; } A solution would be to add an additional option that would let all 'c' have a variable bidning with 'Object' as variable type.
See also bug 110976 for problems with duplicate methods.
This bug is conditionned by progress on bug 156731
Would an API like getResilientBinding() or a better name be enough to fix this issue? The idea would be that this method would return a binding that is potentially incomplete. It would be the case when the binding returns right now is null.
Actually, the compiler binding already exists here; it is just filtered out by the DOM, since DOM bindings must be sound or absent. I think we could provide an alternate API for getting the partial binding. (getPartialBinding() ? getErrorBinding() ?).
Where would this API be? On all AST nodes that offer a resolveBinding? How is the parent (getDeclaringType) of such a variable binding going to look like. Will it contain the incomplete variable binding? Maybe I misunderstood the idea. But it seems that we can't maintain compatibility that way. I would suggest to rather introduce a new flag on the ASTParser to state that 'recovered' bindings can exist. Important is that we have to spec what this means compared to the existing behaviour. a. You can get bindings for things that can't correctly be resolved b. You can get bindings that return 'null' at places where after the old spec it can't (for example IVariableBinding.getDeclaredType) a.) is not a real problem, b.) is problematic and can lead to null pointer exceptions if such a binding is passed to old code. Therefore I suggest to instead of using 'null' we should use a 'dummy' error binding instead. Such a binding would be marked with a special flag. In the given example: When an AST with 'recovered bindings enabled' is created on the code from comment 0: c would have a variable binding as if everything is normal except getDeclaredType returns a error binding %List% %List% would have name 'List', no members, default package and a flags set called 'errorBindning'. If we have a second unresolve reference to 'List' in the code we can think of using the identical binding again, or, to avoid any extra work here, use a new one.
(In reply to comment #5) > Therefore I suggest to instead of using 'null' we should use a 'dummy' error > binding instead. Such a binding would be marked with a special flag. This could break existing users as well since they are expecting null when nothing can be fully resolved. They might miss "invalid" cases if they detect them using a null check. Deferring to 3.3M6 since Martin is on vacations till M5 is out.
That's why I suggested a new flag on the ASTParser to state that 'recovered' binding can exist at places where 'null' was returned before. But what I wanted to say is that is is less problematic to suddendly return something where 'null' returned was before, (example: myASTNode.resolveBinding()) compared to return 'null' where it was spec'ed that 'null' can't be returned (example: variableBinding.getType())
So we should add a setBindingRecovery(boolean). This would allow bindings to be "resolved" even if they had errors. We should define what "default" behavior is expected in case of errors. However without changes in the compiler, the case in comment 0 would continue to fail. The compiler binding for the local declaration c is null. I would expect a missing binary type binding or at least something I could use to "create" a DOM binding not null. Kent, Philippe, Is this doable or null is expected as the returned type of the local inside the compiler?
Same issue for the method bindings.
Postponing post 3.3.
Re: comment 8. I think you have a local var with a null type. It is inserted in scopes, and bound from subsequent references. The DOM could reflect this. I am a bit skeptical about retrofitting this in existing binding APIs (with new mode), since older clients may accidentally start trying to process these. Think for instance of clients traversing a DOM AST obtained through reconcile. Or are we saying this mode wouldn't be made public ? i.e. need to create the AST through a different API... ? Tagging tentatively for M6, for investigation.
(In reply to comment #11) > Re: comment 8. > I think you have a local var with a null type. It is inserted in scopes, and > bound from subsequent references. The DOM could reflect this. The binding for the local variable is null. So there is nothing that can be done to create a fake binding on the DOM side. You can check regression tests org.eclipse.jdt.core.tests.dom.ASTConverterTestAST3_2#test0666/test0667. They reflect the actual DOM bindings. We could add a new flag on the ASTParser that would be used to set that error bindings can be created. I don't see the advantage of getting a non-null binding vs a null binding since there is nothing that can be done with this binding anyway. Adding a new option that would preserve the actual behavior by default should not be an API breakage. Some documentation might need to be updated to reflect it. Any thought?
Olivier - I just debugged through the testcase, and could see that on compiler side, the variable is created with a null type, and subsequent reference from message send is correctly bound to the variable 'c' (null type).
yes, but I would like to get something not null in order to create a fake binding on the DOM side.
The variable binding is not null; only its type is, since unbound. This is an incomplete variable binding, which is what was requested in the first place. What more do you need ?
I would have expected the type to be a missing binary type binding. Martin, do you expect each null value to be converted to "java.lang.Object" type binding ? So we should add a method: setBindingRecovery(boolean) on the org.eclipse.jdt.core.dom.ASTParser class. Then I guess we also need a way for the user to know that the binding is an incomplete binding. So we might need a flag on the binding itself. isRecoveredBinding() defined on org.eclipse.jdt.core.dom.IBinding. Any comment?
However for the method case, it is a bit worse since the binding is null. I don't have a method binding with null for the "unbound" parts (either return type, thrown exception types or any of the parameter types).
comment 16: I think there are two solutions, simple and deluxe: a.) simple: varBinding.getType() return the binding for 'java.lang.Object'. No API is provided for the client to find out that this binding has been added by recovery. But this would already help us. b.) There is a way to mark bindings that are recovered: varBinding.getType() would returned a type binding where 'typeBinding.isRecoveredBinding() == true'. But, this can't be 'java.lang.Object' as there can only be one such binding in the environment and that binding has 'typeBinding.isRecoveredBinding() == false'. So it would be its own binding. It doesn't need any methods, package ect. If you want to be nice it could have a name as found in the code. c.) varBinding.getType() returns null: This is a no-go as this is against the spec of IVariableBinding and changing that would hurt many old clients when they happen to get hands a 'recovered' AST. comment 17: Let's just start with variables. We will later see if methods are useful and important as well.
(In reply to comment #18) > I think there are two solutions, simple and deluxe: > a.) simple: varBinding.getType() return the binding for 'java.lang.Object'. No > API is provided for the client to find out that this binding has been added by > recovery. Without a way for the user to know that the binding is recovered, this could end up being misleading. Object might not be the "actual" type of the variable. > b.) There is a way to mark bindings that are recovered: varBinding.getType() > would returned a type binding where 'typeBinding.isRecoveredBinding() == true'. > But, this can't be 'java.lang.Object' as there can only be one such binding in > the environment and that binding has 'typeBinding.isRecoveredBinding() == > false'. I would have reported the variable binding to be recovered, not its type itself. Or both of them should be reported as recovered? > So it would be its own binding. It doesn't need any methods, package ect. If > you want to be nice it could have a name as found in the code. We might also want to create a binding that has the same source name as in the code. It would belong to the default package, no methods, no fields, etc. > c.) varBinding.getType() returns null: This is a no-go as this is against the > spec of IVariableBinding and changing that would hurt many old clients when > they happen to get hands a 'recovered' AST. Agree. This is not an acceptable solution. This is why null is returned for the binding right now. > comment 17: > Let's just start with variables. We will later see if methods are useful and > important as well. For method, I guess I could create a fake method binding based on the code. Not sure this is helpful. I'll prepare a patch for variable only for now.
Re: comment 16. Missing binary bindings are only created from within binaries, which isn't the case here (i.e. in situations where we have the full signature at hand). I am not convinced of adding Object as a placeholder. I know we do it in case superclass is missing. Can't we introduce use a 'void' type instead or some bottom type object ? Then it would be obvious it representing an unbound type. Alternatively, we could create a type by the simple name (in default package?), this would allow to carry the simple name information... or maybe rather some problem type binding with a simple/qualified name in it ?
I have three cases on which I'd like to know what should be returned: - the array case: protected String foo() { List[] c = bar(); c.add(null); return c; } Do you expect an array of a recovered binding named "List"? - the qualified name case: protected String foo() { p1.p2.T c = bar(); c.add(null); return c; } Do you expect the recovered binding to have a qualified name (p1.p2.T) and a simple name (T) ? - the parameterized type case: protected String foo() { p1.p2.T<String> c = bar(); c.add(null); return c; } Should the recovered binding be a parameterized type binding ? Let me know what you expect. I almost have a prototype working.
It would be good to get an array type resp. a parameterized type binding there. Also having qualification this would be great, but no problem if the type binding is just simple name. After all it could also be that type 'T' is inside a outer type 'p2'.
What kind of equality do you expect between recovered bindings? Should the two recovered bindings for List (for c1 and c) be equals or == in this case ? ... protected String foo() { List c = new ArrayList(); List c2 = new ArrayList(); c.add(null); return c; } ... Same question for array binding of recovered binding. And is such an array binding also considered as a recovered binding ? Same question for parameterized type binding.
I think it's ok if they are _not_ identical. If you want to make them identical, sure, but the the client couldn't and shouldn't rely on this. I think the array bidning shouldn't be marked as recovered: The array part of the type is correct. It's just the element type that has a problem. But I'm also fine with having it marked. I guess there are also good arguments for that.
I remove method bindings from the title and I will open a new bug for the incomplete method binding.
Added bug 176631. Please add yourself to the cc list.
Created attachment 60387 [details] First draft Martin, Please give it a try and let me know if this goes into the right direction.
Created attachment 60388 [details] Regression tests
API-wise this looks good. Maybe the comment on 'IBinding.isRecovered()' could be more precise or give an example. Is it ok to say that a recovered binding stands for an unresolvable reference in the AST? There are other places where you can create an AST and where we have to allow the new AST parser option: - ICompilationUnit.reconcile(..., recoverBindings,...) - ReconcileContext It would also be nice to have a way to know from an AST that binding resolving is enabled, something like AST.isBindingRecovery (bug 130001) While trying out the patch, I got the following exception (I can provide the code example) java.lang.ClassCastException: org.eclipse.jdt.core.dom.RecoveredTypeBinding cannot be cast to org.eclipse.jdt.core.dom.TypeBinding at org.eclipse.jdt.core.dom.DefaultBindingResolver.getTypeBinding(DefaultBindingResolver.java:337) at org.eclipse.jdt.core.dom.DefaultBindingResolver.resolveType(DefaultBindingResolver.java:1512) at org.eclipse.jdt.core.dom.Type.resolveBinding(Type.java:169) at org.eclipse.jdt.astview.views.ASTViewContentProvider.getNodeChildren(ASTViewContentProvider.java:152)
Yes, please provide the test case.
- apply the patch from comment 27 to jdt.core but change 'createAST' to always create an AST with binding recovery - open AST view on the following code package p; public class A { B foo() { } }
Created attachment 60687 [details] Fix for Martin's problem This should fix your issue.
Agree that for consistency, ICompilationUnit#reconcile(...) should also have a flag to say that recovered bindings are requested. However instead of adding yet another boolean, we should replace the boolean parameters with an int paramater that would combine forceProblemDetection, enableStatementsRecovery, and recoverBindings. I also agree that ReconcileContext#getAST3() should take all these flags into account.
comment 33 makes sense to me.
+1
Released for 3.3M6 as a macro patch with fix for 149567. Regression tests added in org.eclipse.jdt.core.tests.dom.ASTConverter15Test#test0253/0257 and org.eclipse.jdt.core.tests.dom.ASTConverterTestAST3_2#test0670/0673
Verified for 3.3 M6 using build I20070320-0010
