Skip to content

Commit 0cdafbf

Browse files
jturcottiDougGregor
jturcotti
authored and
DougGregor
committed
modify Sendable checking of overrides and protocol conformances. In the past, both the results and parameters of overriding (resp. conforming) functions were checked for Sendability. This is overly restrictive. For safety, the parameters of the overridden (resp. requiring) function should be checked for Sendability and the results of the overriding (resp. conforming) should be checked. This commit implements that change.
(cherry picked from commit eb78da2)
1 parent 5ff0f41 commit 0cdafbf

File tree

7 files changed

+238
-63
lines changed

7 files changed

+238
-63
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5163,8 +5163,8 @@ WARNING(non_sendable_param_type,none,
51635163
"non-sendable type %0 %select{passed in call to %4 %2 %3|"
51645164
"exiting %4 context in call to non-isolated %2 %3|"
51655165
"passed in implicitly asynchronous call to %4 %2 %3|"
5166-
"in parameter of %4 %2 %3 satisfying protocol requirement|"
5167-
"in parameter of %4 overriding %2 %3|"
5166+
"in parameter of the protocol requirement satisfied by %4 %2 %3|"
5167+
"in parameter of superclass method overridden by %4 %2 %3|"
51685168
"in parameter of %4 '@objc' %2 %3}1 cannot cross actor boundary",
51695169
(Type, unsigned, DescriptiveDeclKind, DeclName, ActorIsolation))
51705170
WARNING(non_sendable_call_param_type,none,

‎lib/Sema/TypeCheckConcurrency.cpp

Lines changed: 56 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1004,8 +1004,9 @@ bool swift::diagnoseNonSendableTypes(
10041004
}
10051005

10061006
bool swift::diagnoseNonSendableTypesInReference(
1007-
ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc loc,
1008-
SendableCheckReason reason, Optional<ActorIsolation> knownIsolation) {
1007+
ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc refLoc,
1008+
SendableCheckReason refKind, Optional<ActorIsolation> knownIsolation,
1009+
FunctionCheckKind funcCheckKind, SourceLoc diagnoseLoc) {
10091010

10101011
// Retrieve the actor isolation to use in diagnostics.
10111012
auto getActorIsolation = [&] {
@@ -1018,23 +1019,31 @@ bool swift::diagnoseNonSendableTypesInReference(
10181019
// For functions, check the parameter and result types.
10191020
SubstitutionMap subs = declRef.getSubstitutions();
10201021
if (auto function = dyn_cast<AbstractFunctionDecl>(declRef.getDecl())) {
1021-
for (auto param : *function->getParameters()) {
1022-
Type paramType = param->getInterfaceType().subst(subs);
1023-
if (diagnoseNonSendableTypes(
1024-
paramType, fromDC, loc, diag::non_sendable_param_type,
1025-
(unsigned)reason, function->getDescriptiveKind(),
1026-
function->getName(), getActorIsolation()))
1027-
return true;
1022+
if (funcCheckKind != FunctionCheckKind::Results) {
1023+
// only check params if funcCheckKind specifies so
1024+
for (auto param : *function->getParameters()) {
1025+
Type paramType = param->getInterfaceType().subst(subs);
1026+
if (diagnoseNonSendableTypes(
1027+
paramType, fromDC, refLoc, diagnoseLoc.isInvalid() ? refLoc : diagnoseLoc,
1028+
diag::non_sendable_param_type,
1029+
(unsigned)refKind, function->getDescriptiveKind(),
1030+
function->getName(), getActorIsolation()))
1031+
return true;
1032+
}
10281033
}
10291034

10301035
// Check the result type of a function.
10311036
if (auto func = dyn_cast<FuncDecl>(function)) {
1032-
Type resultType = func->getResultInterfaceType().subst(subs);
1033-
if (diagnoseNonSendableTypes(
1034-
resultType, fromDC, loc, diag::non_sendable_result_type,
1035-
(unsigned)reason, func->getDescriptiveKind(), func->getName(),
1036-
getActorIsolation()))
1037-
return true;
1037+
if (funcCheckKind != FunctionCheckKind::Params) {
1038+
// only check results if funcCheckKind specifies so
1039+
Type resultType = func->getResultInterfaceType().subst(subs);
1040+
if (diagnoseNonSendableTypes(
1041+
resultType, fromDC, refLoc, diagnoseLoc.isInvalid() ? refLoc : diagnoseLoc,
1042+
diag::non_sendable_result_type,
1043+
(unsigned)refKind, func->getDescriptiveKind(), func->getName(),
1044+
getActorIsolation()))
1045+
return true;
1046+
}
10381047
}
10391048

10401049
return false;
@@ -1045,33 +1054,40 @@ bool swift::diagnoseNonSendableTypesInReference(
10451054
? var->getType()
10461055
: var->getValueInterfaceType().subst(subs);
10471056
if (diagnoseNonSendableTypes(
1048-
propertyType, fromDC, loc,
1057+
propertyType, fromDC, refLoc,
10491058
diag::non_sendable_property_type,
10501059
var->getDescriptiveKind(), var->getName(),
10511060
var->isLocalCapture(),
1052-
(unsigned)reason,
1061+
(unsigned)refKind,
10531062
getActorIsolation()))
10541063
return true;
10551064
}
10561065

10571066
if (auto subscript = dyn_cast<SubscriptDecl>(declRef.getDecl())) {
10581067
for (auto param : *subscript->getIndices()) {
1059-
Type paramType = param->getInterfaceType().subst(subs);
1068+
if (funcCheckKind != FunctionCheckKind::Results) {
1069+
// Check params of this subscript override for sendability
1070+
Type paramType = param->getInterfaceType().subst(subs);
1071+
if (diagnoseNonSendableTypes(
1072+
paramType, fromDC, refLoc, diagnoseLoc.isInvalid() ? refLoc : diagnoseLoc,
1073+
diag::non_sendable_param_type,
1074+
(unsigned)refKind, subscript->getDescriptiveKind(),
1075+
subscript->getName(), getActorIsolation()))
1076+
return true;
1077+
}
1078+
}
1079+
1080+
if (funcCheckKind != FunctionCheckKind::Results) {
1081+
// Check the element type of a subscript.
1082+
Type resultType = subscript->getElementInterfaceType().subst(subs);
10601083
if (diagnoseNonSendableTypes(
1061-
paramType, fromDC, loc, diag::non_sendable_param_type,
1062-
(unsigned)reason, subscript->getDescriptiveKind(),
1063-
subscript->getName(), getActorIsolation()))
1084+
resultType, fromDC, refLoc, diagnoseLoc.isInvalid() ? refLoc : diagnoseLoc,
1085+
diag::non_sendable_result_type,
1086+
(unsigned)refKind, subscript->getDescriptiveKind(),
1087+
subscript->getName(), getActorIsolation()))
10641088
return true;
10651089
}
10661090

1067-
// Check the element type of a subscript.
1068-
Type resultType = subscript->getElementInterfaceType().subst(subs);
1069-
if (diagnoseNonSendableTypes(
1070-
resultType, fromDC, loc, diag::non_sendable_result_type,
1071-
(unsigned)reason, subscript->getDescriptiveKind(),
1072-
subscript->getName(), getActorIsolation()))
1073-
return true;
1074-
10751091
return false;
10761092
}
10771093

@@ -3893,7 +3909,7 @@ static ActorIsolation getOverriddenIsolationFor(ValueDecl *value) {
38933909
return isolation.subst(subs);
38943910
}
38953911

3896-
static ConcreteDeclRef getDeclRefInContext(ValueDecl *value) {
3912+
ConcreteDeclRef swift::getDeclRefInContext(ValueDecl *value) {
38973913
auto declContext = value->getInnermostDeclContext();
38983914
if (auto genericEnv = declContext->getGenericEnvironmentOfContext()) {
38993915
return ConcreteDeclRef(
@@ -4369,9 +4385,18 @@ void swift::checkOverrideActorIsolation(ValueDecl *value) {
43694385
return;
43704386

43714387
case OverrideIsolationResult::Sendable:
4388+
// Check that the results of the overriding method are sendable
43724389
diagnoseNonSendableTypesInReference(
43734390
getDeclRefInContext(value), value->getInnermostDeclContext(),
4374-
value->getLoc(), SendableCheckReason::Override);
4391+
value->getLoc(), SendableCheckReason::Override,
4392+
getActorIsolation(value), FunctionCheckKind::Results);
4393+
4394+
// Check that the parameters of the overridden method are sendable
4395+
diagnoseNonSendableTypesInReference(
4396+
getDeclRefInContext(overridden), overridden->getInnermostDeclContext(),
4397+
overridden->getLoc(), SendableCheckReason::Override,
4398+
getActorIsolation(value), FunctionCheckKind::Params,
4399+
value->getLoc());
43754400
return;
43764401

43774402
case OverrideIsolationResult::Disallowed:

‎lib/Sema/TypeCheckConcurrency.h

Lines changed: 59 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,17 @@ struct ActorReferenceResult {
246246
operator Kind() const { return kind; }
247247
};
248248

249+
/// Specifies whether checks applied to function types should
250+
/// apply to their params, results, or both
251+
enum class FunctionCheckKind {
252+
/// Check params and results
253+
ParamsResults,
254+
/// Check params only
255+
Params,
256+
/// Check results only
257+
Results,
258+
};
259+
249260
/// Diagnose the presence of any non-sendable types when referencing a
250261
/// given declaration from a particular declaration context.
251262
///
@@ -260,17 +271,26 @@ struct ActorReferenceResult {
260271
///
261272
/// \param fromDC The context from which the reference occurs.
262273
///
263-
/// \param loc The location at which the reference occurs, which will be
274+
/// \param refLoc The location at which the reference occurs, which will be
264275
/// used when emitting diagnostics.
265276
///
266277
/// \param refKind Describes what kind of reference is being made, which is
267278
/// used to tailor the diagnostic.
268279
///
280+
/// \param funcCheckKind Describes whether function types in this reference
281+
/// should be checked for sendability of their results, params, or both
282+
///
283+
/// \param diagnoseLoc Provides an alternative source location to `refLoc`
284+
/// to be used for reporting the top level diagnostic while auxiliary
285+
/// warnings and diagnostics are reported at `refLoc`.
286+
///
269287
/// \returns true if an problem was detected, false otherwise.
270288
bool diagnoseNonSendableTypesInReference(
271-
ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc loc,
289+
ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc refLoc,
272290
SendableCheckReason refKind,
273-
Optional<ActorIsolation> knownIsolation = None);
291+
Optional<ActorIsolation> knownIsolation = None,
292+
FunctionCheckKind funcCheckKind = FunctionCheckKind::ParamsResults,
293+
SourceLoc diagnoseLoc = SourceLoc());
274294

275295
/// Produce a diagnostic for a missing conformance to Sendable.
276296
void diagnoseMissingSendableConformance(
@@ -283,6 +303,9 @@ void diagnoseMissingExplicitSendable(NominalTypeDecl *nominal);
283303
/// Warn about deprecated `Executor.enqueue` implementations.
284304
void tryDiagnoseExecutorConformance(ASTContext &C, const NominalTypeDecl *nominal, ProtocolDecl *proto);
285305

306+
// Get a concrete reference to a declaration
307+
ConcreteDeclRef getDeclRefInContext(ValueDecl *value);
308+
286309
/// How the Sendable check should be performed.
287310
enum class SendableCheck {
288311
/// Sendable conformance was explicitly stated and should be
@@ -359,6 +382,37 @@ namespace detail {
359382
};
360383
}
361384

385+
/// Diagnose any non-Sendable types that occur within the given type, using
386+
/// the given diagnostic.
387+
///
388+
/// \param typeLoc is the source location of the type being diagnosed
389+
///
390+
/// \param diagnoseLoc is the source location at which the main diagnostic should
391+
/// be reported, which can differ from typeLoc
392+
///
393+
/// \returns \c true if any errors were produced, \c false if no diagnostics or
394+
/// only warnings and notes were produced.
395+
template<typename ...DiagArgs>
396+
bool diagnoseNonSendableTypes(
397+
Type type, SendableCheckContext fromContext,
398+
SourceLoc typeLoc, SourceLoc diagnoseLoc,
399+
Diag<Type, DiagArgs...> diag,
400+
typename detail::Identity<DiagArgs>::type ...diagArgs) {
401+
402+
ASTContext &ctx = fromContext.fromDC->getASTContext();
403+
return diagnoseNonSendableTypes(
404+
type, fromContext, typeLoc, [&](Type specificType,
405+
DiagnosticBehavior behavior) {
406+
407+
if (behavior != DiagnosticBehavior::Ignore) {
408+
ctx.Diags.diagnose(diagnoseLoc, diag, type, diagArgs...)
409+
.limitBehavior(behavior);
410+
}
411+
412+
return false;
413+
});
414+
}
415+
362416
/// Diagnose any non-Sendable types that occur within the given type, using
363417
/// the given diagnostic.
364418
///
@@ -369,17 +423,9 @@ bool diagnoseNonSendableTypes(
369423
Type type, SendableCheckContext fromContext, SourceLoc loc,
370424
Diag<Type, DiagArgs...> diag,
371425
typename detail::Identity<DiagArgs>::type ...diagArgs) {
372-
ASTContext &ctx = fromContext.fromDC->getASTContext();
373-
return diagnoseNonSendableTypes(
374-
type, fromContext, loc, [&](Type specificType,
375-
DiagnosticBehavior behavior) {
376-
if (behavior != DiagnosticBehavior::Ignore) {
377-
ctx.Diags.diagnose(loc, diag, type, diagArgs...)
378-
.limitBehavior(behavior);
379-
}
380426

381-
return false;
382-
});
427+
return diagnoseNonSendableTypes(type, fromContext, loc, loc, diag,
428+
std::forward<decltype(diagArgs)>(diagArgs)...);
383429
}
384430

385431
/// Diagnose this sendability error with behavior based on the import of

‎lib/Sema/TypeCheckProtocol.cpp

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3007,16 +3007,6 @@ static bool hasExplicitGlobalActorAttr(ValueDecl *decl) {
30073007

30083008
Optional<ActorIsolation> ConformanceChecker::checkActorIsolation(
30093009
ValueDecl *requirement, ValueDecl *witness) {
3010-
/// Retrieve a concrete witness for Sendable checking.
3011-
auto getConcreteWitness = [&] {
3012-
if (auto genericEnv = witness->getInnermostDeclContext()
3013-
->getGenericEnvironmentOfContext()) {
3014-
return ConcreteDeclRef(
3015-
witness, genericEnv->getForwardingSubstitutionMap());
3016-
}
3017-
3018-
return ConcreteDeclRef(witness);
3019-
};
30203010

30213011
// Determine the isolation of the requirement itself.
30223012
auto requirementIsolation = getActorIsolation(requirement);
@@ -3032,7 +3022,7 @@ Optional<ActorIsolation> ConformanceChecker::checkActorIsolation(
30323022
loc = Conformance->getLoc();
30333023

30343024
auto refResult = ActorReferenceResult::forReference(
3035-
getConcreteWitness(), witness->getLoc(), DC, None, None,
3025+
getDeclRefInContext(witness), witness->getLoc(), DC, None, None,
30363026
None, requirementIsolation);
30373027
bool sameConcurrencyDomain = false;
30383028
switch (refResult) {
@@ -3049,7 +3039,7 @@ Optional<ActorIsolation> ConformanceChecker::checkActorIsolation(
30493039

30503040
case ActorReferenceResult::ExitsActorToNonisolated:
30513041
diagnoseNonSendableTypesInReference(
3052-
getConcreteWitness(), DC, loc, SendableCheckReason::Conformance);
3042+
getDeclRefInContext(witness), DC, loc, SendableCheckReason::Conformance);
30533043
return None;
30543044

30553045
case ActorReferenceResult::EntersActor:
@@ -3131,8 +3121,19 @@ Optional<ActorIsolation> ConformanceChecker::checkActorIsolation(
31313121
witness->getAttrs().hasAttribute<NonisolatedAttr>())
31323122
return None;
31333123

3124+
// Check that the results of the witnessing method are sendable
31343125
diagnoseNonSendableTypesInReference(
3135-
getConcreteWitness(), DC, loc, SendableCheckReason::Conformance);
3126+
getDeclRefInContext(witness), DC, loc, SendableCheckReason::Conformance,
3127+
getActorIsolation(witness), FunctionCheckKind::Results);
3128+
3129+
// If this requirement is a function, check that its parameters are Sendable as well
3130+
if (isa<AbstractFunctionDecl>(requirement)) {
3131+
diagnoseNonSendableTypesInReference(
3132+
getDeclRefInContext(requirement),
3133+
requirement->getInnermostDeclContext(), requirement->getLoc(),
3134+
SendableCheckReason::Conformance, getActorIsolation(witness),
3135+
FunctionCheckKind::Params, loc);
3136+
}
31363137

31373138
// If the witness is accessible across actors, we don't need to consider it
31383139
// isolated.

‎test/Concurrency/concurrent_value_checking.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ protocol AsyncProto {
223223
}
224224

225225
extension A1: AsyncProto {
226-
func asyncMethod(_: NotConcurrent) async { } // expected-warning{{non-sendable type 'NotConcurrent' in parameter of actor-isolated instance method 'asyncMethod' satisfying protocol requirement cannot cross actor boundary}}
226+
func asyncMethod(_: NotConcurrent) async { } // expected-warning{{non-sendable type 'NotConcurrent' in parameter of the protocol requirement satisfied by actor-isolated instance method 'asyncMethod' cannot cross actor boundary}}
227227
}
228228

229229
protocol MainActorProto {
@@ -232,7 +232,7 @@ protocol MainActorProto {
232232

233233
class SomeClass: MainActorProto {
234234
@SomeGlobalActor
235-
func asyncMainMethod(_: NotConcurrent) async { } // expected-warning{{non-sendable type 'NotConcurrent' in parameter of global actor 'SomeGlobalActor'-isolated instance method 'asyncMainMethod' satisfying protocol requirement cannot cross actor boundary}}
235+
func asyncMainMethod(_: NotConcurrent) async { } // expected-warning{{non-sendable type 'NotConcurrent' in parameter of the protocol requirement satisfied by global actor 'SomeGlobalActor'-isolated instance method 'asyncMainMethod' cannot cross actor boundary}}
236236
}
237237

238238
// ----------------------------------------------------------------------

0 commit comments

Comments
 (0)