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

Don't allow casting from void to def in painless #22969

Merged

Conversation

Projects
None yet
3 participants
@nik9000
Copy link
Contributor

commented Feb 3, 2017

Painless can cast anything into the magic type def but it
really shouldn't try to cast nothing into def. That causes
the byte code generation library to freak out a little.

Closes #22908

Painless: Don't allow casting from void to def
Painless can cast anything into the magic type `def` but it
really shouldn't try to cast **nothing** into `def`. That causes
the byte code generation library to freak out a little.

Closes #22908
if ( actual.sort == Sort.DEF
|| (actual.sort != Sort.VOID && expected.sort == Sort.DEF)
|| expected.clazz.isAssignableFrom(actual.clazz)
|| (explicit && actual.clazz.isAssignableFrom(expected.clazz))) {

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

I'm paranoid about order of operations here.

This comment has been minimized.

Copy link
@jdconrad

jdconrad Feb 3, 2017

Contributor

Is there a specific case you're worried about?

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

No. I just can never remember if && comes before || so I always use parentheses to make them clear to me.

This comment has been minimized.

Copy link
@jdconrad

jdconrad Feb 3, 2017

Contributor

Ahh. I completely missed the parenthesis had been added.

expected.clazz.isAssignableFrom(actual.clazz) ||
explicit && actual.clazz.isAssignableFrom(expected.clazz)) {
if ( actual.sort == Sort.DEF
|| (actual.sort != Sort.VOID && expected.sort == Sort.DEF)

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

This one line is the actual fix.

@@ -318,6 +318,19 @@ public Variable(Location location, String name, Type type, int slot, boolean rea
public int getSlot() {
return slot;
}

@Override
public String toString() {

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

I added this because I was frustrated that I couldn't see what a Variable was while debugging.

@@ -208,7 +211,7 @@ public void testNotFunctionalInterface() {
IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> {
exec("List l = new ArrayList(); l.add(2); l.add(1); l.add(Integer::bogus); return l.get(0);");
});
assertTrue(expected.getMessage().contains("Cannot convert function reference"));
assertThat(expected.getMessage(), containsString("Cannot convert function reference"));

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

I was tired of the garbage error messages these were making

assertThat(expected.getMessage(), containsString("Unknown reference"));
}

public void testReturnVoid() {

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

None of these methods actually had the problem.

}

public void testReturnVoid() {
assertEquals(null, exec("void test(StringBuilder b, int i) {b.setLength(i)} test(new StringBuilder(), 1)"));

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

These were fine.

exec("int test(StringBuilder b, int i) {return b.setLength(i)} test(new StringBuilder(), 1)");
});
assertEquals("Cannot cast from [void] to [int].", expected.getMessage());
expected = expectScriptThrows(ClassCastException.class, () -> {

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

This one was broken.

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

It had a VerificationError.


public void testReturnVoidDef() {
// If we can catch the error at compile time we do
Exception expected = expectScriptThrows(ClassCastException.class, () -> {

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

This one was broken - it had a VerificationError.

@@ -231,4 +234,25 @@ public void testReservedCapture() {
assertEquals(false, exec(compare + "compare(() -> { if (params['number'] == 1) { return params['number'] }" +
"else { return params['key'] } }, 2)", params, true));
}

public void testReturnVoid() {
Throwable expected = expectScriptThrows(ClassCastException.class, () -> {

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

This one worked.

assertThat(expected.getMessage(), containsString("Cannot cast from [void] to [def]."));

// Otherwise we convert the void into a null
assertEquals(Arrays.asList(null, null),

This comment has been minimized.

Copy link
@nik9000

nik9000 Feb 3, 2017

Author Contributor

None of this is new behavior.

@jdconrad
Copy link
Contributor

left a comment

Another good bug catch here. Thanks for fixing this!

if ( actual.sort == Sort.DEF
|| (actual.sort != Sort.VOID && expected.sort == Sort.DEF)
|| expected.clazz.isAssignableFrom(actual.clazz)
|| (explicit && actual.clazz.isAssignableFrom(expected.clazz))) {

This comment has been minimized.

Copy link
@jdconrad

jdconrad Feb 3, 2017

Contributor

Is there a specific case you're worried about?

@nik9000 nik9000 merged commit b0c9759 into elastic:master Feb 3, 2017

1 of 2 checks passed

elasticsearch-ci Build finished.
Details
CLA Commit author has signed the CLA
Details

@nik9000 nik9000 changed the title Painless: Don't allow casting from void to def Don't allow casting from void to def in painless Feb 3, 2017

nik9000 added a commit that referenced this pull request Feb 3, 2017

Painless: Don't allow casting from void to def (#22969)
Painless can cast anything into the magic type `def` but it
really shouldn't try to cast **nothing** into `def`. That causes
the byte code generation library to freak out a little.

Closes #22908
@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2017

Thanks for reviewing @jdconrad!

master: b0c9759
5.x: a48c273
5.2: 07a73ef

nik9000 added a commit that referenced this pull request Feb 3, 2017

Painless: Don't allow casting from void to def (#22969)
Painless can cast anything into the magic type `def` but it
really shouldn't try to cast **nothing** into `def`. That causes
the byte code generation library to freak out a little.

Closes #22908
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.