[Concurrency] warn about 'final' on actors being ineffective #85893
Conversation
ktoso
requested review from
Xazax-hun,
egorzhdan,
hborla,
hnrklssn,
j-hui,
slavapestov,
susmonteiro and
xedin
as code owners
|
@swift-ci please smoke test |
|
|
||
| @_expose(Cxx) | ||
| public final actor ActorWithField { | ||
| public final actor ActorWithField { // expected-warning{{'final' on actor 'ActorWithField' has no effect}} |
There was a problem hiding this comment.
Perhaps we want to remove this final to reduce noise, since it's not really intentional in this test case, from what I can tell.
There was a problem hiding this comment.
Sure, I'll follow up later adding a separate test for it.
| TypeChecker::checkConcurrencyAvailability(CD->getLoc(), CD); | ||
|
|
||
| if (CD->isFinal()) { | ||
| CD->diagnose(diag::actor_final_no_effect, CD) |
There was a problem hiding this comment.
this came up somewhat recently on the forums (see here), and when i had suggested banning this, @tshortli had this to say:
finalis not meaningless on actor, unfortunately [...] marking an actor methodfinal, or marking the whole actorfinal, changes the ABI of calling methods on the actor since calls will no longer go through dynamic dispatch.
given that, should this warning be further conditioned upon the declaration's access level or something?
There was a problem hiding this comment.
Hm ouch that's unfortunate... FYI @rjmccall
We could then make it only earn for non public declarations probably 🤔
There was a problem hiding this comment.
although... it should never actually be possible to inherit from an actor right (except for maybe some objc runtime trickery?)? does that mean, at least for optimized WMO builds, the vtable entries should always be removed? maybe the risk of issues in practice would be pretty low.
|
So there's ongoing discussion about this one; this would be adding more noise for a "harmless" thing potentially, so maybe we just leave it be |
3 participants
Add a warning about 'final' in front of actor declarations. We've never started rejecting it, however it doesn't do anything, so we should warn about it because otherwise people may think this is actually doing something