[Sema] Improving implicit closure capture diagnostic wording

Author: LucianoPAlmeida

include/swift/AST/DiagnosticsSema.def

@@ -1965,6 +1965,9 @@ NOTE(non_sendable_nominal,none,
 NOTE(add_nominal_sendable_conformance,none,
      "consider making %0 %1 conform to the 'Sendable' protocol",
      (DescriptiveDeclKind, DeclName))
+NOTE(add_generic_parameter_sendable_conformance,none,
+     "consider making generic parameter %0 conform to the 'Sendable' protocol",
+     (Type))
 REMARK(add_predates_concurrency_import,none,
      "add '@preconcurrency' to %select{suppress|treat}0 "
      "'Sendable'-related %select{warnings|errors}0 from module %1"
@@ -4613,6 +4616,12 @@ ERROR(concurrent_access_of_inout_param,none,
 ERROR(non_sendable_capture,none,
       "capture of %1 with non-sendable type %0 in a `@Sendable` closure",
       (Type, DeclName))
+ERROR(implicit_async_let_non_sendable_capture,none,
+      "capture of %1 with non-sendable type %0 in 'async let' binding",
+      (Type, DeclName))
+ERROR(implicit_non_sendable_capture,none,
+      "implicit capture of %1 requires that %0 conforms to `Sendable`",
+      (Type, DeclName))
 
 NOTE(actor_isolated_sync_func,none,
      "calls to %0 %1 from outside of its actor context are "

lib/Sema/TypeCheckConcurrency.cpp

@@ -569,18 +569,27 @@ static void addSendableFixIt(
     const NominalTypeDecl *nominal, InFlightDiagnostic &diag, bool unchecked) {
   if (nominal->getInherited().empty()) {
     SourceLoc fixItLoc = nominal->getBraces().Start;
-    if (unchecked)
-      diag.fixItInsert(fixItLoc, ": @unchecked Sendable");
-    else
-      diag.fixItInsert(fixItLoc, ": Sendable");
+    diag.fixItInsert(fixItLoc,
+                     unchecked ? ": @unchecked Sendable" : ": Sendable");
   } else {
-    ASTContext &ctx = nominal->getASTContext();
-    SourceLoc fixItLoc = nominal->getInherited().back().getSourceRange().End;
-    fixItLoc = Lexer::getLocForEndOfToken(ctx.SourceMgr, fixItLoc);
-    if (unchecked)
-      diag.fixItInsert(fixItLoc, ", @unchecked Sendable");
-    else
-      diag.fixItInsert(fixItLoc, ", Sendable");
+    auto fixItLoc = nominal->getInherited().back().getLoc();
+    diag.fixItInsertAfter(fixItLoc,
+                          unchecked ? ", @unchecked Sendable" : ", Sendable");
+  }
+}
+
+/// Add Fix-It text for the given generic param declaration type to adopt
+/// Sendable.
+static void addSendableFixIt(const GenericTypeParamDecl *genericArgument,
+                             InFlightDiagnostic &diag, bool unchecked) {
+  if (genericArgument->getInherited().empty()) {
+    auto fixItLoc = genericArgument->getLoc();
+    diag.fixItInsertAfter(fixItLoc,
+                          unchecked ? ": @unchecked Sendable" : ": Sendable");
+  } else {
+    auto fixItLoc = genericArgument->getInherited().back().getLoc();
+    diag.fixItInsertAfter(fixItLoc,
+                          unchecked ? ", @unchecked Sendable" : ", Sendable");
   }
 }
 
@@ -874,6 +883,18 @@ static bool diagnoseSingleNonSendableType(
       nominal->diagnose(
           diag::non_sendable_nominal, nominal->getDescriptiveKind(),
           nominal->getName());
+    } else if (auto genericArchetype = type->getAs<ArchetypeType>()) {
+      auto interfaceType = genericArchetype->getInterfaceType();
+      if (auto genericParamType =
+              interfaceType->getAs<GenericTypeParamType>()) {
+        auto *genericParamTypeDecl = genericParamType->getDecl();
+        if (genericParamTypeDecl &&
+            genericParamTypeDecl->getModuleContext() == module) {
+          auto diag = genericParamTypeDecl->diagnose(
+              diag::add_generic_parameter_sendable_conformance, type);
+          addSendableFixIt(genericParamTypeDecl, diag, /*unchecked=*/false);
+        }
+      }
     }
 
     return false;
@@ -1527,6 +1548,7 @@ namespace {
     SmallVector<const DeclContext *, 4> contextStack;
     SmallVector<ApplyExpr*, 4> applyStack;
     SmallVector<std::pair<OpaqueValueExpr *, Expr *>, 4> opaqueValues;
+    SmallVector<const PatternBindingDecl *, 2> patternBindingStack;
 
     /// Keeps track of the capture context of variables that have been
     /// explicitly captured in closures.
@@ -1539,6 +1561,10 @@ namespace {
     using MutableVarParent
         = llvm::PointerUnion<InOutExpr *, LoadExpr *, AssignExpr *>;
 
+    const PatternBindingDecl *getTopPatternBindingDecl() const {
+      return patternBindingStack.empty() ? nullptr : patternBindingStack.back();
+    }
+
     /// Mapping from mutable variable reference exprs, or inout expressions,
     /// to the parent expression, when that parent is either a load or
     /// an inout expr.
@@ -1694,9 +1720,24 @@ namespace {
         Type type = getDeclContext()
             ->mapTypeIntoContext(decl->getInterfaceType())
             ->getReferenceStorageReferent();
-        diagnoseNonSendableTypes(
-            type, getDeclContext(), capture.getLoc(),
-            diag::non_sendable_capture, decl->getName());
+
+        if (closure->isImplicit()) {
+          auto *patternBindingDecl = getTopPatternBindingDecl();
+          if (patternBindingDecl && patternBindingDecl->isAsyncLet()) {
+            diagnoseNonSendableTypes(
+                type, getDeclContext(), capture.getLoc(),
+                diag::implicit_async_let_non_sendable_capture, decl->getName());
+          } else {
+            // Fallback to a generic implicit capture missing sendable
+            // conformance diagnostic.
+            diagnoseNonSendableTypes(type, getDeclContext(), capture.getLoc(),
+                                     diag::implicit_non_sendable_capture,
+                                     decl->getName());
+          }
+        } else {
+          diagnoseNonSendableTypes(type, getDeclContext(), capture.getLoc(),
+                                   diag::non_sendable_capture, decl->getName());
+        }
       }
     }
 
@@ -1759,6 +1800,10 @@ namespace {
         contextStack.push_back(func);
       }
 
+      if (auto *PBD = dyn_cast<PatternBindingDecl>(decl)) {
+        patternBindingStack.push_back(PBD);
+      }
+
       return true;
     }
 
@@ -1768,6 +1813,11 @@ namespace {
         contextStack.pop_back();
       }
 
+      if (auto *PBD = dyn_cast<PatternBindingDecl>(decl)) {
+        assert(patternBindingStack.back() == PBD);
+        patternBindingStack.pop_back();
+      }
+
       return true;
     }
 

test/Concurrency/concurrent_value_checking.swift

@@ -255,7 +255,7 @@ var concurrentFuncVar: (@Sendable (NotConcurrent) -> Void)? = nil
 // ----------------------------------------------------------------------
 func acceptConcurrentUnary<T>(_: @Sendable (T) -> T) { }
 
-func concurrentClosures<T>(_: T) {
+func concurrentClosures<T>(_: T) { // expected-note{{consider making generic parameter 'T' conform to the 'Sendable' protocol}} {{26-26=: Sendable}}
   acceptConcurrentUnary { (x: T) in
     _ = x // ok
     acceptConcurrentUnary { _ in x } // expected-warning{{capture of 'x' with non-sendable type 'T' in a `@Sendable` closure}}
@@ -269,7 +269,7 @@ struct S1: Sendable {
   var nc: NotConcurrent // expected-warning{{stored property 'nc' of 'Sendable'-conforming struct 'S1' has non-sendable type 'NotConcurrent'}}
 }
 
-struct S2<T>: Sendable {
+struct S2<T>: Sendable { // expected-note{{consider making generic parameter 'T' conform to the 'Sendable' protocol}} {{12-12=: Sendable}}
   var nc: T // expected-warning{{stored property 'nc' of 'Sendable'-conforming generic struct 'S2' has non-sendable type 'T'}}
 }
 

test/Concurrency/sr15049.swift

@@ -19,26 +19,33 @@ func testAsyncSequence1Sendable<Seq: AsyncSequence>(_ seq: Seq) async throws whe
    async let _ = seq.reduce(0) { $0 + $1 } // OK
 }
 
-// TODO(diagnostics) [SR-16092]: Add a tailored wording for implicit autoclosure captures in sendable warnings, because 
-// from user perspective there is no closure capture, so diagnostic can be confusing.
-func testAsyncSequenceTypedPattern<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int {
+func testAsyncSequenceTypedPattern<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int { // expected-note{{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{54-54=, Sendable}}
    async let result: Int = seq.reduce(0) { $0 + $1 } // OK
    // expected-warning@-1{{immutable value 'result' was never used; consider replacing with '_' or removing it}}
-   // expected-warning@-2{{capture of 'seq' with non-sendable type 'Seq' in a `@Sendable` closure}}
+   // expected-warning@-2{{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
 }
 
-func testAsyncSequenceTypedPattern1<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int {
+func testAsyncSequenceTypedPattern1<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int { // expected-note{{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{55-55=, Sendable}}
    async let _: Int = seq.reduce(0) { $0 + $1 } // OK
-   // expected-warning@-1{{capture of 'seq' with non-sendable type 'Seq' in a `@Sendable` closure}}
+   // expected-warning@-1{{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
 }
 
-func testAsyncSequence<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int {
+func testAsyncSequence<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int { // expected-note{{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{42-42=, Sendable}}
    async let result = seq.reduce(0) { $0 + $1 } // OK
    // expected-warning@-1{{initialization of immutable value 'result' was never used; consider replacing with assignment to '_' or removing it}}
-   // expected-warning@-2{{capture of 'seq' with non-sendable type 'Seq' in a `@Sendable` closure}}
+   // expected-warning@-2{{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
 }
 
-func testAsyncSequence1<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int {
+func testAsyncSequence1<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int { // expected-note{{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{43-43=, Sendable}}
    async let _ = seq.reduce(0) { $0 + $1 } // OK
-   // expected-warning@-1{{capture of 'seq' with non-sendable type 'Seq' in a `@Sendable` closure}}
+   // expected-warning@-1{{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
+}
+
+func testAsyncSequence3<Seq>(_ seq: Seq) async throws where Seq: AsyncSequence, Seq.Element == Int { // expected-note{{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{28-28=: Sendable}}
+   async let result = seq // expected-warning{{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
+   //expected-warning@-1{{initialization of immutable value 'result' was never used; consider replacing with assignment to '_' or removing it}}
+}
+
+func testAsyncSequence4<Seq>(_ seq: Seq) async throws where Seq: AsyncSequence, Seq.Element == Int { // expected-note{{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{28-28=: Sendable}}
+   async let _ = seq // expected-warning{{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
 }

test/Distributed/distributed_protocol_isolation.swift

@@ -127,6 +127,7 @@ protocol Server {
   func send<Message: Codable>(message: Message) async throws -> String
 }
 actor MyServer : Server {
+  // expected-note@+1{{consider making generic parameter 'Message' conform to the 'Sendable' protocol}} {{29-29=, Sendable}}
   func send<Message: Codable>(message: Message) throws -> String { "" }  // expected-warning{{non-sendable type 'Message' in parameter of actor-isolated instance method 'send(message:)' satisfying non-isolated protocol requirement cannot cross actor boundary}}
 }