Enable contract checking

[msridhar]

https://github.com/uber/NullAway/wiki/Configuration#contract-checking

By default, NullAway simply trusts @Contract annotations and does not verify that they match the behavior of the annotated method. Passing the CheckContracts flag enables checking of contracts, which should add more safety. If you all try this and run into issues we are happy to look into them on the NullAway side. We may switch the defaults and make contract checking an opt-out setting in the future rather than opt-in.

[wilkinsona]

Thanks for the suggestion, @msridhar.

With this implemented, I think I may have found a contract-checking bug triggered by this code:

/**
 * Parse certificates from the specified string.
 * @param text the text to parse
 * @return the parsed certificates
 */
@Contract("!null -> !null")
static @Nullable List<X509Certificate> parse(@Nullable String text) {
	if (text == null) {
		return null;
	}
	CertificateFactory factory = getCertificateFactory();
	List<X509Certificate> certs = new ArrayList<>();
	readCertificates(text, factory, certs::add);
	Assert.state(!CollectionUtils.isEmpty(certs), "Missing certificates or unrecognized format");
	return List.copyOf(certs);
}

Compilation fails:

PemCertificateParser.java:63: error: [NullAway] Method parse has @Contract(!null -> !null), but this appears to be violated, as a @Nullable value may be returned when parameter text is non-null.
                        return null;

I see similar problems for other !null -> !null contracts:

PemContent.java:164:

PemContent.java:164: error: [NullAway] Method of has @Contract(!null -> !null), but this appears to be violated, as a @Nullable value may be returned when parameter text is non-null.
                return (text != null) ? new PemContent(text) : null;

ConfigurationProperty.java:131:

ConfigurationProperty.java:131: error: [NullAway] Method of has @Contract(_, !null -> !null), but this appears to be violated, as a @Nullable value may be returned when parameter value is non-null.
                        return null;

ConfigurationProperty.java:140:

ConfigurationProperty.java:140: error: [NullAway] Method of has @Contract(_, _, !null, _ -> !null), but this appears to be violated, as a @Nullable value may be returned when parameter value is non-null.
                        return null;

ConfigurationPropertiesBean.java:292:

ConfigurationPropertiesBean.java:292: error: [NullAway] Method create has @Contract(_, _, !null -> !null), but this appears to be violated, as a @Nullable value may be returned when parameter bindTarget is non-null.
                return (bindTarget != null) ? new ConfigurationPropertiesBean(name, instance, bindTarget) : null;

DefaultApplicationContextFactory.java:87:

DefaultApplicationContextFactory.java:87: error: [NullAway] Method getFromSpringFactories has @Contract(_, _, !null -> !null), but this appears to be violated, as a @Nullable value may be returned when parameter defaultResult is non-null.
                return (defaultResult != null) ? defaultResult.get() : null;

CollectionToDelimitedStringConverter.java:63:

CollectionToDelimitedStringConverter.java:63: error: [NullAway] Method convert has @Contract(!null, _, _ -> !null), but this appears to be violated, as a @Nullable value may be returned when parameter source is non-null.
                        return null;

Apologies if we've misunderstood the contract syntax but these all look like false-positive violations and may have the same underlying cause. Please let me know if we should open a NullAway issue for this.

[wilkinsona]

I'm wondering if we've misunderstood @Contract. NullAway reports an error for this method which seems pretty straightforward in terms of nullability:

@Contract("!null -> !null")
public static @Nullable Integer of(@Nullable String text) {
	if (text != null) {
		return Integer.parseInt(text);
	}
	else {
		return null;
	}
}

These variants also report a contract violation:

@Contract("!null -> !null")
public static @Nullable Integer of(@Nullable String text) {
	if (text != null) {
		return Integer.parseInt(text);
	}
	return null;
}

@Contract("!null -> !null")
public static @Nullable Integer of(@Nullable String text) {
	return (text != null) ? Integer.parseInt(text) : null;
}

Both of these do not report a violation:

@Contract("null -> null")
public static @Nullable Integer of(@Nullable String text) {
	if (text != null) {
		return Integer.parseInt(text);
	}
	else {
		return null;
	}
}

@Contract("null -> null; !null -> !null")
public static @Nullable Integer of(@Nullable String text) {
	if (text != null) {
		return Integer.parseInt(text);
	}
	else {
		return null;
	}
}

[msridhar]

This looks like a NullAway bug at first glance. Sorry about it. I may not be able to fix immediately, so for now I suggest disabling contract checking. If you could open a bug on the NullAway repo I can take a look when I have time.

[wilkinsona]

Thanks, @msridhar. I've opened uber/NullAway#1440.