Fix RecordPattern generation for standalone instanceof #2016

datho7561 · 2024-02-13T22:16:17Z

Supercedes #1816

Signed-off-by: David Thompson [email protected]

What it does

Fixes #1804

How to test

Try running the snippets in the linked issue.

Author checklist

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

datho7561 · 2024-02-13T22:18:08Z

@srikanth-sankaran here is the updated fix for the bug

srikanth-sankaran · 2024-02-14T00:38:37Z

Thanks David. These changes will be useful - I am still deciding whether this needs to go into 4.31 or be a part of the dozen or more cumulative fixes I expected to accumulate under the umbrella #2011 and be merged in when master reopens for 4.32 M1 development (first week of March 2024)

The changes look in the right direction, but I am not done cleaning up the current implementation! In particular reference to the region you are touching this is my plan:

See if meat of org.eclipse.jdt.internal.compiler.ast.InstanceOfExpression.generateCode(BlockScope, CodeStream, boolean) and org.eclipse.jdt.internal.compiler.ast.InstanceOfExpression.generateOptimizedBoolean(BlockScope, CodeStream, BranchLabel, BranchLabel, boolean) can be extracted into a single method and called from both the places or alternately, see if one can call the other.
Remove this code from org.eclipse.jdt.internal.compiler.ast.InstanceOfExpression.generateOptimizedBoolean

if (this.elementVariable == null && this.pattern == null) {
		super.generateOptimizedBoolean(currentScope, codeStream, trueLabel, falseLabel, valueRequired);
		return;
}

and stop the buck right there.
3. Make the code more OO: (#1986)
This fragment for example in org.eclipse.jdt.internal.compiler.ast.InstanceOfExpression.generateOptimizedBoolean is an anti-pattern:

	if (this.pattern instanceof RecordPattern) {
		this.pattern.generateOptimizedBoolean(currentScope, codeStream, trueLabel, nextSibling);
	} else {
		codeStream.checkcast(this.type, this.type.resolvedType, codeStream.position);
		codeStream.store(this.elementVariable.binding, false);
	}

this should simply be:

this.pattern.generateOptimizedBoolean(currentScope, codeStream, trueLabel, nextSibling);

Get rid of the calls to addPatternVariables - that is due to wrong design (see [Patterns] [Whitebox] Redundant calls to org.eclipse.jdt.internal.compiler.codegen.CodeStream.addVisibleLocalVariable(LocalVariableBinding) #1889)

Your changes will be useful, they just need to be adjusted for the other changes, I'll work on it.

srikanth-sankaran · 2024-02-14T00:46:03Z

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

+
+		for (LocalVariableBinding patternVariableBinding : this.pattern.bindingsWhenTrue()) {
+			codeStream.removeVariable(patternVariableBinding);
+		}


This is wrong placement of code. You are perhaps following the problematic coding pattern prevalent in the code base that I am trying to weed out. In pattern matching implementation, variables are added and removed without much rhyme or reason. In this place, instanceof is true and the pattern binding is live - it should not be removed. The flow analysis in general should determine when it becomes not definitely assigned and close the ranges with a call to record to org.eclipse.jdt.internal.compiler.lookup.LocalVariableBinding.recordInitializationEndPC(int)

Note however the call to codeStream.removeVariable(this.secretExpressionValue); just above in line 127 is correct. We are done with the use of secret variable and it can be done away with. It being a compiler injected variable, its scope and live range is entirely upto us. We want to keep it alive for the minimum range required.

But pattern bindings are programmer defined variables and their scope and live ranges (pc intervals where are they are definitely assigned per flow scope) are spelled out by JLS. analyzeCode() phase computes the definite assignment ranges for locals.

srikanth-sankaran · 2024-02-14T01:05:09Z

BTW, you may find this aggregate junit org.eclipse.jdt.core.tests.RunVariousPatternsTests useful - I use it as a smoke test before using the Jenkins automation

srikanth-sankaran · 2024-02-14T01:13:52Z

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

+		codeStream.iconst_1();
+		codeStream.goto_(continueLabel);
+		falseLabel.place();
+		codeStream.iconst_0();


That for loop from lines 133-135 should actually be placed here just above the iconst_0 - in this place, the instanceof check is false and any pattern bindings injected by the instanceof true path cannot be live

Not the for loop as is - but a modified version should be placed here.

The variable should not be removed. Its definite assignment status must be adjusted and the live range closed.

Variable removable for user scoped variables should happen via org.eclipse.jdt.internal.compiler.codegen.CodeStream.exitUserScope(BlockScope, Predicate<LocalVariableBinding>) which will happen already at the right places

srikanth-sankaran · 2024-02-14T01:31:53Z

OK, I have made the change for live range adjustment and also added regression tests for #1985 - which passes with this PR - and pushed. Let us see if we can get this included for M3, failing that for RC1.

However there is some infra issue with Jenkins it seems, let us see.

srikanth-sankaran · 2024-02-14T01:43:52Z

@mpalat @jarthana @iloveeclipse - I would like this included for 4.31 RC1, if not M3 which may be a bit late I agree

srikanth-sankaran

Changes look good to me. However since I also contributed, I'll ask Manoj for an additional review

srikanth-sankaran · 2024-02-14T05:16:22Z

Changes look good to me. However since I also contributed, I'll ask Manoj for an additional review

While the changes are correct per my review and testing, there is room for further cleanup and improvement. The original code prior to this PR itself has suffered from copy + paste and so has some strange behavior which this PR persists with:

It is odd to populate the operand stack with true or false with iconst_1/iconst_0 and then pop it if (!valueRequired) 😆
I think the call to codeStream.generateImplicitConversion(this.implicitConversion); towards the end is bogus. Already the operand stack has a boolean and we are supposed to produce a boolean - so what conversion is possible/required there ? See that the version of InstanceOfExpression.java prior to the start of pattern matching work (commit f679249) has all of this for method body:

@Override
public void generateCode(BlockScope currentScope, CodeStream codeStream, boolean valueRequired) {
	int pc = codeStream.position;
	this.expression.generateCode(currentScope, codeStream, true);
	codeStream.instance_of(this.type, this.type.resolvedType);
	if (valueRequired) {
		codeStream.generateImplicitConversion(this.implicitConversion);
	} else {
		codeStream.pop();
	}
	codeStream.recordPositionsFrom(pc, this.sourceStart);
}

I'll clean up this up later along with all the tasks mentioned in #2016 (comment)

Fixes eclipse-jdt#1804 Supercedes eclipse-jdt#1816 Signed-off-by: David Thompson <[email protected]>

- Add regression test for eclipse-jdt#1985

mpalat

+1
Am fine with this

mpalat · 2024-02-14T09:46:09Z

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

+		this.pattern.generateOptimizedBoolean(currentScope, codeStream, trueLabel, falseLabel);
+
+		trueLabel.place();
+		codeStream.iconst_1();


codeStream.dup() in the original code should have been kept instead of const_1() and const_0() [later]. However, not a critical change.

codeStream.dup() in the original code should have been kept instead of const_1() and const_0() [later]. However, not a critical change.

I will follow up on this along with various other cleanups I have called out in #2016 (comment)

mpalat · 2024-02-14T09:51:47Z

@mpalat @jarthana @iloveeclipse - I would like this included for 4.31 RC1, if not M3 which may be a bit late I agree

+1 for M3 -
(a) changes look fine
(b) in case there are issues, we can address in RC1

srikanth-sankaran · 2024-02-14T09:55:33Z

Thanks @mpalat and @datho7561

) * Fix RecordPattern generation for standalone instanceof * Fixes eclipse-jdt#1804 * Fixes eclipse-jdt#1985 Signed-off-by: David Thompson <[email protected]> Co-authored-by: Srikanth Sankaran <[email protected]>

datho7561 mentioned this pull request Feb 13, 2024

Parse multiple Patterns in a case statement eclipse-jdtls/eclipse-jdt-core-incubator#9

Closed

srikanth-sankaran requested changes Feb 14, 2024

View reviewed changes

srikanth-sankaran linked an issue Feb 14, 2024 that may be closed by this pull request

[Patterns][records] ECJ fails to generate code to deconstruct record in pattern #1985

Closed

srikanth-sankaran reviewed Feb 14, 2024

View reviewed changes

srikanth-sankaran assigned datho7561 and srikanth-sankaran Feb 14, 2024

srikanth-sankaran requested a review from mpalat February 14, 2024 01:37

srikanth-sankaran approved these changes Feb 14, 2024

View reviewed changes

datho7561 and others added 3 commits February 14, 2024 10:59

Fix RecordPattern generation for standalone instanceof

63cdf2e

Fixes eclipse-jdt#1804 Supercedes eclipse-jdt#1816 Signed-off-by: David Thompson <[email protected]>

- Correct live ranges computation

57f15cb

- Add regression test for eclipse-jdt#1985

- Just close live range allowing variable to stay visible

af2b845

srikanth-sankaran force-pushed the 1804-new-record-pattern-fix branch from 6004a86 to af2b845 Compare February 14, 2024 05:30

mpalat approved these changes Feb 14, 2024

View reviewed changes

srikanth-sankaran merged commit d0c6a4d into eclipse-jdt:master Feb 14, 2024
9 checks passed

This was referenced Feb 15, 2024

[Patterns][Unnamed] VerifyError with unnamed pattern variable in instanceof #2024

Closed

VerifyError with unnamed pattern variable in instanceof #2025

Merged

datho7561 deleted the 1804-new-record-pattern-fix branch January 2, 2025 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RecordPattern generation for standalone instanceof #2016

Fix RecordPattern generation for standalone instanceof #2016

datho7561 commented Feb 13, 2024

datho7561 commented Feb 13, 2024

srikanth-sankaran commented Feb 14, 2024 •

edited

Loading

srikanth-sankaran Feb 14, 2024 •

edited

Loading

srikanth-sankaran Feb 14, 2024

srikanth-sankaran commented Feb 14, 2024 •

edited

Loading

srikanth-sankaran Feb 14, 2024

srikanth-sankaran Feb 14, 2024

srikanth-sankaran commented Feb 14, 2024

srikanth-sankaran commented Feb 14, 2024

srikanth-sankaran left a comment

srikanth-sankaran commented Feb 14, 2024

mpalat left a comment

mpalat Feb 14, 2024

srikanth-sankaran Feb 14, 2024

mpalat commented Feb 14, 2024

srikanth-sankaran commented Feb 14, 2024

Fix RecordPattern generation for standalone instanceof #2016

Fix RecordPattern generation for standalone instanceof #2016

Conversation

datho7561 commented Feb 13, 2024

What it does

How to test

Author checklist

datho7561 commented Feb 13, 2024

srikanth-sankaran commented Feb 14, 2024 • edited Loading

srikanth-sankaran Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

srikanth-sankaran Feb 14, 2024

Choose a reason for hiding this comment

srikanth-sankaran commented Feb 14, 2024 • edited Loading

srikanth-sankaran Feb 14, 2024

Choose a reason for hiding this comment

srikanth-sankaran Feb 14, 2024

Choose a reason for hiding this comment

srikanth-sankaran commented Feb 14, 2024

srikanth-sankaran commented Feb 14, 2024

srikanth-sankaran left a comment

Choose a reason for hiding this comment

srikanth-sankaran commented Feb 14, 2024

mpalat left a comment

Choose a reason for hiding this comment

mpalat Feb 14, 2024

Choose a reason for hiding this comment

srikanth-sankaran Feb 14, 2024

Choose a reason for hiding this comment

mpalat commented Feb 14, 2024

srikanth-sankaran commented Feb 14, 2024

srikanth-sankaran commented Feb 14, 2024 •

edited

Loading

srikanth-sankaran Feb 14, 2024 •

edited

Loading

srikanth-sankaran commented Feb 14, 2024 •

edited

Loading