Community
Participate
Working Groups
Created attachment 171960 [details] jar file for reproducing the problem Build Id: 3.6RC4 Using the attached JAR file, and trying content assist on the following code: class Test { void test() { new foo.Foo.Bar([CTRL-SPACE } } we get wrong proposals for the arguments of the constructor i.e. arg0 and arg1 in place of a and b. Further investigation showed that JavaElement.getElementInfo(IProgressMonitor) throws a JavaModelException in this case. This only happened for the inner class Bar, and worked fine for Foo. To reproduce: 1) Use above source code and attached jar file(add it to build path). 2) try content assist where indicated 3) put a breakpoint on BinaryMethod.java:183 i.e. at IBinaryMethod info = (IBinaryMethod) getElementInfo(), and then watch a JavaModelException being thrown. 4) Replace new foo.Foo.Bar( with new foo.Foo( and then try ctrl-space. Works fine!
Jay, can you please have a look? Thanks!
In this case you actually need an enclosing instance to create instances of Bar. So you would need to write: new foo.Foo(1).new Bar( That might explain the exception, but we should still behave better.
Forgot to mention - To reproduce this, the patch in bug 151500 needs to be there in the workspace.
I am seeing strange things with SourceMapper - the constructors are getting an additional parameter - e.g Bar(int, int) becomes Bar(Foo, int, int). Will dig deeper to find what's happening.
(In reply to comment #4) > I am seeing strange things with SourceMapper - the constructors are getting an > additional parameter - e.g Bar(int, int) becomes Bar(Foo, int, int). Will dig > deeper to find what's happening. That seems to be expected behavior. For constructors in binary types, we add the enclosing type as the first parameter. However, in InternalCompletionProposal when we are trying to get a handle to the method from SourceMapper, we have only the original parameters (e.g. {I, I}), which returns null. Our options are: 1. Let InternalCompletionProposal figure out whether or not a this is a constructor for an enclosed type and in which case, it has to include the additional parameter to find the method. 2. Provide a utility method in SourceMapper, which takes the method name and original constructor parameters and return the parameter names. I will see which one is better/simpler and come up with a patch.
Created attachment 172479 [details] Proposed patch Proposed patch with the second approach. All tests pass with this patch. Might add more tests. Ayush, let me know if this works for you.
(In reply to comment #6) > Created an attachment (id=172479) [details] > Proposed patch > > Proposed patch with the second approach. All tests pass with this patch. Might > add more tests. > > Ayush, let me know if this works for you. Patch looks good. There's just another place where the new method could be used: InternalCompletionProposal#findConstructorParameterNames(char[], char[], char[], char[][])
Created attachment 172729 [details] Modified patch This is a better patch, because, 1) This works even when there is no source attached. 2) No additional HashMap and hence lesser memory consumption Ayush, Let me know if patch is alright. I suppose the tests for bug 151500 should be good enough for this one too.
(In reply to comment #8) > Created an attachment (id=172729) [details] > Modified patch > > This is a better patch, because, > 1) This works even when there is no source attached. > 2) No additional HashMap and hence lesser memory consumption > > Ayush, > Let me know if patch is alright. > > I suppose the tests for bug 151500 should be good enough for this one too. Yup this patch looks much much better, and solves the root problem of not being able to get info for inner classes in external jar. Once again, InternalCompletionProposal#findConstructorParameterNames(char[], char[], char[], char[][]) also has to have the same changes. Otherwise its fine.
Created attachment 172890 [details] Updated patch I have applied the fix to InternalCompletionProposal.findConstructorParameterNames() as well. Also extracted the changes in to a private method. Patch is currently under test.
Released in HEAD for 3.7M1.
I tried the steps from comment 0 and I am not getting 'a' and 'b' but arg0 and arg1. Reopening.
The line: final int paramCount = info.getArgumentNames() == null ? info.getArgumentNames().length : Signature.getParameterCount(new String(info.getMethodDescriptor())); should be: final int paramCount = info.getArgumentNames() != null ? info.getArgumentNames().length : Signature.getParameterCount(new String(info.getMethodDescriptor())); The current problem comes from the synthetic argument in constructor of the member type. It is not "removed" so the check (line 290 in org.eclipse.jdt.internal.core.BinaryMethod): char[][] argumentNames = info.getArgumentNames(); if (argumentNames != null && argumentNames.length == paramCount) { returns false. I also noticed that we are calling info.getArgumentNames(); many times. We might want to cache the result. In fact replacing == null with != null in the paramCount assignment fixes the issue. Will run all tests with this patch.
(In reply to comment #13) > > In fact replacing == null with != null in the paramCount assignment fixes the > issue. > Will run all tests with this patch. This is actually not enough. getArgumentNames() should only be used once the javadoc has been checked. Working on a new patch.
Created attachment 175894 [details] Proposed fix Patch not fully tested. Jay, please review and add proper regression test for this. Thanks.
(In reply to comment #15) > This is actually not enough. getArgumentNames() should only be used once the > javadoc has been checked. > Working on a new patch. If the IMethodInfo.getArgumentNames() returns a non-null value and only the original parameter names (in this case a and b), is there a reason why we don't want to use it or rely on it?
Jay, does the latest patch being consideration address the bug 322880 ?
(In reply to comment #17) > Jay, does the latest patch being consideration address the > bug 322880 ? No. I'll check what is going on.
Created attachment 177811 [details] Patch with testcase The patch contains the fix from comment #15 plus tests. All model tests pass. Will update once the all tests pass.
(In reply to comment #19) > Created an attachment (id=177811) [details] > Patch with testcase > > The patch contains the fix from comment #15 plus tests. All model tests pass. > Will update once the all tests pass. All tests pass.
Released in HEAD for 3.7M2.
I don't see the parameter names for static inner classes. Filed bug 325270 to take care of it.
It works good for non-static inner classes. Hence marking it verified. Verified for 3.7M2 using build I20100909-1700
The fix is not 100% correct as it caches arg0, arg1, etc. which means if my connection times out on the first attempt, I will always get raw names instead of the ones from the attached Javadoc. Filed bug 329671 to track that.
