ensure mixed in classes preserve `@mustCallSuper` semantics

[pq]

Mixins that implement methods that are marked @mustCallSuper can be mixed into classes that break the @mustCallSuper contract (by not calling super).

motivating example

abstract class HowToScream {
  void scream();
}

class WillScream implements HowToScream {
  @override
  @mustCallSuper
  void scream() {
    print('Aaaaaaaaaaaaaaaaaaaaaaaaaaaa!');
  }
}

mixin DontScream implements HowToScream {
  @override
  void scream() {
    print('Sssssh!');
  }
}

// Should not be allowed, since DontScream will override
// the HowToScream implementation of WillScream,
// without calling super.
class ShouldScream extends WillScream with DontScream {} // LINT

implementation

To produce a diagnostic on ShouldScream, we need information that is not currently in the element model. Since on-the-fly non-local analysis is expensive, we propose adding the required information to the element model.

Specifically, we'd like to:

• create a new modifier (flag) stored in the element model (invokesSuper), that
• is updated during resolution such that:
  • for each mixin, for every method, we look to see if there is a method in the super call chain that’s marked @mustCallSuper and verify that super is invoked by the mixin method.

This will require us to:

• update the modifier bits to allow for a new invokesSuper flag bitmask
• update summaries accordingly

(Unless @scheglov, you see a better approach?)

/fyi @bwilkerson

[scheglov]

SGTM

Interestingly, we have MixinElementImpl.superInvokedNames, but here we would add a method specific flag, maybe invokedSuperSelf.

Hm... So, we will track, at least for mixins, whether each methods call super-self, and remember as a flag.

And in MustCallSuperVerifier._findOverriddenMemberWithMustCallSuper we walk the hierarchy and looking for a @mustCallSuper annotations. Could this also be a flag on the method element?

[eernstg]

[Edit: Haha, the lint already works the way I suggested. So the text below just spells out what I was thinking about, it doesn't propose anything new.]

---

We just discussed this issue at the language meeting, and I had a comment on it. Here's some more detail on that.

I think it makes sense to consider two different cases here:

1. A member of a given mixin is designed for usages where it is not known that a superinvocation is required, and the error arises in specific mixin applications (like the one in ShouldScream.
2. A member of a given mixin is designed for usages where the superinvocation is required, so we take the opportunity to detect missing superinvocations early.

Case 1 perfectly matches the description in the initial text of this issue.

Case 2 allows for early error detection, and it occurs when a concrete member declaration in the mixin overrides a member in an on type whose declaration has the @mustCallSuper annotation:

abstract class HowToScream {
  @mustCallSuper
  void scream() {
    print('Aaaaaaaaaaaaaaaaaaaaaaaaaaaa!');
  }
}

mixin DontScream on HowToScream {
  @override
  void scream() { // LINT.
    print('Sssssh!');
  }
}

We're using the on type that enables a superinvocation that targets a declaration that has @mustCallSuper as an indicator: No matter which actual superclass this mixin is applied to, its scream method is designed to be part of a superinvocation chain. So we treat DontScream.scream as an overriding declaration (that inherits @mustCallSuper, as if the on type had been a superclass—it's the whole point of each on type to represent parts of the superclass). So we lint the body if it does not perform the required superinvocation.

It could be argued that @mustCallSuper is concerned with a specific implementation, and an on class is an interface, but I do think that the notion of "being part of a superinvocation chain" needs to be associated with the on types of a mixin: There is nothing that matches more precisely, it is unsurprising that the declaration-site role of the superclass is played by the on clause, and it is a missed opportunity if @mustCallSuper annotations in on types are ignored.

[srawlins]

Duplicate of https://github.com/dart-lang/sdk/issues/36657 but now this has more history.

[pq]

Hm... So, we will track, at least for mixins, whether each methods call super-self, and remember as a flag.

And in MustCallSuperVerifier._findOverriddenMemberWithMustCallSuper we walk the hierarchy and looking for a @mustCallSuper annotations. Could this also be a flag on the method element?

I think so. Specifically, a flag like invokesSuperSelf on MethodElement(Impl)?

[scheglov]

I thought about auto-inheriting hasMustCallSuper, we have something like this for hasDoNotStore and hasOrInheritsDoNotStore - although these are not hierarchy based, but enclosingElement based. It could be MethodElement.hasOrInheritsMustCallSuper.

Or we could repurpose hasMustCallSuper to return the "effective" state of the flag. But then on the element model level we would not know whether it was declared with @mustCallSuper or inherited it. OTOH, we already do so for all types - omitted types are inherited.

[bwilkerson]

I believe that all of the hasFoo getters should have the same semantic: return true if this element is annotated with the foo annotation. If we want a getter for the "inherited" form of hasMustCallSuper, I'd propose a different name, such as hasOrOverridesMustCallSuper. Or maybe we can find something better. :-/

[scheglov]

There are two arguments to keep hasMustCallSuper as is - first, the name says "has", and second, as you mentioned, the consistency. For name, maybe bool get mustCallSuper? It looks grammatically correct to me, but I might be wrong here as well. And it is the same name as the annotation itself, for better or worse.

One question that bothers me is whether we need the current hasMustCallSuper at all, maybe we always need the higher level semantics? From searches in the analyzer and google3, we use hasMustCallSuper only in MustCallSuperVerifier, so we only need the semantics that is useful there.

Oh... Actually, the element that has @mustCallSuper itself does not have to call super. Its overrides have to. Hm... Maybe we are better leave it all as is.

[bwilkerson]

One question that bothers me is whether we need the current hasMustCallSuper at all, maybe we always need the higher level semantics?

I don't know whether we need the current semantics, but if not I'd want to leave the name unused in case we had a need for it in the future so that it could still be consistent.

Actually, the element that has @mustCallSuper itself does not have to call super.

Yeah, in hindsight it might have been better to name the annotation overridesMustCallSuper. It's longer, but more accurate. But we'd still be in the same position when trying to name getters on MethodElement.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/240652 adds ExecutableElementImpl.invokesSuperSelf, which I think might be enough to implement a fix on top of it.

[pq]

This is fantastic. Looking now. Thanks!

[pq]

https://dart-review.googlesource.com/c/sdk/+/240880 adds the diagnostic and https://github.com/flutter/flutter/pull/102328 updates DiagnosticableTreeMixin in flutter which it flags.

[pq]

~My initial attempt to fix DiagnosticableTreeMixin hit a snag. To ensure mustCallSuper semantics, DiagnosticableTreeMixin has to be on Diagnosticable but if we do that, DiagnosticableTreeMixin can only be mixed into classes that mix in Diagnosticable and that would be breaking (in flutter and downstream).~

UPDATE: the flutter issue was actually a false positive; working on a fix.