Community
Participate
Working Groups
The signatures returned by lambda IMethod APIs should be dot-based. Currently, getReturnType() and getParameterTypes() return slash-based signatures. This needs to be fixed to make Javadoc hovers on Lambda expressions work (once codeSelect on "->" returns the lambda method and not the SAM). Example: interface I { /** * Does it! Really. * @param number the int * @param str the String */ Object doit(int number, String str); } class X { I i = (i, s) -> { return null; }; } E.g. use the JavaElement view to inspect the local variable "s" and then check its declaring member. - second parameter type is "Ljava/lang/String;". Should be "Ljava.lang.String;". - return type is "Qjava/lang/Object;", which is wrong. An unresolved signature ("Q") cannot be qualified. Should be "Ljava/lang/Object;" - please also check the other API methods
Jay, thanks for following up - this is targetted for GA.
Markus, just as information: We don't have two abstractions one for unresolved lambda method and another for resolved lambda method as is the case with say SourceMethod. Because lambdas are not stored in the model and are fabricated on the fly, we work with only resolved lambda methods - so a call to unresolved() would return the same method. We do maintain for the type an unresolved one and a resolved one - this seems to be required for type hierarchy to work properly.
See org.eclipse.jdt.core.BindingKey.toSignature() org.eclipse.jdt.core.IMethod.getExceptionTypes() org.eclipse.jdt.core.IMethod.getParameterTypes() org.eclipse.jdt.core.IMethod.getReturnType() org.eclipse.jdt.core.IMethod.getSignature() would appear to be the APIs affected.
(In reply to Markus Keller from comment #0) > - second parameter type is "Ljava/lang/String;". Should be > "Ljava.lang.String;". > - return type is "Qjava/lang/Object;", which is wrong. An unresolved > signature ("Q") cannot be qualified. Should be "Ljava/lang/Object;" > - please also check the other API methods Or could it be "QObject"? I am asking because when I inspect the method declaration doit(), I see the second parameter type to be "QString".
Created attachment 240640 [details] Patch under test The problem with TypeBinding#signature() was it was coming from a resolved binding, which was prefixing 'L' always. I have made changes to use constantPoolName() instead and this brings up expected results in Java element view. Though some unit tests are failing, this is expected and need to be fixed accordingly.
(In reply to Jayaprakash Arthanareeswaran from comment #5) > Created attachment 240640 [details] > Patch under test > > The problem with TypeBinding#signature() was it was coming from a resolved > binding, which was prefixing 'L' always. I have made changes to use > constantPoolName() instead and this brings up expected results in Java > element view. Though some unit tests are failing, this is expected and need > to be fixed accordingly. Couple of additional points: 1. TypeBinding#signature() internally uses constantPoolName() 2. The other scenario where we create Lambda method from memento should be taken care of with this.
Note that Signature's Javadoc has examples that contradict the solution from comment 5: "Ljava.lang.String;" denotes java.lang.String in compiled code "QString;" denotes String in source code "Qjava.lang.String;" denotes java.lang.String in source code Since there's no source code for these type references, I would not expect unresolved ("Q") types here. But "QObject;" would still be better than "Qjava/lang/Object;". (In reply to Markus Keller from comment #0) > An unresolved signature ("Q") cannot be qualified. Scrap that. A Q-signature can be qualified, which means it was qualified in source. The first parameter of type "int" is now "QI;". Should be just "I". And for for lambda methods with parameterized parameter types, Signature#getParameterCount() throws an IAE with this patch. To reproduce, e.g. search for references to "arg" here: Function<List<String>, List<String>> sup = (arg) -> { return new ArrayList<>(arg); }; I don't fully understand why this happens, but I believe it's because CharOperation.replace(name, '/', '.'); modifies the char[] that stores the TypeBinding's constantPoolName (should make a copy). java.lang.IllegalArgumentException at org.eclipse.jdt.core.Signature.getParameterCount(Signature.java:1610) at org.eclipse.jdt.core.Signature.getParameterTypes(Signature.java:1656) at org.eclipse.jdt.core.Signature.getParameterTypes(Signature.java:1694) at org.eclipse.jdt.internal.ui.viewsupport.JavaElementLabelComposer.appendMethodLabel(JavaElementLabelComposer.java:398) ...
(In reply to Markus Keller from comment #7) > Note that Signature's Javadoc has examples that contradict the solution from > comment 5: > > "Ljava.lang.String;" denotes java.lang.String in compiled code > "QString;" denotes String in source code > "Qjava.lang.String;" denotes java.lang.String in source code That sounds like trouble. We are constructing the signature from bindings and I am not sure if we can find whether the used names were qualified. What happens in the case where the argument type is not explicitly stated?
Okay, I think I will proceed with this as the requirement: For all non primitive types, the signatures will be of the format "Qjava.lang.String;" and of course, for int it should be just "I". Also, I think TypeBinding#readableName() can be useful too.
Created attachment 240663 [details] Updated patch Patch updated with what has been discussed so far. Still under test. And tests to be updated.
Created attachment 240669 [details] Patch 3 (In reply to Jayaprakash Arthanareeswaran from comment #8) > That sounds like trouble. We are constructing the signature from bindings > and I am not sure if we can find whether the used names were qualified. What > happens in the case where the argument type is not explicitly stated? That's exactly why I'd prefer resolved signatures (starting with "L"). The new CharOperation.deepCopy is unnecessary. You can just use clone() on the char[]. But in this case, you best use CharOperation.replaceOnCopy. The many-args LambdaMethod.make(..) method must pass a signature to info.setReturnType(..). Clients expect a "name" there, not a signature. And LambdaMethod needs to override getSignature().
(In reply to Markus Keller from comment #11) > Created attachment 240669 [details] > Patch 3 ... > The many-args LambdaMethod.make(..) method must pass a signature to > info.setReturnType(..). Clients expect a "name" there, not a signature. This is what forced me to override getReturnType() in LambdaMethod(). But I failed to recognize that SourceMethodInfo#getReturnTypeName() can be used by clients. I don't know how to view the result of LambdaMethod#getSignature() on Java element view, but looking at code, it looks like will produce the same result as the one it is overriding - SourceMethod#getSignature(). Is it because you want to reuse the signature and not want to convert the return type name into signature again? Also we should be prepared to expect that for the same "String", depending on where it occurs, the signature could either be "QString;" or "Ljava.lang.String;" Apart the above, the patch looks good.
Created attachment 240714 [details] Same patch + updated tests All but 2 failing tests have been updated to go with the new signature format involving lambdas. Wanted to draw your attention to the failing tests. These tests have type parameter as lambda argument such as the following case: private static <I, R> Function<I, R> castingIdentity() { return i -> (R) i; } This code is in JavaSearchBugs8Tests.testBug400905_0017(). Earlier the return type signature was "java.lang.Object" which has become "R" now. This looks okay to me. If you think otherwise, please let me know so I can look into this.
I glanced through the changes and they look good. Jay, please proceed to release. Thanks Markus & Jay.
Released via: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=6bbe71e24e5d5d76445f30e5351f63286e328fac
(In reply to Markus Keller from comment #11) > The many-args LambdaMethod.make(..) method must pass a signature to > info.setReturnType(..). Clients expect a "name" there, not a signature. "... must *not* pass a signature ..." of course (as done in the patch). (In reply to Jayaprakash Arthanareeswaran from comment #12) > I don't know how to view the result of LambdaMethod#getSignature() on Java > element view, codeSelect currently (bug 429814) doesn't return the lambda method, but it does return a local variable that is a lambda parameter. => My path to get the lambda method in the JavaElement view is to select the parameter ("i" in comment 13) and then set the input from that element. The PARENT or DECLARING MEMBER is the lmabda method. The signature is visible in the IMethod section in the Properties view (e.g. open it via context menu). Note that E4 has a bug that doesn't correctly initialize the Properties view, so you have to select the element again in order to see its properties. > ... but looking at code, it looks like will produce the same > result as the one it is overriding - SourceMethod#getSignature(). Is it > because you want to reuse the signature and not want to convert the return > type name into signature again? Yes, and there's a subtle difference: SourceMethod#getSignature() always returns an unresolved return type. With "this.returnTypeString", we keep the resolved type. It would be odd to have resolved parameterTypes but unresolved return type.
We should actually use TypeBinding#genericTypeSignature() here. That also correctly writes type variables as "TU;" instead of the wrong "LU;". Example: import java.util.List; interface Getter<E> { E get(List<E> list, int i); } public class X<U> { public void foo(List<U> l) { Getter<U> g= (x, i) -> x.get(i); } } Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=30b874e8717a53ccaa75e8a5f368d88b909a5d14
(In reply to Markus Keller from comment #17) > Fixed with > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=30b874e8717a53ccaa75e8a5f368d88b909a5d14 Markus, this commit is missing on BETA_JAVA8, link above leads to bad commit reference page, thanks for checking.
(In reply to Srikanth Sankaran from comment #18) > (In reply to Markus Keller from comment #17) > > > Fixed with > > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > > ?id=30b874e8717a53ccaa75e8a5f368d88b909a5d14 > > Markus, this commit is missing on BETA_JAVA8, link above leads to bad commit > reference page, thanks for checking. Jay, if the commit from Markus does not include a test, please grab the example from his comment and include, TIA.
(In reply to Srikanth Sankaran from comment #19) > Jay, if the commit from Markus does not include a test, please grab the > example > from his comment and include, TIA. I don't see the commit either.
Sorry, had a non-FF conflict. Here's the fix and test case for comment 17: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=8c9e1c8b4d0dd342090e4cf1131f504c44c1f1d9
