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
base: main
Are you sure you want to change the base?
Add support for instanceof pattern matching feature #9917
Conversation
LGTM |
8cf8a3e
to
aea3f89
Compare
After some discussion, we've discovered that this replacement isn't sufficient, consider the case
The current impl would rewrite to something like this:
which would result in the same method We should have a specific test for this case, and instead consider another pattern, maybe closer to:
|
aea3f89
to
8d1c2ff
Compare
d2ad877
to
999c1bf
Compare
user/test-super/com/google/gwt/dev/jjs/super/com/google/gwt/dev/jjs/test/Java17Test.java
Outdated
Show resolved
Hide resolved
999c1bf
to
edebb95
Compare
edebb95
to
e37cc2f
Compare
e37cc2f
to
1a6cfb6
Compare
new JBinaryOperation(info, variableRef.getType(), JBinaryOperator.ASG, variableRef, | ||
jCastOperation); | ||
|
||
// null != (foo = (Foo) $instanceof_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// null != (foo = (Foo) $instanceof_1 | |
// null != (foo = (Foo) $instanceof_1) |
// 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)) |
There was a problem hiding this comment.
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
// 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; |
There was a problem hiding this comment.
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?
// 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) { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
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.