@Deprecated should warn if you're overriding a deprecated method

[tvolkert]

Consider the following code:

class Foo {
  @protected
  @Deprecated('Do not override this; override betterFoo() instead.')
  void foo() {}

  @protected
  void betterFoo() {}
}

class Bar extends Foo {
  @override
  void foo() {}
}

The fact that Bar is overriding a deprecated method does not trigger an analyzer hint code (_the closest hint code that exists is DEPRECATED_MEMBER_USE, but that's for call-sites, not override-sites_). This makes it almost impossible for classes to deprecate protected APIs and get subclasses to migrate away from extending the deprecated API.

We should add a new hint code, DEPRECATED_OVERRIDE, to cover this case.

[bwilkerson]

As I understand it, a common use case of the deprecated annotation is to mark a method as one that is slated to be removed while continuing to support the use of the method so that clients of the package have some period of time to convert their code to stop using the deprecated method. In order to continue to support the use of a method it is often necessary to leave the overridden implementations of the method.

While it is sometimes possible to re-implement the deprecated method in terms of its replacement, that isn't always possible, such as when there is no direct replacement. In cases where it is possible, you definitely want to remove the overrides because they break the implementation, and wanting analysis support to enforce that is reasonable. But it seems to me that disallowing an override of a deprecated method that can't be re-implemented would be doing our users a disservice.

As an alternative, we should consider adding a separate annotation (presumably in meta) that will prevent any overrides of a method or field (doNotOverride, nonOverridable?). Not only would that preserve an important use case for deprecated, but it would also provide an annotation that might be useful even for methods that are not deprecated.

[tvolkert]

See https://github.com/flutter/flutter/issues/63269 for details of the specific use case that spawned this. Basically, we deprecated abstract methods and turned them into non-abstract stubs that throw. Then we created new methods that were direct replacements for the original methods, and that delegated to the deprecated ones (so as to call subclasses existing implementations). In this setup, we needed a way to tell subclasses "you should be switching from overriding the old methods to the new ones".

[lrhn]

The @Deprecated(...) annotation was deliberately not made to be implicitly inherited by subclass members. That makes it possible to deprecate a method in a superclass, but keep the the methods in one or more subclasses (say, if you figure out that not all Floops can doop, so you deprecate Floop.doop, but keep the doop method on the DoopyFloop subclasses. Or something).

[tvolkert]

That use case seems like a stretch to me - seems like it'd be less cognitive overhead for clients to just create a new method in DoopyFloop.

[jakemac53]

Could we make the existing Deprecated class configurable here? The user of the annotation probably knows what is best for their situation. Specifically I would suggest adding a bool allowOverrides = true named parameter. That would retain the existing behavior but allow for people to get the behavior of also warning for overrides of the deprecated method?

[felangel]

Could we make the existing Deprecated class configurable here? The user of the annotation probably knows what is best for their situation. Specifically I would suggest adding a bool allowOverrides = true named parameter. That would retain the existing behavior but allow for people to get the behavior of also warning for overrides of the deprecated method?

This would be amazing -- I'm currently trying to deprecate methods on an abstract class and want it to be extremely obvious to end users that the methods they are overriding are deprecated. With the current implementation most people would likely not notice until the next major release when the deprecated methods no longer exist.

[jorgecoca]

Agreed with @felangel and @jakemac53 This is a critical use case for library developers: you might want to deprecate an API that requires the consumer to provide an implementation of an abstract class, but if the @Deprecated annotation does not surface that warning up to the developer, then it makes the migration/deprecation process much harder.

[Levi-Lesches]

To make this explicit, there are two places where the analyze can help here:

class A { 
  @Deprecated("Hola") 
  void test() { } 
}

class B extends A { 
  @override 
  void test() { }  // <-- overriding a deprecated member: nothing
}

void main() { 
  A().test();  // <-- using a deprecated member: info
  B().test();  // <-- using an overridden deprecated member: nothing
}

These sort of need two separate approaches:

1. Overriding a deprecated member: If the package maintainers don't have control over the class they're inheriting from, they can either switch to use the new code in the @Deprecated annotation, or they might have to continue maintaining the deprecated override until they can create and publish a new approach. Similarly, users overriding deprecated members in their own code might be wasting their time by overriding members that shouldn't be used.
2. Using an overriden deprecated member: If the subclass being used provides another way to do the same thing, users should use that. Otherwise, even though A.test is deprecated, the author of B might decide that B.test is still relevant, and thus users should not be warned.

My two cents are that subclasses overriding deprecated members should get a warning by default, which signals they need to evaluate the situation. If they can use the alternative provided by the @Deprecated annotation, they should do so and slap a @Deprecated on the subclass warning users of the subclass to do the same. If not, they can decide to keep their current implementation, and simply remove the @override once the superclass removes the member.

[lrhn]

How about making a member deprecated if it overrides a deprecated member and is marked with @override. Also make it a warning to override a deprecated member, have @override and not be @deprecated yourself, but not to override (only) a deprecated member and not have an @override (even with "always annotate overrides").

That forces an overriding member to either opt-in to the deprecation (and thereby promise to also remember their member when the superclass member is removed) or opt-out of the override relation (and keep the member when the superclass member is removed).

In the former case, calling the member on the subclass type will give deprecation warnings.

In the latter case, calling the member on the subclass won't give any warnings. When the superclass member is removed, the code stays valid, it is as if the superclass member was already removed, only with warnings instead of errors (which is basically what deprecation means).

Which means that when a superclass member is deprecated, and it has a subclass member with an @override overriding it:

• First the subclass member gives a warning by itself. We don't yet know whether it will be removed or unchain itself from the superclass member, so we treat all references to it as (potentially) deprecated, and urge the author to make a choice.
• If the subclass member removes @override, its warning goes away. Other code can use it without warning.
• If the subclass member adds @deprecated, its warning goes away. Other code using it still gets warnings.

(All this is fine with annotations, but won't work if we make override a language feature because then it shouldn't be affected by a @deprecated annotation. We may need to make deprecated a language feature too then.)

[felangel]

How about making a member deprecated if it overrides a deprecated member and is marked with @override. Also make it a warning to override a deprecated member, have @override and not be @deprecated yourself, but not to override (only) a deprecated member and not have an @override (even with "always annotate overrides").

That forces an overriding member to either opt-in to the deprecation (and thereby promise to also remember their member when the superclass member is removed) or opt-out of the override relation (and keep the member when the superclass member is removed).

In the former case, calling the member on the subclass type will give deprecation warnings.

In the latter case, calling the member on the subclass won't give any warnings. When the superclass member is removed, the code stays valid, it is as if the superclass member was already removed, only with warnings instead of errors (which is basically what deprecation means).

Which means that when a superclass member is deprecated, and it has a subclass member with an @override overriding it:

• First the subclass member gives a warning by itself. We don't yet know whether it will be removed or unchain itself from the superclass member, so we treat all references to it as (potentially) deprecated, and urge the author to make a choice.
• If the subclass member removes @override, its warning goes away. Other code can use it without warning.
• If the subclass member adds @deprecated, its warning goes away. Other code using it still gets warnings.

(All this is fine with annotations, but won't work if we make override a language feature because then it shouldn't be affected by a @deprecated annotation. We may need to make deprecated a language feature too then.)

The main issue I see with this approach is it would conflict with the existing annotate_overrides lint. It would likely need to be adjusted to ignore cases where the superclass member is deprecated in order for developers to remove the @override from a subclass member (not sure how that plays with making override a language feature) .

[alexeyinkin]

As an alternative, we should consider adding a separate annotation (presumably in meta) that will prevent any overrides of a method or field (doNotOverride, nonOverridable?). Not only would that preserve an important use case for deprecated, but it would also provide an annotation that might be useful even for methods that are not deprecated.

There is @nonVirtual that does exactly that.