Skip to content

Commit

Permalink
Perform NullAway build-time checks in spring-expression
Browse files Browse the repository at this point in the history
  • Loading branch information
sdeleuze committed Mar 20, 2024
1 parent 1ccd551 commit f648fd7
Show file tree
Hide file tree
Showing 14 changed files with 41 additions and 12 deletions.
2 changes: 1 addition & 1 deletion gradle/spring-module.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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<String> messageSupplier) {
if (ObjectUtils.isEmpty(array)) {
throw new IllegalArgumentException(nullSafeGet(messageSupplier));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.StringJoiner;
import java.util.TimeZone;

import org.springframework.lang.Contract;
import org.springframework.lang.Nullable;

/**
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")));
}
Expand All @@ -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);
}
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -565,6 +566,7 @@ public PropertyIndexingValueRef(Object targetObject, String value,
}

@Override
@SuppressWarnings("NullAway")
public TypedValue getValue() {
Class<?> targetObjectRuntimeClass = getObjectClass(this.targetObject);
try {
Expand Down Expand Up @@ -603,6 +605,7 @@ public TypedValue getValue() {
}

@Override
@SuppressWarnings("NullAway")
public void setValue(@Nullable Object newValue) {
Class<?> contextObjectClass = getObjectClass(this.targetObject);
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,19 +134,19 @@ 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);
}
}
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());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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)) {
Expand All @@ -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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -515,6 +516,7 @@ protected Field findField(String name, Class<?> clazz, boolean mustBeStatic) {
* <p>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) {
Expand Down

0 comments on commit f648fd7

Please sign in to comment.