Skip to content

enforce under strict concurrency that globals and statics be either… #67974

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
9 changes: 9 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -5243,6 +5243,15 @@ NOTE(actor_mutable_state,none,
WARNING(shared_mutable_state_access,none,
"reference to %kind0 is not concurrency-safe because it involves "
"shared mutable state", (const ValueDecl *))
WARNING(shared_mutable_state_decl,none,
"%kind0 is not concurrency-safe because it is non-isolated global "
"shared mutable state", (const ValueDecl *))
NOTE(shared_mutable_state_decl_note,none,
"isolate %0 to a global actor, or convert it to a 'let' constant and "
"conform it to 'Sendable'", (const ValueDecl *))
WARNING(shared_immutable_state_decl,none,
"%kind0 is not concurrency-safe because it is not either conforming to "
"'Sendable' or isolated to a global actor", (const ValueDecl *))
ERROR(actor_isolated_witness,none,
"%select{|distributed }0%1 %kind2 cannot be used to satisfy %3 protocol "
"requirement",
Expand Down
3 changes: 3 additions & 0 deletions include/swift/Basic/Features.def
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,9 @@ EXPERIMENTAL_FEATURE(StrictConcurrency, true)
/// Defer Sendable checking to SIL diagnostic phase.
EXPERIMENTAL_FEATURE(SendNonSendable, false)

/// Within strict concurrency, narrow global variable isolation requirements.
EXPERIMENTAL_FEATURE(GlobalConcurrency, false)

/// Enable extended callbacks (with additional parameters) to be used when the
/// "playground transform" is enabled.
EXPERIMENTAL_FEATURE(PlaygroundExtendedCallbacks, true)
Expand Down
2 changes: 2 additions & 0 deletions lib/AST/ASTPrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3504,6 +3504,8 @@ static bool usesFeatureSendNonSendable(Decl *decl) {
return false;
}

static bool usesFeatureGlobalConcurrency(Decl *decl) { return false; }

static bool usesFeaturePlaygroundExtendedCallbacks(Decl *decl) {
return false;
}
Expand Down
9 changes: 1 addition & 8 deletions lib/Frontend/CompilerInvocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -953,14 +953,7 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
if (Opts.isSwiftVersionAtLeast(6)) {
Opts.StrictConcurrencyLevel = StrictConcurrency::Complete;
} else if (const Arg *A = Args.getLastArg(OPT_strict_concurrency)) {
auto value =
llvm::StringSwitch<llvm::Optional<StrictConcurrency>>(A->getValue())
.Case("minimal", StrictConcurrency::Minimal)
.Case("targeted", StrictConcurrency::Targeted)
.Case("complete", StrictConcurrency::Complete)
.Default(llvm::None);

if (value)
if (auto value = parseStrictConcurrency(A->getValue()))
Opts.StrictConcurrencyLevel = *value;
else
Diags.diagnose(SourceLoc(), diag::error_invalid_arg_value,
Expand Down
166 changes: 97 additions & 69 deletions lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3145,10 +3145,7 @@ namespace {
isolatedActor, llvm::None, llvm::None, getClosureActorIsolation);
switch (result) {
case ActorReferenceResult::SameConcurrencyDomain:
if (diagnoseReferenceToUnsafeGlobal(decl, loc))
return true;

return false;
return diagnoseReferenceToUnsafeGlobal(decl, loc);

case ActorReferenceResult::ExitsActorToNonisolated:
if (diagnoseReferenceToUnsafeGlobal(decl, loc))
Expand Down Expand Up @@ -4073,6 +4070,33 @@ ActorIsolation ActorIsolationRequest::evaluate(
return ActorIsolation::forActorInstanceParameter(actor, *paramIdx);
}

// Diagnose global state that is not either immutable plus Sendable or
// isolated to a global actor.
auto checkGlobalIsolation = [var = dyn_cast<VarDecl>(value)](
ActorIsolation isolation) {
if (var && var->getLoc() &&
var->getASTContext().LangOpts.hasFeature(Feature::GlobalConcurrency) &&
!isolation.isGlobalActor()) {
const bool isGlobalState =
var->isStatic() || var->getDeclContext()->isModuleScopeContext() ||
(var->getDeclContext()->isTypeContext() && !var->isInstanceMember());
auto *classDecl = var->getDeclContext()->getSelfClassDecl();
const bool isActorType = classDecl && classDecl->isAnyActor();
if (isGlobalState && !isActorType) {
if (var->isLet()) {
if (!isSendableType(var->getModuleContext(),
var->getInterfaceType())) {
var->diagnose(diag::shared_immutable_state_decl, var);
}
} else {
var->diagnose(diag::shared_mutable_state_decl, var);
var->diagnose(diag::shared_mutable_state_decl_note, var);
}
}
}
return isolation;
};

auto isolationFromAttr = getIsolationFromAttributes(value);
if (FuncDecl *fd = dyn_cast<FuncDecl>(value)) {
// Main.main() and Main.$main are implicitly MainActor-protected.
Expand All @@ -4099,7 +4123,7 @@ ActorIsolation ActorIsolationRequest::evaluate(
checkClassGlobalActorIsolation(classDecl, *isolationFromAttr);
}

return *isolationFromAttr;
return checkGlobalIsolation(*isolationFromAttr);
}

// Determine the default isolation for this declaration, which may still be
Expand Down Expand Up @@ -4130,78 +4154,82 @@ ActorIsolation ActorIsolationRequest::evaluate(
}

// Function used when returning an inferred isolation.
auto inferredIsolation = [&](
ActorIsolation inferred, bool onlyGlobal = false) {
// check if the inferred isolation is valid in the context of
// its overridden isolation.
if (overriddenValue) {
// if the inferred isolation is not valid, then carry-over the overridden
// declaration's isolation as this decl's inferred isolation.
switch (validOverrideIsolation(
value, inferred, overriddenValue, *overriddenIso)) {
case OverrideIsolationResult::Allowed:
case OverrideIsolationResult::Sendable:
break;

case OverrideIsolationResult::Disallowed:
inferred = *overriddenIso;
break;
}
}
auto inferredIsolation = [&](ActorIsolation inferred,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

inlining an immediately invoked lambda added an extra level of indentation to the body of inferredIsolation, so if you filter white space from the diff, it should reduce it significantly.

bool onlyGlobal = false) {
// Invoke the body within checkGlobalIsolation to check the result.
return checkGlobalIsolation([&] {
// check if the inferred isolation is valid in the context of
// its overridden isolation.
if (overriddenValue) {
// if the inferred isolation is not valid, then carry-over the
// overridden declaration's isolation as this decl's inferred isolation.
switch (validOverrideIsolation(value, inferred, overriddenValue,
*overriddenIso)) {
case OverrideIsolationResult::Allowed:
case OverrideIsolationResult::Sendable:
break;

// Add an implicit attribute to capture the actor isolation that was
// inferred, so that (e.g.) it will be printed and serialized.
ASTContext &ctx = value->getASTContext();
switch (inferred) {
case ActorIsolation::Independent:
// Stored properties cannot be non-isolated, so don't infer it.
if (auto var = dyn_cast<VarDecl>(value)) {
if (!var->isStatic() && var->hasStorage())
return ActorIsolation::forUnspecified()
.withPreconcurrency(inferred.preconcurrency());
case OverrideIsolationResult::Disallowed:
inferred = *overriddenIso;
break;
}
}

// Add an implicit attribute to capture the actor isolation that was
// inferred, so that (e.g.) it will be printed and serialized.
ASTContext &ctx = value->getASTContext();
switch (inferred) {
case ActorIsolation::Independent:
// Stored properties cannot be non-isolated, so don't infer it.
if (auto var = dyn_cast<VarDecl>(value)) {
if (!var->isStatic() && var->hasStorage())
return ActorIsolation::forUnspecified().withPreconcurrency(
inferred.preconcurrency());
}

if (onlyGlobal)
return ActorIsolation::forUnspecified()
.withPreconcurrency(inferred.preconcurrency());
if (onlyGlobal) {
return ActorIsolation::forUnspecified().withPreconcurrency(
inferred.preconcurrency());
}

value->getAttrs().add(new (ctx) NonisolatedAttr(/*IsImplicit=*/true));
break;
value->getAttrs().add(new (ctx) NonisolatedAttr(/*IsImplicit=*/true));
break;

case ActorIsolation::GlobalActorUnsafe:
case ActorIsolation::GlobalActor: {
// Stored properties of a struct don't need global-actor isolation.
if (ctx.isSwiftVersionAtLeast(6))
if (auto *var = dyn_cast<VarDecl>(value))
if (!var->isStatic() && var->isOrdinaryStoredProperty())
if (auto *varDC = var->getDeclContext())
if (auto *nominal = varDC->getSelfNominalTypeDecl())
if (isa<StructDecl>(nominal) &&
!isWrappedValueOfPropWrapper(var))
return ActorIsolation::forUnspecified()
.withPreconcurrency(inferred.preconcurrency());

auto typeExpr = TypeExpr::createImplicit(inferred.getGlobalActor(), ctx);
auto attr = CustomAttr::create(
ctx, SourceLoc(), typeExpr, /*implicit=*/true);
if (inferred == ActorIsolation::GlobalActorUnsafe)
attr->setArgIsUnsafe(true);
value->getAttrs().add(attr);
break;
}
case ActorIsolation::GlobalActorUnsafe:
case ActorIsolation::GlobalActor: {
// Stored properties of a struct don't need global-actor isolation.
if (ctx.isSwiftVersionAtLeast(6))
if (auto *var = dyn_cast<VarDecl>(value))
if (!var->isStatic() && var->isOrdinaryStoredProperty())
if (auto *varDC = var->getDeclContext())
if (auto *nominal = varDC->getSelfNominalTypeDecl())
if (isa<StructDecl>(nominal) &&
!isWrappedValueOfPropWrapper(var))
return ActorIsolation::forUnspecified().withPreconcurrency(
inferred.preconcurrency());

auto typeExpr =
TypeExpr::createImplicit(inferred.getGlobalActor(), ctx);
auto attr =
CustomAttr::create(ctx, SourceLoc(), typeExpr, /*implicit=*/true);
if (inferred == ActorIsolation::GlobalActorUnsafe)
attr->setArgIsUnsafe(true);
value->getAttrs().add(attr);
break;
}

case ActorIsolation::ActorInstance:
case ActorIsolation::Unspecified:
if (onlyGlobal)
return ActorIsolation::forUnspecified()
.withPreconcurrency(inferred.preconcurrency());
case ActorIsolation::ActorInstance:
case ActorIsolation::Unspecified:
if (onlyGlobal)
return ActorIsolation::forUnspecified().withPreconcurrency(
inferred.preconcurrency());

// Nothing to do.
break;
}
// Nothing to do.
break;
}

return inferred;
return inferred;
}());
};

// If this is a local function, inherit the actor isolation from its
Expand Down Expand Up @@ -4332,7 +4360,7 @@ ActorIsolation ActorIsolationRequest::evaluate(
}

// Default isolation for this member.
return defaultIsolation;
return checkGlobalIsolation(defaultIsolation);
}

bool HasIsolatedSelfRequest::evaluate(
Expand Down
41 changes: 39 additions & 2 deletions test/Concurrency/experimental_feature_strictconcurrency.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// RUN: %target-typecheck-verify-swift -enable-experimental-feature StrictConcurrency
// RUN: %target-typecheck-verify-swift -enable-experimental-feature StrictConcurrency=complete
// RUN: %target-typecheck-verify-swift -disable-availability-checking -parse-as-library -enable-experimental-feature StrictConcurrency -enable-experimental-feature GlobalConcurrency
// RUN: %target-typecheck-verify-swift -disable-availability-checking -parse-as-library -enable-experimental-feature StrictConcurrency=complete -enable-experimental-feature GlobalConcurrency
// REQUIRES: concurrency

class C1 { } // expected-note{{class 'C1' does not conform to the 'Sendable' protocol}}
Expand All @@ -20,3 +20,40 @@ struct Test2: TestProtocol { // expected-warning{{conformance of 'C2' to 'Sendab
// expected-note@-1{{in associated type 'Self.Value' (inferred as 'C2')}}
typealias Value = C2
}

@globalActor
actor TestGlobalActor {
static var shared = TestGlobalActor()
}

@TestGlobalActor
var mutableIsolatedGlobal = 1

var mutableNonisolatedGlobal = 1 // expected-warning{{var 'mutableNonisolatedGlobal' is not concurrency-safe because it is non-isolated global shared mutable state}}
// expected-note@-1{{isolate 'mutableNonisolatedGlobal' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}

let immutableGlobal = 1

final class TestSendable: Sendable {
init() {}
}

final class TestNonsendable {
init() {}
}

struct A {
static let immutableExplicitSendable = TestSendable()
static let immutableNonsendableGlobal = TestNonsendable() // expected-warning{{static property 'immutableNonsendableGlobal' is not concurrency-safe because it is not either conforming to 'Sendable' or isolated to a global actor}}
static let immutableInferredSendable = 0
static var mutable = 0 // expected-warning{{static property 'mutable' is not concurrency-safe because it is non-isolated global shared mutable state}}
// expected-note@-1{{isolate 'mutable' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
// expected-note@-2{{static property declared here}}
}

@TestGlobalActor
func f() {
print(A.immutableExplicitSendable)
print(A.immutableInferredSendable)
print(A.mutable) // expected-warning{{reference to static property 'mutable' is not concurrency-safe because it involves shared mutable state}}
}