Community
Participate
Working Groups
I20100107-1410 - new workspace - check out org.eclipse.text.tests from CVS - open class TextEditTests - set caret into "TestSuite" in the suite() method - press F3 => "Current text selection cannot be opened in an editor" in status line. The unusual thing here is that TestSuite comes from org.junit version 4.7.0, where it is a generic class. But org.eclipse.text.tests has only 1.4 compliance. I guess the the problem is that the TestSuite class gets parsed in 1.4 mode, although the class file is known to contain 1.5 code.
I'll take a look.
TestSuite is not actually a generic class. It just has a constructor that has a generic parameter type.
The problem comes from the method that is reported as not existing. We are searching for a IMethod that has Ljava.lang.Class; as a type parameter. where the binary type contains the method [Ljava.lang.Class<+Ljunit.framework.TestCase;>;]. So when we check if the method exists, it reports false and therefore we end up reporting that there is nothing to open. This means we should open the binary type as a 1.4 type if we want to be able to match the IMethod. I am investigating.
One way to fix this is to initialize the java element using the project's options. Then the IMethod should be consistent with the one that is searched by the SelectionEngine. This might have the consequence of not being able to match 1.5 elements when inside a 1.4 project for types defined in its libraries.
Olivier, do you want me to take this one up ?
(In reply to comment #5) > Olivier, do you want me to take this one up ? Sure. Thanks.
I am able to reproduce this problem with Build id: I20101025-1800 but not with HEAD. Markus, if you can take a moment to confirm, that would be super. I'll add a regression test and close this as WORKSFORME.
Created attachment 182269 [details] Regression test If I back out the change to org.eclipse.jdt.internal.compiler.lookup.BinaryTypeBinding.createMethod(IBinaryMethod, long, char[][][]) brought about by the fix to bug 328775 problem resurfaces. So there is no question whatsoever that this bug is fixed on HEAD :)
Released regression test in HEAD for 3.7 M4
*** Bug 80742 has been marked as a duplicate of this bug. ***
*** Bug 90021 has been marked as a duplicate of this bug. ***
(In reply to comment #7) > I am able to reproduce this problem with Build id: I20101025-1800 > but not with HEAD. Markus, if you can take a moment to confirm, that > would be super. Verified in N20101102-2000. I also checked with comment 0 with tag v20100112-0800 and with various combinations of installed and not installed JREs. Everything looked fine and resolved.
(In reply to comment #3) > The problem comes from the method that is reported as not existing. > > We are searching for a IMethod that has Ljava.lang.Class; as a type parameter. > where the binary type contains the method > [Ljava.lang.Class<+Ljunit.framework.TestCase;>;]. So when we check if the > method exists, it reports false and therefore we end up reporting that there is > nothing to open. > > This means we should open the binary type as a 1.4 type if we want to be able > to match the IMethod. I am investigating. This bug is likely to reopened when the fix for bug 330435 is released. Here is some more analysis. When the java model cache is populated by reading a class file, we always internalize any generic artifacts that exist in the class file. This is because the model is shared by all projects in the workspace and some of them could be 1.5+. But when a BinaryTypeBinding is created on behalf of a 1.4 project, we skip certain generic information and this results in a mismatch between lookup and repository. In order to support a workspace that contains both 1.4 and 1.5+ the model should theoretically support erasure akin to the compiler's support and answer the appropriate method when queried using an IMethod with parameter type key set to Ljava.lang.Class; or with [Ljava.lang.Class<+Ljunit.framework.TestCase;>;] Under investigation.
Needs further investigation after fix for bug 330435.
Created attachment 183661 [details] Early version of patch I basically see two approaches: (1) Tweak BinaryTypeBinding to always admit generic information associated with a method parameter. (2) Tweak BinaryMethod's hashCode and equals methods to always ignore parameterization information associated with parameters (effectively using erased parameter types in hashCode and equals) The current patch (works on the test case concerned but hasn't beed subjected to much testing besides yet) takes the latter approach. The former approach can be tried too, it is just that every time I have allowed some generics to be internalized out of necessity in some situation, it stirs up a hornet's nest :-) :-(. If we have no other option, we can/should bite the bullet and go ahead with (1), but I would like to hear opinions on approach (2) embodied by this patch. Olivier, Markus, let me know what you think. If this line of attack stands scrutiny, I can look into a cleaner, faster implementation (assuming the automated tests don't uncover any major issues of course)
See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=82558
Passes all JDT/Core tests.
Created attachment 183675 [details] Revised patch under test Same patch as before, but this one avoids needless object creation, call delegation etc.
Olivier, please review, TIA.
SourceMethod doesn't need a similar change since SourceTypeConverter is directly based on the model.
I am not against the principle, but I think we should cache the erased parameter types array. Using the equals or hashcode method a lot can lead to potential performance issues.
Created attachment 183813 [details] Patch incorporating suggestions from comment#21
For safety in concurrent accesses, I prefer not to initialize a field as long as the correct value is not fully computed. So instead of: private String [] getErasedParameterTypes() { if (this.erasedParamaterTypes == null) { int paramCount = this.parameterTypes.length; this.erasedParamaterTypes = new String [paramCount]; for (int i = 0; i < paramCount; i++) { String parameterType = this.parameterTypes[i]; this.erasedParamaterTypes[i] = parameterType.indexOf(Signature.C_GENERIC_START, 0) >= 0 ? new String(Signature.getTypeErasure(parameterType.toCharArray())) : parameterType; } } return this.erasedParamaterTypes; } I would write: private String [] getErasedParameterTypes() { if (this.erasedParamaterTypes == null) { int paramCount = this.parameterTypes.length; String[] temp = new String [paramCount]; for (int i = 0; i < paramCount; i++) { temp[i] = Signature.getTypeErasure(this.parameterTypes[i]); } this.erasedParamaterTypes = temp; } return this.erasedParamaterTypes; } I would also remove the check for generic start and directly call the Signature.getTypeErasure(..) as it already returns the current signature if this is not a generic signature. So with the current code the index is search twice. And one last thing: /** * The parameter type signatures of the method - stored locally * to perform equality test. <code>null</code> indicates no * parameters. */ should be updated to reflect that parameterTypes cannot be null as in the constructor they are initialized to an empty array if the parameter is null.
(In reply to comment #23) > For safety in concurrent accesses, I prefer not to initialize a field as long > as the correct value is not fully computed. Yes, coincidently I had already made this changes in my workspace. > I would write: > private String [] getErasedParameterTypes() { > if (this.erasedParamaterTypes == null) { > int paramCount = this.parameterTypes.length; > String[] temp = new String [paramCount]; > for (int i = 0; i < paramCount; i++) { > temp[i] = > Signature.getTypeErasure(this.parameterTypes[i]); > } > this.erasedParamaterTypes = temp; > } > return this.erasedParamaterTypes; > } In the revised patch I have the original array itself is assigned to erasedParameterTypes if none of the parameters types involve generics (to save memory foot print) > I would also remove the check for generic start and directly call the > Signature.getTypeErasure(..) as it already returns the current signature if > this is not a generic signature. So with the current code the index is search > twice. Yes, > And one last thing: > /** > * The parameter type signatures of the method - stored locally > * to perform equality test. <code>null</code> indicates no > * parameters. > */ > should be updated to reflect that parameterTypes cannot be null as in the > constructor they are initialized to an empty array if the parameter is null. yes, will do. Thanks.
(In reply to comment #23) > I would write: > private String [] getErasedParameterTypes() { > if (this.erasedParamaterTypes == null) { > int paramCount = this.parameterTypes.length; > String[] temp = new String [paramCount]; > for (int i = 0; i < paramCount; i++) { > temp[i] = > Signature.getTypeErasure(this.parameterTypes[i]); > } > this.erasedParamaterTypes = temp; > } > return this.erasedParamaterTypes; > } > > I would also remove the check for generic start and directly call the > Signature.getTypeErasure(..) as it already returns the current signature if > this is not a generic signature. So with the current code the index is search > twice. Now I remember, actually it was a conscious decision to have the check for generic start before calling Signature.getTypeErasure. This is to avoid needless object creation. I reckon for most methods, the parameter type doesn't involve generics, so calling temp[i] = Signature.getTypeErasure(this.parameterTypes[i]); will wastefully duplicate the strings.
Created attachment 183819 [details] Patch to be released. I think this patch is the best compromise between time & space. - For safety in concurrent situations, the field gets initialized only after fully computed. - No array duplication, or array element duplication occurs if none of the parameters's type involves generics. - indexOf(Signature.C_GENERIC_START) check still happens to avoid needless duplication of strings as org.eclipse.jdt.core.Signature.getTypeErasure(String) doesn't quite return the current string, but a new copy. - javadoc is fixed regarding null reference. Let me know if I have overlooked something.
(In reply to comment #26) > - indexOf(Signature.C_GENERIC_START) check still happens > to avoid needless duplication of strings as > org.eclipse.jdt.core.Signature.getTypeErasure(String) doesn't > quite return the current string, but a new copy. You could also fix Signature#getTypeErasure(String). The current implementation doesn't completely fulfill its contract "Returns the given type signature if it is not parameterized.". Fix: char[] signature = parameterizedTypeSignature.toCharArray(); char[] erasure = getTypeErasure(signature); return signature == erasure ? parameterizedTypeSignature : new String(erasure);
(In reply to comment #27) Actually, Signature#removeCapture(String) is already implemented like this, and #getElementType(String) could also use the same trick.
I have raised bug 331120 to track issues raised by comment 27 and comment 28.
Released in HEAD for 3.7 M4
I still believe that the indexOf should not be checked twice. CharOperation implementation should be fixed instead. Otherwise once bug 331120 is fixed, the check is really duplicated for no good reason. Am I missing something ?
(In reply to comment #31) > I still believe that the indexOf should not be checked twice. CharOperation > implementation should be fixed instead. Otherwise once bug 331120 is fixed, the > check is really duplicated for no good reason. > Am I missing something ? (I think you meant Signature and not CharOperation implementation.) Without having actually measured it, I would venture that MOST of method parameters DON'T use generics in their parameter types. (Anyways at 1.4 and below 0% of them do). I think the current implementation pays a small extra price for these minority cases but is faster for the majority common case since the cost of the code chunk char[] signature = parameterizedTypeSignature.toCharArray(); char[] erasure = getTypeErasure(signature); return signature == erasure ? parameterizedTypeSignature : new String(erasure); is eliminated. In general I am opposed to these kind of micro optimizations that have dubious value but have a potential to clutter code, but in this case, IMHO, the code reads clean enough.
(In reply to comment #32) > (In reply to comment #31) > > I still believe that the indexOf should not be checked twice. To clarify, for a huge majority of methods, indexOf will not be checked twice (dynamically that is).
(In reply to comment #33) > To clarify, for a huge majority of methods, indexOf will not be > checked twice (dynamically that is). I know. I am not so much for micro optimization as well, but in this case I find the code that is using directly: temp[i] = Signature.getTypeErasure(this.parameterTypes[i]); to be more legible that the one using the indexOf call. Now I won't argue too much on that. I think with the fix for bug 331120 this would be identical.
(In reply to comment #34) > (In reply to comment #33) > > To clarify, for a huge majority of methods, indexOf will not be > > checked twice (dynamically that is). > I know. I am not so much for micro optimization as well, but in this case I > find the code that is using directly: > > temp[i] = Signature.getTypeErasure(this.parameterTypes[i]); > > to be more legible that the one using the indexOf call. I have incorporated this suggestion and released the change in HEAD.
Verified using I20101207-0250 (4.1 I-build). The problem described in comment 0 cannot be reproduced anymore.
