Community
Participate
Working Groups
Here is a white-box test against 3.2RC3, which shows that TypeDeclaration.createDefaultConstructorWithBinding() may break method sorting in a - admittedly contrived - situation: public class T { interface I { } void test() { new I() { void foo() { } }.foo(); // compiles OK. new I() { void $foo() { } }.$foo(); // The method $foo() is undefined for the type new T.I(){} } } I constructed this test to prove the assumption wrong that constructors should always be the first methods in the sorted array of methods. The legal character '$' is less then '<'. I was inspired by bug 137744, where the issue of adding methods to the sorted array of methods is discussed. The above situation seems to be the only remaining situation where methods are indeed added after sorting. In our own adaptation of the compiler we have several situations of adding (generated) fields/methods after sorting and so I implemented methods ReferenceBinding.sortedInsert(..), which for individual members should be faster than resorting the whole array. Would you be interested in such a solution? Of course, the downside of sorted insertion is that the array has to grow for each individual member being inserted.
This is a regression over 3.2M5. Problem started to appear in 3.2M6.
Thanks for reporting this in such a nice and simple way. I will candidate this for being a "great bug".
Created attachment 40693 [details] Proposed patch This patch doesn't perform sorting eagerly, simply reset the tagbits, so that next client will cause sorting. In case multiple members were to be inserted, this could allow batching them all.
Raising severity since this is a regression introduced during 3.2 cycle (as a consequence of sorting methods for improved performance). Fix is very local and uses same approach as already used in other places (just one place ommitted). Dani/Martin: pls cast your vote. Without the fix, we reject valid code by mistake, and though this is a corner situation, it is still real code (considering code generators etc...).
Added LookupTest#test070
patch reviewed and ok for 3.2 RC4.
Adding Darin, since Dani is presenting at a conf right now. Darin - would you cast your vote ?
+1
Fixed released.
Created attachment 40932 [details] Additional patch Resort methods explicitely instead of make it lazy as DOM needs eager sorting...
New fix proved better, as UI tests need it.
The TagBits.AreMethodsSorted bit gives the impression that there are states when the methods are not sorted. Are you saying that can not be the case (is TagBits.AreMethodsSorted dispensable?)
The bit is used to record the fact the method array got sorted. We do not eagerly sort methods, until someone is trying to access them.
Created attachment 40964 [details] alternative methods Since eager vs. lazy seems to be an issue after all, here are the methods we use for sorted insertion. The first one (basically copied from binarySearch) sits in ReferenceBinding, the second in SourceTypeBinding. With that, TypeDeclaration.createDefaultConstructorWithBinding simply calls this.binding.addMethod(constructor.binding); Use it if you like, else ignore it ;-) PS: how come DOM seems to succeed reading an unsorted list? Is someone bypassing ReferenceBinding.methods()?
Thanks, looks nice. A bit overkill, but may include post 3.2. The DOM didn't read unsorted list, simply failed to sort it lazily (since the code is also lazily resolving the methods, which is unneeded here, and a scope is required, but missing in DOM case after cleanup has run).
Verified with I20060511-2000 for 3.2RC4. Failed with RC3 and fixed with RC4.
