Skip to content

[Concurrency] Prevent misuse of isolated keyword to avoid crashes #80390

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Concurrency] Prevent misuse of isolated keyword to avoid crashes #80390

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 @@ -6035,6 +6035,9 @@ ERROR(isolated_deinit_on_value_type,none,
ERROR(isolated_deinit_unavailable,none,
"isolated deinit is only available in %0 %1 or newer",
(AvailabilityDomain, AvailabilityRange))
ERROR(isolated_misuse_in_decl,none,
"invalid use of isolated in this context",
())
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't isolated be single-quoted?
Or... Can't we use attr_only_one_decl_kind instead of new diagnosis?

Edit: We might not need new diagnosis if we can trap invalid isolated declarations in a different way. Please see below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that could be done.


//------------------------------------------------------------------------------
// MARK: String Processing
Expand Down
12 changes: 12 additions & 0 deletions lib/Sema/TypeCheckConcurrency.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4943,6 +4943,18 @@ getIsolationFromAttributes(const Decl *decl, bool shouldDiagnose = true,
// If the declaration is explicitly marked 'isolated', infer actor isolation
// from the context. Currently applies only to DestructorDecl
if (isolatedAttr) {
if (auto varDecl = dyn_cast<VarDecl>(decl)) {
ASTContext &ctx = varDecl->getASTContext();
ctx.Diags.diagnose(isolatedAttr->getLocation(),
diag::isolated_misuse_in_decl);
return std::nullopt;
}
if (auto nominalTypeDecl = dyn_cast<NominalTypeDecl>(decl)) {
ASTContext &ctx = nominalTypeDecl->getASTContext();
ctx.Diags.diagnose(isolatedAttr->getLocation(),
diag::isolated_misuse_in_decl);
return std::nullopt;
}
assert(isa<DestructorDecl>(decl));
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a reason why this is not if (!isa<DestructorDecl>(decl)) { ... } but assert(isa<DestructorDecl>(decl));.
I mean I suspect we should catch any declarations other than deinit declarations more upstream.

💭 I guess the cause of #80363 is that getActorIsolation(ValueDecl*) is called before TypeChecker::checkDeclAttributes(Decl*) is called:

  • isolated let c = C(): checkGlobalIsolation which calls getActorIsolation is called before checkDeclAttributes in DeclChecker::visitBoundVariable.
  • isolated struct S {}: createImplicitConstructor calls getActorIsolation early if Feature::IsolatedDefaultValues is enabled.
    • That's why the compiler doesn't crash with isolated struct S { init() {} }.

I want to hear opinions from the author here. @ktoso
(cc: @sophiapoirier as the author of checkGlobalIsolation, @hborla as the author of IsolatedDefaultValues)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the first time when I too saw the backtrace it wasn't quite it to just add the if's to handle it.


auto dc = decl->getDeclContext();
Expand Down
23 changes: 23 additions & 0 deletions test/Concurrency/isolated_invalid_uses.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// RUN: %target-swift-frontend -target %target-swift-5.1-abi-triple -swift-version 6 -strict-concurrency=complete %s -emit-sil -o /dev/null -verify -DALLOW_TYPECHECKER_ERRORS -verify-additional-prefix typechecker-

// RUN: %target-swift-frontend -target %target-swift-5.1-abi-triple -swift-version 6 -strict-concurrency=complete %s -emit-sil -o /dev/null -verify -verify-additional-prefix tns-

Copy link
Member

Choose a reason for hiding this comment

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

Let me leave some questions here:

  • Why are there two RUNs?
  • Is -verify-additional-prefix necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was debugging and left this out accidentally.

// REQUIRES: asserts
// REQUIRES: concurrency
// REQUIRES: swift_swift_parser

isolated struct S {} // expected-error {{invalid use of isolated in this context}}
// expected-error@-1 {{'isolated' may only be used on 'deinit' declarations}}

isolated class C {} // expected-error {{'isolated' may only be used on 'deinit' declarations}}

func f() {
isolated let c = C() // expected-error {{invalid use of isolated in this context}}
// expected-error@-1 {{'isolated' may only be used on 'deinit' declarations}}
// expected-warning@-2 {{initialization of immutable value 'c' was never used; consider replacing with assignment to '_' or removing it}}
}

actor A {
isolated var state: Int? // expected-error {{invalid use of isolated in this context}}
// expected-error@-1 {{'isolated' may only be used on 'deinit' declarations}}
}