[Concurrency] Diagnose non-Sendable 'self' arguments crossing actor isolation

hborla · hborla · commit 84eaeb766fa4 · 2023-08-22T21:28:33.000-07:00
boundaries in member reference expressions.
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -999,7 +999,8 @@ bool swift::diagnoseNonSendableTypes(
 }
 
 bool swift::diagnoseNonSendableTypesInReference(
-    ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc refLoc,
+    Expr *base, ConcreteDeclRef declRef,
+    const DeclContext *fromDC, SourceLoc refLoc,
     SendableCheckReason refKind, llvm::Optional<ActorIsolation> knownIsolation,
     FunctionCheckKind funcCheckKind, SourceLoc diagnoseLoc) {
   // Retrieve the actor isolation to use in diagnostics.
@@ -1010,6 +1011,17 @@ bool swift::diagnoseNonSendableTypesInReference(
     return swift::getActorIsolation(declRef.getDecl());
   };
 
+  // Check the 'self' argument.
+  if (base) {
+    if (diagnoseNonSendableTypes(
+            base->getType(),
+            fromDC, base->getStartLoc(),
+            diag::non_sendable_param_type,
+            (unsigned)refKind, declRef.getDecl(),
+            getActorIsolation()))
+      return true;
+  }
+
   // For functions, check the parameter and result types.
   SubstitutionMap subs = declRef.getSubstitutions();
   if (auto function = dyn_cast<AbstractFunctionDecl>(declRef.getDecl())) {
@@ -2625,7 +2637,6 @@ namespace {
       FoundAsync, // successfully marked an implicitly-async operation
       NotFound,  // fail: no valid implicitly-async operation was found
       SyncContext, // fail: a valid implicitly-async op, but in sync context
-      NotSendable,  // fail: valid op and context, but not Sendable
       NotDistributed, // fail: non-distributed declaration in distributed actor
     };
 
@@ -2744,16 +2755,6 @@ namespace {
         }
       }
 
-      if (result == AsyncMarkingResult::FoundAsync) {
-        // Check for non-sendable types.
-        bool problemFound =
-            diagnoseNonSendableTypesInReference(
-              concDeclRef, getDeclContext(), declLoc,
-              SendableCheckReason::SynchronousAsAsync);
-        if (problemFound)
-          result = AsyncMarkingResult::NotSendable;
-      }
-
       return result;
     }
 
@@ -3155,7 +3156,7 @@ namespace {
           return true;
 
         return diagnoseNonSendableTypesInReference(
-                   declRef, getDeclContext(), loc,
+                   base, declRef, getDeclContext(), loc,
                    SendableCheckReason::ExitingActor,
                    result.isolation);
 
@@ -3190,7 +3191,7 @@ namespace {
       // Sendable checking and we're done.
       if (!result.options) {
         return diagnoseNonSendableTypesInReference(
-                   declRef, getDeclContext(), loc,
+                   base, declRef, getDeclContext(), loc,
                    SendableCheckReason::CrossActor);
       }
 
@@ -3203,11 +3204,11 @@ namespace {
           loc, declRef, context, result.isolation, isDistributed);
       switch (implicitAsyncResult) {
       case AsyncMarkingResult::FoundAsync:
-        // Success! We're done.
-        return false;
+        return diagnoseNonSendableTypesInReference(
+            base, declRef, getDeclContext(), loc,
+            SendableCheckReason::SynchronousAsAsync);
 
       case AsyncMarkingResult::NotDistributed:
-      case AsyncMarkingResult::NotSendable:
         // Failed, but diagnostics have already been emitted.
         return true;
 
@@ -4426,12 +4427,14 @@ void swift::checkOverrideActorIsolation(ValueDecl *value) {
   case OverrideIsolationResult::Sendable:
     // Check that the results of the overriding method are sendable
     diagnoseNonSendableTypesInReference(
+        /*base=*/nullptr,
         getDeclRefInContext(value), value->getInnermostDeclContext(),
         value->getLoc(), SendableCheckReason::Override,
         getActorIsolation(value), FunctionCheckKind::Results);
 
     // Check that the parameters of the overridden method are sendable
     diagnoseNonSendableTypesInReference(
+        /*base=*/nullptr,
         getDeclRefInContext(overridden), overridden->getInnermostDeclContext(),
         overridden->getLoc(), SendableCheckReason::Override,
         getActorIsolation(value), FunctionCheckKind::Params,
diff --git a/lib/Sema/TypeCheckConcurrency.h b/lib/Sema/TypeCheckConcurrency.h
@@ -265,6 +265,9 @@ enum class FunctionCheckKind {
 /// concurrency domain, whether in/out of an actor or in/or of a concurrent
 /// function or closure.
 ///
+/// \param base The base expression of the reference, which must be 'Sendable'
+/// in order to cross actor isolation boundaries.
+///
 /// \param declRef The declaration being referenced from another concurrency
 /// domain, including the substitutions so that (e.g.) we can consider the
 /// specific types at the use site.
@@ -286,7 +289,8 @@ enum class FunctionCheckKind {
 ///
 /// \returns true if an problem was detected, false otherwise.
 bool diagnoseNonSendableTypesInReference(
-    ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc refLoc,
+    Expr *base, ConcreteDeclRef declRef,
+    const DeclContext *fromDC, SourceLoc refLoc,
     SendableCheckReason refKind,
     llvm::Optional<ActorIsolation> knownIsolation = llvm::None,
     FunctionCheckKind funcCheckKind = FunctionCheckKind::ParamsResults,
diff --git a/lib/Sema/TypeCheckDeclObjC.cpp b/lib/Sema/TypeCheckDeclObjC.cpp
@@ -470,7 +470,7 @@ static bool checkObjCActorIsolation(const ValueDecl *VD, ObjCReason Reason) {
 
     // FIXME: Substitution map?
     diagnoseNonSendableTypesInReference(
-        const_cast<ValueDecl *>(VD), VD->getDeclContext(),
+        /*base=*/nullptr, const_cast<ValueDecl *>(VD), VD->getDeclContext(),
         VD->getLoc(), SendableCheckReason::ObjC);
     return false;
 
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -3041,7 +3041,8 @@ ConformanceChecker::checkActorIsolation(ValueDecl *requirement,
 
   case ActorReferenceResult::ExitsActorToNonisolated:
     diagnoseNonSendableTypesInReference(
-        getDeclRefInContext(witness), DC, loc, SendableCheckReason::Conformance);
+        /*base=*/nullptr, getDeclRefInContext(witness),
+        DC, loc, SendableCheckReason::Conformance);
     return llvm::None;
   case ActorReferenceResult::EntersActor:
     // Handled below.
@@ -3124,13 +3125,14 @@ ConformanceChecker::checkActorIsolation(ValueDecl *requirement,
 
     // Check that the results of the witnessing method are sendable
     diagnoseNonSendableTypesInReference(
-        getDeclRefInContext(witness), DC, loc, SendableCheckReason::Conformance,
+        /*base=*/nullptr, getDeclRefInContext(witness),
+        DC, loc, SendableCheckReason::Conformance,
         getActorIsolation(witness), FunctionCheckKind::Results);
 
     // If this requirement is a function, check that its parameters are Sendable as well
     if (isa<AbstractFunctionDecl>(requirement)) {
       diagnoseNonSendableTypesInReference(
-          getDeclRefInContext(requirement),
+          /*base=*/nullptr, getDeclRefInContext(requirement),
           requirement->getInnermostDeclContext(), requirement->getLoc(),
           SendableCheckReason::Conformance, getActorIsolation(witness),
           FunctionCheckKind::Params, loc);
diff --git a/test/Concurrency/Inputs/other_global_actor_inference.swift b/test/Concurrency/Inputs/other_global_actor_inference.swift
@@ -5,7 +5,7 @@ public dynamic func dynamicOnMainActor() { }
 
 // property wrapper + main actor
 @propertyWrapper
-public struct IntWrapper {
+public struct IntWrapper: Sendable {
 
   public init(wrappedValue: Int) {
     self.wrappedValue = wrappedValue
diff --git a/test/Concurrency/actor_isolation.swift b/test/Concurrency/actor_isolation.swift
@@ -147,10 +147,11 @@ func checkIsolationValueType(_ formance: InferredFromConformance,
   _ = await ext.point // expected-warning {{non-sendable type 'Point' in implicitly asynchronous access to main actor-isolated property 'point' cannot cross actor boundary}}
   _ = await formance.counter
   _ = await anno.point // expected-warning {{non-sendable type 'Point' in implicitly asynchronous access to global actor 'SomeGlobalActor'-isolated property 'point' cannot cross actor boundary}}
-  _ = anno.counter
+  // expected-warning@-1 {{non-sendable type 'NoGlobalActorValueType' passed in implicitly asynchronous call to global actor 'SomeGlobalActor'-isolated property 'point' cannot cross actor boundary}}
+  _ = anno.counter // expected-warning {{non-sendable type 'NoGlobalActorValueType' passed in call to main actor-isolated property 'counter' cannot cross actor boundary}}
 
   // these will always need an await
-  _ = await (formance as MainCounter).counter
+  _ = await (formance as MainCounter).counter // expected-warning {{non-sendable type 'any MainCounter' passed in implicitly asynchronous call to main actor-isolated property 'counter' cannot cross actor boundary}}
   _ = await ext[1]
   _ = await formance.ticker
   _ = await ext.polygon // expected-warning {{non-sendable type '[Point]' in implicitly asynchronous access to main actor-isolated property 'polygon' cannot cross actor boundary}}
@@ -159,6 +160,7 @@ func checkIsolationValueType(_ formance: InferredFromConformance,
 }
 
 // check for instance members that do not need global-actor protection
+// expected-note@+1 2 {{consider making struct 'NoGlobalActorValueType' conform to the 'Sendable' protocol}}
 struct NoGlobalActorValueType {
   @SomeGlobalActor var point: Point // expected-warning {{stored property 'point' within struct cannot have a global actor; this is an error in Swift 6}}
 
diff --git a/test/Concurrency/actor_isolation_swift6.swift b/test/Concurrency/actor_isolation_swift6.swift
@@ -60,6 +60,7 @@ func checkIsolationValueType(_ formance: InferredFromConformance,
 
   // these do need await, regardless of reference or value type
   _ = await (formance as any MainCounter).counter
+  // expected-warning@-1 {{non-sendable type 'any MainCounter' passed in implicitly asynchronous call to main actor-isolated property 'counter' cannot cross actor boundary}}
   _ = await ext[1]
   _ = await formance.ticker
   _ = await ext.polygon
diff --git a/test/Concurrency/sendable_checking.swift b/test/Concurrency/sendable_checking.swift
@@ -233,7 +233,7 @@ func testConversionsAndSendable(a: MyActor, s: any Sendable, f: @Sendable () ->
 
 @available(SwiftStdlib 5.1, *)
 final class NonSendable {
-  // expected-note@-1 3 {{class 'NonSendable' does not conform to the 'Sendable' protocol}}
+  // expected-note@-1 6 {{class 'NonSendable' does not conform to the 'Sendable' protocol}}
   var value = ""
 
   @MainActor
@@ -247,12 +247,24 @@ final class NonSendable {
 
     await self.update()
     // expected-warning@-1 {{passing argument of non-sendable type 'NonSendable' into main actor-isolated context may introduce data races}}
+
+    _ = await x
+    // expected-warning@-1 {{non-sendable type 'NonSendable' passed in implicitly asynchronous call to main actor-isolated property 'x' cannot cross actor boundary}}
+
+    _ = await self.x
+    // expected-warning@-1 {{non-sendable type 'NonSendable' passed in implicitly asynchronous call to main actor-isolated property 'x' cannot cross actor boundary}}
   }
+
+  @MainActor
+  var x: Int { 0 }
 }
 
 @available(SwiftStdlib 5.1, *)
 func testNonSendableBaseArg() async {
   let t = NonSendable()
   await t.update()
   // expected-warning@-1 {{passing argument of non-sendable type 'NonSendable' into main actor-isolated context may introduce data races}}
+
+  _ = await t.x
+  // expected-warning@-1 {{non-sendable type 'NonSendable' passed in implicitly asynchronous call to main actor-isolated property 'x' cannot cross actor boundary}}
 }
diff --git a/validation-test/Sema/SwiftUI/rdar76252310.swift b/validation-test/Sema/SwiftUI/rdar76252310.swift
@@ -9,7 +9,7 @@ class Visibility: ObservableObject { // expected-note 2{{class 'Visibility' does
     @Published var yes = false // some nonsense
 }
 
-struct CoffeeTrackerView: View { // expected-note{{consider making struct 'CoffeeTrackerView' conform to the 'Sendable' protocol}}
+struct CoffeeTrackerView: View { // expected-note 4{{consider making struct 'CoffeeTrackerView' conform to the 'Sendable' protocol}}
     @ObservedObject var showDrinkList: Visibility = Visibility()
 
     var storage: Visibility = Visibility()
@@ -39,15 +39,19 @@ func fromConcurrencyAware() async {
   // expected-warning@+1 {{non-sendable type 'CoffeeTrackerView' returned by call to main actor-isolated function cannot cross actor boundary}}
   let view = CoffeeTrackerView()
 
+  // expected-warning@+4 {{non-sendable type 'CoffeeTrackerView' passed in implicitly asynchronous call to main actor-isolated property 'body' cannot cross actor boundary}}
   // expected-note@+3 {{property access is 'async'}}
   // expected-warning@+2 {{non-sendable type 'some View' in implicitly asynchronous access to main actor-isolated property 'body' cannot cross actor boundary}}
   // expected-error@+1 {{expression is 'async' but is not marked with 'await'}}
   _ = view.body
 
+  // expected-warning@+4 {{non-sendable type 'CoffeeTrackerView' passed in implicitly asynchronous call to main actor-isolated property 'showDrinkList' cannot cross actor boundary}}
   // expected-note@+3 {{property access is 'async'}}
   // expected-warning@+2 {{non-sendable type 'Visibility' in implicitly asynchronous access to main actor-isolated property 'showDrinkList' cannot cross actor boundary}}
   // expected-error@+1 {{expression is 'async' but is not marked with 'await'}}
   _ = view.showDrinkList
 
-  _ = await view.storage // expected-warning {{non-sendable type 'Visibility' in implicitly asynchronous access to main actor-isolated property 'storage' cannot cross actor boundary}}
+  // expected-warning@+2 {{non-sendable type 'CoffeeTrackerView' passed in implicitly asynchronous call to main actor-isolated property 'storage' cannot cross actor boundary}}
+  // expected-warning@+1 {{non-sendable type 'Visibility' in implicitly asynchronous access to main actor-isolated property 'storage' cannot cross actor boundary}}
+  _ = await view.storage
 }
