Consistent use of @nullable across the codebase (even for internals)

Beyond just formally declaring the current behavior, this revision actually enforces non-null behavior in selected signatures now, not tolerating null values anymore when not explicitly documented. It also changes some utility methods with historic null-in/null-out tolerance towards enforced non-null return values, making them a proper citizen in non-null assignments.

Some issues are left as to-do: in particular a thorough revision of spring-test, and a few tests with unclear failures (ignored as "TODO: NULLABLE") to be sorted out in a follow-up commit.

Issue: SPR-15540

Author: jhoeller

spring-aop/src/main/java/org/springframework/aop/Advisor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

spring-aop/src/main/java/org/springframework/aop/TargetSource.java

@@ -1,5 +1,5 @@
 /*<
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -31,24 +31,23 @@
  * {@code TargetSources} directly: this is an AOP framework interface.
  *
  * @author Rod Johnson
+ * @author Juergen Hoeller
  */
 public interface TargetSource extends TargetClassAware {
 
 	/**
 	 * Return the type of targets returned by this {@link TargetSource}.
-	 * <p>Can return {@code null}, although certain usages of a
-	 * {@code TargetSource} might just work with a predetermined
-	 * target class.
+	 * <p>Can return {@code null}, although certain usages of a {@code TargetSource}
+	 * might just work with a predetermined target class.
 	 * @return the type of targets returned by this {@link TargetSource}
 	 */
 	@Override
 	Class<?> getTargetClass();
 
 	/**
 	 * Will all calls to {@link #getTarget()} return the same object?
-	 * <p>In that case, there will be no need to invoke
-	 * {@link #releaseTarget(Object)}, and the AOP framework can cache
-	 * the return value of {@link #getTarget()}.
+	 * <p>In that case, there will be no need to invoke {@link #releaseTarget(Object)},
+	 * and the AOP framework can cache the return value of {@link #getTarget()}.
 	 * @return {@code true} if the target is immutable
 	 * @see #getTarget
 	 */

spring-aop/src/main/java/org/springframework/aop/aspectj/AbstractAspectJAdvice.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -208,6 +208,7 @@ public final AspectInstanceFactory getAspectInstanceFactory() {
 	/**
 	 * Return the ClassLoader for aspect instances.
 	 */
+	@Nullable
 	public final ClassLoader getAspectClassLoader() {
 		return this.aspectInstanceFactory.getAspectClassLoader();
 	}
@@ -296,8 +297,8 @@ protected void setReturningNameNoCheck(String name) {
 			}
 			catch (Throwable ex) {
 				throw new IllegalArgumentException("Returning name '" + name  +
-						"' is neither a valid argument name nor the fully-qualified name of a Java type on the classpath. " +
-						"Root cause: " + ex);
+						"' is neither a valid argument name nor the fully-qualified " +
+						"name of a Java type on the classpath. Root cause: " + ex);
 			}
 		}
 	}
@@ -306,6 +307,7 @@ protected Class<?> getDiscoveredReturningType() {
 		return this.discoveredReturningType;
 	}
 
+	@Nullable
 	protected Type getDiscoveredReturningGenericType() {
 		return this.discoveredReturningGenericType;
 	}
@@ -330,8 +332,8 @@ protected void setThrowingNameNoCheck(String name) {
 			}
 			catch (Throwable ex) {
 				throw new IllegalArgumentException("Throwing name '" + name  +
-						"' is neither a valid argument name nor the fully-qualified name of a Java type on the classpath. " +
-						"Root cause: " + ex);
+						"' is neither a valid argument name nor the fully-qualified " +
+						"name of a Java type on the classpath. Root cause: " + ex);
 			}
 		}
 	}
@@ -549,7 +551,9 @@ private void configurePointcutParameters(int argumentIndexOffset) {
 	 * @param ex the exception thrown by the method execution (may be null)
 	 * @return the empty array if there are no arguments
 	 */
-	protected Object[] argBinding(JoinPoint jp, JoinPointMatch jpMatch, @Nullable Object returnValue, @Nullable Throwable ex) {
+	protected Object[] argBinding(JoinPoint jp, @Nullable JoinPointMatch jpMatch,
+			@Nullable Object returnValue, @Nullable Throwable ex) {
+
 		calculateArgumentBindings();
 
 		// AMC start
@@ -608,13 +612,16 @@ else if (this.joinPointStaticPartArgumentIndex != -1) {
 	 * @return the invocation result
 	 * @throws Throwable in case of invocation failure
 	 */
-	protected Object invokeAdviceMethod(JoinPointMatch jpMatch, @Nullable Object returnValue, @Nullable Throwable ex) throws Throwable {
+	protected Object invokeAdviceMethod(
+			@Nullable JoinPointMatch jpMatch, @Nullable Object returnValue, @Nullable Throwable ex)
+			throws Throwable {
+
 		return invokeAdviceMethodWithGivenArgs(argBinding(getJoinPoint(), jpMatch, returnValue, ex));
 	}
 
 	// As above, but in this case we are given the join point.
-	protected Object invokeAdviceMethod(JoinPoint jp, JoinPointMatch jpMatch, Object returnValue, Throwable t)
-			throws Throwable {
+	protected Object invokeAdviceMethod(JoinPoint jp, @Nullable JoinPointMatch jpMatch,
+			@Nullable Object returnValue, @Nullable Throwable t) throws Throwable {
 
 		return invokeAdviceMethodWithGivenArgs(argBinding(jp, jpMatch, returnValue, t));
 	}
@@ -649,6 +656,7 @@ protected JoinPoint getJoinPoint() {
 	/**
 	 * Get the current join point match at the join point we are being dispatched on.
 	 */
+	@Nullable
 	protected JoinPointMatch getJoinPointMatch() {
 		MethodInvocation mi = ExposeInvocationInterceptor.currentInvocation();
 		if (!(mi instanceof ProxyMethodInvocation)) {
@@ -663,8 +671,10 @@ protected JoinPointMatch getJoinPointMatch() {
 	// 'last man wins' which is not what we want at all.
 	// Using the expression is guaranteed to be safe, since 2 identical expressions
 	// are guaranteed to bind in exactly the same way.
+	@Nullable
 	protected JoinPointMatch getJoinPointMatch(ProxyMethodInvocation pmi) {
-		return (JoinPointMatch) pmi.getUserAttribute(this.pointcut.getExpression());
+		String expression = this.pointcut.getExpression();
+		return (expression != null ? (JoinPointMatch) pmi.getUserAttribute(expression) : null);
 	}
 
 

spring-aop/src/main/java/org/springframework/aop/aspectj/AspectInstanceFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,6 +17,7 @@
 package org.springframework.aop.aspectj;
 
 import org.springframework.core.Ordered;
+import org.springframework.lang.Nullable;
 
 /**
  * Interface implemented to provide an instance of an AspectJ aspect.
@@ -40,8 +41,10 @@ public interface AspectInstanceFactory extends Ordered {
 
 	/**
 	 * Expose the aspect class loader that this factory uses.
-	 * @return the aspect class loader (never {@code null})
+	 * @return the aspect class loader (or {@code null} for the bootstrap loader)
+	 * @see org.springframework.util.ClassUtils#getDefaultClassLoader()
 	 */
+	@Nullable
 	ClassLoader getAspectClassLoader();
 
 }

spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJAdviceParameterNameDiscoverer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -183,10 +183,11 @@ public class AspectJAdviceParameterNameDiscoverer implements ParameterNameDiscov
 	 * Create a new discoverer that attempts to discover parameter names
 	 * from the given pointcut expression.
 	 */
-	public AspectJAdviceParameterNameDiscoverer(String pointcutExpression) {
+	public AspectJAdviceParameterNameDiscoverer(@Nullable String pointcutExpression) {
 		this.pointcutExpression = pointcutExpression;
 	}
 
+
 	/**
 	 * Indicate whether {@link IllegalArgumentException} and {@link AmbiguousBindingException}
 	 * must be thrown as appropriate in the case of failing to deduce advice parameter names.
@@ -214,6 +215,7 @@ public void setThrowingName(String throwingName) {
 		this.throwingName = throwingName;
 	}
 
+
 	/**
 	 * Deduce the parameter names for an advice method.
 	 * <p>See the {@link AspectJAdviceParameterNameDiscoverer class level javadoc}
@@ -475,8 +477,8 @@ else if (numAnnotationSlots == 1) {
 	 * If the token starts meets Java identifier conventions, it's in.
 	 */
 	@Nullable
-	private String maybeExtractVariableName(String candidateToken) {
-		if (candidateToken == null || candidateToken.equals("")) {
+	private String maybeExtractVariableName(@Nullable String candidateToken) {
+		if (!StringUtils.hasLength(candidateToken)) {
 			return null;
 		}
 		if (Character.isJavaIdentifierStart(candidateToken.charAt(0)) &&
@@ -498,7 +500,7 @@ private String maybeExtractVariableName(String candidateToken) {
 	 * Given an args pointcut body (could be {@code args} or {@code at_args}),
 	 * add any candidate variable names to the given list.
 	 */
-	private void maybeExtractVariableNamesFromArgs(String argsSpec, List<String> varNames) {
+	private void maybeExtractVariableNamesFromArgs(@Nullable String argsSpec, List<String> varNames) {
 		if (argsSpec == null) {
 			return;
 		}
@@ -781,7 +783,7 @@ private static class PointcutBody {
 
 		private String text;
 
-		public PointcutBody(int tokens, String text) {
+		public PointcutBody(int tokens, @Nullable String text) {
 			this.numTokensConsumed = tokens;
 			this.text = text;
 		}

spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJAroundAdvice.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -57,6 +57,7 @@
 import org.springframework.beans.factory.annotation.BeanFactoryAnnotationUtils;
 import org.springframework.beans.factory.config.ConfigurableBeanFactory;
 import org.springframework.lang.Nullable;
+import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
 import org.springframework.util.ObjectUtils;
 import org.springframework.util.StringUtils;
@@ -196,6 +197,7 @@ private void checkReadyToMatch() {
 	/**
 	 * Determine the ClassLoader to use for pointcut evaluation.
 	 */
+	@Nullable
 	private ClassLoader determinePointcutClassLoader() {
 		if (this.beanFactory instanceof ConfigurableBeanFactory) {
 			return ((ConfigurableBeanFactory) this.beanFactory).getBeanClassLoader();
@@ -209,21 +211,27 @@ private ClassLoader determinePointcutClassLoader() {
 	/**
 	 * Build the underlying AspectJ pointcut expression.
 	 */
-	private PointcutExpression buildPointcutExpression(ClassLoader classLoader) {
+	private PointcutExpression buildPointcutExpression(@Nullable ClassLoader classLoader) {
 		PointcutParser parser = initializePointcutParser(classLoader);
 		PointcutParameter[] pointcutParameters = new PointcutParameter[this.pointcutParameterNames.length];
 		for (int i = 0; i < pointcutParameters.length; i++) {
 			pointcutParameters[i] = parser.createPointcutParameter(
 					this.pointcutParameterNames[i], this.pointcutParameterTypes[i]);
 		}
-		return parser.parsePointcutExpression(replaceBooleanOperators(getExpression()),
+		return parser.parsePointcutExpression(replaceBooleanOperators(resolveExpression()),
 				this.pointcutDeclarationScope, pointcutParameters);
 	}
 
+	private String resolveExpression() {
+		String expression = getExpression();
+		Assert.state(expression != null, "No expression set");
+		return expression;
+	}
+
 	/**
 	 * Initialize the underlying AspectJ pointcut parser.
 	 */
-	private PointcutParser initializePointcutParser(ClassLoader classLoader) {
+	private PointcutParser initializePointcutParser(@Nullable ClassLoader classLoader) {
 		PointcutParser parser = PointcutParser
 				.getPointcutParserSupportingSpecifiedPrimitivesAndUsingSpecifiedClassLoaderForResolution(
 						SUPPORTED_PRIMITIVES, classLoader);
@@ -301,7 +309,8 @@ else if (shadowMatch.neverMatches()) {
 			// we say this is not a match as in Spring there will never be a different
 			// runtime subtype.
 			RuntimeTestWalker walker = getRuntimeTestWalker(shadowMatch);
-			return (!walker.testsSubtypeSensitiveVars() || walker.testTargetInstanceOfResidue(targetClass));
+			return (!walker.testsSubtypeSensitiveVars() ||
+					(targetClass != null && walker.testTargetInstanceOfResidue(targetClass)));
 		}
 	}
 
@@ -354,7 +363,7 @@ public boolean matches(Method method, @Nullable Class<?> targetClass, Object...
 			 * type but not 'this' (as would be the case of JDK dynamic proxies).
 			 * <p>See SPR-2979 for the original bug.
 			 */
-			if (pmi != null) {  // there is a current invocation
+			if (pmi != null && thisObject != null) {  // there is a current invocation
 				RuntimeTestWalker originalMethodResidueTest = getRuntimeTestWalker(originalShadowMatch);
 				if (!originalMethodResidueTest.testThisInstanceOfResidue(thisObject.getClass())) {
 					return false;
@@ -375,6 +384,7 @@ public boolean matches(Method method, @Nullable Class<?> targetClass, Object...
 		}
 	}
 
+	@Nullable
 	protected String getCurrentProxiedBeanName() {
 		return ProxyCreationContext.getCurrentProxiedBeanName();
 	}
@@ -411,7 +421,7 @@ private void bindParameters(ProxyMethodInvocation invocation, JoinPointMatch jpm
 		// 'last man wins' which is not what we want at all.
 		// Using the expression is guaranteed to be safe, since 2 identical expressions
 		// are guaranteed to bind in exactly the same way.
-		invocation.setUserAttribute(getExpression(), jpm);
+		invocation.setUserAttribute(resolveExpression(), jpm);
 	}
 
 	private ShadowMatch getShadowMatch(Method targetMethod, Method originalMethod) {
@@ -600,7 +610,7 @@ public boolean mayNeedDynamicTest() {
 			return false;
 		}
 
-		private FuzzyBoolean contextMatch(Class<?> targetType) {
+		private FuzzyBoolean contextMatch(@Nullable Class<?> targetType) {
 			String advisedBeanName = getCurrentProxiedBeanName();
 			if (advisedBeanName == null) {  // no proxy creation in progress
 				// abstain; can't return YES, since that will make pointcut with negation fail

spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcutAdvisor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@
 import org.springframework.aop.support.AbstractGenericPointcutAdvisor;
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.BeanFactoryAware;
+import org.springframework.lang.Nullable;
 
 /**
  * Spring AOP Advisor that can be used for any AspectJ pointcut expression.
@@ -37,6 +38,7 @@ public void setExpression(String expression) {
 		this.pointcut.setExpression(expression);
 	}
 
+	@Nullable
 	public String getExpression() {
 		return this.pointcut.getExpression();
 	}
@@ -45,6 +47,7 @@ public void setLocation(String location) {
 		this.pointcut.setLocation(location);
 	}
 
+	@Nullable
 	public String getLocation() {
 		return this.pointcut.getLocation();
 	}

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -212,6 +212,7 @@ private AspectJAnnotationType determineAnnotationType(A annotation) {
 			throw new IllegalStateException("Unknown annotation type: " + annotation.toString());
 		}
 
+		@Nullable
 		private String resolveExpression(A annotation) throws Exception {
 			String expression = null;
 			for (String methodName : EXPRESSION_PROPERTIES) {

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/AspectJProxyFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -119,7 +119,9 @@ public void addAspect(Class<?> aspectClass) {
 	 */
 	private void addAdvisorsFromAspectInstanceFactory(MetadataAwareAspectInstanceFactory instanceFactory) {
 		List<Advisor> advisors = this.aspectFactory.getAdvisors(instanceFactory);
-		advisors = AopUtils.findAdvisorsThatCanApply(advisors, getTargetClass());
+		Class<?> targetClass = getTargetClass();
+		Assert.state(targetClass != null, "Unresolvable target class");
+		advisors = AopUtils.findAdvisorsThatCanApply(advisors, targetClass);
 		AspectJProxyUtils.makeAdvisorChainAspectJCapableIfNecessary(advisors);
 		AnnotationAwareOrderComparator.sort(advisors);
 		addAdvisors(advisors);