NPE: Cannot invoke org.eclipse.jdt.internal.compiler.lookup.BlockScope.getBinding() because scope is null" on hyperlink request

[josediazfer]

…e.getBinding() because scope is null" on hyperlink request (#4217)

• Fixes "NPE: Cannot invoke org.eclipse.jdt.internal.compiler.lookup.BlockScope.getBinding(char[], int, org.eclipse.jdt.internal.compiler.lookup.InvocationSite, boolean) because scope is null" on hyperlink request #4217

What it does

How to test

Author checklist

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

[srikanth-sankaran]

Have you found out why scope is null there ? Slightly wary of null checks as solution unless fuller analysis justifies it.

[josediazfer]

@srikanth-sankaran

A smaller example of the problem:

public void test() {
	ArrayList<String> testArray = new ArrayList<>();
	
	testArray.testMethod(e -> {
		int valueVar = 1;
	});
}

The problem occurs when the "valueVar" variable is selected within the lambda passed as a parameter to the "testMethod" method.The "testMethod" method not be resolved because it does not exist in the ArrayList class.

The scope is null because it hasn't been created in the resolveType method of the LambdaExpression class. Specifically, it is created when calling resolveType of the LambdaExpression class from the resolvePolyExpressionArguments0 method of the ASTNode class but since the method "testMethod" does not exist ("candidateMethod" is null), the resolveType of LambaExpression is not called from the resolvePolyExpressionArguments0 method.

I put a piece of source code of ASTNode.resolvePolyExpressionArguments0:

	private static MethodBinding resolvePolyExpressionArguments0(Invocation invocation, MethodBinding method, TypeBinding[] argumentTypes, BlockScope scope) {
		MethodBinding candidateMethod = method.isValidBinding() ? method : method instanceof ProblemMethodBinding ? ((ProblemMethodBinding) method).closestMatch : null;
		if (candidateMethod == null)
			return method;
			.....
		if (argumentTypes[i] != null && argumentTypes[i].isPolyType()) {
			argument.setExpectedType(parameterType);
			TypeBinding updatedArgumentType;
			if (argument instanceof LambdaExpression) {
				LambdaExpression lambda = (LambdaExpression) argument;
				// avoid complaining about non-kosher descriptor as secondary problem
				boolean skipKosherCheck = method.problemId() == ProblemReasons.Ambiguous;
				updatedArgumentType = lambda.resolveType(scope, skipKosherCheck);

If it were an existing method, for example, the forEach method of testArray, lamba.resolveType() would be called from resolvePolyExpressionArguments0.
Then, the lamba scoped method would be created within the resolveType method of the LambaExpression class, and therefore the type of the "valueVar" variable would be resolved, generating the SelectionNodeFound exception with its type.

In the above example, the node tree associated with the "test" method will be traversed, and when the selected node "valueVar" is visited, an attempt will be made to resolve its type, but since the scope of the lambda is not initialized, it will fail with a NullPointerException

[stephan-herrmann]

... generating the SelectionNodeFound exception ...

should we assume that the problem occurs only during selection, not during regular compilation?

[josediazfer]

@stephan-herrmann Yes, exactly when you hover over the variable name, the problem does not appear during compilation.

[stephan-herrmann]

The problem vaguely reminds me of 991c1db

The general topic would be: completion and selection similarly abort compilation by throwing {Completion,Selection}NodeFound. If this exception will leave AST & bindings in an incomplete / inconsistent state, it may be better to stash the exception, continue regular compilation, and only pull out the exception when all is properly resolved.

Just mentioning.

[josediazfer]

@stephan-herrmann

I fully agree with your proposal to unify the same functionality for both selection and completion, but I don't think it's useful for this specific case. Both CompletionEngine and SelectionEngine don't generate any type of (Selection/Completion)NodeFound exception after calling resolve(). The difference between SelectionEngine and Completion is that after calling resolve() in SelectionEngine, the AST is traversed, causing a NullPointerException. Another possible fix, but more dangerous, would be to remove the following lines:

							if (node != null) {
								selectLocalDeclaration(node);
							}

I'm not sure if it still makes sense to traverse the AST if a SelectionNodeFound exception wasn't previously generated. In CompletionEngine, nothing similar is done after calling resolve().
What do you think is better?