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

Add support for instanceof pattern matching feature #9917

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vegegoku
Copy link
Contributor

@vegegoku vegegoku commented Feb 6, 2024

Add compiler implementation, tests for Java 16 pattern matching instanceof

JEP : https://openjdk.org/jeps/394
fix #9893
related to #9869

Thanks for your contribution to the GWT Project!

Please make sure your pull request has a short, concise title, and a
clear description that can be used as a commit message for the entire
pull request when it is squashed into a single commit.

The message should end with a reference to the github issue being
addressed in this pull request, e.g. "Fixes #42" to close the linked
issue, or "Partial #42" to indicate that this merge will address it,
but that it isn't ready to be closed yet.

@FrankHossfeld
Copy link
Member

LGTM

@vegegoku vegegoku force-pushed the java-14-instanceof-pattern-matching-support-test branch from 8cf8a3e to aea3f89 Compare February 7, 2024 09:47
@niloc132
Copy link
Contributor

niloc132 commented Feb 7, 2024

After some discussion, we've discovered that this replacement isn't sufficient, consider the case

return hasSideEffects() instanceof Foo foo && foo.isEmpty();

The current impl would rewrite to something like this:

Foo foo;
return hasSideEffects() instanceof Foo && (foo = (Foo) hasSideEffects()) && foo.isEmpty();

which would result in the same method hasSideEffects() being invoked twice, when it is only expected to go off once.

We should have a specific test for this case, and instead consider another pattern, maybe closer to:

Foo foo;
Object old;
return (old = hasSideEffects()) instanceof Foo && (foo = (Foo) old) && foo.isEmpty();

@vegegoku vegegoku force-pushed the java-14-instanceof-pattern-matching-support-test branch from aea3f89 to 8d1c2ff Compare February 8, 2024 09:43
@vegegoku vegegoku force-pushed the java-14-instanceof-pattern-matching-support-test branch 3 times, most recently from d2ad877 to 999c1bf Compare February 11, 2024 12:59
@vegegoku vegegoku force-pushed the java-14-instanceof-pattern-matching-support-test branch from 999c1bf to edebb95 Compare February 19, 2024 10:20
@vegegoku vegegoku force-pushed the java-14-instanceof-pattern-matching-support-test branch from edebb95 to e37cc2f Compare February 20, 2024 13:04
@vegegoku vegegoku force-pushed the java-14-instanceof-pattern-matching-support-test branch from e37cc2f to 1a6cfb6 Compare March 19, 2024 20:00
@vegegoku vegegoku requested a review from niloc132 March 19, 2024 20:07
new JBinaryOperation(info, variableRef.getType(), JBinaryOperator.ASG, variableRef,
jCastOperation);

// null != (foo = (Foo) $instanceof_1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// null != (foo = (Foo) $instanceof_1
// null != (foo = (Foo) $instanceof_1)

Comment on lines +1110 to +1116
// If o is of type X, then
//
// rewrite (o instanceof Foo foo)
// to
// Foo foo;
// X o$instanceof_1;
// (($instanceof_1 = o) instanceof Foo && null != (foo = (Foo) $instanceof_1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make this a bit more clear, let's replace o here (and below) with something like <expr> so it is clear that this isn't just using a variable, but for any method call or other expression. If it was always a variable, we wouldn't need o$instanceof_1, that's the purpose that this extra variable serves.

Something like

Suggested change
// If o is of type X, then
//
// rewrite (o instanceof Foo foo)
// to
// Foo foo;
// X o$instanceof_1;
// (($instanceof_1 = o) instanceof Foo && null != (foo = (Foo) $instanceof_1))
// If <expr> is of type X, then
//
// rewrite (<expr> instanceof Foo foo)
// to
// Foo foo;
// X o$instanceof_1;
// (($instanceof_1 = <expr>) instanceof Foo && null != (foo = (Foo) $instanceof_1))

curMethod.instanceOfDeclarations.put(patternDeclarationName, jDeclarationStatement);
}

// X o$instanceof_1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes you use the o prefix, others you don't, lets remove it?

Suggested change
// X o$instanceof_1;
// X $instanceof_1;

if (shape1 instanceof Square square && square.getLength() > 0) {
a = true;
}
if (shape2 instanceof Square square && square.getLength() > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test - but what if the two variables with the same name have different types, or is that covered and I missed it? This looks like it could get bitten by the "flow scope" that the JEP discussed.

I'm reasonably confident that this is already correct form looking at some of the AST test output, but I don't actually see a test that tries these cases.

}
assertEquals("Switch expressions not yet supported", e.getMessage());
}
public void testInstanceOfPatternMatching() throws UnableToCompleteException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more set of tests I can think of that might break with the X $instanceof_1 decl: a var type, for something that can't be expressed easily, like an anonymous inner class, or a type intersection (like was fixed by ffe6183) since that was also using typeMap.get(...).

var obj = new SomeClass() {
  public String addAnotherField = "hello";
};
if (obj instanceof SomeClass a) {
  //...
}

This could really only matter if there was some other logic going on that would let the type be something else entirely, I'm not thinking about "how this makes sense", but "how this might break the new declaration".

var value = true ? "a" : 'c';
if (obj instanceof CharSequence c) {
  //...
}

This one sort of makes more sense, but still is mostly about "hmm, that code looks like something else that has been able to break before".

I'm not sure if this test belongs here or in the Java17Test, up to you.

@niloc132 niloc132 added this to the 2.12 milestone May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add compiler implementation, tests for Java 16 pattern matching instanceof Change the text of ListBox item
3 participants