Skip to content

[Concurrency] Handle more expr types for inout checking #35325

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 2 commits into from
Jan 27, 2021
Merged

[Concurrency] Handle more expr types for inout checking #35325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f68fa1
Handle more Expr types in inout actor diags
etcwilde Jan 8, 2021
3a3aaf1
Use forEachChild for traversal instead of ad-hoc traversal
etcwilde Jan 22, 2021
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
113 changes: 57 additions & 56 deletions lib/Sema/TypeCheckConcurrency.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,68 +958,69 @@ namespace {
/// \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();
ValueDecl *valueDecl = nullptr;
if (auto binding = dyn_cast<BindOptionalExpr>(subArg))
subArg = binding->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

valueDecl = baseArg->getMember().getDecl();
} else if (DeclRefExpr *declExpr = dyn_cast<DeclRefExpr>(subArg)) {
valueDecl = declExpr->getDecl();
} else {
llvm_unreachable("Inout argument is neither a lookup nor decl.");
}
assert(valueDecl != nullptr && "valueDecl was never set!");
auto isolation = ActorIsolationRestriction::forDeclaration(valueDecl);
switch (isolation) {
case ActorIsolationRestriction::Unrestricted:
case ActorIsolationRestriction::LocalCapture:
case ActorIsolationRestriction::Unsafe:
break;
case ActorIsolationRestriction::GlobalActor: {
ctx.Diags.diagnose(call->getLoc(),
diag::actor_isolated_inout_state,
valueDecl->getDescriptiveKind(),
valueDecl->getName(),
call->implicitlyAsync());
valueDecl->diagnose(diag::kind_declared_here,
valueDecl->getDescriptiveKind());
return true;
}
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(), valueDecl->getDescriptiveKind(),
valueDecl->getName());
return true;
bool result = false;
auto checkDiagnostic = [this, call, isPartialApply,
&result](ValueDecl *decl, SourceLoc argLoc) {
auto isolation = ActorIsolationRestriction::forDeclaration(decl);
switch (isolation) {
case ActorIsolationRestriction::Unrestricted:
case ActorIsolationRestriction::LocalCapture:
case ActorIsolationRestriction::Unsafe:
break;
case ActorIsolationRestriction::GlobalActor: {
ctx.Diags.diagnose(argLoc, diag::actor_isolated_inout_state,
decl->getDescriptiveKind(), decl->getName(),
call->implicitlyAsync());
decl->diagnose(diag::kind_declared_here, decl->getDescriptiveKind());
result = true;
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(), decl->getDescriptiveKind(),
decl->getName());
result = true;
}
} else {
ctx.Diags.diagnose(argLoc, diag::actor_isolated_inout_state,
decl->getDescriptiveKind(), decl->getName(),
call->implicitlyAsync());
result = true;
}
} else {
ctx.Diags.diagnose(
subArg->getLoc(), diag::actor_isolated_inout_state,
valueDecl->getDescriptiveKind(), valueDecl->getName(), call->implicitlyAsync());
return true;
break;
}
}
}
return false;
}
};
auto expressionWalker = [baseArg = arg->getSubExpr(),
checkDiagnostic](Expr *expr) -> Expr * {
if (isa<InOutExpr>(expr))
return nullptr; // AST walker will hit this again
if (LookupExpr *lookup = dyn_cast<LookupExpr>(expr)) {
if (isa<DeclRefExpr>(lookup->getBase())) {
checkDiagnostic(lookup->getMember().getDecl(), baseArg->getLoc());
return nullptr; // Diagnosed. Don't keep walking
}
}
if (DeclRefExpr *declRef = dyn_cast<DeclRefExpr>(expr)) {
checkDiagnostic(declRef->getDecl(), baseArg->getLoc());
return nullptr; // Diagnosed. Don't keep walking
}
return expr;
};
arg->getSubExpr()->forEachChildExpr(expressionWalker);
return result;
}

/// Get the actor isolation of the innermost relevant context.
Expand Down
60 changes: 60 additions & 0 deletions test/Concurrency/actor_inout_isolation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
struct Point {
var x: Int
var y: Int
var z: Int? = nil

mutating func setComponents(x: inout Int, y: inout Int) async {
defer { (x, y) = (self.x, self.y) }
Expand All @@ -22,10 +23,12 @@ struct Point {
}

actor class TestActor {
// expected-note@+1{{mutable state is only available within the actor instance}}
var position = Point(x: 0, y: 0)
var nextPosition = Point(x: 0, y: 1)
var value1: Int = 0
var value2: Int = 1
var points: [Point] = []
}

func modifyAsynchronously(_ foo: inout Int) async { foo += 1 }
Expand Down Expand Up @@ -56,6 +59,15 @@ extension TestActor {
// expected-error@+1{{actor-isolated property 'position' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&position.x)
}

func nestedExprs() async {
// expected-error@+1{{actor-isolated property 'position' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&position.z!)

// expected-error@+1{{actor-isolated property 'points' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&points[0].z!)
}

}

// internal method call
Expand Down Expand Up @@ -132,6 +144,40 @@ extension TestActor {
}
}

actor class MyActor {
var points: [Point] = []
var int: Int = 0
var maybeInt: Int?
var maybePoint: Point?
var myActor: TestActor = TestActor()

// Checking that various ways of unwrapping emit the right error messages at
// the right times and that illegal operations are caught
func modifyStuff() async {
// expected-error@+1{{actor-isolated property 'points' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&points[0].x)
// expected-error@+1{{actor-isolated property 'points' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&points[0].z!)
// expected-error@+1{{actor-isolated property 'int' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&int)
// expected-error@+1{{actor-isolated property 'maybeInt' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&maybeInt!)
// expected-error@+1{{actor-isolated property 'maybePoint' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&maybePoint!.z!)
// expected-error@+1{{actor-isolated property 'int' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&(int))

// This warning is emitted because this fails to typecheck before the
// async-ness is attached.
// expected-warning@+2{{no calls to 'async' functions occur within 'await' expression}}
// expected-error@+1{{cannot pass immutable value of type 'Int' as inout argument}}
await modifyAsynchronously(&(maybePoint?.z)!)
// expected-error@+2{{actor-isolated property 'position' can only be referenced inside the actor}}
// expected-error@+1{{actor-isolated property 'myActor' cannot be passed 'inout' to 'async' function call}}
await modifyAsynchronously(&myActor.position.x)
}
}

// Verify global actor protection

@globalActor
Expand Down Expand Up @@ -159,3 +205,17 @@ func globalSyncFunction(_ foo: inout Int) { }
@MyGlobalActor func globalActorAsyncOkay() async { globalActorSyncFunction(&number) }
@MyGlobalActor func globalActorAsyncOkay2() async { globalSyncFunction(&number) }
@MyGlobalActor func globalActorSyncOkay() { globalSyncFunction(&number) }

// Gently unwrap things that are fine
struct Cat {
mutating func meow() async { }
}

struct Dog {
var cat: Cat?

mutating func woof() async {
// This used to cause the compiler to crash, but should be fine
await cat?.meow()
}
}