Skip to content

Commit

Permalink
Remove suppressions for ShouldNotSubclass.
Browse files Browse the repository at this point in the history
We have disabled the check for our packages internally:

- Often it fires because we're implementing `Future`. It's very true that "normal" code shouldn't implement `Future`, but _someone_ needs to, and that someone is us.
- Sometimes it fires because we're extending a type like `AtomicReference` to shave off an object and some indirection. I won't claim to have data that shows that this is actually justified, but for better or for worse, it's a conscious decision that we're making.
- I also see at least two suppressions for extending `Throwable`. There's probably no compelling reason for this, but we don't actually throw them, and changing them now would carry at least some risk.

Of course, it's possible that `ShouldNotSubclass` would eventually have caught new occurrences that we _would_ have cared about. But historically that doesn't seem to have been the case.

RELNOTES=n/a
PiperOrigin-RevId: 375982730
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed May 26, 2021
1 parent e1cc195 commit 357db07
Show file tree
Hide file tree
Showing 25 changed files with 2 additions and 26 deletions.
Expand Up @@ -65,7 +65,7 @@
* @since 1.0
*/
// we use non-short circuiting comparisons intentionally
@SuppressWarnings({"ShortCircuitBoolean", "ShouldNotSubclass"})
@SuppressWarnings("ShortCircuitBoolean")
@GwtCompatible(emulated = true)
@ReflectionSupport(value = ReflectionSupport.Level.FULL)
public abstract class AbstractFuture<V> extends InternalFutureFailureAccess
Expand Down
Expand Up @@ -284,7 +284,6 @@ enum RunningState {
* properties; for example, calling WeakReference.get() on Android will block during an
* otherwise-concurrent GC cycle.
*/
@SuppressWarnings("ShouldNotSubclass") // Saving an allocation here is worth it
private static final class TaskNonReentrantExecutor extends AtomicReference<RunningState>
implements Executor, Runnable {

Expand Down
Expand Up @@ -36,7 +36,6 @@
*/
@CanIgnoreReturnValue // TODO(cpovirk): Consider being more strict.
@GwtCompatible
@SuppressWarnings("ShouldNotSubclass")
@ElementTypesAreNonnullByDefault
public abstract class ForwardingFuture<V extends @Nullable Object> extends ForwardingObject
implements Future<V> {
Expand Down
Expand Up @@ -30,7 +30,6 @@
* @author Shardul Deo
* @since 4.0
*/
@SuppressWarnings("ShouldNotSubclass")
@CanIgnoreReturnValue // TODO(cpovirk): Consider being more strict.
@GwtCompatible
@ElementTypesAreNonnullByDefault
Expand Down
Expand Up @@ -470,7 +470,6 @@ public static <I, O> ListenableFuture<O> transform(
*/
@Beta
@GwtIncompatible // TODO
@SuppressWarnings("ShouldNotSubclass")
public static <I, O> Future<O> lazyTransform(
final Future<I> input, final Function<? super I, ? extends O> function) {
checkNotNull(input);
Expand Down
Expand Up @@ -29,7 +29,6 @@
@GwtCompatible
@ElementTypesAreNonnullByDefault
// TODO(cpovirk): Make this final (but that may break Mockito spy calls).
@SuppressWarnings("ShouldNotSubclass")
class ImmediateFuture<V extends @Nullable Object> implements ListenableFuture<V> {
static final ListenableFuture<?> NULL = new ImmediateFuture<@Nullable Object>(null);

Expand Down
Expand Up @@ -24,7 +24,6 @@
import java.util.concurrent.locks.LockSupport;
import org.checkerframework.checker.nullness.qual.Nullable;

@SuppressWarnings("ShouldNotSubclass")
@GwtCompatible(emulated = true)
@ReflectionSupport(value = ReflectionSupport.Level.FULL)
@ElementTypesAreNonnullByDefault
Expand Down
Expand Up @@ -97,7 +97,6 @@ public final class JdkFutureAdapters {
* <p>If the delegate future is interrupted or throws an unexpected unchecked exception, the
* listeners will not be invoked.
*/
@SuppressWarnings("ShouldNotSubclass")
private static class ListenableFutureAdapter<V extends @Nullable Object>
extends ForwardingFuture<V> implements ListenableFuture<V> {

Expand Down
Expand Up @@ -115,7 +115,6 @@
* that affects users, especially users of the Android Gradle Plugin, since the plugin developers
* put in a special hack for us: https://issuetracker.google.com/issues/131431257)
*/
@SuppressWarnings("ShouldNotSubclass")
@DoNotMock("Use the methods in Futures (like immediateFuture) or SettableFuture")
/*
* It would make sense to also annotate this class with @ElementTypesAreNonnullByDefault. However,
Expand Down
Expand Up @@ -40,7 +40,6 @@
* @author Sven Mawson
* @since 1.0
*/
@SuppressWarnings("ShouldNotSubclass")
@GwtIncompatible
@ElementTypesAreNonnullByDefault
public class ListenableFutureTask<V extends @Nullable Object> extends FutureTask<V>
Expand Down
Expand Up @@ -27,7 +27,6 @@
*/
@Beta
@GwtCompatible
@SuppressWarnings("ShouldNotSubclass")
@ElementTypesAreNonnullByDefault
public interface ListenableScheduledFuture<V extends @Nullable Object>
extends ScheduledFuture<V>, ListenableFuture<V> {}
Expand Up @@ -823,6 +823,5 @@ protected void doStop() {
}

/** This is never thrown but only used for logging. */
@SuppressWarnings("ShouldNotSubclass")
private static final class EmptyServiceManagerWarning extends Throwable {}
}
Expand Up @@ -115,7 +115,6 @@
* that affects users, especially users of the Android Gradle Plugin, since the plugin developers
* put in a special hack for us: https://issuetracker.google.com/issues/131431257)
*/
@SuppressWarnings("ShouldNotSubclass")
@DoNotMock("Use the methods in Futures (like immediateFuture) or SettableFuture")
/*
* It would make sense to also annotate this class with @ElementTypesAreNonnullByDefault. However,
Expand Down
Expand Up @@ -65,7 +65,7 @@
* @since 1.0
*/
// we use non-short circuiting comparisons intentionally
@SuppressWarnings({"ShortCircuitBoolean", "ShouldNotSubclass"})
@SuppressWarnings("ShortCircuitBoolean")
@GwtCompatible(emulated = true)
@ReflectionSupport(value = ReflectionSupport.Level.FULL)
public abstract class AbstractFuture<V> extends InternalFutureFailureAccess
Expand Down
Expand Up @@ -284,7 +284,6 @@ enum RunningState {
* properties; for example, calling WeakReference.get() on Android will block during an
* otherwise-concurrent GC cycle.
*/
@SuppressWarnings("ShouldNotSubclass") // Saving an allocation here is worth it
private static final class TaskNonReentrantExecutor extends AtomicReference<RunningState>
implements Executor, Runnable {

Expand Down
Expand Up @@ -36,7 +36,6 @@
*/
@CanIgnoreReturnValue // TODO(cpovirk): Consider being more strict.
@GwtCompatible
@SuppressWarnings("ShouldNotSubclass")
@ElementTypesAreNonnullByDefault
public abstract class ForwardingFuture<V extends @Nullable Object> extends ForwardingObject
implements Future<V> {
Expand Down
Expand Up @@ -30,7 +30,6 @@
* @author Shardul Deo
* @since 4.0
*/
@SuppressWarnings("ShouldNotSubclass")
@CanIgnoreReturnValue // TODO(cpovirk): Consider being more strict.
@GwtCompatible
@ElementTypesAreNonnullByDefault
Expand Down
1 change: 0 additions & 1 deletion guava/src/com/google/common/util/concurrent/Futures.java
Expand Up @@ -503,7 +503,6 @@ public static <I, O> ListenableFuture<O> transform(
*/
@Beta
@GwtIncompatible // TODO
@SuppressWarnings("ShouldNotSubclass")
public static <I, O> Future<O> lazyTransform(
final Future<I> input, final Function<? super I, ? extends O> function) {
checkNotNull(input);
Expand Down
Expand Up @@ -29,7 +29,6 @@
@GwtCompatible
@ElementTypesAreNonnullByDefault
// TODO(cpovirk): Make this final (but that may break Mockito spy calls).
@SuppressWarnings("ShouldNotSubclass")
class ImmediateFuture<V extends @Nullable Object> implements ListenableFuture<V> {
static final ListenableFuture<?> NULL = new ImmediateFuture<@Nullable Object>(null);

Expand Down
Expand Up @@ -24,7 +24,6 @@
import java.util.concurrent.locks.LockSupport;
import org.checkerframework.checker.nullness.qual.Nullable;

@SuppressWarnings("ShouldNotSubclass")
@GwtCompatible(emulated = true)
@ReflectionSupport(value = ReflectionSupport.Level.FULL)
@ElementTypesAreNonnullByDefault
Expand Down
Expand Up @@ -97,7 +97,6 @@ public final class JdkFutureAdapters {
* <p>If the delegate future is interrupted or throws an unexpected unchecked exception, the
* listeners will not be invoked.
*/
@SuppressWarnings("ShouldNotSubclass")
private static class ListenableFutureAdapter<V extends @Nullable Object>
extends ForwardingFuture<V> implements ListenableFuture<V> {

Expand Down
Expand Up @@ -115,7 +115,6 @@
* that affects users, especially users of the Android Gradle Plugin, since the plugin developers
* put in a special hack for us: https://issuetracker.google.com/issues/131431257)
*/
@SuppressWarnings("ShouldNotSubclass")
@DoNotMock("Use the methods in Futures (like immediateFuture) or SettableFuture")
/*
* It would make sense to also annotate this class with @ElementTypesAreNonnullByDefault. However,
Expand Down
Expand Up @@ -40,7 +40,6 @@
* @author Sven Mawson
* @since 1.0
*/
@SuppressWarnings("ShouldNotSubclass")
@GwtIncompatible
@ElementTypesAreNonnullByDefault
public class ListenableFutureTask<V extends @Nullable Object> extends FutureTask<V>
Expand Down
Expand Up @@ -27,7 +27,6 @@
*/
@Beta
@GwtCompatible
@SuppressWarnings("ShouldNotSubclass")
@ElementTypesAreNonnullByDefault
public interface ListenableScheduledFuture<V extends @Nullable Object>
extends ScheduledFuture<V>, ListenableFuture<V> {}
Expand Up @@ -875,10 +875,8 @@ protected void doStop() {
}

/** This is never thrown but only used for logging. */
@SuppressWarnings("ShouldNotSubclass")
private static final class EmptyServiceManagerWarning extends Throwable {}

@SuppressWarnings("ShouldNotSubclass")
private static final class FailedService extends Throwable {
FailedService(Service service) {
super(
Expand Down

0 comments on commit 357db07

Please sign in to comment.