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/linter#4752 (comment) (and that fix has shipped in an sdk).

[devoncarew]

@pq @lrhn - from the discussion here: dart-lang/linter#4752 (comment), 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?

[lrhn]

I'd keep the lint, restricted to only warning about discrepancies in pairs of instance field/initializing formal (one, but not both, deprecated).

It's a heuristic lint, it can have false positives, and probably false negatives. But it also applies rarely (requires something to be deprecated) and is easy to turn off (add an // ignore), so the hint that something might be wrong is considered valuable enough to make the false positives worth it. (I hope.)

I want the lint description to say clearly that there can be false positives. In the (presumed) rare case of a field/initializing formal pair where you deprecate one and intend to keep the other, using // ignore to silence the warnings is intended. That's considered OK because the // ignore should go away when the deprecated declaration is eventually removed. We should make sure to warn about unnecessary //ignores!

(I don't want someone who uses // ignore to get dinged in code-reviews. They should be able to refer to the documentation and say that thye are using // ignore as intended, and the code, including the // ignore comment, is idiomatic. Definitely don't want someone with a style-guide disallowing // ignore comments completely try to get the lint changed, or another feature added to turn off the warning instead of just using // ignore. They can turn the lint off, or use // ignore comments as intended, those are the supported options. Anything else is a problem of their own making.)

This is a lint about two (or more) places in the code, so we should be clear on how they interact, including where // ignores can go. The "instance variable"/"initializing formal for that variable" relation is a one-to-many relation when the class has more than one constructor with an initializing formal for that field.

If the field is deprecated, it should cause a warning for every non-deprecated initializing formal. That one's simple.

If the field is not deprecated, but some initializing formals are, we have two choices:

• If one initializing formal is deprecated, give a warning.
• Only if all initializing formals are deprecated, give a warning.

The latter is "safer" (fewer warnings), but maybe not deprecating the other initializing formals was also a mistake.
There is a discrepancy. We do allow false positives.
So I'd go with the former (which is probably the current implementation, if so, yey!)

That will also means that we can treat each pair of (variable, initializing formal) independently
(although we should probably consider not generating too many individual warnings if a field has many initializing formals, maybe only one per variable declaration.)

Then we should make sure that the // ignores work as intended.

An // ignore at the instance variable declaration should turn off warnings for all initializing formal parameters, no matter which of them is deprecated.

An // ignore on an initializing formal parameter should only turn off the warning for itself (the pair consisting of it and the variable), and should not affect any other initializing formal which has a discrepancy wrt the same variable.

I'd allow the // ignore: deprecated_consistency to be applied to either the line declaring the variable/initializing formal or to the @deprecated/@Deprecated(...) annotation. (I'd probably prefer having the comment after the annotation, which is usually on a line by itself, and where a traiing // ignore: ... won't be moved around by dartfmt.)

Then the guide would be:

Warns if an instance variable is deprecated and an initializing formal for that variable is not, or vice-versa.

If a class intends to deprecate only one of the two, it should add an // ignore: deprecated_consistency comment
to either the instance variable declaration (either to the declaration line or to the @deprecated/@Deprecated(...)
annotation line), or to the initializing formal declaration.
Examples:

class Example {
  // ignore: deprecated_consistency
  @deprecated
  final int goingAway;

  @deprecated // ignore: deprecated_consistency
  final int goingAwayToo;

  @deprecated
  // ignore: deprecated_consistency
  final int goingAwayThree;

  @deprecated
  final int goingAwayFour;  // ignore: deprecated_consistency

  // ignore: deprecated_consistency 
  final int stillHere;
  final int stillHereToo;  // ignore: deprecated_consistency 
  final int stillHereThree;
  final int stillHereFour;

  Example(
    // ignore: deprecated_consistency
    this.goingAway, 
    this.goingAwayToo, // ignore: deprecated_consistency
    this.goingAwayThree,
    this.goingAwayFour,
    // ignore: deprecated_consistency
    @deprecated 
    this.stillHere,
    @deprecated // ignore: deprecated_consistency 
    this.stillHereToo,
    @deprecated 
    // ignore: deprecated_consistency
    this.stillHereThree,
    @deprecated
    this.stillHereFour,  // ignore: deprecated_consistency 
  );
}

Without the // ignore: deprecated_consistency comments, every pair of
field and initializer would cause a warning.
It's only necessary to put a comment on one of the variable or initializing formal
to avoid a warning, here all of the possible positions are shown.

If a class has multiple constructors with initializing formals for the same variable,
each initializing formal is compared to the variable independently.
Putting an // ignore: deprecated_consistency on the variable turns off warnings
for all initializing formals, whether the variable or the initializing formals are
the ones that are deprecated.
Putting the comment on the initializing formal only turns off warnings
for that one initializing formal.

WDYT?

[bwilkerson]

We should make sure to warn about unnecessary //ignores!

I have a CL that introduces such a check (first uploaded in March of 2021), that has not yet been able to land for various reasons that I won't go into here. Suffice it to say that warning about unnecessary ignores presents some social challenges. (Happy to talk about it more, but maybe in a different thread.)

This is a lint about two (or more) places in the code, so we should be clear on how they interact, including where // ignores can go.
...
Then we should make sure that the // ignores work as intended.

I'm not saying that we couldn't enhance the current implementation to support what you're proposing, but the implementation we have today requires that the ignore comment be on either the same line as the beginning of the diagnostic's highlight region, or by itself on the line before the beginning of the highlight region.

[lrhn]

..., that has not yet been able to land for various reasons that I won't go into here.

Let's hope we can do something. :)

... the implementation we have today requires that the ignore comment be on either the same line as the beginning of the diagnostic's highlight region, or by itself on the line before the beginning of the highlight region.

So commenting the @override is out. (I'm assuming it's not part of the region. If it was, things would get much weirder than they are today.)

This diagnostic relates to two places in the code, the variable declaration and the initializing formal, being out of sync. I'm assuming we only make one diagnostic for it (or if we make two, we'll need two ignores).

It's probably better to always report it at the initializing formal, because there can be more than one of those,
which means you can, and must, ignore each one of those independently. Annoying, but doable. The assumption is that's it's going to be rare to need an ignore, and in those cases, the first few steps of the necessary refactoring will likely remove the warning, leaving only a deprecated getter backed by something else. So not a big issue.

An initializing formal is usually not that long, and easy to break onto a line of its own, so it should be easy to add a comment to that line, even if it might mean reformatting parameter list to give it a new line.
So the analysis server can insert a suitable comment, which won't be moved by dartfmt. That's probably good enough.

SGTM.

Aka:

If an initializing formal is deprecated, and the instance variable it's initializing is not, or vice versa,
the deprecation is considered inconsistent, and a warning is emitted for the initializing formal.

If the design is deliberate, and one of the two will be removed while the other stays in the API,
an // ignore: deprecated_consistency comment should be added to the initializing formal,
either at the end of the line where the initializing format starts, or alone on the line before.

Using an // ignore: deprecated_consistency comment is the recommended way to signal
that the initializing formal and instance variable are not both going away.
The comment will become unnecessary as soon as the deprecated member has been removed,
and possibly earlier if the class is refactored to prepare for removing the deprecated declaration.

Example:

class Example {
  @deprecated
  final int goingAway;

  final int hereToStay;

  Example.v1(
      // ignore: deprecated_consistency
      this.goingAway,
      @deprecated
      // ignore: deprecated_consistency
      this.hereToStay);

  Example.v2(
      this.goingAway, // ignore: deprecated_consistency
      @deprecated this.hereToStay // ignore: deprecated_consistency
      );

  // No warnings, everything is consistent.
  Example.v3(@deprecated this.goingAway, this.hereToStay);
}

This should be enough. Again, it's not going to be used often, it's likely that the comment goes away before the deprecated declaration, if any refactoring is done in anticipation.
All in all, a short-lived // ignore to opt out of a warning for something which otherwise has a good chance of being a mistake, and it's easy to remove the need for the ignore (just change the initializing formal to a normal parameter, and initialize in the initializer list, if it's that important).

[bwilkerson]

I'm assuming we only make one diagnostic for it (or if we make two, we'll need two ignores).

Correct. Given

class B {
  @Deprecated('soon')
  Object? field;

  B({this.field});
}

there is a single diagnostic and the highlight covers this.field.

It's probably better to always report it at the initializing formal, because there can be more than one of those ...

That was our reasoning too.

An initializing formal is usually not that long, and easy to break onto a line of its own, so it should be easy to add a comment to that line, even if it might mean reformatting parameter list to give it a new line.

While that's possible, it isn't necessary. Either of the following solutions will also work:

class B {
  @Deprecated('soon')
  Object? field;

  // ignore: deprecated_consistency
  B({this.field});
}

or

class B {
  @Deprecated('soon')
  Object? field;

  B({this.field}); // ignore: deprecated_consistency
}

[devoncarew]

Given the relative value we get from this lint, and the work necessary to fix it, we're unlikely to add this to our recommended lint set. Closing.