Skip to content

[Concurrency] Revert 'nonisolated let' change. #37305

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 11 additions & 5 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -4441,9 +4441,6 @@ NOTE(actor_isolated_sync_func,none,
NOTE(actor_mutable_state,none,
"mutation of this %0 is only permitted within the actor",
(DescriptiveDeclKind))
NOTE(actor_isolated_let,none,
"use `nonisolated` to allow synchronous access to 'let' from outside "
"the actor", ())
WARNING(shared_mutable_state_access,none,
"reference to %0 %1 is not concurrency-safe because it involves "
"shared mutable state", (DescriptiveDeclKind, DeclName))
Expand Down Expand Up @@ -4476,12 +4473,13 @@ WARNING(non_concurrent_property_type,none,
WARNING(non_concurrent_keypath_capture,none,
"cannot form key path that captures non-sendable type %0",
(Type))
WARNING(non_concurrent_keypath_access,none,
"cannot form key path that accesses non-sendable type %0",
(Type))
ERROR(non_concurrent_type_member,none,
"%select{stored property %1|associated value %1}0 of "
"'Sendable'-conforming %2 %3 has non-sendable type %4",
(bool, DeclName, DescriptiveDeclKind, DeclName, Type))
ERROR(non_sendable_nonisolated_let,none,
"non-isolated let property %0 has non-Sendable type %1", (DeclName, Type))
ERROR(concurrent_value_class_mutable_property,none,
"stored property %0 of 'Sendable'-conforming %1 %2 is mutable",
(DeclName, DescriptiveDeclKind, DeclName))
Expand All @@ -4497,13 +4495,21 @@ ERROR(concurrent_value_inherit,none,
"%select{| other than 'NSObject'}0",
(bool, DeclName))

ERROR(actorindependent_let,none,
"'@actorIndependent' is meaningless on 'let' declarations because "
"they are immutable",
())
ERROR(actorindependent_mutable_storage,none,
"'@actorIndependent' can not be applied to stored properties",
())
ERROR(actorindependent_local_var,none,
"'@actorIndependent' can not be applied to local variables",
())

ERROR(nonisolated_let,none,
"'nonisolated' is meaningless on 'let' declarations because "
"they are immutable",
())
ERROR(nonisolated_mutable_storage,none,
"nonisolated' can not be applied to stored properties",
())
Expand Down
33 changes: 16 additions & 17 deletions lib/Sema/TypeCheckAttr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5423,14 +5423,17 @@ void AttributeChecker::visitActorIndependentAttr(ActorIndependentAttr *attr) {
// that do not have storage.
auto dc = D->getDeclContext();
if (auto var = dyn_cast<VarDecl>(D)) {
// @actorIndependent can not be applied to mutable stored properties, unless if
// @actorIndependent is meaningless on a `let`.
if (var->isLet()) {
diagnoseAndRemoveAttr(attr, diag::actorindependent_let);
return;
}

// @actorIndependent can not be applied to stored properties, unless if
// the 'unsafe' option was specified
if (var->hasStorage()) {
switch (attr->getKind()) {
case ActorIndependentKind::Safe:
if (var->isLet())
break;

diagnoseAndRemoveAttr(attr, diag::actorindependent_mutable_storage);
return;

Expand Down Expand Up @@ -5462,20 +5465,16 @@ void AttributeChecker::visitNonisolatedAttr(NonisolatedAttr *attr) {
// that do not have storage.
auto dc = D->getDeclContext();
if (auto var = dyn_cast<VarDecl>(D)) {
// 'nonisolated' can only be applied to 'let' stored properties.
// Those must be Sendable.
if (var->hasStorage()) {
if (!var->isLet()) {
diagnoseAndRemoveAttr(attr, diag::nonisolated_mutable_storage);
return;
}
// 'nonisolated' is meaningless on a `let`.
if (var->isLet()) {
diagnoseAndRemoveAttr(attr, diag::nonisolated_let);
return;
}

// nonisolated lets must have Sendable type.
if (shouldDiagnoseNonSendableViolations(dc->getASTContext().LangOpts) &&
!isSendableType(dc, var->getType())) {
var->diagnose(
diag::non_sendable_nonisolated_let, var->getName(), var->getType());
}
// 'nonisolated' can not be applied to stored properties.
if (var->hasStorage()) {
diagnoseAndRemoveAttr(attr, diag::nonisolated_mutable_storage);
return;
}

// @actorIndependent can not be applied to local properties.
Expand Down
57 changes: 46 additions & 11 deletions lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -597,14 +597,21 @@ ActorIsolationRestriction ActorIsolationRestriction::forDeclaration(
if (cast<ValueDecl>(decl)->isLocalCapture())
return forUnrestricted();

// 'let' declarations are immutable, so they can be accessed across
// actors.
bool isAccessibleAcrossActors = false;
if (auto var = dyn_cast<VarDecl>(decl)) {
if (var->isLet())
isAccessibleAcrossActors = true;
}

// A function that provides an asynchronous context has no restrictions
// on its access.
//
// FIXME: technically, synchronous functions are allowed to be cross-actor.
// The call-sites are just conditionally async based on where they appear
// (outside or inside the actor). This suggests that the implicitly-async
// concept could be merged into the CrossActorSelf concept.
bool isAccessibleAcrossActors = false;
if (auto func = dyn_cast<AbstractFunctionDecl>(decl)) {
if (func->isAsyncContext())
isAccessibleAcrossActors = true;
Expand Down Expand Up @@ -917,7 +924,8 @@ static bool diagnoseNonConcurrentProperty(

/// Whether we should diagnose cases where Sendable conformances are
/// missing.
bool swift::shouldDiagnoseNonSendableViolations(const LangOptions &langOpts) {
static bool shouldDiagnoseNonSendableViolations(
const LangOptions &langOpts) {
return langOpts.WarnConcurrency;
}

Expand Down Expand Up @@ -1421,17 +1429,11 @@ namespace {
// was it an attempt to mutate an actor instance's isolated state?
} else if (auto environment = kindOfUsage(decl, context)) {

if (isa<VarDecl>(decl) && cast<VarDecl>(decl)->isLet()) {
auto diag = decl->diagnose(diag::actor_isolated_let);
SourceLoc fixItLoc =
decl->getAttributeInsertionLoc(/*forModifier=*/true);
if (fixItLoc.isValid())
diag.fixItInsert(fixItLoc, "nonisolated ");
} else if (environment.getValue() == VarRefUseEnv::Read) {
if (environment.getValue() == VarRefUseEnv::Read)
decl->diagnose(diag::kind_declared_here, decl->getDescriptiveKind());
} else {
else
decl->diagnose(diag::actor_mutable_state, decl->getDescriptiveKind());
}

} else {
decl->diagnose(diag::kind_declared_here, decl->getDescriptiveKind());
}
Expand Down Expand Up @@ -1604,6 +1606,11 @@ namespace {

// is it an access to a property?
if (isPropOrSubscript(decl)) {
// we assume let-bound properties are taken care of elsewhere,
// since they are never implicitly async.
assert(!isa<VarDecl>(decl) || cast<VarDecl>(decl)->isLet() == false
&& "unexpected let-bound property; never implicitly async!");

if (auto declRef = dyn_cast_or_null<DeclRefExpr>(context)) {
if (usageEnv(declRef) == VarRefUseEnv::Read) {

Expand Down Expand Up @@ -1938,6 +1945,27 @@ namespace {
bool checkKeyPathExpr(KeyPathExpr *keyPath) {
bool diagnosed = false;

// returns None if it is not a 'let'-bound var decl. Otherwise,
// the bool indicates whether a diagnostic was emitted.
auto checkLetBoundVarDecl = [&](KeyPathExpr::Component const& component)
-> Optional<bool> {
auto decl = component.getDeclRef().getDecl();
if (auto varDecl = dyn_cast<VarDecl>(decl)) {
if (varDecl->isLet()) {
auto type = component.getComponentType();
if (shouldDiagnoseNonSendableViolations(ctx.LangOpts)
&& !isSendableType(getDeclContext(), type)) {
ctx.Diags.diagnose(
component.getLoc(), diag::non_concurrent_keypath_access,
type);
return true;
}
return false;
}
}
return None;
};

// check the components of the keypath.
for (const auto &component : keyPath->getComponents()) {
// The decl referred to by the path component cannot be within an actor.
Expand Down Expand Up @@ -1965,6 +1993,13 @@ namespace {
LLVM_FALLTHROUGH; // otherwise, it's invalid so diagnose it.

case ActorIsolationRestriction::CrossActorSelf:
// 'let'-bound decls with this isolation are OK, just check them.
if (auto wasLetBound = checkLetBoundVarDecl(component)) {
diagnosed = wasLetBound.getValue();
break;
}
LLVM_FALLTHROUGH; // otherwise, it's invalid so diagnose it.

case ActorIsolationRestriction::ActorSelf: {
auto decl = concDecl.getDecl();
ctx.Diags.diagnose(component.getLoc(),
Expand Down
5 changes: 0 additions & 5 deletions lib/Sema/TypeCheckConcurrency.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ class EnumElementDecl;
class Expr;
class FuncDecl;
class Initializer;
class LangOptions;
class PatternBindingDecl;
class ProtocolConformance;
class TopLevelCodeDecl;
Expand Down Expand Up @@ -214,10 +213,6 @@ bool diagnoseNonConcurrentTypesInReference(
ConcurrentReferenceKind refKind,
DiagnosticBehavior behavior = DiagnosticBehavior::Unspecified);

/// Whether we should diagnose cases where Sendable conformances are
/// missing.
bool shouldDiagnoseNonSendableViolations(const LangOptions &langOpts);

/// How the concurrent value check should be performed.
enum class SendableCheck {
/// Sendable conformance was explicitly stated and should be
Expand Down
4 changes: 2 additions & 2 deletions test/Concurrency/Runtime/actor_keypaths.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
// UNSUPPORTED: back_deployment_runtime

actor Page {
nonisolated let initialNumWords : Int
let initialNumWords : Int

@actorIndependent(unsafe)
var numWords : Int
Expand All @@ -19,7 +19,7 @@ actor Page {
}

actor Book {
nonisolated let pages : [Page]
let pages : [Page]

init(_ numPages : Int) {
var stack : [Page] = []
Expand Down
14 changes: 4 additions & 10 deletions test/Concurrency/actor_isolation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class Point {

@available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)
actor MyActor: MySuperActor { // expected-error{{actor types do not support inheritance}}
nonisolated let immutable: Int = 17
let immutable: Int = 17
// expected-note@+2 2{{property declared here}}
// expected-note@+1 6{{mutation of this property is only permitted within the actor}}
var mutable: Int = 71
Expand All @@ -58,8 +58,7 @@ actor MyActor: MySuperActor { // expected-error{{actor types do not support inhe
// expected-note@+1 4{{property declared here}}
var text: [String] = []

nonisolated let point : Point = Point() // expected-error{{non-isolated let property 'point' has non-Sendable type 'Point'}}
let otherPoint = Point()
let point : Point = Point()

@MainActor
var name : String = "koala" // expected-note{{property declared here}}
Expand Down Expand Up @@ -103,12 +102,7 @@ func checkAsyncPropertyAccess() async {

act.text[0] += "hello" // expected-error{{actor-isolated property 'text' can only be mutated from inside the actor}}

_ = act.point
_ = act.otherPoint
// expected-error@-1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
// expected-note@-2{{property access is 'async'}}
// expected-warning@-3{{cannot use property 'otherPoint' with a non-sendable type 'Point' across actors}}
_ = await act.otherPoint // expected-warning{{cannot use property 'otherPoint' with a non-sendable type 'Point' across actors}}
_ = act.point // expected-warning{{cannot use property 'point' with a non-sendable type 'Point' across actors}}
}

@available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)
Expand Down Expand Up @@ -652,7 +646,7 @@ actor LazyActor {
var v: Int = 0
// expected-note@-1 6 {{property declared here}}

nonisolated let l: Int = 0
let l: Int = 0

lazy var l11: Int = { v }()
lazy var l12: Int = v
Expand Down
2 changes: 1 addition & 1 deletion test/Concurrency/actor_isolation_objc.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ actor A {
_ = #keyPath(A.z)
}

nonisolated let w: Int = 0 // expected-note{{add '@objc' to expose this property to Objective-C}}
let w: Int = 0 // expected-note{{add '@objc' to expose this property to Objective-C}}

var x: Int = 0 // expected-note{{add '@objc' to expose this property to Objective-C}}

Expand Down
10 changes: 5 additions & 5 deletions test/Concurrency/actor_keypath_isolation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ class Box {
}

actor Door {
nonisolated let immutable : Int = 0
let immutable : Int = 0
let letBox : Box? = nil
let letDict : [Int : Box] = [:]
nonisolated let immutableNeighbor : Door? = nil
let immutableNeighbor : Door? = nil


var mutableNeighbor : Door? = nil
Expand Down Expand Up @@ -47,7 +47,7 @@ func tryKeyPathsMisc(d : Door) {

// in combination with other key paths

_ = (\Door.letBox).appending(path: // expected-error {{cannot form key path to actor-isolated property 'letBox'}}
_ = (\Door.letBox).appending(path: // expected-warning {{cannot form key path that accesses non-sendable type 'Box?'}}
\Box?.?.size)

_ = (\Door.varBox).appending(path: // expected-error {{cannot form key path to actor-isolated property 'varBox'}}
Expand All @@ -61,9 +61,9 @@ func tryKeyPathsFromAsync() async {
}

func tryNonSendable() {
_ = \Door.letDict[0] // expected-error {{cannot form key path to actor-isolated property 'letDict'}}
_ = \Door.letDict[0] // expected-warning {{cannot form key path that accesses non-sendable type '[Int : Box]'}}
_ = \Door.varDict[0] // expected-error {{cannot form key path to actor-isolated property 'varDict'}}
_ = \Door.letBox!.size // expected-error {{cannot form key path to actor-isolated property 'letBox'}}
_ = \Door.letBox!.size // expected-warning {{cannot form key path that accesses non-sendable type 'Box?'}}
}

func tryKeypaths() {
Expand Down
6 changes: 3 additions & 3 deletions test/Concurrency/concurrent_value_checking.swift
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ extension A1 {
_ = await self.asynchronous(nil)

// Across to a different actor, so Sendable restriction is enforced.
_ = await other.localLet // expected-warning{{cannot use property 'localLet' with a non-sendable type 'NotConcurrent' across actors}}
_ = other.localLet // expected-warning{{cannot use property 'localLet' with a non-sendable type 'NotConcurrent' across actors}}
_ = await other.synchronous() // expected-warning{{cannot call function returning non-sendable type 'NotConcurrent?' across actors}}
_ = await other.asynchronous(nil) // expected-warning{{cannot pass argument of non-sendable type 'NotConcurrent?' across actors}}
}
Expand Down Expand Up @@ -67,7 +67,7 @@ func globalAsync(_: NotConcurrent?) async {
}

func globalTest() async {
let a = await globalValue // expected-warning{{cannot use let 'globalValue' with a non-sendable type 'NotConcurrent?' across actors}}
let a = globalValue // expected-warning{{cannot use let 'globalValue' with a non-sendable type 'NotConcurrent?' across actors}}
await globalAsync(a) // expected-warning{{cannot pass argument of non-sendable type 'NotConcurrent?' across actors}}
await globalSync(a) // expected-warning{{cannot pass argument of non-sendable type 'NotConcurrent?' across actors}}
}
Expand All @@ -88,7 +88,7 @@ class ClassWithGlobalActorInits {

@MainActor
func globalTestMain(nc: NotConcurrent) async {
let a = await globalValue // expected-warning{{cannot use let 'globalValue' with a non-sendable type 'NotConcurrent?' across actors}}
let a = globalValue // expected-warning{{cannot use let 'globalValue' with a non-sendable type 'NotConcurrent?' across actors}}
await globalAsync(a) // expected-warning{{cannot pass argument of non-sendable type 'NotConcurrent?' across actors}}
await globalSync(a) // expected-warning{{cannot pass argument of non-sendable type 'NotConcurrent?' across actors}}
_ = await ClassWithGlobalActorInits(nc) // expected-warning{{cannot pass argument of non-sendable type 'NotConcurrent' across actors}}
Expand Down
15 changes: 6 additions & 9 deletions test/Concurrency/global_actor_from_ordinary_context.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ actor Alex {
func referenceGlobalActor() async {
let a = Alex()
_ = a.method
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
_ = a.const_memb // expected-note{{property access is 'async'}}
_ = a.const_memb
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
_ = a.mut_memb // expected-note{{property access is 'async'}}

Expand Down Expand Up @@ -117,14 +116,12 @@ func fromAsync() async {
y() // expected-note{{call is 'async'}}

let a = Alex()

let fn = a.method
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{3-3=await }}
fn() //expected-note{{calls to let 'fn' from outside of its actor context are implicitly asynchronous}}
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
_ = a.const_memb // expected-note{{property access is 'async'}}
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
_ = a.mut_memb // expected-note{{property access is 'async'}}
fn() // expected-error{{expression is 'async' but is not marked with 'await'}}
// expected-note@-1{{calls to let 'fn' from outside of its actor context are implicitly asynchronous}}
_ = a.const_memb
_ = a.mut_memb // expected-error{{expression is 'async' but is not marked with 'await'}}
// expected-note@-1{{property access is 'async'}}

// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
_ = a[1] // expected-note{{subscript access is 'async'}}
Expand Down