From f648fd7c3ba3e9e1da6ebae85cb190407476d204 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Deleuze?= Date: Wed, 20 Mar 2024 10:09:04 +0100 Subject: [PATCH] Perform NullAway build-time checks in spring-expression See gh-32475 --- gradle/spring-module.gradle | 2 +- .../org/springframework/core/convert/TypeDescriptor.java | 2 ++ .../src/main/java/org/springframework/util/Assert.java | 2 ++ .../main/java/org/springframework/util/ObjectUtils.java | 3 +++ .../org/springframework/expression/spel/CodeFlow.java | 7 +++++++ .../org/springframework/expression/spel/ast/Indexer.java | 3 +++ .../org/springframework/expression/spel/ast/OpAnd.java | 2 ++ .../org/springframework/expression/spel/ast/OpEQ.java | 6 ++---- .../org/springframework/expression/spel/ast/OpNE.java | 6 ++---- .../org/springframework/expression/spel/ast/OpOr.java | 2 ++ .../springframework/expression/spel/ast/Operator.java | 1 + .../expression/spel/ast/PropertyOrFieldReference.java | 6 +++--- .../spel/standard/InternalSpelExpressionParser.java | 9 +++++++++ .../spel/support/ReflectivePropertyAccessor.java | 2 ++ 14 files changed, 41 insertions(+), 12 deletions(-) diff --git a/gradle/spring-module.gradle b/gradle/spring-module.gradle index 883712b5b756..5f4104100748 100644 --- a/gradle/spring-module.gradle +++ b/gradle/spring-module.gradle @@ -117,7 +117,7 @@ tasks.withType(JavaCompile).configureEach { options.errorprone { disableAllChecks = true option("NullAway:CustomContractAnnotations", "org.springframework.lang.Contract") - option("NullAway:AnnotatedPackages", "org.springframework.core") + option("NullAway:AnnotatedPackages", "org.springframework.core,org.springframework.expression") option("NullAway:UnannotatedSubPackages", "org.springframework.instrument,org.springframework.context.index," + "org.springframework.asm,org.springframework.cglib,org.springframework.objenesis," + "org.springframework.javapoet,org.springframework.aot.nativex.substitution") diff --git a/spring-core/src/main/java/org/springframework/core/convert/TypeDescriptor.java b/spring-core/src/main/java/org/springframework/core/convert/TypeDescriptor.java index 109674a565f9..8c73e66fde4b 100644 --- a/spring-core/src/main/java/org/springframework/core/convert/TypeDescriptor.java +++ b/spring-core/src/main/java/org/springframework/core/convert/TypeDescriptor.java @@ -30,6 +30,7 @@ import org.springframework.core.MethodParameter; import org.springframework.core.ResolvableType; import org.springframework.core.annotation.AnnotatedElementUtils; +import org.springframework.lang.Contract; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; @@ -562,6 +563,7 @@ public String toString() { * @return the type descriptor */ @Nullable + @Contract("!null -> !null; null -> null") public static TypeDescriptor forObject(@Nullable Object source) { return (source != null ? valueOf(source.getClass()) : null); } diff --git a/spring-core/src/main/java/org/springframework/util/Assert.java b/spring-core/src/main/java/org/springframework/util/Assert.java index 9b0cba8e9fdf..e6fd456fb490 100644 --- a/spring-core/src/main/java/org/springframework/util/Assert.java +++ b/spring-core/src/main/java/org/springframework/util/Assert.java @@ -312,6 +312,7 @@ public static void doesNotContain(@Nullable String textToSearch, String substrin * @param message the exception message to use if the assertion fails * @throws IllegalArgumentException if the object array is {@code null} or contains no elements */ + @Contract("null, _ -> fail") public static void notEmpty(@Nullable Object[] array, String message) { if (ObjectUtils.isEmpty(array)) { throw new IllegalArgumentException(message); @@ -330,6 +331,7 @@ public static void notEmpty(@Nullable Object[] array, String message) { * @throws IllegalArgumentException if the object array is {@code null} or contains no elements * @since 5.0 */ + @Contract("null, _ -> fail") public static void notEmpty(@Nullable Object[] array, Supplier messageSupplier) { if (ObjectUtils.isEmpty(array)) { throw new IllegalArgumentException(nullSafeGet(messageSupplier)); diff --git a/spring-core/src/main/java/org/springframework/util/ObjectUtils.java b/spring-core/src/main/java/org/springframework/util/ObjectUtils.java index 8657738dc623..c807e0b6cfad 100644 --- a/spring-core/src/main/java/org/springframework/util/ObjectUtils.java +++ b/spring-core/src/main/java/org/springframework/util/ObjectUtils.java @@ -27,6 +27,7 @@ import java.util.StringJoiner; import java.util.TimeZone; +import org.springframework.lang.Contract; import org.springframework.lang.Nullable; /** @@ -110,6 +111,7 @@ public static boolean isArray(@Nullable Object obj) { * @param array the array to check * @see #isEmpty(Object) */ + @Contract("null -> true") public static boolean isEmpty(@Nullable Object[] array) { return (array == null || array.length == 0); } @@ -331,6 +333,7 @@ public static Object[] toObjectArray(@Nullable Object source) { * @see Object#equals(Object) * @see java.util.Arrays#equals */ + @Contract("null, null -> true; null, _ -> false; _, null -> false") public static boolean nullSafeEquals(@Nullable Object o1, @Nullable Object o2) { if (o1 == o2) { return true; diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java index 2f3e4468371b..9437835e7244 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java @@ -28,6 +28,7 @@ import org.springframework.asm.ClassWriter; import org.springframework.asm.MethodVisitor; import org.springframework.asm.Opcodes; +import org.springframework.lang.Contract; import org.springframework.lang.Nullable; import org.springframework.util.ClassUtils; import org.springframework.util.CollectionUtils; @@ -583,6 +584,7 @@ public static String toDescriptorFromObject(@Nullable Object value) { * @param descriptor type descriptor * @return {@code true} if the descriptor is boolean compatible */ + @Contract("null -> false") public static boolean isBooleanCompatible(@Nullable String descriptor) { return (descriptor != null && (descriptor.equals("Z") || descriptor.equals("Ljava/lang/Boolean"))); } @@ -592,6 +594,7 @@ public static boolean isBooleanCompatible(@Nullable String descriptor) { * @param descriptor type descriptor * @return {@code true} if a primitive type or {@code void} */ + @Contract("null -> false") public static boolean isPrimitive(@Nullable String descriptor) { return (descriptor != null && descriptor.length() == 1); } @@ -601,6 +604,7 @@ public static boolean isPrimitive(@Nullable String descriptor) { * @param descriptor the descriptor for a possible primitive array * @return {@code true} if the descriptor a primitive array */ + @Contract("null -> false") public static boolean isPrimitiveArray(@Nullable String descriptor) { if (descriptor == null) { return false; @@ -653,6 +657,7 @@ private static boolean checkPairs(String desc1, String desc2) { * @param descriptor the descriptor for a type * @return {@code true} if the descriptor is for a supported numeric type or boolean */ + @Contract("null -> false") public static boolean isPrimitiveOrUnboxableSupportedNumberOrBoolean(@Nullable String descriptor) { if (descriptor == null) { return false; @@ -670,6 +675,7 @@ public static boolean isPrimitiveOrUnboxableSupportedNumberOrBoolean(@Nullable S * @param descriptor the descriptor for a type * @return {@code true} if the descriptor is for a supported numeric type */ + @Contract("null -> false") public static boolean isPrimitiveOrUnboxableSupportedNumber(@Nullable String descriptor) { if (descriptor == null) { return false; @@ -690,6 +696,7 @@ public static boolean isPrimitiveOrUnboxableSupportedNumber(@Nullable String des * @param number the number to check * @return {@code true} if it is an {@link Integer}, {@link Short} or {@link Byte} */ + @Contract("null -> false") public static boolean isIntegerForNumericOp(Number number) { return (number instanceof Integer || number instanceof Short || number instanceof Byte); } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java index ab61712c7738..445b9d8407ba 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Indexer.java @@ -325,6 +325,7 @@ else if (this.indexedType == IndexedType.OBJECT) { CompilablePropertyAccessor compilablePropertyAccessor = (CompilablePropertyAccessor) this.cachedReadAccessor; Assert.state(compilablePropertyAccessor != null, "No cached read accessor"); String propertyName = (String) stringLiteral.getLiteralValue().getValue(); + Assert.state(propertyName != null, "No property name"); compilablePropertyAccessor.generateCode(propertyName, mv, cf); } @@ -565,6 +566,7 @@ public PropertyIndexingValueRef(Object targetObject, String value, } @Override + @SuppressWarnings("NullAway") public TypedValue getValue() { Class targetObjectRuntimeClass = getObjectClass(this.targetObject); try { @@ -603,6 +605,7 @@ public TypedValue getValue() { } @Override + @SuppressWarnings("NullAway") public void setValue(@Nullable Object newValue) { Class contextObjectClass = getObjectClass(this.targetObject); try { diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpAnd.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpAnd.java index baccf2411035..99c5694095f1 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpAnd.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpAnd.java @@ -25,6 +25,7 @@ import org.springframework.expression.spel.SpelEvaluationException; import org.springframework.expression.spel.SpelMessage; import org.springframework.expression.spel.support.BooleanTypedValue; +import org.springframework.lang.Contract; import org.springframework.lang.Nullable; /** @@ -64,6 +65,7 @@ private boolean getBooleanValue(ExpressionState state, SpelNodeImpl operand) { } } + @Contract("null -> fail") private void assertValueNotNull(@Nullable Boolean value) { if (value == null) { throw new SpelEvaluationException(SpelMessage.TYPE_CONVERSION_ERROR, "null", "boolean"); diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java index 7916093afe2b..e8e08cc9ed8e 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java @@ -68,19 +68,17 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) { cf.loadEvaluationContext(mv); String leftDesc = getLeftOperand().exitTypeDescriptor; String rightDesc = getRightOperand().exitTypeDescriptor; - boolean leftPrim = CodeFlow.isPrimitive(leftDesc); - boolean rightPrim = CodeFlow.isPrimitive(rightDesc); cf.enterCompilationScope(); getLeftOperand().generateCode(mv, cf); cf.exitCompilationScope(); - if (leftPrim) { + if (CodeFlow.isPrimitive(leftDesc)) { CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0)); } cf.enterCompilationScope(); getRightOperand().generateCode(mv, cf); cf.exitCompilationScope(); - if (rightPrim) { + if (CodeFlow.isPrimitive(rightDesc)) { CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0)); } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java index 504dfa615295..def919d0a524 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java @@ -69,19 +69,17 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) { cf.loadEvaluationContext(mv); String leftDesc = getLeftOperand().exitTypeDescriptor; String rightDesc = getRightOperand().exitTypeDescriptor; - boolean leftPrim = CodeFlow.isPrimitive(leftDesc); - boolean rightPrim = CodeFlow.isPrimitive(rightDesc); cf.enterCompilationScope(); getLeftOperand().generateCode(mv, cf); cf.exitCompilationScope(); - if (leftPrim) { + if (CodeFlow.isPrimitive(leftDesc)) { CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0)); } cf.enterCompilationScope(); getRightOperand().generateCode(mv, cf); cf.exitCompilationScope(); - if (rightPrim) { + if (CodeFlow.isPrimitive(rightDesc)) { CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0)); } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpOr.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpOr.java index 3afee612901d..4caf753c85a5 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpOr.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpOr.java @@ -24,6 +24,7 @@ import org.springframework.expression.spel.SpelEvaluationException; import org.springframework.expression.spel.SpelMessage; import org.springframework.expression.spel.support.BooleanTypedValue; +import org.springframework.lang.Contract; import org.springframework.lang.Nullable; /** @@ -63,6 +64,7 @@ private boolean getBooleanValue(ExpressionState state, SpelNodeImpl operand) { } } + @Contract("null -> fail") private void assertValueNotNull(@Nullable Boolean value) { if (value == null) { throw new SpelEvaluationException(SpelMessage.TYPE_CONVERSION_ERROR, "null", "boolean"); diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java index c84ec0a2d86c..c6d1c23bc89f 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java @@ -352,6 +352,7 @@ private DescriptorComparison(boolean areNumbers, boolean areCompatible, char com * @param rightActualDescriptor the dynamic/runtime right object descriptor * @return a DescriptorComparison object indicating the type of compatibility, if any */ + @SuppressWarnings("NullAway") public static DescriptorComparison checkNumericCompatibility( @Nullable String leftDeclaredDescriptor, @Nullable String rightDeclaredDescriptor, @Nullable String leftActualDescriptor, @Nullable String rightActualDescriptor) { diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java index b84c78210452..1460734a6ef8 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/PropertyOrFieldReference.java @@ -134,7 +134,7 @@ else if (Map.class == resultDescriptor.getType()) { // 'simple' object try { if (isWritableProperty(this.name,contextObject, evalContext)) { - Class clazz = result.getTypeDescriptor().getType(); + Class clazz = resultDescriptor.getType(); Object newObject = ReflectionUtils.accessibleConstructor(clazz).newInstance(); writeProperty(contextObject, evalContext, this.name, newObject); result = readProperty(contextObject, evalContext, this.name); @@ -142,11 +142,11 @@ else if (Map.class == resultDescriptor.getType()) { } catch (InvocationTargetException ex) { throw new SpelEvaluationException(getStartPosition(), ex.getTargetException(), - SpelMessage.UNABLE_TO_DYNAMICALLY_CREATE_OBJECT, result.getTypeDescriptor().getType()); + SpelMessage.UNABLE_TO_DYNAMICALLY_CREATE_OBJECT, resultDescriptor.getType()); } catch (Throwable ex) { throw new SpelEvaluationException(getStartPosition(), ex, - SpelMessage.UNABLE_TO_DYNAMICALLY_CREATE_OBJECT, result.getTypeDescriptor().getType()); + SpelMessage.UNABLE_TO_DYNAMICALLY_CREATE_OBJECT, resultDescriptor.getType()); } } } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java b/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java index 886b2090287a..33af0c254fcc 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java @@ -76,6 +76,7 @@ import org.springframework.expression.spel.ast.Ternary; import org.springframework.expression.spel.ast.TypeReference; import org.springframework.expression.spel.ast.VariableReference; +import org.springframework.lang.Contract; import org.springframework.lang.Nullable; import org.springframework.util.StringUtils; @@ -164,6 +165,7 @@ private void checkExpressionLength(String string) { // | (QMARK^ expression COLON! expression) // | (ELVIS^ expression))?; @Nullable + @SuppressWarnings("NullAway") private SpelNodeImpl eatExpression() { SpelNodeImpl expr = eatLogicalOrExpression(); Token t = peekToken(); @@ -274,6 +276,7 @@ private SpelNodeImpl eatRelationalExpression() { //sumExpression: productExpression ( (PLUS^ | MINUS^) productExpression)*; @Nullable + @SuppressWarnings("NullAway") private SpelNodeImpl eatSumExpression() { SpelNodeImpl expr = eatProductExpression(); while (peekToken(TokenKind.PLUS, TokenKind.MINUS, TokenKind.INC)) { @@ -313,6 +316,7 @@ else if (t.kind == TokenKind.MOD) { // powerExpr : unaryExpression (POWER^ unaryExpression)? (INC || DEC) ; @Nullable + @SuppressWarnings("NullAway") private SpelNodeImpl eatPowerIncDecExpression() { SpelNodeImpl expr = eatUnaryExpression(); if (peekToken(TokenKind.POWER)) { @@ -333,6 +337,7 @@ private SpelNodeImpl eatPowerIncDecExpression() { // unaryExpression: (PLUS^ | MINUS^ | BANG^ | INC^ | DEC^) unaryExpression | primaryExpression ; @Nullable + @SuppressWarnings("NullAway") private SpelNodeImpl eatUnaryExpression() { if (peekToken(TokenKind.NOT, TokenKind.PLUS, TokenKind.MINUS)) { Token t = takeToken(); @@ -755,6 +760,7 @@ private SpelNodeImpl eatPossiblyQualifiedId() { qualifiedIdPieces.getLast().getEndPosition(), qualifiedIdPieces.toArray(new SpelNodeImpl[0])); } + @Contract("null -> false") private boolean isValidQualifiedId(@Nullable Token node) { if (node == null || node.kind == TokenKind.LITERAL_STRING) { return false; @@ -1040,17 +1046,20 @@ public String toString(@Nullable Token t) { return t.kind.toString().toLowerCase(); } + @Contract("_, null, _ -> fail; _, _, null -> fail") private void checkOperands(Token token, @Nullable SpelNodeImpl left, @Nullable SpelNodeImpl right) { checkLeftOperand(token, left); checkRightOperand(token, right); } + @Contract("_, null -> fail") private void checkLeftOperand(Token token, @Nullable SpelNodeImpl operandExpression) { if (operandExpression == null) { throw internalException(token.startPos, SpelMessage.LEFT_OPERAND_PROBLEM); } } + @Contract("_, null -> fail") private void checkRightOperand(Token token, @Nullable SpelNodeImpl operandExpression) { if (operandExpression == null) { throw internalException(token.startPos, SpelMessage.RIGHT_OPERAND_PROBLEM); diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java index 115a3c412776..0d03779441ef 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java @@ -155,6 +155,7 @@ public boolean canRead(EvaluationContext context, @Nullable Object target, Strin } @Override + @SuppressWarnings("NullAway") public TypedValue read(EvaluationContext context, @Nullable Object target, String name) throws AccessException { Assert.state(target != null, "Target must not be null"); Class type = (target instanceof Class clazz ? clazz : target.getClass()); @@ -515,6 +516,7 @@ protected Field findField(String name, Class clazz, boolean mustBeStatic) { *

Note: An optimized accessor is currently only usable for read attempts. * Do not call this method if you need a read-write accessor. */ + @SuppressWarnings("NullAway") public PropertyAccessor createOptimalAccessor(EvaluationContext context, @Nullable Object target, String name) { // Don't be clever for arrays or a null target... if (target == null) {