Resolve MethodBinding$LambdaMethod to LambdaMethod java element

[mickaelistria]

Fixes #2047

What it does

Fixes the resolution of Binding->JavaElement for lambda methods: now returns a LambdaMethod Java Element as expected by eg codeSelect in test0024 instead of returning the binding of the overridden method.

How to test

Run new test with or without fix.

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[mickaelistria]

Note that while it improves things, the case of eg test0027 is not resolved by that case.

[mickaelistria]

I have looked at the case of test0027, and it looks to me that for this case, the issue is not really the Binding -> JavaElement resolution (which may be fixed with this patch), but more that the DefaultBindingResolver only inserts 1 of the LambdaMethod/LambdaExpression couple in the bindingToASTNodes map instead of the 3 ones existing in the code; line 941 this.bindingsToAstNodes.put(methodBinding, lambda); is only called once.
I'll keep investigating, but I would suggest that this patch would be considered for a review/merge as it, and we'll create new issues/PRs for the "other" symptom of the DefaultBindingResolver.

[mickaelistria]

I could fix my patch to also address the concern of former comment.

[mickaelistria]

@jjohnstn @rgrunber this one is ready for review. Can you please have a look at it?

[akurtakov]

@srikanth-sankaran Can you help with reviewing this one?

[jjohnstn]

I am wondering if the test should be in ASTConverter18Tests or perhaps an additional test. There you can call JavaElement.toStringWithAncestors(false) to verify the hierarchy.

[jjohnstn]

Does this need to be done every time or can it be done once and then marked as completed or can the LambdaFactory LambdaMethod be cached to avoid this in the future?

[mickaelistria]

Usage of this method is relatively sporadic (happens only when user Ctrl+clicks it at the moment) and execution remains relatively fast. On the other hand, the DOM may be retained for a long time, so adding some cache here increase memory consumption.
Overall, I think it's best to not cache and keep more memory free for that case. We can easily revisit later if my impressions are proven wrong though.

[srikanth-sankaran]

This looks very close/similar to https://bugs.eclipse.org/bugs/show_bug.cgi?id=573834 - Need to check why that solution is not covering this case. There also code selection is involved

[mickaelistria]

Need to check why that solution is not covering this case. There also code selection is involved

The previous patch doesn't involve the DOM nor its bindings. It fixes the SelectionEngine and the SelectionParser , which are not involved when building a DOM.

[srikanth-sankaran]

Need to check why that solution is not covering this case. There also code selection is involved

The previous patch doesn't involve the DOM nor its bindings. It fixes the SelectionEngine and the SelectionParser , which are not involved when building a DOM.

Correct, I realized that as started reviewing.

[srikanth-sankaran]

At every level lambda ordinal is 1 - is that right ??

[mickaelistria]

It's a similar assertion that's implemented in test0027, with 1 being the ordinal on every level (unlike anonymous which have an ordinal based on the including type because it's used to generate the .class, lambdas do not generate a .class and thus ordinal seems only relative to the direct parent)

[srikanth-sankaran]

There appears to be a serious pre-existing defect in org.eclipse.jdt.core.dom.DefaultBindingResolver.getBindingsToNodesMap() - can we discuss that and understand if that somehow impacts the current behavior ?

What I am wondering is:

org.eclipse.jdt.core.dom.DefaultBindingResolver.getBindingsToNodesMap().new BindingsToNodesMap() {...}.get(Binding) takes a compiler binding as argument and does this with it:

return (org.eclipse.jdt.internal.compiler.ast.ASTNode)
					DefaultBindingResolver.this.newAstToOldAst.get(DefaultBindingResolver.this.bindingsToAstNodes.get(dBinding));

Now DefaultBindingResolver.this.bindingsToAstNodes maps from DOM bindings to DOM AST nodes. Using compiler bindings as key there is guaranteed to return null in this call stack:

Util.getUnresolvedJavaElement(MethodBinding, WorkingCopyOwner, Util$BindingsToNodesMap) line: 1413	
MethodBinding$LambdaMethod(MethodBinding).getUnresolvedJavaElement() line: 318	
MethodBinding$LambdaMethod.getUnresolvedJavaElement() line: 612	
MethodBinding$LambdaMethod(MethodBinding).getJavaElement() line: 304	
VariableBinding.getUnresolvedJavaElement() line: 292	
VariableBinding.getJavaElement() line: 160	
ResolveTests18.test0027_BindingForLambdaMethod() line: 3169	

(call stack while debugging your new test)

I am wondering if the fix should really belong in the then clause of if (node != null && !declaringType.isBinary()) { after the above bug is corrected.

WDYT ?

[srikanth-sankaran]

OK, I am closing shop for the week, I'll continue to look at this early next week. Just fixing the above bug doesn't change much, but I would still like to discuss what is the impact of such glaringly defective code:

If I change it to:

	Util.BindingsToNodesMap getBindingsToNodesMap() {
		return new Util.BindingsToNodesMap() {
			@Override
			public org.eclipse.jdt.internal.compiler.ast.ASTNode get(Binding binding) {
				IMethodBinding dBinding = (IMethodBinding) DefaultBindingResolver.this.bindingTables.compilerBindingsToASTBindings.get(binding);
				return (org.eclipse.jdt.internal.compiler.ast.ASTNode)
					DefaultBindingResolver.this.newAstToOldAst.get(DefaultBindingResolver.this.bindingsToAstNodes.get(dBinding));
			}
		};
	}

it starts returning the mapping properly although in the context of this bug, it doesn't change much. Still I wonder if the fix should perhaps belong in the then clause.

Will study more early next week

[srikanth-sankaran]

Amazingly that code dates back 15 years. Someone please tell me I misunderstood it! 😆

[mickaelistria]

I will try to investigate further later today or early next week. But 1 particular issue I've found is that the "parent" lambda bindings are not included in the maps unless we do explicitly call resolveBinding on related ASTNode. This seems to me a bit unusual compared to other types where the bindings seem to exist in the map.
It indeed looks like we might be facing a combination of bugs for which fixes are not so related.

Note that the bug about the requested type in the map was made visible with #1617 (comment) some time ago.

[srikanth-sankaran]

@stephan-herrmann Take a bow! 😊

This is the third whopper of a bug I find in the past 6 weeks.

For the last week's entry see the most recent change to org.eclipse.jdt.internal.compiler.codegen.CodeStream.invoke(byte, MethodBinding, TypeBinding, TypeReference[])

But usually these nasty bugs will manifest in corner case situations. This one seems very broken. How do things work ?? 😊

[srikanth-sankaran]

Other than the pre-existing issue point out already, this looks ok. I haven't worked in these parts in almost 10 years - I must state that too :)

[akurtakov]

As the patch is approved and covered with test - merging.