Community
Participate
Working Groups
For many IDE functionalities, we need an IJavaElement for a lambda expression: - CodeSelect should resolve the lambda element when the caret is on the '->'. Currently, there's no easy way to see the contract of the functional interface method that the lambda implements. We should show a Javadoc hover for lambdas. - (Quick) Type Hierarchy should include all implementations of a functional interface (bug 423123); ITypeHierarchyChangedListener should notify of changes. - Reference search matches should show up in a 'lambda' element, not in the element that contains the lambda expression. - The parent of a lambda parameter (ILocalVariable) should be the lambda element, not the enclosing element. About the only place where lambdas are not relevant for the UI is in IJavaElement#getChildren(), since we don't intend to show lambdas in the Outline (but we can also filter them out in the UI if that's easier). Lambdas share many properties of IType and IMethod, but on the other hand also don't support some of their methods. E.g. IType#create*(*) doesn't make sense, the ISourceManipulation methods can just throw a JME, the type name will be "" (like for anonymous types) and the name range will be null. The API of ITypeHierarchy looks like we need at least an IType for a lambda. For existing Java model clients, the best solution would be to have an IType as well as an IMethod for a lambda expression (rather than a new kind of IJavaElement or only an IType but not an IMethod). If it reduces the implementation effort, we can also try to treat lambda elements as pseudo-elements like ILocalVariable , ITypeParameter, IAnnotation, which are not part of IJavaElementDeltas, and which are not returned by getChildren().
Markus, is this a must fix for GA ?
This blocks bug 427082. private static I3 m1() { I3 i3a= i -> { int x= 100; return i+x; }; return i3a; } Java model returns m1() as the parent for ILocalVariable x, JDT debug expects the parent as Lambda expression.
I'd say it's a "must fix", but with lower priority than compiler errors. I.e. we really need it for a good IDE experience, but if the tradeoff is gaps in the compiler, then I'd rather compromise on the IDE side. And if we don't get it done in the next 2-3 weeks, we will have trouble adopting the Java model changes in the UI in time for GA.
(In reply to Markus Keller from comment #3) > I'd say it's a "must fix", but with lower priority than compiler errors. > I.e. we really need it for a good IDE experience, but if the tradeoff is > gaps in the compiler, then I'd rather compromise on the IDE side. > > And if we don't get it done in the next 2-3 weeks, we will have trouble > adopting the Java model changes in the UI in time for GA. Thanks for weighing in Markus (welcome back!). The compiler side of the house is totally under control and can be managed at this point by Stephan and I - so Jay can be freed up to totally devote to this task. Jay, let us proceed on the basis that this is must fix for Java 8 GA. TIA.
(In reply to Srikanth Sankaran from comment #4) > > And if we don't get it done in the next 2-3 weeks, we will have trouble > > adopting the Java model changes in the UI in time for GA. [...] > Jay, let us proceed on the basis that this is must fix for Java 8 GA. TIA. Jay, let us make early draft patches available (when possible) to UI & Debug for testing and feedback - as long as we call out it is work in progress with a known set of limitations/problems, it leaves the UI/Debug teams with the choice to try adoption sooner than if we chose to make available only a finished solution.
*** Bug 424101 has been marked as a duplicate of this bug. ***
Created attachment 240150 [details] Patch - a WIP This patch introduces LambdaExpression in java model and adds code in SelectionRequester to insert it between the LocalVariable and Method. 1. Hover displays the lambda (as anonymous type). But at the moment clicking on Functional interface type doesn't open the type's javadoc in certain scenarios. This is mainly because LambdaExpression is an IJavaElement#TYPE and when we reconstruct from memento, we end up creating a SourceType and not LambdaExpression and trouble starts there as SourceType looks for element info, which doesn't exist. 2. When a lambda parameter (or it's reference) is selected, java element view correctly displays the LocalVariable element and LambdaExpression as it's parent. The search result or a lambda param reference still appear attached to the method.
(In reply to Jayaprakash Arthanareeswaran from comment #7) > This is mainly because LambdaExpression is an IJavaElement#TYPE > and when we reconstruct from memento, we end up creating a SourceType and > not LambdaExpression and trouble starts there as SourceType looks for > element info, which doesn't exist. That observation was wrong. What happens is somewhere along the path of finding the element for the referenced functional interface, we hit the ast converter, which has no idea of lambda (because we don't store them in the model) and as a result we never get the correct element. Need to find out if we can work around this.
(In reply to Jayaprakash Arthanareeswaran from comment #8) > That observation was wrong. What happens is somewhere along the path of > finding the element for the referenced functional interface, we hit the ast > converter, which has no idea of lambda (because we don't store them in the > model) and as a result we never get the correct element. Need to find out if > we can work around this. Given we have the hammer, doesn't this look like a nail to you ? :)
(In reply to Srikanth Sankaran from comment #9) > Given we have the hammer, doesn't this look like a nail to you ? :) For the records, what Srikanth implied here was to mark the compilation unit info with a flag that we are dealing with lambdas here so the SourceTypeConverter can do a full parse instead of constructing the ast from java elements. But ... This doesn't seem to help either. All roads seem to hit the same block - that the AST we get do not yet have the bindings and we have no way of finding what the lambda expression is implementing (i.e. in the complex case where the lamdba is a method argument).
(In reply to Jayaprakash Arthanareeswaran from comment #10) > (In reply to Srikanth Sankaran from comment #9) > > Given we have the hammer, doesn't this look like a nail to you ? :) > This doesn't seem to help either. All roads seem to hit the same block - > that the AST we get do not yet have the bindings and we have no way of > finding what the lambda expression is implementing (i.e. in the complex case > where the lamdba is a method argument). We need to resolve the AST. https://bugs.eclipse.org/bugs/show_bug.cgi?id=400905 is also about the same issue. I'll share a patch for that later today - there could be elements leverageable from there.
Created attachment 240321 [details] Updated patch This gets rid of ILambdaExpression and LambdaElementInfo. Also fixes the issue with navigating to the functional interface declaration from hover. LambdaExpression element need to be merged with bug 400905. The NPE is still present. One way to deal with it would be to resolve the CompilationUnit before SourceElementNotifier kicks in.
Created attachment 240362 [details] Updated patch Previous patch synched up with latest changes with bug 400905. Things appear to work in UI but some unit tests are failing in Resolve18Tests, because in this patch I create LambdaExpression element before it was resolved and hence have incorrect ordinal (and effectively incorrect occurrence count). There are also other failures because I am setting the element's name as "" instead of "<lambda>" because this takes care of many existing things on the UI side.
Srikanth, looks like relying on LambdaExpression#ordinal to compute occurrence count will create problems with code select, because what the selection requester gets is a dummy LambdaExpression which will always have ordinal value of '1'. Any idea?
(In reply to Jayaprakash Arthanareeswaran from comment #14) > Srikanth, looks like relying on LambdaExpression#ordinal to compute > occurrence count will create problems with code select, because what the > selection requester gets is a dummy LambdaExpression which will always have > ordinal value of '1'. Any idea? The problem with making occurrence count derive from 'ordinal' is in the following case where we have both lambdas and anonymous types mixed, the occurrence count will not be contiguous. One way out I can think of is to differentiate a lambda element from an anonymous type by coming up with a new token and different memento string for lambda element. Will investigate to see if there will issues with this.
(In reply to Jayaprakash Arthanareeswaran from comment #15) > The problem with making occurrence count derive from 'ordinal' is in the > following case where we have both lambdas and anonymous types mixed, the > occurrence count will not be contiguous. One way out I can think of is to > differentiate a lambda element from an anonymous type by coming up with a > new token and different memento string for lambda element. Will investigate > to see if there will issues with this. If it wasn't clear, this will allow us to recreate a LambdaExpression element and NOT a SourceType element (as it's the case with the last patch) from a hover link.
(In reply to Jayaprakash Arthanareeswaran from comment #13) > There are also other failures because I am setting the element's name as "" > instead of "<lambda>" because this takes care of many existing things on the > UI side. Could you elaborate what are these existing things that get taken care of ? (In reply to Jayaprakash Arthanareeswaran from comment #14) > Srikanth, looks like relying on LambdaExpression#ordinal to compute > occurrence count will create problems with code select, because what the > selection requester gets is a dummy LambdaExpression which will always have > ordinal value of '1'. Any idea? What exactly is the origin of this dummy node ? (In reply to Jayaprakash Arthanareeswaran from comment #15) > The problem with making occurrence count derive from 'ordinal' is in the > following case where we have both lambdas and anonymous types mixed, the > occurrence count will not be contiguous. This happens perhaps because you reset the name from <lambda> to "" and that clashes with anonymous types. The way the lambda ordinals are computed they are unique and so there will never be a clash among lambdas or between lambdas and anonymous types. (In reply to Jayaprakash Arthanareeswaran from comment #16) > If it wasn't clear, this will allow us to recreate a LambdaExpression > element and NOT a SourceType element (as it's the case with the last patch) > from a hover link. I don't yet understand this, will study the patch in detail. I was leaning towards the suggestion in the last para of comment#0 where we would fabricate them on the fly, only if they feature somewhere in the chain between focus element and the CU.
(In reply to Srikanth Sankaran from comment #17) > (In reply to Jayaprakash Arthanareeswaran from comment #13) > > > There are also other failures because I am setting the element's name as "" > > instead of "<lambda>" because this takes care of many existing things on the > > UI side. > > Could you elaborate what are these existing things that get taken care of ? When the element is an IType and with name as empty string, it's treated as an anonymous and presented accordingly in the UI, such as hover etc. > (In reply to Jayaprakash Arthanareeswaran from comment #14) > > Srikanth, looks like relying on LambdaExpression#ordinal to compute > > occurrence count will create problems with code select, because what the > > selection requester gets is a dummy LambdaExpression which will always have > > ordinal value of '1'. Any idea? > > What exactly is the origin of this dummy node ? I think this originates from SelectionEngine where, depending on the case, we insert one of SelectionOn* nodes and resolve the unit so we get notified with a SelectionNodeFound exception. So, when this happens, regardless of how many lambdas or anonymous types we have in a method, the SelectionRequestor#acceptLocalVariable always sees just one lambda, which contains the parameter we are dealing with. As a result the java element we create will always have 1 as occurrence count.
(In reply to Jayaprakash Arthanareeswaran from comment #18) > When the element is an IType and with name as empty string, it's treated as > an anonymous and presented accordingly in the UI, such as hover etc. I'll take a look at this. I don't yet have the picture of what you describe in my mind, but a lambda should be rendered as a lambda. > I think this originates from SelectionEngine where, depending on the case, > we insert one of SelectionOn* nodes and resolve the unit so we get notified > with a SelectionNodeFound exception. So, when this happens, regardless of > how many lambdas or anonymous types we have in a method, the > SelectionRequestor#acceptLocalVariable always sees just one lambda, which > contains the parameter we are dealing with. As a result the java element we > create will always have 1 as occurrence count. I see. I think you are running into this problem only because you are trying to store in the model and/or recreate from memento string - I think we can safely punt on this support. At least for now. More after I study in detail.
(In reply to Markus Keller from comment #0) > For many IDE functionalities, we need an IJavaElement for a lambda > expression: > > - CodeSelect should resolve the lambda element when the caret is on the > '->'. Currently, there's no easy way to see the contract of the functional > interface method that the lambda implements. We should show a Javadoc hover > for lambdas. This is completed in https://bugs.eclipse.org/bugs/show_bug.cgi?id=429262 > - (Quick) Type Hierarchy should include all implementations of a functional > interface (bug 423123); ITypeHierarchyChangedListener should notify of > changes. Well, hierarchy + search work is mostly complete in https://bugs.eclipse.org/bugs/show_bug.cgi?id=400905, but I am not sure of the ITypeHierarchyChangedListener part - I am not familiar with this. What exactly did you have in mind. > - Reference search matches should show up in a 'lambda' element, not in the > element that contains the lambda expression. > > - The parent of a lambda parameter (ILocalVariable) should be the lambda > element, not the enclosing element. These are in progress and I agree these should be done - just a curiosity question: How/what is it impacting if anything today ?
(In reply to Srikanth Sankaran from comment #20) > > - The parent of a lambda parameter (ILocalVariable) should be the lambda > > element, not the enclosing element. > > These are in progress and I agree these should be done - just a curiosity > question: How/what is it impacting if anything today ? We also need to decide how we want to represent the lambda element on hover? My initial idea was to present it just like an anonymous (new I{}...). Or are other thoughts?
(In reply to Jayaprakash Arthanareeswaran from comment #21) > We also need to decide how we want to represent the lambda element on hover? > My initial idea was to present it just like an anonymous (new I{}...). Or > are other thoughts? I have a patch in the works that changes things from: abc [in foo() [in X [in [Working copy] X.java [in <default> [in src [in Resolve]]]]]] to abc [in lambda$1(int) [in <lambda> [in foo() [in X [in [Working copy] X.java [in <default> [in src [in Resolve]]]]]]]] I think it is useless to encode the lambda method name as lambda$1(int) - we may as well mention the SAM selector ? <lambda> as a type name again is not very useful. I don't think we should make it appear as an anonymous class, that it is a lambda should be clear to the user. I wonder if we should do something like: abc [in apply(String) [in λ(Function) [in foo() [in X [in [Working copy] X.java [in <default> [in src [in Resolve]]]]]]]]
Created attachment 240396 [details] Another approach This patch fabricates and inserts lambda elements at the right place when the focus element is a local variable of the lambda. (Lambda local types (!) will still need to be handled. Jay, please review. A note: Only upward traversal from the focus element to ICU will find the lambda. You can's ask the lambda for its children for example. Such support is not planned now.
Created attachment 240400 [details] Latest patch using another approach
Failing tests have been correct, an NPE has been fixed. I think this addresses the remaining needs of comment#0, Jay, please review. TIA. (This is mostly same as last patch - if you have started review using earlier version, continuing with that is good enough)
(In reply to Srikanth Sankaran from comment #24) > Created attachment 240400 [details] > Latest patch using another approach Some comments: 1. As I said earlier, we should decide how we want to represent the lambda as a Java model element. With the patch, the ILocalVariable gets a SourceMethod as a parent. I haven't yet investigated, but surprised that it's a SourceMethod even though LambdaExpression is a SourceType. This results in following problems: (i) Hovering and clicking on "lambda$2" throws up an exception because the method element doesn't have a corresponding element info. (ii) Java element view has the SourceMethod as the LocalVariable's parent. But several elements inside the SourceMethod element have wrong ranges and errors. 2. There is something wrong with LambdaExpression#ordinal is computed. Consider the following case: public static void main(String[] args) { goo((x) -> { int y = 3; return x * y; }); FunctionalInterface i2 = (y) -> { return y * 3; }; FunctionalInterface j1 = (z) -> { return z * 3; }; } Hovering over both (y) and (z) bring up "..lambda$2(int)". Hovering over any of x, y or Z inside the lambda body always show lambda$1. Continuing to review...
(In reply to Jayaprakash Arthanareeswaran from comment #26) > 1. As I said earlier, we should decide how we want to represent the lambda > as a Java model element. With the patch, the ILocalVariable gets a > SourceMethod as a parent. I haven't yet investigated, but surprised that > it's a SourceMethod even though LambdaExpression is a SourceType. This > results in following problems: It should not be LambdaExpression, the SourceType. It should be LambdaMethod which is not yet written: as planned in https://bugs.eclipse.org/bugs/show_bug.cgi?id=400905#c27, I'll work on it. > (i) Hovering and clicking on "lambda$2" throws up an exception because the > method element doesn't have a corresponding element info. This should get addressed as part of that exercise. > (ii) Java element view has the SourceMethod as the LocalVariable's parent. > But several elements inside the SourceMethod element have wrong ranges and > errors. I need to see how to reproduce this. Basically the plan is not to support getChildren on a LambdaMethod > 2. There is something wrong with LambdaExpression#ordinal is computed. > Consider the following case: > > public static void main(String[] args) { > goo((x) -> { > int y = 3; > return x * y; > }); > FunctionalInterface i2 = (y) -> { > return y * 3; > }; > FunctionalInterface j1 = (z) -> { > return z * 3; > }; > } > > > Hovering over both (y) and (z) bring up "..lambda$2(int)". Hovering over any > of x, y or Z inside the lambda body always show lambda$1. > > Continuing to review... I'll take a look, Thanks.
(In reply to Jayaprakash Arthanareeswaran from comment #26) > Hovering over both (y) and (z) bring up "..lambda$2(int)". Hovering over any > of x, y or Z inside the lambda body always show lambda$1. It could be very confusing for a user - but there is a method behind the madness. The selection parser adds only those nodes that are relevant to the selection process. So some lambdas are missing - shifting the ordinal numbers around from what would be expected. Fix should be simple - get rid of the useless lambda@<no> format and move to the format proposed in comment#22.
See https://bugs.eclipse.org/bugs/show_bug.cgi?id=400905#c19 and https://bugs.eclipse.org/bugs/show_bug.cgi?id=400905#37. That issue needs to be handled on behalf of this ticket.
(In reply to Srikanth Sankaran from comment #29) > See https://bugs.eclipse.org/bugs/show_bug.cgi?id=400905#c19 and > https://bugs.eclipse.org/bugs/show_bug.cgi?id=400905#37. > > That issue needs to be handled on behalf of this ticket. Also https://bugs.eclipse.org/bugs/show_bug.cgi?id=400905#c29
Created attachment 240462 [details] Patch ready for review. Jay, please review. I would like to release it today after a round of high level review. I'll leave the bug open so we can accommodate review comments and follow ups and then close eventually. TIA. Note: Tests are still being remastered to match the new format.
(In reply to Srikanth Sankaran from comment #31) > Created attachment 240462 [details] > Patch ready for review. I played around with the patch in place and things look good. Some comments: 1) When hovering over a lambda parameter and then clicking on "Lambda(FunctionalInterface)" where the view takes us to is not consistent. When LHS is a field, the view changes to the field's doc but in case of local variable, it is the enclosing method. I think we need to be consistent. 2) I don't know if this is a problem, but the LambdaExpression and enclosed LambdaMethod (which is the only child element) have the same source ranges. I am not sure if it's a problem from client's perspective. Markus might know better. Rest all look good to me. I didn't review the code, though.
Implementation and tests released here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=aacce274176303cc524a6360232ca1201922c452. (In reply to Jayaprakash Arthanareeswaran from comment #32) > 1) When hovering over a lambda parameter and then clicking on > "Lambda(FunctionalInterface)" where the view takes us to is not consistent. > When LHS is a field, the view changes to the field's doc but in case of > local variable, it is the enclosing method. I think we need to be > consistent. I found the behavior inconsistent alright - but I couldn't quite figure out what is expected. Please test some more and raise follow up bugs as required. > 2) I don't know if this is a problem, but the LambdaExpression and enclosed > LambdaMethod (which is the only child element) have the same source ranges. > I am not sure if it's a problem from client's perspective. Markus might know > better. That is OK, they are one and the same. They are modelled separately as IType and IMethod for client convenience. > Rest all look good to me. I didn't review the code, though. Please do and let us follow up in fresh CRs. Thanks!
UI team: the work on search, index, hierarchy, model and selection and their interplay with each other should be considered complete and ready for integration. Thanks for early testing.
Verified as working for Eclipse + Java 8 RC2 using Kepler SR2 + Eclipse Java Development Tools Patch for Java 8 Support (BETA) 1.0.0.v20140306-1935. Verification done by code checking as ui bits are not yet available at RC2.
