Document @mustBeOverridden contract to make more clear

[matanlurey]

import 'package:meta/meta.dart';

class A {
  @mustBeOverridden
  void a() {}
}

class B {
  @override
  void a() => super.a();
}

I can't win in this scenario, either I get a unnecessary_overrides lint or a must_be_overridden hint.

[srawlins]

I never understood the @mustBeOverridden annotation, but I think #30175 was the original request.

In any case, I can't imagine the annotation means "subclasses must override this, even if they just call super." (Is that the case in whatever code motivated this report?) I imagine the annotation means "must be overridden with code that does something interesting."

This is kind of why I don't understand the annotation; the presence of an override in no way confirms that you've adhered to some contract between a subclass an a superclass. 🤷 . Maybe flip-flopping between "must be overridden" and "unnecessary override" is a signal that you're not doing the override right?

[matanlurey]

I guess let's imagine A.a is non-empty then.

Perhaps some sort of "default" garbage compacting, but you're encouraged to provide your own (or make it explicit you're relying on some unoptimized default). I definitely can see how this isn't clearly useful :)

[srawlins]

Perhaps some sort of "default" garbage compacting, but you're encouraged to provide your own (or make it explicit you're relying on some unoptimized default)

In this case, I think // ignore comments would be best. If this @mustBeOverridden is really @overridesEncouraged then I think an ignore: must_be_overridden would be the canonical way to suppress a warning. If you want to write void foo() => super.foo() then you should include // ignore: unnecessary_overrides with a comment above it like "the super class encourages an override, but it's actually not necessary."

[pq]

You might also be able to suppress the warning like this:

class B extends A {
  @override
  void a() {
    // An override is required by `A` but the default behavior is fine.
    super.a();
  }
}

IIRC a comment in the body is enough to silence unnecessary_overrides.

I can't imagine the annotation means "subclasses must override this, even if they just call super."

In some cases I guess this might be reasonable but in general probably infrequently enough that an ignore (or gratuitous comment in the body) might be OK?

[srawlins]

In some cases I guess this might be reasonable but in general probably infrequently enough that an ignore (or gratuitous comment in the body) might be OK?

Overriding with a body that only calls super is the same as not overriding, but with more typing. If that was an acceptable override, I think the @mustBeOverridden would be completely unnecessary.

[pq]

Overriding with a body that only calls super is the same as not overriding

In this case the annotation is just saying that a subclass has to make an explicit decision about the behavior of a method. That decision could be to inherit the default behavior.

[srawlins]

OK, it looks like there are two behavior choices, when extending a class which has a @mustBeOverridden member, and the member is overridden with a super call:

1. We can report "Hey your superclass says you need to do this thing."
2. Or we can be silent, indicating the developer did a good job; the override has been made.

I really really posit that more bugs will result from the second choice than the first. But again, I've never seen the concrete use cases for this annotation, so I may be wrong, and we can instead implement (2.).

[bwilkerson]

I don't really understand the use case for @mustBeOverridden either. When we figure it out the documentation should be updated to explain when it's useful (or the annotation should be removed if it's no longer useful).

In the meantime, I'd stick with the current behavior because (a) I agree that the proposed change is likely to be less helpful and (b) I don't think we should change it without fully understanding why.

[matanlurey]

From what I can tell, mustBeOverridden basically only makes sense in the following scenario:

abstract base class Wrapper<T> {
  final T value;

  const Wrapper(this.value);

  @override
  @mustBeOverridden
  bool operator ==(Object other);

  @override
  @mustBeOverridden
  int get hashCode;
}

Here, it serves as a reminder that classes that extend Wrapper need to provide new implementations of hashCode and == to be compliant with the class contract (this is a contrived example, there are for sure better ones).

Seems... reasonable? I would probably update the docs to explain that mustBeOverridden effectively "re-abstracts" a method, and it is WAI that the original method shouldn't be used/forwarded to (as my original example).

[matanlurey]

(Honestly a better name for this method would be @mustBeUsedExplicitly or something)