[Concurrency] Prevent misuse of isolated keyword to avoid crashes

[Rajveer100]

Resolves #80363

Using isolated in unintended contexts caused the compiler to crash in many scenarios.

[YOCKOW]

Thank you for fixing #80363.

By the way, #80363 indeed focuses on constant declarations and this PR also focuses on them.
However, there is another case that the compiler crashes at the same point (only with Swift 6 language mode):

// -swift-version 6
isolated struct S {} // 💥

Is it possible to cover such cases as well in one fix?

[Rajveer100]

This works without any further changes:

debug-80363.swift:1:1: error: 'isolated' may only be used on 'deinit' declarations
 1 | isolated class C {}
   | `- error: 'isolated' may only be used on 'deinit' declarations
 2 | isolated struct S {}
 3 | 

debug-80363.swift:2:1: error: 'isolated' may only be used on 'deinit' declarations
 1 | isolated class C {}
 2 | isolated struct S {}
   | `- error: 'isolated' may only be used on 'deinit' declarations
 3 | 
 4 | func f() {

debug-80363.swift:9:3: error: invalid use of isolated in this context
 7 | 
 8 | actor A {
 9 |   isolated var state: Int?
   |   `- error: invalid use of isolated in this context
10 | }
11 | 

debug-80363.swift:9:3: error: 'isolated' may only be used on 'deinit' declarations
 7 | 
 8 | actor A {
 9 |   isolated var state: Int?
   |   `- error: 'isolated' may only be used on 'deinit' declarations
10 | }
11 | 

debug-80363.swift:5:3: error: invalid use of isolated in this context
 3 | 
 4 | func f() {
 5 |   isolated let c = C()
   |   `- error: invalid use of isolated in this context
 6 | }
 7 | 

debug-80363.swift:5:3: error: 'isolated' may only be used on 'deinit' declarations
 3 | 
 4 | func f() {
 5 |   isolated let c = C()
   |   `- error: 'isolated' may only be used on 'deinit' declarations
 6 | }
 7 | 

debug-80363.swift:5:16: warning: initialization of immutable value 'c' was never used; consider replacing with assignment to '_' or removing it [#no-usage]
 3 | 
 4 | func f() {
 5 |   isolated let c = C()
   |                `- warning: initialization of immutable value 'c' was never used; consider replacing with assignment to '_' or removing it [#no-usage]
 6 | }
 7 | 

[YOCKOW]

@Rajveer100

This works without any further changes:

Hmm.
On my local machine, the compiler with your patch still crashes with Swift 6 language mode while compiling isolated struct S.
Specifically speaking, the test fails when, in your "/test/Concurrency/isolated_invalid_uses.swift",

• Replacing // RUN: lines with // RUN: %target-swift-frontend -target %target-swift-5.1-abi-triple -swift-version 6 %s -emit-sil -o /dev/null -verify to specify Swift 6 language mode.
• Replacing class C {} with isolated struct S {}.

[Rajveer100]

@YOCKOW
My bad, I hadn't added the version flag earlier. Made few more changes.

[YOCKOW]

Let me leave some questions here:

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

[Rajveer100]

I was debugging and left this out accidentally.

[YOCKOW]

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.

[Rajveer100]

Indeed, that could be done.

[YOCKOW]

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)

[Rajveer100]

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