Prepare `deprecated_consistency` for `package:lints` inclusion

[pq]

Follow-up from dart-lang/core#760,

• verify the impl. works as expected
• ensure docs for it are correct

[lrhn]

Also, make sure the lint doesn't warn about non-deprecated constructors of deprecated classes.

[pq]

Also, make sure the lint doesn't warn about non-deprecated constructors of deprecated classes.

Right. Looking back to dart-archive/linter#2451, I see a reference to flutter/flutter#75813 which provides the motivation for linting on constructors. There @Piinks observes:

While working on deprecations and dart fixes recently, I found that not all uses of deprecated API were receiving warnings. This is because the annotation must be applied to every use - constructors, parameters and field members.

In retrospect it's really unfortunate that we didn't fix this in the right place. 😬 Anyway, it looks like this has been fixed in the meantime and we do currently report DEPRECATED_MEMBER_USE on undeprecated constructors in deprecated classes. Given that, I think we can remove that case from the lint and current users won't lose any diagnostics.

[pq]

we do currently report DEPRECATED_MEMBER_USE on undeprecated constructors

One clarification, and maybe this is the rub. Given a class like this:

@deprecated
class C {
  C();
  C.named();
}

C is considered deprecated and uses are flagged as follows.

void f() {
  C();
  ^-- diagnostic
  C.named();
  ^-- diagnostic
}

@Piinks can you recall your motivation for flutter/flutter#75813? Where you surprised not to see named constructor invocations flagged? Does flagging the type not seem a sufficient signal to users that they shouldn't be creating an instance of C?

[pq]

@natebosch points out a wrinkle wrt type aliases too in dart-archive/lints#150 (comment).

Specifically, you won't get a deprecation warning on an (unannotated) aliased constructor invocation (though you will see one when you declare the type alias).

@deprecated
class C {
  C();
  @deprecated
  C.namedDeprecated();
}

typedef Renamed = C; // Diagnostic

void main() {
  Renamed(); // No diagnostic
  Renamed.namedDeprecated(); // Diagnostic
}

[Piinks]

@Piinks can you recall your motivation for flutter/flutter#75813? Where you surprised not to see named constructor invocations flagged? Does flagging the type not seem a sufficient signal to users that they shouldn't be creating an instance of C?

IIRC, the issue was that we would deprecate a whole widget or class Foo, but the developers would not get a warning for Foo.bar() named constructor, so they would never be warned to migrate to the new class or widget.

Also I think I remember this coming up more often with field members, such as:

class Foo {
  Foo(this.bar);
  
  @Deprecated('...')
  final String bar;
}

In this case, where the constructor set the value for the property, developers were also receiving no warning.
This made breaking changes really painful since deprecations are meant to give folks a heads up.

[lrhn]

If you deprectate a class, it means that entire class will go away, and therefore every member of it should also be considered deprecated.
If we don't report it as such, then that's a bug.
Invoking a constructor of a deprecated class should definitely give a warning.

Aliasing isn't as simple, though, because it may be deliberate.
We should figure out what the right thing to do is there, and do that.
We shouldn't need lints to make people do something else, that thing should just be the default.

So, a deliberately un-deprecated alias may seem reasonable if you plan to rename a class, to introduce the new name before actually removing the old.
And then it's precisely because you want that name, and all member accesses through that name, to keep working.

The alias declaration itself will get the deprecation warning when it references the old class name, so you have a chance to recognize that you may want to deprecate the alias too. So maybe it's reasonable to assume that an alias for a deprecated class, which is not itself deprecated, represents a promise to keep that name working. Or maybe only if the warning is //ignored.
(Or at least if it's in the same library/package?)
At the API level, the API exposes both names, only one is deprecated. If you only remove the class, not the alias (you might just make the class private, and keep aliasing it with a public name), then it's correct that only one of them is deprecated.

But if you have an alias for a class, and deprecate one member of that class, I'd expect the alias to reflect that deprecation. An alias for a class which still exists is distinguishable from a separate class, so it would itself be breaking to only remove the member from one - because that would mean it's no longer an alias. An alias for a class which no longer exists (publicly) is indistinguishable from just a class.

But (again) I don't expect the analyzer to track whether a variable is typed as the class directly, or through an alias, so even if we special case direct accesses through an alias, it shouldn't, and cannot, affect member accesses. And that's inconsistent. You can't just use AliasName foo = AliasName(); foo.member();, because the member is being called on a static type of OriginalName, and OriginalName.member is just as deprecated as OriginalName is.
(It may be that we don't report that, if we consider the warning at the declaration of, effectively, OriginalName foo sufficient, but if we omit that warning because it uses the alias, we'd have to report foo.member(). Maybe.)

My suggestion is that an alias for a deprecated class is treated as just as deprecated, because any use of the alias is really is just an indirect use of the deprecated class.

That would make typedef Renamed = C; ... Renamed(); report a warning, because the Renamed.new constructor, aka C.new, is deprecated. So is Renamed itself.

I'm not absolutely sure about this. But I would be, if I knew that the alias was not written by the same person as the deprecated class, and I have no way of knowing whether it is.

If you want to have your class, and deprecate it too, you should give the class the new name, then make the old name the alias, and deprecate the typedef declaration. Then any reference to the old name is deprecated, but using the new name is not. When that alias goes away, the state precisely matches what the warnings were warning about.

[pq]

Thanks @lrhn!

Tracking transitive member deprecation warnings in #53587. (Still mulling over the aliasing case -- thanks for thinking it through; most appreciated.)

[scheglov]

class A {
  void foo() {}
}

class B extends A {}

How does migration process work?
We deprecate A because we want to fold it into B.
We deprecate A, the foo invocations even though B get highlighted.
We cannot yet remove foo from A because there are clients of A which we want to switch to B.
So, we add B.foo() that invokes A.foo. But this is deprecated, so we get super.foo() in B highlighted.
Which we have to // ignore, right?

Currently we have to // ignore only A in class B extends A {}, not every class member.

Oh, not just ignore, but also create such pass-through class member declarations in B.
It seems quite punishing for the person doing deprecation and migration :-)

[scheglov]

If we want to rename a class, it is fine to have a deprecated alias of it.

@deprecated
typedef OldName = NewName;

class NewName {}

If you had an alias of a class (why?), then its deprecation should be the same as the deprecation of the class.
So, clients will get a signal that the class is deprecated, even if used via alias.

@deprecated
class OriginalName {}

@deprecated
typedef AliasName = OriginalName;