Skip to content

[Concurrency] Allow 'nonisolated' to be applied to mutable storage of 'Sendable' type on a globally-isolated value type. #74958

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

Merged
merged 1 commit into from
Jul 5, 2024
+71 −17
Merged

[Concurrency] Allow 'nonisolated' to be applied to mutable storage of 'Sendable' type on a globally-isolated value type. #74958

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
32 changes: 23 additions & 9 deletions lib/Sema/TypeCheckAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7010,18 +7010,32 @@ void AttributeChecker::visitNonisolatedAttr(NonisolatedAttr *attr) {

if (auto var = dyn_cast<VarDecl>(D)) {
// stored properties have limitations as to when they can be nonisolated.
auto type = var->getTypeInContext();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could test that we are checking a contextual vs. interface type with a type decl that conditionally conforms to Sendable. For example:

@MainActor
struct S {
  nonisolated var x: [Int]
}

if (var->hasStorage()) {
// 'nonisolated' can not be applied to mutable stored properties unless
// qualified as 'unsafe'.
if (var->supportsMutation() && !attr->isUnsafe()) {
diagnoseAndRemoveAttr(attr, diag::nonisolated_mutable_storage)
.fixItInsertAfter(attr->getRange().End, "(unsafe)");
var->diagnose(diag::nonisolated_mutable_storage_note, var);
return;
{
// 'nonisolated' can not be applied to mutable stored properties unless
// qualified as 'unsafe', or is of a Sendable type on a
// globally-isolated value type.
bool canBeNonisolated = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

This can probably have a better name, for now I just put it in a scope.

if (dc->isTypeContext()) {
if (auto nominal = dc->getSelfStructDecl()) {
if (!var->isStatic() && type->isSendableType() &&
getActorIsolation(nominal).isGlobalActor()) {
canBeNonisolated = true;
}
}
}

if (var->supportsMutation() && !attr->isUnsafe() && !canBeNonisolated) {
diagnoseAndRemoveAttr(attr, diag::nonisolated_mutable_storage)
.fixItInsertAfter(attr->getRange().End, "(unsafe)");
var->diagnose(diag::nonisolated_mutable_storage_note, var);
return;
}
}

// 'nonisolated' without '(unsafe)' is not allowed on non-Sendable variables.
auto type = var->getTypeInContext();
// 'nonisolated' without '(unsafe)' is not allowed on non-Sendable
// variables.
if (!attr->isUnsafe() && !type->hasError()) {
bool diagnosed = diagnoseIfAnyNonSendableTypes(
type,
Expand Down
9 changes: 4 additions & 5 deletions lib/Sema/TypeCheckConcurrency.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,11 +516,10 @@ static bool varIsSafeAcrossActors(const ModuleDecl *fromModule,
// A mutable storage of a value type accessed from within the module is
// okay.
if (dyn_cast_or_null<StructDecl>(var->getDeclContext()->getAsDecl()) &&
!var->isStatic() &&
var->hasStorage() &&
var->getTypeInContext()->isSendableType() &&
accessWithinModule) {
return true;
!var->isStatic() && var->hasStorage() &&
var->getTypeInContext()->isSendableType()) {
if (accessWithinModule || varIsolation.isNonisolated())
return true;
}
// Otherwise, must be immutable.
return false;
Expand Down
9 changes: 7 additions & 2 deletions test/Concurrency/actor_isolation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,7 @@ struct InferredFromContext {
get { [] }
}

nonisolated var status: Bool = true // expected-error {{'nonisolated' cannot be applied to mutable stored properties}}{{3-15=}}{{3-15=}}{{14-14=(unsafe)}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Side note: This error message needs some love now that the rules have changed.

Copy link
Member Author

@simanerush simanerush Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, but it's not obvious to me in what way should we change it. This case of allowing nonisolated on a mutable storage is too narrow, so updating the general case to include all conditions under which the attribute can be allowed seems too convoluted. I think I can see a possibility of updating this error to something like "'nonisolated' cannot be applied to this mutable stored property", and if it's declared on a globally-isolated value type, emit fix-its like "consider making the property type Sendable". Similarly, in the case where the property is of Sendable type and is on a value type, emit a fix-it saying "consider isolating the value type to the main actor."
Another possibility would be splitting this into 2 diagnostics, one for value type and another for reference types. In that case, we could add all the conditions in which this rule does not apply for the value type case.
In short, I'm just trying to think of a way to make this diagnostic make more sense from usability perspective (e.g. a programmer not familiar with subtleties in concurrency rules should get helpful fix-its)

// expected-note@-1{{convert 'status' to a 'let' constant or consider declaring it 'nonisolated(unsafe)' if manually managing concurrency safety}}
nonisolated var status: Bool = true // okay

nonisolated let flag: Bool = false

Expand Down Expand Up @@ -1243,6 +1242,12 @@ func test_conforming_actor_to_global_actor_protocol() {
// expected-error@-1 {{actor 'MyValue' cannot conform to global actor isolated protocol 'GloballyIsolatedProto'}}
}

func test_nonisolated_variable() {
struct S: GloballyIsolatedProto {
nonisolated var x: Int = 0 // okay
}
Comment on lines +1245 to +1248
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have any nonisolated(unsafe) tests for stored var?

}

func test_invalid_reference_to_actor_member_without_a_call_note() {
actor A {
func partial() { }
Expand Down
2 changes: 1 addition & 1 deletion test/Concurrency/derived_conformances_nonisolated.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ struct X1: Equatable, Hashable, Codable {
@MainActor
struct X2: Equatable, Hashable, Codable {
let x: Int
var y: String
nonisolated var y: String // okay
}

class NonSendable {
Expand Down
36 changes: 36 additions & 0 deletions test/Concurrency/nonisolated_access.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// RUN: %empty-directory(%t/src)
// RUN: split-file %s %t/src

/// Build the library A
// RUN: %target-swift-frontend -emit-module %t/src/A.swift \
// RUN: -disable-availability-checking \
// RUN: -module-name A -swift-version 6 \
// RUN: -emit-module-path %t/A.swiftmodule

// Build the client
// RUN: %target-swift-frontend -emit-module %t/src/Client.swift \
// RUN: -disable-availability-checking \
// RUN: -module-name Client -I %t -swift-version 6 \
// RUN: -emit-module-path %t/Client.swiftmodule

// REQUIRES: concurrency

//--- A.swift
@MainActor
public protocol P {}

public struct S: P {
nonisolated public var x: Int = 0

nonisolated public init() {}
}

//--- Client.swift
import A

actor A {
func test() {
var s = S()
s.x += 0 // okay
}
}