Detect instanceof pattern variables in scope

[timtebeek]

What's your motivation?

Used in InstanceOfPatternMatch to detect variables that exist in the surrounding context, which now fails to pick up pattern matching for instance of variables, and as such introduces two identically named variables.

• InstanceOfPatternMatch: handle same type for multiple operands rewrite-static-analysis#265
• Duplicate variable name in if statement with two different instanceof rewrite-static-analysis#175

[knutwannheden]

This is probably a hood start and we could go ahead and integrate this, if it already fixes some issues. To be on the safe side, the pattern variable should also be added to the scope of the containing block, as it can in fact also be visible there, depending on what the if and else statements contain.

[timtebeek]

@knutwannheden is this what you meant by adding it to the parent scope?

Suggested change

	public J.InstanceOf visitInstanceOf(J.InstanceOf instanceOf, Set<String> strings) {
	if (instanceOf.getPattern() instanceof J.Identifier) {
	Set<String> names = nameScopes.get(currentScope.peek());
	if (names != null) {
	names.add(((J.Identifier)instanceOf.getPattern()).getSimpleName());
	}
	}
	return super.visitInstanceOf(instanceOf, strings);
	public J.InstanceOf visitInstanceOf(J.InstanceOf instanceOf, Set<String> namesInScope) {
	if (instanceOf.getPattern() instanceof J.Identifier) {
	Set<String> names = nameScopes.get(currentScope.peek());
	if (names != null) {
	String simpleName = ((J.Identifier) instanceOf.getPattern()).getSimpleName();
	names.add(simpleName);
	namesInScope.add(simpleName);
	}
	}
	return super.visitInstanceOf(instanceOf, namesInScope);

[knutwannheden]

Not exactly sure what the effect would be of this change. But For example the following code must not end up creating two pattern variables with the same name, as that would give a compile error:

        Object o = "";
        if (!(o instanceof String)) {
            throw new IllegalArgumentException(((String) o));
        }
        if (o instanceof String) {
            System.out.println((String) o);
        }

[knutwannheden]

The given example is very contrived, as it would result in a runtime exception. But I wanted to get the point across regarding the compiler error. I could otherwise also come up with more sensible examples.

So, to be on the safe side, a pattern variable as in the following code should be regarded as belonging to the surrounding block (not just the if statement's then block):

        Object o = "";
        if (o instanceof String s) {
            // ...
        }

That way the VariableNameUtils should not propose s as a variable name in any statements following the if statement.

[timtebeek]

Looking this over again and how it's used in InstanceOfPattern match I think right now we don't need the above suggested fix, given how we aggregate the scope names in postVisit

rewrite/rewrite-java/src/main/java/org/openrewrite/java/VariableNameUtils.java

Lines 193 to 213 in 0f0dc80

	public @Nullable J postVisit(J tree, Set<String> namesInScope) {
	if (!currentScope.isEmpty() && currentScope.peek().getValue().equals(tree)) {
	currentScope.pop();
	}
	if (scope.getValue().equals(tree)) {
	Cursor aggregatedScope = getCursor().getValue() instanceof JavaSourceFile ? getCursor() : aggregateNameScope();
	// Add names from parent scope.
	Set<String> names = nameScopes.get(aggregatedScope);
	// Add the names created in the target scope.
	namesInScope.addAll(names);
	nameScopes.forEach((key, value) -> {
	if (key.isScopeInPath(scope.getValue())) {
	namesInScope.addAll(value);
	}
	});
	return tree;
	}
	return super.postVisit(tree, namesInScope);

I'll try to pick this up in rewrite-static-analysis.