Community
Participate
Working Groups
Steps: 1) Hilite the first 'asList(side)' expression. 2) Pick the Refactor>>ExtractLocalVariable menu item. 3) Be sure the "Replace all occurrences" checkbox is checked. 4) Press OK button. ---------------------- Bug.java ---------------- import static java.util.Arrays.*; class Bug { { String[]side=new String[0]; if(true){ System.out.println(asList(side)); } else{ System.out.println(asList(side)); } } }
Moving to JDT/Core. The problem is that the method bindings for the two method invocations 'asList(side)' are not identical (but they are isEqualTo(..) each other and their keys are equal). The JdtASTMatcher compares bindings by identity, which fails here.
DOM AST bindings are mapping 1 to 1 to compiler ones, which in this case are not identical (2 invocations of a generic method). I don't think the spec ever said that method bindings would be identical.
Markus, Where did you find that method bindings are supposed to be identical?
From IBinding: /** * There is no special definition of equality for bindings; equality is * simply object identity. Within the context of a single cluster of * bindings, each binding is represented by a distinct object. However, * between different clusters of bindings, the binding objects may or may * not be different; in these cases, the client should compare bindings * using {@link #isEqualTo(IBinding)}, which checks their keys. * * @param obj {@inheritDoc} * @return {@inheritDoc} */ public boolean equals(Object obj); It's not crystal clear, but "Within the context of a single cluster of bindings, each binding is represented by a distinct object" is IMO the main definition. The Javadoc then explains that users should use #isEqualTo(IBinding) for different clusters, from which I assume that #isEqualTo(IBinding) is not necessary when comparing bindings from the same cluster.
I think this is true for type bindings, but I doubt this is true for field or method bindings.
Jim, I think this spec is wrong for method and fields. Philippe, is this true for generic types? I would recommend to always use isEqualTo(...).
Prior to generics, I believe the story was simple. Each binding was a distinct object and all bindings were represented canonically. Generics has changed that: now some of the bindings are not represented canonically. The original spec did not really deal with the issue of canonical representations, because it was a non-issue at the time it was written. Since it would be difficult/expensive for the compiler or AST to canonicalize all bindings, I believe we should clarify the spec. and tell clients that the bindings are usually canonically but get a little weird in and around generics. If the different matters to the client (as it does to JDT refactoring), they should use #isEqualTo(IBinding) even when comparing bindings from the same cluster. Markus, Are you ok with this?
Hm, since generics are everywhere now, that would mean that we have to change all our uses of == to #isEqualTo(IBinding). Wouldn't it be better then to implement #equals(Object) and #hashCode(), such that bindings can also be used in hashsets, etc.? Doing this change would basically mean we have to look over all our code, and I'd rather do this only once :-).
Interesting suggestion. The spec for IBinding.equals(Object) clearly states that bindings are compared with ==. This means that changing it to use isEqualTo(IBinding) is clearly an API contract change. The contract change would be a breaking API change. As a practical matter, returning non-canonical bindings breaks some clients. JDT UI is one example. Philippe/Olivier, What would be involved in making sure all bindings exposed by the AST binding API are canonical within a cluster?
Is it really that expensive to canonicalize the bindings of this example? I find the effects on the API client side much more severe. The spec of equals is really clear. Adding a loosly specified 'doubt' will lead to have us converting all == to isEqualTo, just to make sure we don't miss a case e were not aware of. If references to List<String> are always the same, so should all references to e.g. Collections.asList(new ArrayList()) be.
Anything is possible in theory. Either the DOM could protect itself, or the compiler could cache generic method invocation binding so as to reuse the same one over and over again. The latter approach would be more in line with the general approach for canonicalizing types. Kent: do you agree ?
In theory - yes. But in reality, I'm not convinced we can find all the places to cache generated method bindings.
As pointed out by Markus and Martin our current code relies on the fact the bindings for identical elements are indentical for AST created in the same cluster. Removing this assumption is quite some for us to do (we have to find all places as well and we can't even search for IBinding == IBinding). Additional other clients have to adapt their code as well.
I understand but that doesn't make it doable for us either.
What are we going to do here ? The current behaviour will become more and more a problem (in mark occurrences, source actions, refactoring) as soon as users start to make more use of 1.5 features. IMO this is even an issue we have to consider for 3.1.1.
I agree with Philippe that we should try to canonicalize generic method invocation bindings in the compiler. Barring that, we should try to canonicalize them within the AST implementation. Kent: could you investigate further to see whether it would feasible to find all the places to cache generated generic method bindings ?
Olivier added ASTConverter15Test#test0214
Implementing canonicalization for 3.2RC2. Need to double check it has no impact on JDT/UI.
Created attachment 38510 [details] Patch implementing method canonicalization
Fixed.
Martin - pls cast your vote for RC2.
added ASTConverter15Test#test0215,0216
+1 from JDT/UI. All our tests pass.
Verified with I20060427-1600 for RC2.
