Declaration isn't referenced review for abstract private declarations

[FMorschel]

Edit:

See #54374 (comment) for a better explanation of what this issue is about.

Old:

Describe the issue
In the below example, I'd like for someone to please explain to me why this lint is being triggered in N._fn, since it is a protected method that can only be called from within a library, this lint should potentially know that.

Also, in a class like C, I'm not sure this should be triggered. However, I'm not aware if this lint can be aware of other members in the super method it is being overridden, because if it is aware, of them, then in C I don't believe it should be triggered. Still, probably in D but that would also bring a question as to why would this be called and not maybe warn that the mixin is unused instead.

Maybe the "Unused-Mixin" could be called when the mixin is a private mixin like in E?

To Reproduce

mixin _M {
  void _fn();
}

mixin M {
  void _fn(); // The declaration '_fn' isn't referenced.
}

mixin N implements M {
  @override
  void _fn(); // The declaration '_fn' isn't referenced.
  void a() {}
}

class A with N {
  void fn() {
    _fn();
  }
  
  @override
  void _fn() {}
}

class B with N { } // Missing concrete implementation of 'N._fn'.

class C with N {
  void _fn() {} // The declaration '_fn' isn't referenced.
}

class D with M {
  @override
  void _fn() {} // The declaration '_fn' isn't referenced.
}

class E with _M {
  @override
  void _fn() {}
}

Additional context
I'm really out of my depth here, but maybe someone can help me make my questions clearer.

[srawlins]

I'm a little confused about the question, but we can dig in. N._fn is never called. Can you point to a spot where it is called?

C._fn is also never called. Can you expand your repro to a file where C._fn is called, but the warning is still reported?

I don't really understand what your comment is about D. It looks identical to C (but with an annotation, which does not come into play).

The thread gets too spread out for me after that. 😄 . I won't address your comments about E here (or, yet), to keep the issue to one topic.

[FMorschel]

I'm a little confused about the question, but we can dig in. N._fn is never called. Can you point to a spot where it is called?

It is called at A:

class A with N {
  void fn() {
    _fn();
  }
  
  @override
  void _fn() {}
}

C._fn is also never called. Can you expand your repro to a file where C._fn is called, but the warning is still reported?

This one is working as intended, added it there as a reference (more explanations below).

I don't really understand what your comment is about D. It looks identical to C (but with an annotation, which does not come into play).

The difference is that while C uses N and D uses M.

My point here was: that while N actually has a public method that could be called somewhere else, M does not, so maybe there could be a warning about "unused mixin" here? Or at least with E where even the mixin is private and still has no references to it.

The thread gets too spread out for me after that. 😄 . I won't address your comments about E here (or, yet), to keep the issue to one topic.

Sorry about that, didn't mean for it to have so many questions and examples in one case. This issue should probably be split into more than one, if you can help me reason it to make sure that the questions are clearer I'd really appreciate it.

[srawlins]

It is called at A:

class A with N {
  void fn() {
    _fn();
  }
  
  @override
  void _fn() {}
}

N._fn is not called in this code. A._fn is.

[srawlins]

This issue should probably be split into more than one, if you can help me reason it to make sure that the questions are clearer I'd really appreciate it.

Yeah I think "unused mixin" is something we haven't really looked into. Could be a reasonable diagnostic.

[FMorschel]

N._fn is not called in this code. A._fn is.

I see that, but also there is no way for me to call N._fn since it is an abstract method, should I always remember to add the ignore in that case then?

For me, as a user, it doesn't seem like the lint should be triggered there. Only if it was never implemented anywhere (in the case of abstract declarations).

Edit:
Once the implementations are all gone (not used anymore, for example), then I believe this should be triggering.

Since it can only be used inside the same library, in my company and personal projects we use this kind of declaration to help us remember some specific names for methods when we want one of our related classes to do a specific inner processing and don't want that to be visible to the rest of the code since maybe calling that somewhere else could potentially cause some confusion or even break something.

[FMorschel]

This issue should probably be split into more than one, if you can help me reason it to make sure that the questions are clearer I'd really appreciate it.

Yeah I think "unused mixin" is something we haven't really looked into. Could be a reasonable diagnostic.

This, IMO should be considered only for private classes/mixins that are implemented, or mixed-in that have only private declarations that are not being called anywhere. I'll take some time to create better (and simpler hahaha) examples than the ones presented above and create a new issue mentioning this one then.

[srawlins]

I see that, but also there is no way for me to call N._fn since it is an abstract method, should I always remember to add the ignore in that case then?

No, I think N._fn should be removed.

Since it can only be used inside the same library, in my company and personal projects we use this kind of declaration to help us remember some specific names for methods when we want one of our related classes to do a specific inner processing and don't want that to be visible to the rest of the code since maybe calling that somewhere else could potentially cause some confusion or even break something.

Ah yes, if it is acknowledged dead code for documentation purposes, then it can be marked with // ignore:.

[FMorschel]

Thanks a lot for helping me with these questions. I'll create the new issue for unused mixins now.

[eernstg]

Interesting! @srawlins, perhaps you could comment on the following?

(Yes, I know this issue has been closed, but let me just revisit one thing... ;-)

The notion of being unused is important here, and it is indicated here that N._fn in the original example is never referenced.

Indeed, I haven't noticed any locations in that example where N._fn is the result of resolution for a statically checked method invocation or tear-off. Of course, it's an abstract declaration so we couldn't actually run it or tear it off, but it might be the statically known declaration for any given call site, in which case we might not be able to remove the declaration from N (because those call sites could now be compile-time errors). OK, in this case we actually have a declaration M._fn that N._fn overrides, but N._fn could still have a different signature (say, it could add an optional parameter to the signature of M._fn), which means that we could break the program by removing N._fn, if it were the statically known declaration for any call site.

In this case we don't even have that. So surely we could just delete N._fn?

I'd still say "not necessarily", because N._fn could serve as a kind of template for other declarations named _fn in the same library: If they override N._fn then they need to be correct overrides, which is a rather strong constraint on those other declarations named _fn (and not the same thing as saying that they must all have the exact same signature).

So, @srawlins, do you think we need to broaden the notion of being "used" slightly, such that an abstract declaration is considered "used" even in the case where it isn't used for any other purpose than being overridden? We could still delete that abstract declaration without breaking any code, but we would eliminate the consistency constraint that is expressed via the requirement that other declarations need to be a correct override of this one. We could be more strict and say that an abstract declaration is only considered to be used based on overrides if it is overridden by at least two declarations (because otherwise it isn't enforcing consistency).

[FMorschel]

Thank you for bringing my words to a reasonable request. That is exactly what I was wondering but wasn't able to make a concise explanation for it.

[srawlins]

I'd still say "not necessarily", because N._fn could serve as a kind of template for other declarations named _fn in the same library: If they override N._fn then they need to be correct overrides, which is a rather strong constraint on those other declarations named _fn (and not the same thing as saying that they must all have the exact same signature).

Then this can be ignored with // ignore.

So, @srawlins, do you think we need to broaden the notion of being "used" slightly, such that an abstract declaration is considered "used" even in the case where it isn't used for any other purpose than being overridden?

we would eliminate the consistency constraint that is expressed via the requirement that other declarations need to be a correct override of this one

No, this is not in the purview of unused code detection. There is not a technical reason for A._fn to have the same signature as C._fn, because there is no call to N._fn. (There may be a consistency reason, which is outside the purview of the warning.) In the original code in this issue, if there was one reference to N._fn or M._fn (like void foo(N n) => n._fn()) then it would be used, and the hierarchy and overrides and signatures are important from a static analysis viewpoint.

What would be the downside of allowing N._fn, not reporting it as unused? The warning would not be serving it's purpose. The purpose is to report unused code. Unused code leads to code bloat, bit-rotted code, confusing code, code with unnecessary imports, packages with unnecessary dependencies. In order for the warning to best serve its purpose, it must stick to analysis from a static analysis viewpoint, otherwise it under-reports.

If there is a desire to keep around unused code for some other purpose (other than "use"), like idiomatic unused parameters, then // ignore: is a great tool, and sends a strong signal, "I know this is unused, but I want to keep it."

[eernstg]

@srawlins wrote:

No, this is not in the purview of unused code detection.

I'm a little bit confused about what it takes for code to be "unused". Would we consider _A._m to be unused in the following example if no call site resolves statically to _A._m? Going further, would we consider the entire class _A to be unused if _A is not used as a type?

class SomeTypeAnnotationWhichIsRatherVerbose {}
class AnotherTypeAnnotationWhichIsRatherVerbose {}
class YetAnotherTypeAnnotationWhichIsRatherVerbose {}

abstract class _A {
  SomeTypeAnnotationWhichIsRatherVerbose _m(
    AnotherTypeAnnotationWhichIsRatherVerbose x,
    YetAnotherTypeAnnotationWhichIsRatherVerbose y,
  );
}

class _B implements _A {
  @override
  _m(x, y) {....}
}

class _C implements _A {
  @override
  _m(x, y) {....}
}

You could say that _B and _C do not have a common member signature for any technical reason (given that _A is not used as a type we could simply delete the class _A, remove implements _A and @override, and add the return type and parameter types to _B._m and _C._m). However, it might still seem reasonable to have _A because it allows for the verbose member signature to be reused by override inference rather than duplicated.

If we're just considering to remove the member declaration _A._m then the story is shorter, but similar.

[srawlins]

Would we consider _A._m to be unused in the following example if no call site resolves statically to _A._m? Going further, would we consider the entire class _A to be unused if _A is not used as a type?

Yes, _A._m is unused, as are _B and _C. If _B and _C are removed, then _A is unused, and can be removed.

However, it might still seem reasonable to have _A because it allows for the verbose member signature to be reused by override inference rather than duplicated.

That's true! In that case, I would suggest using // ignore:. I think the possibility of under-reporting unused code outweighs the benefit of using an interface to avoid retyping long type names.

[eernstg]

Yes, _A._m is unused, as are _B and _C.

Oh, I was assuming that _B and _C are being used by some other code in the same library, calling _m. I guess I forgot to mention that explicitly.

It's just _A which is not being used as a type (in the scenario where we conclude that the entire class _A is unused). In the other scenario, _A is not used as the type of an invocation of _m (and we may conclude that _A._m is unused).

under-reporting unused code [matters]

Right, I'm just thinking that an override relationship might be considered to be "usage", especially in the case where it involves override inference.

[FMorschel]

I'm just thinking that an override relationship might be considered to be "usage", especially in the case where it involves override inference.

This is exactly what I was thinking. Thank you for expressing that for me.