Remove unnecessary null check against configuration properties

[quaff]

Thanks to JSpecify.

[wilkinsona]

Thanks for this, @quaff. I'm curious to know how you found these. Could you let us know what you did? I'm wondering if there's something we can enable in our NullAway configuration to detect them automatically.

[quaff]

Thanks for this, @quaff. I'm curious to know how you found these. Could you let us know what you did? I'm wondering if there's something we can enable in our NullAway configuration to detect them automatically.

I used IDEA to search the source files using regex if \(.*roperties.get\w+\(\) .= null\), then open matched files one by one, IDEA will report if null check is redundant.

[nosan]

The following configuration should handle this automatically:

     //JavaConventions.java
	private void configureNullability(Project project) {
		project.getPlugins().apply(NullabilityPlugin.class);
		NullabilityPluginExtension extension = project.getExtensions().getByType(NullabilityPluginExtension.class);
		String nullAwayVersion = (String) project.findProperty("nullAwayVersion");
		if (nullAwayVersion != null) {
			extension.getNullAwayVersion().set(nullAwayVersion);
		}
		String errorProneVersion = (String) project.findProperty("errorProneVersion");
		if (errorProneVersion != null) {
			extension.getErrorProneVersion().set(errorProneVersion);
		}
		project.getTasks().withType(JavaCompile.class).configureEach((javaCompile) -> {
			CompileOptions options = javaCompile.getOptions();
			if (options instanceof ExtensionAware extensionAware) {
				extensionAware.getExtensions()
					.configure(ErrorProneOptions.class,
							(errorProneOptions) -> errorProneOptions.check("RedundantNullCheck", CheckSeverity.ERROR));
			}
		});
	}

error: [RedundantNullCheck] Null check on an expression that is statically determined to be non-null according to language semantics or nullness annotations.
Assert.state(createdLayout != null, "Unable to detect layout");
^
(see https://errorprone.info/bugpattern/RedundantNullCheck)

Documentation: https://errorprone.info/bugpattern/RedundantNullCheck

[wilkinsona]

Nice! Thanks very much, @nosan. I went looking for a NullAway option but didn't think to check Error Prone. I've opened spring-gradle-plugins/nullability-plugin#30.

[philwebb]

I think we should be a little careful with where we remove existing null checks because not everyone will be using nullaway so we should probably enforce more things with Assert.notNull.

We certainly get away with it for configuration properties, but if these were regular classes removing a check like:

if (this.properties.getCloseTimeout() != null) {
	factory.setCloseTimeout((int) this.properties.getCloseTimeout().toMillis());
}

Means that if someone did ActiveMQProperties.setCloseTimeout(null) we'd now get an NPE. Ideally setCloseTimeout(null) would throw an exception so that ActiveMQProperties never gets into an invalid state.

[quaff]

Means that if someone did ActiveMQProperties.setCloseTimeout(null) we'd now get an NPE. Ideally setCloseTimeout(null) would throw an exception so that ActiveMQProperties never gets into an invalid state.

IIRC, configuration properties class is not public API, that's why this PR focus on properties.get*().

[philwebb]

Indeed, that's what I meant by "We certainly get away with it for configuration properties".

[wilkinsona]

After some experimentation with Error Prone's redundant null check I've closed spring-gradle-plugins/nullability-plugin#30. It's too blunt an instrument and doesn't work well for libraries, resulting in many false positives for redundant checks.

[wilkinsona]

Some of the proposed changes are completely safe because they're accessing a final field that's always initialised. DataRedisProperties.getLettuce() is one example of this. CassandraProperties.getSsl() could be another if we polished it a little. Others have some risk if the nullability of the configuration properties class hasn't been enforced. If we're going to be touching the code that's using those getters, I wonder if we should take the opportunity to use PropertyMapper. There's also a related question of whether or not we should be enforcing things in the @ConfigurationProperties classes as Phil described above. We may want to broaden that to the whole codebase so that we catch illegal null arguments when they're passed in.