Out of scope pattern variables suggested for completion

[datho7561]

eg. open completion at | and you will get jello despite the fact that it's out of scope

import java.util.Collections;
import java.util.List;
import java.util.Set;

public class HelloWorld {
	private static record MyRecord(String a, int b) {
	}
	public static void main(String... args) {
		List myList = List.of(new MyRecord("hello", 12), new Object());		
		if (1 < 2 && myList.get(0) instanceof MyRecord(String jello, int kilometer)) {
		}
		
		String asdf = jel|;
	}
}

same with the following case:

import java.util.Collections;
import java.util.List;
import java.util.Set;

public class HelloWorld {
	private static record MyRecord(String a, int b) {
	}
	public static void main(String... args) {
		List myList = List.of(new MyRecord("hello", 12), new Object());		
		for (int i = 0; i < myList.size() && myList.get(i) instanceof MyRecord(String jello, int kilometer); i++) {
		}
		
		String asdf = jel|;
	}
}

[datho7561]

I spent a lot of time today debugging this. Some pointers if someone wants to take a crack at it (I probably won't):

1. This needs to go:

   eclipse.jdt.core/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ForStatement.java

   Line 434 in 8050026

   this.scope.reparentLocals(true);

   . The pattern variables should NOT outlive the for loop body. Adding them to the parent scope also allows them to be suggested before the for statement.
   1.a This "fix" causes problems though. The pattern variable is not suggested in the for loop body. Not sure why. Perhaps it needs to be fully added to the for loop's scope rather than treated as a parameter? The bug also exists for the block attached to an if statement.

2. After addressing (1), the following line in the CompletionEngine will still collect jello:

   eclipse.jdt.core/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/CompletionEngine.java

   Line 3872 in 8050026

   findUnresolvedReference(

   
   2.a This is because UnresolvedReferenceNameFinder doesn't have visit( implementations for if, for, while etc.
   2.b Even if you add them, that code will never be hit. The AST being visited is generated here: https://github.com/eclipse-jdt/eclipse.jdt.core/blob/master/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/UnresolvedReferenceNameFinder.java#L206 . Its contents look like this:

{
  List myList;
  int i;
  {
  }
  String asdf;
}

As a result, no if/for will be visited. The visitor is used to remove invalid identifiers instead of populating a list of identifiers. So, the declarations from the record pattern get suggested.

[srikanth-sankaran]

The pattern variables should NOT outlive the for loop body. Adding them to the parent scope also allows them to be suggested before the for statement.

Do you have a test case that shows a problem in compiler proper with pattern variables outliving the for loop's body incorrectly ?? Or being accessible before the for statement?

There are cases where the pattern variables MUST outlive. Hence their addition to the parent scope.

It could be that this is a completion specific defect.

Please mark me for review on any changes made to core compiler for this. Thanks!

[srikanth-sankaran]

1. The pattern variables should NOT outlive the for loop body. Adding them to the parent scope also allows them to be suggested before the for statement.

Here is an example that must compile that shows the pattern variable introduced by the for's condition must outlive the for loop:

public class X {
	
	public static void main(String[] args) {
		Object o = new X();
		for (;!(o instanceof String str); ) {
			throw new RuntimeException();
		}
		System.out.println(str);
	}
}

Can you browse through the uses of org.eclipse.jdt.internal.compiler.lookup.ExtraCompilerModifiers.AccOutOfFlowScope ? The problem may be that this bit is not consulted before proposing a choice. A binding variable may exist in a scope without being actually in scope

[datho7561]

There are cases where the pattern variables MUST outlive. Hence their addition to the parent scope.

Okay that makes sense to me. However, in the case I presented, the pattern variables should not outlive the for loop, yet they are being added to the method scope anyways.

It could be that this is a completion specific defect.

It is completion specific. The compiler outputs errors for trying to use the pattern variables when they are invalid, and allows them to be used after the loop in the snippet you mentioned. The changes I tried making to the scope are probably wrong.

Can you browse through the uses of org.eclipse.jdt.internal.compiler.lookup.ExtraCompilerModifiers.AccOutOfFlowScope ?

This flag is not checked anywhere in the completion engine.

A binding variable may exist in a scope without being actually in scope

This is pretty confusing. Why is it done this way instead of adding the variable to the BlockScope or MethodScope it belongs to? I'll try adding some code to the completion engine to take this into account.

[srikanth-sankaran]

This is pretty confusing. Why is it done this way instead of adding the variable to the BlockScope or MethodScope it belongs to? I'll try adding some code to the completion engine to take this into account

Well, that is Java for you.

The variable is added to the BlockScope or MeehodScope it belongs to but that doesn't mean it is in scope - ie can be accessed.

Pattern matching goes hand in hand with "flow scoping". A pattern binding variable is accessible only in regions of code where it is definitely assigned and not from point of declaration to the closing } that follows.

So we flip the said flag at the right places to signal a pattern variable is out of scope.

[datho7561]

I tried using the flag to filter out the pattern variables, but I couldn't figure it out. It seemed to me like the flag was always set. My guess why this is happening is because, in the CompletionEngine, we get the resolved AST, and when the AST has finished resolving, all the pattern variables are marked out of the flow scope. Does this make sense?

I looked into improving recovery for for statements into order to address 2 (the undeclared identifier completion feature). I copied the hacks used for ForeachStatement in AssistParser, and I was able to get a better recovered AST from CompletionParser when completion was triggered. However, these improvements didn't seem to affect CompletionParser.recoverSomeStatements at all, the recovery there was still awful. As a result, I wasn't able to filter out the pattern variables from the list of "undeclared" identifiers.

[srikanth-sankaran]

There are many problems in completion with respect to various constructs introduced in the last few years. A comprehensive overhaul is what is really called for - but allocating time may be a challenge. I have been busy overhauling core compiler implementation of several JEPs - this is nearly done with only Record classes in the queue for review/rewrite. After that hopefully I can look into this.