-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ensure mixed in classes preserve @mustCallSuper
semantics
#48352
Comments
SGTM Interestingly, we have Hm... So, we will track, at least for mixins, whether each methods call super-self, and remember as a flag. And in |
[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:
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 abstract class HowToScream {
@mustCallSuper
void scream() {
print('Aaaaaaaaaaaaaaaaaaaaaaaaaaaa!');
}
}
mixin DontScream on HowToScream {
@override
void scream() { // LINT.
print('Sssssh!');
}
} We're using the It could be argued that |
@mustCallSuper
semantics@mustCallSuper
semantics
Duplicate of #36657 but now this has more history. |
I think so. Specifically, a flag like |
I thought about auto-inheriting Or we could repurpose |
I believe that all of the |
There are two arguments to keep One question that bothers me is whether we need the current Oh... Actually, the element that has |
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.
Yeah, in hindsight it might have been better to name the annotation |
https://dart-review.googlesource.com/c/sdk/+/240652 adds |
This is fantastic. Looking now. Thanks! |
…ing. Bug: #48352 Change-Id: I443df48885eebadbe85d82ff06474aef4db6e422 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/240652 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Reviewed-by: Phil Quitslund <pquitslund@google.com> Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
https://dart-review.googlesource.com/c/sdk/+/240880 adds the diagnostic and flutter/flutter#102328 updates |
UPDATE: the flutter issue was actually a false positive; working on a fix. |
Mixins that implement methods that are marked
@mustCallSuper
can be mixed into classes that break the@mustCallSuper
contract (by not callingsuper
).motivating example
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:
invokesSuper
), that@mustCallSuper
and verify that super is invoked by the mixin method.This will require us to:
invokesSuper
flag bitmask(Unless @scheglov, you see a better approach?)
/fyi @bwilkerson
The text was updated successfully, but these errors were encountered: