Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Patterns] Alternate, simpler pattern binding management implementation #1884

Closed
srikanth-sankaran opened this issue Jan 18, 2024 · 0 comments · Fixed by #1885
Closed

[Patterns] Alternate, simpler pattern binding management implementation #1884

srikanth-sankaran opened this issue Jan 18, 2024 · 0 comments · Fixed by #1885
Assignees

Comments

@srikanth-sankaran
Copy link
Contributor

srikanth-sankaran commented Jan 18, 2024

The original design of pattern resolution and pattern binding management in JDT takes the approach of "attaching" with every expression node (i.e., with every ASTNode <: Expression) two set of bindings variables - patternVarsWhenTrue and patternVarsWhenFalse - These are computed by calls to collectPatternVariablesToScope at various places.

The fundamental problem with this approach is that the method collectPatternVariablesToScope creates a twisted/contrived tree traversal. Some nodes are traversed multiple times, sometimes in out of order fashion.

For example, take how if statements are handled:

@Override
public void resolve(BlockScope scope) {
	this.condition.collectPatternVariablesToScope(null, scope);
	LocalVariableBinding[] patternVariablesInTrueScope = this.condition.getPatternVariablesWhenTrue();
	LocalVariableBinding[] patternVariablesInFalseScope = this.condition.getPatternVariablesWhenFalse();
	TypeBinding type = this.condition.resolveTypeExpecting(scope, TypeBinding.BOOLEAN);
	this.condition.computeConversion(scope, type, type);

	if (this.thenStatement != null) {
		this.thenStatement.resolveWithPatternVariablesInScope(patternVariablesInTrueScope, scope);
	}
	if (this.elseStatement != null) {
		this.elseStatement.resolveWithPatternVariablesInScope(patternVariablesInFalseScope, scope);
	}
}

The call to this.condition.collectPatternVariablesToScope(null, scope); results in our descending into the condition node, resolving it and computing the binding variables, But the preexisting call to this.condition.resolveTypeExpecting(scope, TypeBinding.BOOLEAN); already visits the condition node and will resolve it. There is strictly speaking no need to create additional visits to some sub trees in this fashion.

These additional visits are sometimes problematic. See the comment and code in BinaryExpression#resolveType(BlockScope scope) (deleted in this PR) that guards against side effects of multiple traversals:

	if(this.patternVarsWhenFalse == null && this.patternVarsWhenTrue == null &&
			this.containsPatternVariable()) {
		// the null check is to guard against a second round of collection.
		// This usually doesn't happen,
		// except when we call collectPatternVariablesToScope() from here
		this.collectPatternVariablesToScope(null, scope);
	}

We can avoid all these complexities and maintain the natural order of traversal and compute bindings on the fly.

So this PR attempts to flip the approach. Instead of the present "compute and attach to expression nodes, the bindings set patternVarsWhenTrue and patternVarsWhenFalse" we simply need two APIs:

public LocalVariableBinding[] getPatternVariablesWhenTrue() (to be renamed variablesWhenTrue())
public LocalVariableBinding[] getPatternVariablesWhenFalse() (to be renamed variablesWhenFalse()) and

These APIs exist already on master - they return the bindings precomputed by the collectPatternVariablesToScope operation. We now delete this operation altogether and make the APIs compute them on the fly.

Very few nodes really need to implement these APIs (InstanceOfExpression, AND_AND_Expression, OR_OR_Expression, UnaryExpression and ConditionalExpression to be precise)

@srikanth-sankaran srikanth-sankaran self-assigned this Jan 18, 2024
@srikanth-sankaran srikanth-sankaran changed the title [Patterns] Alternate simpler pattern binding management implementation [Patterns] Alternate, simpler pattern binding management implementation Jan 23, 2024
srikanth-sankaran added a commit that referenced this issue Jan 23, 2024
…on (#1885)

* Reimplements pattern resolution and pattern binding management in a much simpler design.

* Fixes [Patterns] Alternate, simpler pattern binding management implementation #1884
* Fixes [Patterns] ECJ tolerates erroneous redeclaration of pattern bindings in some cases.  #1887
robstryker pushed a commit to robstryker/eclipse.jdt.core that referenced this issue Jul 18, 2024
…on (eclipse-jdt#1885)

* Reimplements pattern resolution and pattern binding management in a much simpler design.

* Fixes [Patterns] Alternate, simpler pattern binding management implementation eclipse-jdt#1884
* Fixes [Patterns] ECJ tolerates erroneous redeclaration of pattern bindings in some cases.  eclipse-jdt#1887
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant