Community
Participate
Working Groups
Build Identifier: M20110210-1200 In the attached code, there are 2 anonymous classes within this file. If I place the cursor in the run() method of the second runnable, inside main, the code below returns an IMethod in class Test$1 instead of Test$2 (which it should return). private IMember getCurrentMethod() { IJavaElement element = getSelectedJavaElement(); if (element != null) { try { if (element instanceof ICompilationUnit) { element = ((ICompilationUnit) element).getElementAt(currentSelection.getOffset()); } else if (element instanceof IClassFile) { element = ((IClassFile) element).getElementAt(currentSelection.getOffset()); } if (element instanceof IMethod || element instanceof IInitializer) return (IMember) element; } catch (JavaModelException e) { return null; } } return null; } private IJavaElement getSelectedJavaElement() { if (activeEditor != null) { ITextEditor editor = (ITextEditor) activeEditor.getAdapter(ITextEditor.class); if (editor != null) { ISelection selection = editor.getSelectionProvider().getSelection(); if (selection instanceof ITextSelection) { this.currentSelection = (ITextSelection) selection; return getJavaElement(activeEditor.getEditorInput()); } } } return null; } private IJavaElement getJavaElement(IEditorInput input) { IJavaElement je = JavaUI.getEditorInputJavaElement(input); if (je != null) return je; //try to get from the working copy manager return ((WorkingCopyManager) JavaUI.getWorkingCopyManager()).getWorkingCopy(input, false); } Reproducible: Always Steps to Reproduce: 1. Create a project from the attached file in a pde environment 2. Use the given code to get the java element from the run() method of second runnable 3. The IMethod is from type Test$1 instead of Test$2.
Created attachment 192914 [details] the file to test against
I don't think the number ($1) really means the order of the anonymous class. I think you would see $1 for both elements. To be sure, you can just check what element.getElementName() returns. This usually returns the name of the method that contains the anonymous class.
This is a valid bug. If the second runnable generates the class Test$2, the JDT api should respond with a IJavaElement of name Test$2, not Test$1.
The model classes seem to rely on the occurrence count to produce the anonymous type name. And the context for the occurrence count is the immediately enclosing block/method. I am not sure if we can resolve this for M7. Olivier, please let me know on your thoughts on this.
Created attachment 193352 [details] Proposed Patch After a closer look, we can have the correct occurrence count if we use the CU as the key for the hash table that stores the counts. The patch fixes the problem reported and tests have been added under TypeResolveTests#testBug342393(). Olivier, please review this patch, esp. whether it would be in sync with the class file generation.
(In reply to comment #5) > Created attachment 193352 [details] > Proposed Patch This doesn't seem to work for the nested anonymous types. Looking in to it.
Created attachment 193471 [details] Updated patch This patch is better. It works for nested anonymous types as well plus includes new as well as modified tests.
(In reply to comment #7) > Created attachment 193471 [details] > Updated patch > > This patch is better. It works for nested anonymous types as well plus includes > new as well as modified tests. All JDT/Core tests pass. I think this alters the order of the anonymous types appearing in the type hierarchy due to the different occurrence count. Hence, I am going to run the relevant JDT/UI tests as well.
Created attachment 193672 [details] Updated patch with tests One of the JDT/UI tests (refactor) was failing with the previous patch. This one addresses that and includes additional tests.
Created attachment 193683 [details] Patch with additional test Same patch, with an additional test for handle memento.
Satyam, please review this patch.
Jay, I haven't finished the review completely but I think all local types needs these extra handling. Please look at the attached test for more details. ######## public void testBug342393b() throws Exception { try { IJavaProject project = createJavaProject("Test342393", new String[] {"src"}, new String[] {"JCL15_LIB"}, "bin", "1.5"); project.open(null); String fileContent = "package p;\n" + "public class Test {\n" + "Test() {\n" + " class A {\n" + " // one \n" + " public void foo() {\n" + " Throwable a = new Throwable(){ \n" + " // two \n" + " };\n" + " }\n" + " };\n" + "}\n" + "public static void main(String[] args) throws InterruptedException {\n" + " class A{" + "// three\n" + " };\n" + "}}\n"; System.out.println(fileContent); createFolder("/Test342393/src/p"); createFile( "/Test342393/src/p/Test.java", fileContent); ICompilationUnit unit = getCompilationUnit("/Test342393/src/p/Test.java"); int index = fileContent.indexOf("// one"); IJavaElement element = unit.getElementAt(index); assertEquals("Incorrect Type selected", "p.Test$A", ((SourceType)element.getParent()).getFullyQualifiedName()); index = fileContent.indexOf("// two"); element = unit.getElementAt(index); assertEquals("Incorrect Type selected", "p.Test$A$1", ((SourceType)element).getFullyQualifiedName()); index = fileContent.indexOf("// three"); element = unit.getElementAt(index); assertEquals("Incorrect Type selected", "p.Test$AX", ((SourceType)element).getFullyQualifiedName()); //PROBLEM String handleId = ((SourceType)element).getHandleMemento(); IJavaElement newElement = JavaCore.create(handleId); assertEquals("Incorrect Element", element, newElement); } finally { deleteProject("Test342393"); } } ########
(In reply to comment #12) > Jay, I haven't finished the review completely but I think all local types needs > these extra handling. Please look at the attached test for more details. I think this was always working. Something wrong with the test code. Did you mean to use AX as the name for the second local type?
(In reply to comment #13) > I think this was always working. Something wrong with the test code. Did you > mean to use AX as the name for the second local type? I put in AX to make sure that you find out where I think things are wrong. I don't know what the fullyQualifiedName should really be. However, I believe the OccurrenceCount should be 2 there rather than 1.
(In reply to comment #14) > I put in AX to make sure that you find out where I think things are wrong. I > don't know what the fullyQualifiedName should really be. However, I believe the > OccurrenceCount should be 2 there rather than 1. You are right. The names are not correct. Interestingly this is what I see in the output folder: Test$1A$1.class Test$1A.class Test$2A.class Test.class
Satyam brought this point that we may not be in compliant with the current API documentation if we make this fix. Besides we might break existing clients, that may be relying on the existing implementation of IType#getFullyQualifiedName() 1) IMember#getOccurrenceCount() <p> * The occurrence count can be used to distinguish initializers inside a type * or anonymous types inside a method. * </p> The Javadoc doesn't say the scope is the enclosing type. It appears that the intention was to provide a counter only to differentiate two elements with the same name inside the enclosing element - type for initializers and method for anonymous types etc.. 1) IMember#getType(String, int) Anyone who has been using this API would be broken and forced to use a different count. 3) Until Satyam brought this point, I was convinced that the IType#getFullyQualifiedName() should return a unique name. However, the documentation doesn't mention anywhere along these lines. I would like to hear from Olivier and Dani as well. Satyam, please add if I have missed anything.
(In reply to comment #16) > I would like to hear from Olivier and Dani as well. Satyam, please add if I > have missed anything. Given the scope of changes and the potential risk of breaking existing users, this looks like it is too late to change this for 3.7. I would target 3.8.
The byte code and the AST are correct and I guess the problem is there since day one.
Revisiting this for 3.8. 1. As observed under comment #16, the IType#getFullyQualifiedName() is not consistent with the generated class names for anonymous classes. Do we want to fix it? If we did, we would be breaking the existing clients. But considering this a bug, I guess that's okay. 2. Also need to be addressed are the other two APIs that we will break if we change the definition of the occurrence count. The current patch does not work in favor of this. If we get a patch which manages to fix the first (1) without affecting the APIs (2), can we go ahead? Though, I must admit I don't yet know the complexities involved.
(In reply to comment #19) > Revisiting this for 3.8. > > 1. As observed under comment #16, the IType#getFullyQualifiedName() is not > consistent with the generated class names for anonymous classes. Do we want to > fix it? If we did, we would be breaking the existing clients. But considering > this a bug, I guess that's okay. > > 2. Also need to be addressed are the other two APIs that we will break if we > change the definition of the occurrence count. The current patch does not work > in favor of this. > > If we get a patch which manages to fix the first (1) without affecting the APIs > (2), can we go ahead? Though, I must admit I don't yet know the complexities > involved. Maybe add an extra method to the IType interface getFullyQualifiedGeneratedName()? This will probably have to go in an extension interface though, eg IType2.
Is there any way to use any of the patches or any other piece of code to correctly get the name of the class file corresponding to the anonymous class a user is in?
(In reply to comment #21) > Is there any way to use any of the patches or any other piece of code to > correctly get the name of the class file corresponding to the anonymous class a > user is in? Sorry, I thought I had already answered this. If it's for your internal test purpose, you could just use the last patch with the modification you suggested in comment #20.
Just checking since we will soon approach api freeze for version 3.8, if the correct api for this is still on track for inclusion in 3.8?
(In reply to comment #23) > Just checking since we will soon approach api freeze for version 3.8, if the > correct api for this is still on track for inclusion in 3.8? I will take a look at this for M5.
Srikanth/Dani, this one needs some attention. If we want this for 3.8, then it need to be fixed in M6 since this might require API changes. There was a proposed fix, but Satyam feels that it might break existing API contract, particularly because the API document is not very clear. So, we may have to take the not-so-good route of introducing another API (method), which I don't favor. What do you think?
This is a real bug and I don't see why we can't just fix org.eclipse.jdt.core.ITypeRoot.getElementAt(int).
(In reply to comment #26) > This is a real bug and I don't see why we can't just fix > org.eclipse.jdt.core.ITypeRoot.getElementAt(int). Dani, The ITypeRoot#getElementAt() is working well and need not be fixed. It's the IType#getFullyQualifiedName() that needs to be fixed. I am working on a patch that does this while not affecting the occurrence count and related APIs.
(In reply to comment #27) > (In reply to comment #26) > > This is a real bug and I don't see why we can't just fix > > org.eclipse.jdt.core.ITypeRoot.getElementAt(int). > > Dani, > > The ITypeRoot#getElementAt() is working well and need not be fixed. It's the > IType#getFullyQualifiedName() that needs to be fixed. I am working on a patch > that does this while not affecting the occurrence count and related APIs. I see, so the correct Java element is fetched inside the implementation of getElementAt(...) but only the name is wrong?
(In reply to comment #28) > I see, so the correct Java element is fetched inside the implementation of > getElementAt(...) but only the name is wrong? Yes. To be precise, only the occurrence count for inner types is wrong, which is computed relevant to the compilation unit, instead of the directly enclosing type.
To keep things simple and focused, I will restrict this fix for anonymous types alone. This means the issue reported by Satyam in comment #12 will have to be fixed via another bug. Satyam, are you fine with this?
Created attachment 210951 [details] Updated patch This patch leaves the existing occurrence count untouched and no API changes have been made. A new field to store the count within the context of the enclosing type for anonymous type has been added. The code that computes the fully qualified name has been changed to use the new field. The patch doesn't deal with the named types with duplicate names. Also, the existing tests and utility method SourceType#toStringInfo() have been left untouched and still use the already existing count.
Created attachment 211274 [details] Patch with minor changes Patch incorporates the following minor changes: Moved the new field (non-api) to SourceType from NamedMember Moved the search related test to the JavaSearchBugsTests2 Satyam, if you have some time, please take a look at it.
The changes look good to me.
Released the fix here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=02009b41ec945f5cfcf629234ac75cbc76ce6809
The fix requires additional changes where source types are created from bindings as mentioned at bug 372449, comment #6. Will soon post a patch for the change.
Created attachment 211569 [details] Additional fixes This fix is required in addition to what we already have in HEAD.
(In reply to comment #36) > Created attachment 211569 [details] > Additional fixes Will release it after some testing including the automated ones.
(In reply to comment #36) > Created attachment 211569 [details] Release the fix in HEAD. Will close the bug after confirmation on bug 372449. I will also try to replicate the JDT/Debug test that was failing in JDT/Core.
(In reply to comment #38) > Release the fix in HEAD. Will close the bug after confirmation on bug 372449. I > will also try to replicate the JDT/Debug test that was failing in JDT/Core. Mike has confirmed that the debug tests pass with the released JDT/core changes. Replicating the regression test in JDT/Core proved to be too much of work. I am not doing it since we already have tests that cover the scenario in JDT/Debug.
Verified for 3.8M6 using build I20120312-1300
