[2.4.0] Consider splitting JdkObsolete into individual checks

[boris-petrov]

Description of the problem / feature request:

A few false-positives for JdkObsolete.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

import java.net.NetworkInterface;

Enumeration<NetworkInterface> networkInterfaces = NetworkInterface.getNetworkInterfaces();
while (networkInterfaces.hasMoreElements()) {
	NetworkInterface networkInterface = networkInterfaces.nextElement();
}

This gives a JdkObsolete warning about Enumeration. Well, NetworkInterface.getNetworkInterfaces is JDK code, I can't do anything about it. :) ErrorProne should ignore enumerations that come from code that is not mine.

Date someDate = someMethodReturningADate();
Instant instant = someDate.toInstant();

The second line (that is calling toInstant) is marked with a JdkObsolete warning. Well, I'm literally migrating away from a Date at that point so an error should not be triggered I guess. I would think that only new Date should be marked as JdkObsolete not some random usages of Date which I got from somewhere else.

What version of Error Prone are you using?

2.4.0

Have you found anything relevant by searching the web?

Nope.

P.S.

import javax.mail.internet.MimeMessage;

MimeMessage message = new MimeMessage(session);
message.setSentDate(new Date());

I have to pass a Date here, that's what the API expects.

[Stephan202]

Relates to #1610. The common theme seems to be that the check should match in fewer places, roughly speaking only when:

1. the obsolete type is explicitly constructed, except in order to satisfy an API external to the compilation unit; or
2. the obsolete type is used as a field/parameter type; or
3. the obsolete type is extended.

That first constraint seems rather fuzzy, and the second constraint might require one to introduce additional conversions. :thinking:

[cushon]

except in order to satisfy an API external to the compilation unit

Arguably there's still some signal in that case, it might be worth checking to see if there's a newer version of the library that doesn't use the discouraged type etc., and then use an explicit @SuppressWarnings if the type is still necessary. I realize the other cases are probably more valuable, though.

[codylerum]

com.google.common.collect.TreeBasedTable#rowKeySet returns SortedSet rather than NavigableSet

Perhaps these can be excluded?

[mernst]

I encountered the same problem.

A library I maintain contains an API that takes an Iterator argument. These APIs cannot be removed, because doing so would break backward compatibility. I get a lot of "JdkObsolete" warnings -- one for every method call within those methods' implementations. These are weird places to issue a warning, for a method that takes an Iterator as an argument. If you want Error Prone to reject such code, why not issue a single warning on the formal parameter?

However, I think it would be even better to warn only when code creates an Iterator (that is, a new expression). That way, Error Prone doesn't penalize code that is interfacing with legacy code that uses Iterator.

This is similar to a suggestion above, but I argue that item 2 on that list is not necessary.

[carterkozak]

To add additional signal, this flags on some java standard library functionality that has existed for a long time, and isn't worth breaking, for example NetworkInterface.getInetAddresses() returns an Enumeration, but it's burdensome to suppress all callers.

[mernst]

Here are some other JDK classes with methods that return an Enumeration. Clients cannot do anything about this, and it is burdensome to suppress warnings at every use of the returned Enumeration.

• java.util.jar.JarFile
• java.util.zip.ZipFile
• ClassLoader.getSystemResources

[mernst]

The suggestion to replace Stack by ArrayDeque is not always possible. For example, Stack contains elementAt() but Deque does not.

[mernst]

Another problem with replacing stacks by deques is different iteration order. For example, new ArrayList<>(myStack) returns a list with elements in different order than new ArrayList<>(myDeque), if myStack and myDeque are both treated as stacks, using the translation of methods suggested at https://errorprone.info/bugpattern/JdkObsolete. In other words, following that advice is a breaking behavioral change.

[eraneverlaw]

We were in the process of eliminating all Error Prone warnings (by fixing them, or suppressing where appropriate) and turning many of them into Errors. JdkObsolete was one of the best, but we just upgraded to 2.4.0 and it suddenly added all those warnings (errors) about Date. We had to downgrade it back to a warning, but I think we might be better off turning it off all together, which is a shame!

I don't think warning patterns should be modified over time except to fix bugs. It's not backwards compatible. I believe the Date warning should have been incorporated as a new pattern, rather than piggy back on JdkObsolete, despite the sort of "generic" name of JdkObsolete.

[kluever]

@eraneverlaw warnings can easily be modified over time because they are backwards compatible. Modifications to errors (on the other hand), are not backwards compatible. It sounds like you chose to upgrade JdkObsolete to an error.

The Date API is really bad. Is there a reason you don't want to fix your usages?

[eraneverlaw]

@eraneverlaw warnings can easily be modified over time because they are backwards compatible. Modifications to errors (on the other hand), are not backwards compatible. It sounds like you chose to upgrade JdkObsolete to an error.

The Date API is really bad. Is there a reason you don't want to fix your usages?

Adding potentially unlimited number of new warnings (possibly hundreds in our code base) in a new release, is a recipe for a "warnings overload," in which case they are just ignored, including all other warnings. It causes an unstable development process in that sense, even if it compiles.

Our idea was to eliminate all the existing warnings and then prevent them from being continually introduced in our code, by upgrading the warnings into errors. That is a stable solution to the warnings overload, but it's impossible to achieve if new versions of the compilers consider new things as warnings under existing patterns.

It's a shame since it means we might continue to add usages of LinkedList or Stack when an ArrayDeque is preferable. Or who knows, even more StringBuffers!

We definitely will want to look at our usages of Date but at our own pace and available resources.

Another way to look at it - one of the best things about Error Prone, is its granularity of choice of what to ignore (even temporarily,) what to consider warnings, and what to consider errors, but this choice to overload cases to an existing pattern, breaks decisions that depend on that granularity.

P.S. upgrading warnings to errors is also a feature of Error Prone, yet in this case it in itself is not backwards compatible - requires to at least downgrade, in order to compile successfully with the new version.

[kluever]

Re. "warnings overload" - you must face this when upgrading your JDK and running into new deprecations, no? How do you manage that?

I suppose we could add some degree of configurability to JdkObsolete (e.g., pass a flag of the class names for which you want to disable warnings), but then you're just hiding the problem (since new usages of Date will not be caught).

I'm not sure what the right solution is, other than creating a separate checker for each of the "obsolete" APIs in the JDK (and deleting JdkObsolete. That's a bit messy, but we theoretically could do that...

[carterkozak]

I like the idea of separate obsolescence checks for each type, we’ve done similar migrations using error-prone alias functionality to cover existing suppressions.

For my teams usage I don’t have a strong opinion about broadening existing checks, but I’d prefer that suppressions are as specific as possible and don’t allow new unintentional violations to appear in future changes.

[kluever]

FYI, the java.util.Date handling has already been split out of JdkObsolete. See https://github.com/google/error-prone/commit/d450119b23e9c7c68fcfbb3927a3250493ac3ad3#diff-065165cb9efc7cfdb26debe40d8ea8cc

[eraneverlaw]

Re. "warnings overload" - you must face this when upgrading your JDK and running into new deprecations, no? How do you manage that?

I suppose we could add some degree of configurability to JdkObsolete (e.g., pass a flag of the class names for which you want to disable warnings), but then you're just hiding the problem (since new usages of Date will not be caught).

I'm not sure what the right solution is, other than creating a separate checker for each of the "obsolete" APIs in the JDK (and deleting JdkObsolete. That's a bit messy, but we theoretically could do that...

The deprecation is indeed a problem I have no solution to.

I like both of those ideas.

[eraneverlaw]

FYI, the java.util.Date handling has already been split out of JdkObsolete. See d450119#diff-065165cb9efc7cfdb26debe40d8ea8cc

Thanks! Do you know when it will be released?

[eraneverlaw]

BTW, another issue - It seems, amazingly, that Hibernate still doesn't support NavigableSet and NavigableMap, 15 years after they were introduced? https://forum.hibernate.org/viewtopic.php?f=1&t=1009169&view=previous

This is a slight impediment for another group of new warnings added to JdkObsolete, but I guess it can always be suppressed locally.

I don't blame engineers for declaring the static types of TreeSet instances as SortedSet, if only since that name is much more straight forward than NavigableSet - obviously a pretty steep price paid for keeping SortedSet backwards compatible with existing implementations.

EDIT: This is actually more than a slight impediment. If you have a method getSomeSet that returns a SortedSet (because it is a Hibernate getter, say) - it's not enough to suppress the JdkObsolete warning on the method itself - Every call to that method also gets the warning, that has to be suppressed. e.g.: if (getSomeSet().isEmpty()) { ... }

All those suppressions also incur a cost on code quality (readability)

[cushon]

re: if (getSomeSet().isEmpty()) { ... }, that change that added java.util.Date also added (or re-added?) handling to report findings in invocations of depreated types. When I moved the Date stuff to a separate check in d450119b23e9c7c68fcfbb3927a3250493ac3ad3 I also made JdkObsolete only report direct uses of the type, not just invocations of methods on 'obsolete' types.

i.e. we report a diagnostic for new SomeObsoleteType(), but not for someObsoleteType.someMethod(), so you won't get a diagnostic for isEmpty() anymore.

[hakanai]

We were getting this on Enumeration usages too, so I thought I'd try to migrate away from it, but it turns out even Enumeration#asIterator() gets flagged as obsolete, and as far as I can tell, that's the right thing to migrate to.