Skip to content

[Concurrency] warn about 'final' on actors being ineffective #85893

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.

Sign up for GitHub

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

Closed
ktoso wants to merge 1 commit into from
+13 −2
Closed

[Concurrency] warn about 'final' on actors being ineffective #85893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5699,6 +5699,9 @@ ERROR(async_objc_dynamic_self,none,
ERROR(actor_inheritance,none,
"%select{actor|distributed actor}0 types do not support inheritance",
(bool))
WARNING(actor_final_no_effect,none,
"'final' on %kind0 has no effect",
(const ValueDecl*))

ERROR(actor_protocol_illegal_inheritance,none,
"non-actor type %0 cannot conform to the %1 protocol",
Expand Down
8 changes: 7 additions & 1 deletion lib/Sema/TypeCheckDeclPrimary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3284,9 +3284,15 @@ class DeclChecker : public DeclVisitor<DeclChecker> {

TypeChecker::checkDeclAttributes(CD);

if (CD->isActor())
if (CD->isActor()) {
TypeChecker::checkConcurrencyAvailability(CD->getLoc(), CD);

if (CD->isFinal()) {
CD->diagnose(diag::actor_final_no_effect, CD)
Copy link
Contributor

@jamieQ jamieQ Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this came up somewhat recently on the forums (see here), and when i had suggested banning this, @tshortli had this to say:

final is not meaningless on actor, unfortunately [...] marking an actor method final, or marking the whole actor final, 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?

Copy link
Contributor Author

@ktoso ktoso Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm ouch that's unfortunate... FYI @rjmccall

We could then make it only earn for non public declarations probably 🤔

Copy link
Contributor

@jamieQ jamieQ Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.fixItRemove(CD->getAttrs().getAttribute<FinalAttr>()->getLocation());
}
}

for (Decl *Member : CD->getABIMembers())
visit(Member);

Expand Down
2 changes: 2 additions & 0 deletions test/Distributed/distributed_actor_basic.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,5 @@ distributed actor Second {
try! await first.one(second: self)
}
}

final distributed actor WarnAboutMe { } // expected-warning{{'final' on distributed actor 'WarnAboutMe' has no effect}}
2 changes: 1 addition & 1 deletion test/Interop/SwiftToCxx/class/swift-actor-in-cxx.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
// REQUIRES: concurrency

@_expose(Cxx)
public final actor ActorWithField {
public final actor ActorWithField { // expected-warning{{'final' on actor 'ActorWithField' has no effect}}
Copy link
Member

@hnrklssn hnrklssn Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ktoso ktoso Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll follow up later adding a separate test for it.

var field: Int64

public init() {
Expand Down