Skip to content

Commit

Permalink
Merge pull request swiftlang#34688 from etcwilde/ewilde/disallow-pass…
Browse files Browse the repository at this point in the history
…ing-actor-state-inout

[Concurrency] Ban passing actor state via inout

Resolves: rdar://70635479
  • Loading branch information
etcwilde authored Dec 15, 2020
2 parents fdcb9f8 + 66d307e commit f3da35a
Show file tree
Hide file tree
Showing 3 changed files with 231 additions and 2 deletions.
7 changes: 7 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -4210,6 +4210,13 @@ ERROR(actor_isolated_self_independent_context,none,
"actor-isolated %0 %1 can not be referenced from an "
"'@actorIndependent' context",
(DescriptiveDeclKind, DeclName))
ERROR(actor_isolated_inout_state,none,
"actor-isolated %0 %1 cannot be passed 'inout' to"
"%select{| implicitly}2 'async' function call",
(DescriptiveDeclKind, DeclName, bool))
ERROR(actor_isolated_mutating_func,none,
"cannot call mutating async function %0 on actor-isolated %1 %2",
(DeclName, DescriptiveDeclKind, DeclName))
ERROR(actor_isolated_global_actor_context,none,
"actor-isolated %0 %1 can not be referenced from context of global "
"actor %2",
Expand Down
93 changes: 91 additions & 2 deletions lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,24 @@ findMemberReference(Expr *expr) {
return None;
}

/// Return true if the callee of an ApplyExpr is async
///
/// Note that this must be called after the implicitlyAsync flag has been set,
/// or implicitly async calls will not return the correct value.
static bool isAsyncCall(const ApplyExpr *call) {
if (call->implicitlyAsync())
return true;

// Effectively the same as doing a
// `cast_or_null<FunctionType>(call->getFn()->getType())`, check the
// result of that and then checking `isAsync` if it's defined.
Type funcTypeType = call->getFn()->getType();
if (!funcTypeType)
return false;
FunctionType *funcType = funcTypeType->castTo<FunctionType>();
return funcType->isAsync();
}

namespace {
/// Check for adherence to the actor isolation rules, emitting errors
/// when actor-isolated declarations are used in an unsafe manner.
Expand Down Expand Up @@ -679,6 +697,11 @@ namespace {
return { true, expr };
}

if (auto inout = dyn_cast<InOutExpr>(expr)) {
if (!applyStack.empty())
diagnoseInOutArg(applyStack.back(), inout, false);
}

if (auto lookup = dyn_cast<LookupExpr>(expr)) {
checkMemberReference(lookup->getBase(), lookup->getMember(),
lookup->getLoc());
Expand Down Expand Up @@ -720,11 +743,22 @@ namespace {
Expr *fn = call->getFn()->getValueProvidingExpr();
if (auto memberRef = findMemberReference(fn)) {
checkMemberReference(
call->getArg(), memberRef->first, memberRef->second,
call->getArg(), memberRef->first, memberRef->second,
/*isEscapingPartialApply=*/false, /*maybeImplicitAsync=*/true);

call->getArg()->walk(*this);

if (applyStack.size() >= 2) {
ApplyExpr *outerCall = applyStack[applyStack.size() - 2];
if (isAsyncCall(outerCall)) {
// This call is a partial application within an async call.
// If the partial application take a value inout, it is bad.
if (InOutExpr *inoutArg = dyn_cast<InOutExpr>(
call->getArg()->getSemanticsProvidingExpr()))
diagnoseInOutArg(outerCall, inoutArg, true);
}
}

// manual clean-up since normal traversal is skipped
assert(applyStack.back() == dyn_cast<ApplyExpr>(expr));
applyStack.pop_back();
Expand Down Expand Up @@ -860,6 +894,59 @@ namespace {
return true;
}

/// Diagnose an inout argument passed into an async call
///
/// \returns true if we diagnosed the entity, \c false otherwise.
bool diagnoseInOutArg(const ApplyExpr *call, const InOutExpr *arg,
bool isPartialApply) {
// check that the call is actually async
if (!isAsyncCall(call))
return false;

Expr *subArg = arg->getSubExpr();
if (LookupExpr *baseArg = dyn_cast<LookupExpr>(subArg)) {
while (LookupExpr *nextLayer = dyn_cast<LookupExpr>(baseArg->getBase()))
baseArg = nextLayer;
// subArg: the actual property being passed inout
// baseArg: the property in the actor who's property is being passed
// inout

auto memberDecl = baseArg->getMember().getDecl();
auto isolation = ActorIsolationRestriction::forDeclaration(memberDecl);
switch (isolation) {
case ActorIsolationRestriction::Unrestricted:
case ActorIsolationRestriction::LocalCapture:
case ActorIsolationRestriction::Unsafe:
case ActorIsolationRestriction::GlobalActor: // TODO: handle global
// actors
break;
case ActorIsolationRestriction::ActorSelf: {
if (isPartialApply) {
// The partially applied InoutArg is a property of actor. This can
// really only happen when the property is a struct with a mutating
// async method.
if (auto partialApply = dyn_cast<ApplyExpr>(call->getFn())) {
ValueDecl *fnDecl =
cast<DeclRefExpr>(partialApply->getFn())->getDecl();
ctx.Diags.diagnose(
call->getLoc(), diag::actor_isolated_mutating_func,
fnDecl->getName(), memberDecl->getDescriptiveKind(),
memberDecl->getName());
return true;
}
} else {
ctx.Diags.diagnose(
subArg->getLoc(), diag::actor_isolated_inout_state,
memberDecl->getDescriptiveKind(), memberDecl->getName(),
call->implicitlyAsync());
return true;
}
}
}
}
return false;
}

/// Get the actor isolation of the innermost relevant context.
ActorIsolation getInnermostIsolatedContext(const DeclContext *constDC) {
// Retrieve the actor isolation for a declaration somewhere in our
Expand Down Expand Up @@ -1196,7 +1283,9 @@ namespace {
llvm_unreachable("Locals cannot be referenced with member syntax");

case ActorIsolationRestriction::Unsafe:
return diagnoseReferenceToUnsafe(member, memberLoc);
// This case is hit when passing actor state inout to functions in some
// cases. The error is emitted by diagnoseInOutArg.
return false;
}
llvm_unreachable("unhandled actor isolation kind!");
}
Expand Down
133 changes: 133 additions & 0 deletions test/Concurrency/actor_inout_isolation.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
// RUN: %target-typecheck-verify-swift -enable-experimental-concurrency
// REQUIRES: concurrency

// Verify that we don't allow actor-isolated state to be passed via inout
// Check:
// - can't pass it into a normal async function
// - can't pass it into a first-class async function as a value
// - can't pass it into another actor method
// - can't pass it into a curried/partially applied function
// - can't pass it inout to a function that doesn't directly touch it
// - can't pass it into a function that was passed into the calling method
// - can't call async mutating function on actor isolated state

struct Point {
var x: Int
var y: Int

mutating func setComponents(x: inout Int, y: inout Int) async {
defer { (x, y) = (self.x, self.y) }
(self.x, self.y) = (x, y)
}
}

actor class TestActor {
var position = Point(x: 0, y: 0)
var nextPosition = Point(x: 0, y: 1)
var value1: Int = 0
var value2: Int = 1
}

func modifyAsynchronously(_ foo: inout Int) async { foo += 1 }
let modifyAsyncValue = modifyAsynchronously

// external function call
extension TestActor {

// Can't pass actor-isolated primitive into a function
func inoutAsyncFunctionCall() async {
// expected-error@+1{{actor-isolated property 'value1' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&value1)
}

func inoutAsyncClosureCall() async {
// expected-error@+1{{actor-isolated property 'value1' cannot be passed 'inout' to 'async' function call}}
await { (_ foo: inout Int) async in foo += 1 }(&value1)
}

// Can't pass actor-isolated primitive into first-class function value
func inoutAsyncValueCall() async {
// expected-error@+1{{actor-isolated property 'value1' cannot be passed 'inout' to 'async' function call}}
await modifyAsyncValue(&value1)
}

// Can't pass property of actor-isolated state inout to async function
func inoutPropertyStateValueCall() async {
// expected-error@+1{{actor-isolated property 'position' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&position.x)
}
}

// internal method call
extension TestActor {
func modifyByValue(_ other: inout Int) async {
other += value1
}

func passStateIntoMethod() async {
// expected-error@+1{{actor-isolated property 'value1' cannot be passed 'inout' to 'async' function call}}
await modifyByValue(&value1)
}
}

// external class method call
class NonAsyncClass {
func modifyOtherAsync(_ other : inout Int) async {
// ...
}

func modifyOtherNotAsync(_ other: inout Int) {
// ...
}
}

// Calling external class/struct async function
extension TestActor {
// Can't pass state into async method of another class

func passStateIntoDifferentClassMethod() async {
let other = NonAsyncClass()
let otherCurry = other.modifyOtherAsync
// expected-error@+1{{actor-isolated property 'value2' cannot be passed 'inout' to 'async' function call}}
await other.modifyOtherAsync(&value2)
// expected-error@+1{{actor-isolated property 'value1' cannot be passed 'inout' to 'async' function call}}
await otherCurry(&value1)
other.modifyOtherNotAsync(&value2) // This is okay since it's not async!

}

func callMutatingFunctionOnStruct() async {
// expected-error@+3:20{{cannot call mutating async function 'setComponents(x:y:)' on actor-isolated property 'position'}}
// expected-error@+2:51{{actor-isolated property 'nextPosition' cannot be passed 'inout' to 'async' function call}}
// expected-error@+1:71{{actor-isolated property 'nextPosition' cannot be passed 'inout' to 'async' function call}}
await position.setComponents(x: &nextPosition.x, y: &nextPosition.y)

// expected-error@+3:20{{cannot call mutating async function 'setComponents(x:y:)' on actor-isolated property 'position'}}
// expected-error@+2:38{{actor-isolated property 'value1' cannot be passed 'inout' to 'async' function call}}
// expected-error@+1:50{{actor-isolated property 'value2' cannot be passed 'inout' to 'async' function call}}
await position.setComponents(x: &value1, y: &value2)
}
}

// Check implicit async testing
actor class DifferentActor {
func modify(_ state: inout Int) {}
}

extension TestActor {
func modify(_ state: inout Int) {}

// Actor state passed inout to implicitly async function on an actor of the
// same type
func modifiedByOtherTestActor(_ other: TestActor) async {
//expected-error@+1{{actor-isolated property 'value2' cannot be passed 'inout' to implicitly 'async' function call}}
await other.modify(&value2)
}

// Actor state passed inout to an implicitly async function on an actor of a
// different type
func modifiedByOther(_ other: DifferentActor) async {
//expected-error@+1{{actor-isolated property 'value2' cannot be passed 'inout' to implicitly 'async' function call}}
await other.modify(&value2)
}
}

0 comments on commit f3da35a

Please sign in to comment.