Skip to content

Commit

Permalink
Support safe navigation operator with void methods in SpEL
Browse files Browse the repository at this point in the history
Prior to this commit the Spring Expression Language (SpEL) was able to
properly parse an expression that uses the safe navigation operator
(?.) with a method that has a `void` return type (for example,
"myObject?.doSomething()"); however, SpEL was not able to evaluate or
compile such expressions.

This commit addresses the evaluation issue by selectively not boxing
the exit type descriptor (for inclusion in the generated bytecode) when
the method's return type is `void`.

This commit addresses the compilation issue by pushing a null object
reference onto the stack in the generated byte code when the method's
return type is `void`.

Closes gh-27421
  • Loading branch information
sbrannen committed Sep 29, 2023
1 parent 1fe2216 commit 9aab4a6
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ private void updateExitTypeDescriptor() {
if (executorToCheck != null && executorToCheck.get() instanceof ReflectiveMethodExecutor reflectiveMethodExecutor) {
Method method = reflectiveMethodExecutor.getMethod();
String descriptor = CodeFlow.toDescriptor(method.getReturnType());
if (this.nullSafe && CodeFlow.isPrimitive(descriptor)) {
if (this.nullSafe && CodeFlow.isPrimitive(descriptor) && (descriptor.charAt(0) != 'V')) {
this.originalPrimitiveExitTypeDescriptor = descriptor.charAt(0);
this.exitTypeDescriptor = CodeFlow.toBoxedDescriptor(descriptor);
}
Expand Down Expand Up @@ -359,10 +359,16 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {

if (this.originalPrimitiveExitTypeDescriptor != null) {
// The output of the accessor will be a primitive but from the block above it might be null,
// so to have a 'common stack' element at skipIfNull target we need to box the primitive
// so to have a 'common stack' element at the skipIfNull target we need to box the primitive.
CodeFlow.insertBoxIfNecessary(mv, this.originalPrimitiveExitTypeDescriptor);
}

if (skipIfNull != null) {
if ("V".equals(this.exitTypeDescriptor)) {
// If the method return type is 'void', we need to push a null object
// reference onto the stack to satisfy the needs of the skipIfNull target.
mv.visitInsn(ACONST_NULL);
}
mv.visitLabel(skipIfNull);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -771,6 +771,34 @@ public void nullsafeFieldPropertyDereferencing_SPR16489() throws Exception {
assertThat(expression.getValue(context)).isNull();
}

@Test // gh-27421
public void nullSafeMethodChainingWithNonStaticVoidMethod() throws Exception {
FooObjectHolder foh = new FooObjectHolder();
StandardEvaluationContext context = new StandardEvaluationContext(foh);
SpelExpression expression = (SpelExpression) parser.parseExpression("getFoo()?.doFoo()");

FooObject.doFooInvoked = false;
assertThat(expression.getValue(context)).isNull();
assertThat(FooObject.doFooInvoked).isTrue();

FooObject.doFooInvoked = false;
foh.foo = null;
assertThat(expression.getValue(context)).isNull();
assertThat(FooObject.doFooInvoked).isFalse();

assertCanCompile(expression);

FooObject.doFooInvoked = false;
foh.foo = new FooObject();
assertThat(expression.getValue(context)).isNull();
assertThat(FooObject.doFooInvoked).isTrue();

FooObject.doFooInvoked = false;
foh.foo = null;
assertThat(expression.getValue(context)).isNull();
assertThat(FooObject.doFooInvoked).isFalse();
}

@Test
public void nullsafeMethodChaining_SPR16489() throws Exception {
FooObjectHolder foh = new FooObjectHolder();
Expand Down Expand Up @@ -3898,37 +3926,71 @@ void methodReferenceVarargs() {
tc.reset();
}

@Test
void nullSafeInvocationOfNonStaticVoidWrapperMethod() {
@Test // gh-27421
public void nullSafeInvocationOfNonStaticVoidMethod() {
// non-static method, no args, void return
expression = parser.parseExpression("new %s()?.one()".formatted(TestClass5.class.getName()));

assertCantCompile(expression);

TestClass5._i = 0;
assertThat(expression.getValue()).isNull();
assertThat(TestClass5._i).isEqualTo(1);

TestClass5._i = 0;
assertCanCompile(expression);
assertThat(expression.getValue()).isNull();
assertThat(TestClass5._i).isEqualTo(1);
}

@Test // gh-27421
public void nullSafeInvocationOfStaticVoidMethod() {
// static method, no args, void return
expression = parser.parseExpression("T(%s)?.two()".formatted(TestClass5.class.getName()));

assertCantCompile(expression);

TestClass5._i = 0;
assertThat(expression.getValue()).isNull();
assertThat(TestClass5._i).isEqualTo(1);

TestClass5._i = 0;
assertCanCompile(expression);
assertThat(expression.getValue()).isNull();
assertThat(TestClass5._i).isEqualTo(1);
}

@Test // gh-27421
public void nullSafeInvocationOfNonStaticVoidWrapperMethod() {
// non-static method, no args, Void return
expression = parser.parseExpression("new %s()?.oneVoidWrapper()".formatted(TestClass5.class.getName()));

assertCantCompile(expression);

TestClass5._i = 0;
expression.getValue();
assertThat(expression.getValue()).isNull();
assertThat(TestClass5._i).isEqualTo(1);

TestClass5._i = 0;
assertCanCompile(expression);
expression.getValue();
assertThat(expression.getValue()).isNull();
assertThat(TestClass5._i).isEqualTo(1);
}

@Test
void nullSafeInvocationOfStaticVoidWrapperMethod() {
@Test // gh-27421
public void nullSafeInvocationOfStaticVoidWrapperMethod() {
// static method, no args, Void return
expression = parser.parseExpression("T(%s)?.twoVoidWrapper()".formatted(TestClass5.class.getName()));

assertCantCompile(expression);

TestClass5._i = 0;
expression.getValue();
assertThat(expression.getValue()).isNull();
assertThat(TestClass5._i).isEqualTo(1);

TestClass5._i = 0;
assertCanCompile(expression);
expression.getValue();
assertThat(expression.getValue()).isNull();
assertThat(TestClass5._i).isEqualTo(1);
}

Expand Down Expand Up @@ -5496,7 +5558,10 @@ public FooObject getFoo() {

public static class FooObject {

static boolean doFooInvoked = false;

public Object getObject() { return "hello"; }
public void doFoo() { doFooInvoked = true; }
}


Expand Down Expand Up @@ -5744,7 +5809,10 @@ public void reset() {
field = null;
}

public void one() { i = 1; }
public void one() {
_i = 1;
this.i = 1;
}

public Void oneVoidWrapper() {
_i = 1;
Expand Down

0 comments on commit 9aab4a6

Please sign in to comment.