Consider adding `deprecated_consistency` to recommended

[goderbauer]

It's frustrating to remove something that you think was properly deprecated only to discover that many users of it never saw the deprecation warning. Enabled in Flutter.

[mit-mit]

This seems like a tooling issue: Any use of a member or constructor of a class that is marked deprecated should be flagged.

[mit-mit]

@mit-mit to file tools issue

[mit-mit]

@goderbauer do you have an example of a case where you are not seeing a warning?

I tried to create a class marked deprecated, and I get usage warnings on both static method calls and on constructor calls.

[goderbauer]

@Piinks Do you remember where we saw the inconsistencies around deprecation that let us to adopting the deprecated_consistency lint?

[Piinks]

Yes, say this is a (rough) API

MyWidget(
  this.myProperty
);

final int myProperty;

If you do not put a deprecation annotation on both (the field member and the constructor parameter), then the user will not get a warning from the analyzer for the use case that is not deprecated.

Further examples:

MyWidget(
  @Deprecated(...)
  this.myProperty
);

// Users that access this property will not be warned
final int myProperty;

MyWidget(
  // Users of this parameter will not be warned
  this.myProperty
);

@Deprecated(...)
final int myProperty;

The lint makes sure both get the annotation.

MyWidget(
  @Deprecated(...)
  this.myProperty
);

@Deprecated(...)
final int myProperty;

[mit-mit]

cc @pq, is that fixable?

[bwilkerson]

Just to clarify, I think what you're asking is whether, at least for the purposes of deprecation warnings, we can consider fields and field initializing parameters to be equivalent. That is, if either is deprecated then we consider both to be deprecated.

Personally, I don't have any problem with that, but we might want to update the documentation for deprecated to make this association clear. @lrhn

[lrhn]

I can see hypothetical cases where you want to deprecate one, and not both.

Say, you want to make the field a getter computed from something else, and drop the parameter,
or want to make the field private, but keep the parameter.

So, I don't want to treat the two as equivalent, where annotating one will also deprecate the other.
Rather, I'd want to give a warning if you annotate one, but not the other, which is what the deprecated_consistency lint does.

In practice, I think it's fine to temporarily use an //ignore comment in any exception cases (it'll go away when the deprecation does), because most of the time, people really do mean to make both go away, but forgets (typically) the parameter.

That's a tentative "yes" from me to recommend the lint, and "no" to linking the deprecations, because then there is no way out of it if you really only mean to deprecate one.

The lint description is:

DO apply @Deprecated() consistently:

• if a class is deprecated, its constructors should also be deprecated.
• if a field is deprecated, the constructor parameter pointing to it should also be deprecated.
• if a constructor parameter pointing to a field is deprecated, the field should also be deprecated.

The first item is unnecessary. If a class is deprecated, every member of it is also deprecated.
(Deprecation has precisely one meaning: Warn about code which would stop being valid if the deprecated declaration goes away. If a class goes away, so does its constructor. There should be no need for also deprecating the constructor.)
The lint should not require a deprecation on the constructor. If that deprecation makes any difference, it should be considered a bug in the analyzer.

The two other items are reasonable heuristics. As mentioned above, there can be false positives, but it's likely not a big issue. The lint documentation should say what to do in that case (use // ignore until the deprecated item goes away).

[devoncarew]

This has been approved for the recommended set.

[devoncarew]

An update - this is still approved, but we'll delay adding this to the lint set until after we have a fix for dart-lang/sdk#59302 (and that fix has shipped in an sdk).

[devoncarew]

@pq @lrhn - from the discussion here: dart-lang/sdk#59302, are we considering this lint not necessary - that we should bake a version of deprecated_consistency into the warnings that we normally issues for deprecated members?