Merge pull request #70419 from angela-laar/group-actor-isolation-errors

angela-laar · web-flow · commit c1af52c2f227 · 2023-12-18T08:42:26.000-08:00
Group actor isolation errors
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -5269,11 +5269,10 @@ NOTE(note_add_async_and_throws_to_decl,none,
 NOTE(note_add_distributed_to_decl,none,
      "add 'distributed' to %0 to make this %kindonly0 satisfy the protocol requirement",
      (const ValueDecl *))
-NOTE(note_add_globalactor_to_function,none,
+ERROR(add_globalactor_to_function,none,
      "add '@%0' to make %kind1 part of global actor %2",
      (StringRef, const ValueDecl *, Type))
 FIXIT(insert_globalactor_attr, "@%0 ", (Type))
-
 ERROR(main_function_must_be_mainActor,none,
       "main() must be '@MainActor'", ())
 
diff --git a/include/swift/Basic/Features.def b/include/swift/Basic/Features.def
@@ -278,6 +278,9 @@ EXPERIMENTAL_FEATURE(BitwiseCopyable, true)
 /// Enables the FixedArray data type.
 EXPERIMENTAL_FEATURE(FixedArrays, true)
 
+// Group Main Actor Isolation Errors by Scope
+EXPERIMENTAL_FEATURE(GroupActorErrors, true)
+
 #undef EXPERIMENTAL_FEATURE_EXCLUDED_FROM_MODULE_INTERFACE
 #undef EXPERIMENTAL_FEATURE
 #undef UPCOMING_FEATURE
diff --git a/lib/AST/ASTPrinter.cpp b/lib/AST/ASTPrinter.cpp
@@ -3440,6 +3440,10 @@ static bool usesFeatureDeprecateApplicationMain(Decl *decl) {
   return false;
 }
 
+static bool usesFeatureGroupActorErrors(Decl *decl) {
+  return false;
+}
+
 static bool usesFeatureImportObjcForwardDeclarations(Decl *decl) {
   ClangNode clangNode = decl->getClangNode();
   if (!clangNode)
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -1921,39 +1921,6 @@ static FuncDecl *findAnnotatableFunction(DeclContext *dc) {
   return fn;
 }
 
-/// Note when the enclosing context could be put on a global actor.
-// FIXME: This should handle closures too.
-static void noteGlobalActorOnContext(DeclContext *dc, Type globalActor) {
-  // If we are in a synchronous function on the global actor,
-  // suggest annotating with the global actor itself.
-  if (auto fn = findAnnotatableFunction(dc)) {
-    // Suppress this for accessories because you can't change the
-    // actor isolation of an individual accessor.  Arguably we could
-    // add this to the entire storage declaration, though.
-    // Suppress this for async functions out of caution; but don't
-    // suppress it if we looked through a defer.
-    if (!isa<AccessorDecl>(fn) &&
-        (!fn->isAsyncContext() || fn != dc)) {
-      switch (getActorIsolation(fn)) {
-      case ActorIsolation::ActorInstance:
-      case ActorIsolation::GlobalActor:
-      case ActorIsolation::GlobalActorUnsafe:
-      case ActorIsolation::Nonisolated:
-      case ActorIsolation::NonisolatedUnsafe:
-        return;
-
-      case ActorIsolation::Unspecified:
-        fn->diagnose(diag::note_add_globalactor_to_function,
-            globalActor->getWithoutParens().getString(),
-            fn, globalActor)
-          .fixItInsert(fn->getAttributeInsertionLoc(false),
-            diag::insert_globalactor_attr, globalActor);
-          return;
-      }
-    }
-  }
-}
-
 bool swift::diagnoseApplyArgSendability(ApplyExpr *apply, const DeclContext *declContext) {
   auto isolationCrossing = apply->getIsolationCrossing();
   if (!isolationCrossing.has_value())
@@ -2036,6 +2003,16 @@ namespace {
     /// an expression or function.
     llvm::SmallDenseMap<const DeclContext *, ActorIsolation> requiredIsolation;
 
+    using ActorRefKindPair = std::pair<ReferencedActor::Kind, ActorIsolation>;
+
+    using IsolationPair = std::pair<ActorIsolation, ActorIsolation>;
+
+    using DiagnosticList = std::vector<IsolationError>;
+
+    llvm::DenseMap<ActorRefKindPair, DiagnosticList> refErrors;
+
+    llvm::DenseMap<IsolationPair, DiagnosticList> applyErrors;
+
     /// Keeps track of the capture context of variables that have been
     /// explicitly captured in closures.
     llvm::SmallDenseMap<VarDecl *, TinyPtrVector<const DeclContext *>>
@@ -2054,6 +2031,99 @@ namespace {
       return applyStack.back().dyn_cast<ApplyExpr *>();
     }
 
+    /// Note when the enclosing context could be put on a global actor.
+    // FIXME: This should handle closures too.
+    static bool missingGlobalActorOnContext(DeclContext *dc, Type globalActor, DiagnosticBehavior behavior) {
+      // If we are in a synchronous function on the global actor,
+      // suggest annotating with the global actor itself.
+      if (auto fn = findAnnotatableFunction(dc)) {
+        // Suppress this for accessors because you can't change the
+        // actor isolation of an individual accessor.  Arguably we could
+        // add this to the entire storage declaration, though.
+        // Suppress this for async functions out of caution; but don't
+        // suppress it if we looked through a defer.
+        if (!isa<AccessorDecl>(fn) &&
+            (!fn->isAsyncContext() || fn != dc)) {
+          switch (getActorIsolation(fn)) {
+          case ActorIsolation::ActorInstance:
+          case ActorIsolation::GlobalActor:
+          case ActorIsolation::GlobalActorUnsafe:
+          case ActorIsolation::Nonisolated:
+          case ActorIsolation::NonisolatedUnsafe:
+              return false;
+
+          case ActorIsolation::Unspecified:
+            fn->diagnose(diag::add_globalactor_to_function,
+                         globalActor->getWithoutParens().getString(),
+                         fn, globalActor)
+            .limitBehavior(behavior)
+            .fixItInsert(fn->getAttributeInsertionLoc(false),
+                         diag::insert_globalactor_attr, globalActor);
+              return true;
+          }
+        }
+      }
+      return false;
+    }
+
+  public:
+    bool diagnoseIsolationErrors() {
+      bool diagnosedError = false;
+
+      for (auto list : refErrors) {
+        ActorRefKindPair key = list.getFirst();
+        DiagnosticList errors = list.getSecond();
+        ActorIsolation isolation = key.second;
+
+        auto behavior = DiagnosticBehavior::Error;
+
+        // Add Fix-it for missing @SomeActor annotation
+        if (isolation.isGlobalActor()) {
+          if (missingGlobalActorOnContext(
+                  const_cast<DeclContext *>(getDeclContext()),
+                  isolation.getGlobalActor(), behavior) &&
+              errors.size() > 1) {
+            behavior= DiagnosticBehavior::Note;
+          }
+        }
+
+        for (IsolationError error : errors) {
+          // Diagnose actor_isolated_non_self_reference as note
+          // if fix-it provided in missingGlobalActorOnContext
+          ctx.Diags.diagnose(error.loc, error.diag)
+              .limitBehavior(behavior);
+        }
+      }
+
+      for (auto list : applyErrors) {
+        IsolationPair key = list.getFirst();
+        DiagnosticList errors = list.getSecond();
+        ActorIsolation isolation = key.first;
+
+        auto behavior = DiagnosticBehavior::Error;
+
+        // Add Fix-it for missing @SomeActor annotation
+        if (isolation.isGlobalActor()) {
+          if (missingGlobalActorOnContext(
+                  const_cast<DeclContext *>(getDeclContext()),
+                  isolation.getGlobalActor(), behavior) &&
+              errors.size() > 1) {
+            behavior= DiagnosticBehavior::Note;
+          }
+        }
+
+        for (IsolationError error : errors) {
+          // Diagnose actor_isolated_call as note if
+          // fix-it provided in missingGlobalActorOnContext
+          ctx.Diags.diagnose(error.loc, error.diag)
+              .limitBehavior(behavior);
+        }
+      }
+
+      return diagnosedError;
+    }
+
+  private:
     const PatternBindingDecl *getTopPatternBindingDecl() const {
       return patternBindingStack.empty() ? nullptr : patternBindingStack.back();
     }
@@ -3185,27 +3255,58 @@ namespace {
       // If we need to mark the call as implicitly asynchronous, make sure
       // we're in an asynchronous context.
       if (requiresAsync && !getDeclContext()->isAsyncContext()) {
-        if (calleeDecl) {
-          auto preconcurrency = getContextIsolation().preconcurrency() ||
-              calleeDecl->preconcurrency();
-          ctx.Diags.diagnose(
-              apply->getLoc(), diag::actor_isolated_call_decl,
-              *unsatisfiedIsolation,
-              calleeDecl,
-              getContextIsolation())
-            .warnUntilSwiftVersionIf(preconcurrency, 6);
-          calleeDecl->diagnose(diag::actor_isolated_sync_func, calleeDecl);
+
+        if (ctx.LangOpts.hasFeature(Feature::GroupActorErrors)) {
+
+          IsolationError mismatch([calleeDecl, apply, unsatisfiedIsolation, getContextIsolation]() {
+            if (calleeDecl) {
+              return IsolationError(
+                             apply->getLoc(),
+                             Diagnostic(diag::actor_isolated_call_decl,
+                                        *unsatisfiedIsolation,
+                                        calleeDecl,
+                                        getContextIsolation()));
+            } else {
+              return IsolationError(
+                             apply->getLoc(),
+                             Diagnostic(diag::actor_isolated_call,
+                                        *unsatisfiedIsolation,
+                                        getContextIsolation()));
+            }
+          }());
+
+          auto iter = applyErrors.find(std::make_pair(*unsatisfiedIsolation, getContextIsolation()));
+          if (iter != applyErrors.end()){
+            iter->second.push_back((mismatch));
+          } else {
+            DiagnosticList list;
+            list.push_back((mismatch));
+            auto keyPair = std::make_pair(*unsatisfiedIsolation, getContextIsolation());
+            applyErrors.insert(std::make_pair(keyPair, list));
+          }
         } else {
-          ctx.Diags.diagnose(
-              apply->getLoc(), diag::actor_isolated_call, *unsatisfiedIsolation,
-              getContextIsolation())
+          if (calleeDecl) {
+            auto preconcurrency = getContextIsolation().preconcurrency() ||
+            calleeDecl->preconcurrency();
+            ctx.Diags.diagnose(
+                               apply->getLoc(), diag::actor_isolated_call_decl,
+                               *unsatisfiedIsolation,
+                               calleeDecl,
+                               getContextIsolation())
+            .warnUntilSwiftVersionIf(preconcurrency, 6);
+            calleeDecl->diagnose(diag::actor_isolated_sync_func, calleeDecl);
+          } else {
+            ctx.Diags.diagnose(
+                               apply->getLoc(), diag::actor_isolated_call, *unsatisfiedIsolation,
+                               getContextIsolation())
             .warnUntilSwiftVersionIf(getContextIsolation().preconcurrency(), 6);
-        }
+          }
 
-        if (unsatisfiedIsolation->isGlobalActor()) {
-          noteGlobalActorOnContext(
-              const_cast<DeclContext *>(getDeclContext()),
-              unsatisfiedIsolation->getGlobalActor());
+          if (unsatisfiedIsolation->isGlobalActor()) {
+            missingGlobalActorOnContext(
+                                        const_cast<DeclContext *>(getDeclContext()),
+                                        unsatisfiedIsolation->getGlobalActor(), DiagnosticBehavior::Note);
+          }
         }
 
         return true;
@@ -3574,22 +3675,38 @@ namespace {
         bool preconcurrencyContext =
           result.options.contains(ActorReferenceResult::Flags::Preconcurrency);
 
-        ctx.Diags.diagnose(
-            loc, diag::actor_isolated_non_self_reference,
-            decl,
-            useKind,
-            refKind + 1, refGlobalActor,
-            result.isolation)
-          .warnUntilSwiftVersionIf(preconcurrencyContext, 6);
-
-        noteIsolatedActorMember(decl, context);
-
-        if (result.isolation.isGlobalActor()) {
-          noteGlobalActorOnContext(
-              const_cast<DeclContext *>(getDeclContext()),
-              result.isolation.getGlobalActor());
-        }
+          if (ctx.LangOpts.hasFeature(Feature::GroupActorErrors)) {
+            IsolationError mismatch = IsolationError(loc, Diagnostic(diag::actor_isolated_non_self_reference,
+                                                                        decl,
+                                                                        useKind,
+                                                                        refKind + 1, refGlobalActor,
+                                                                        result.isolation));
 
+            auto iter = refErrors.find(std::make_pair(refKind,result.isolation));
+            if (iter != refErrors.end()){
+              iter->second.push_back(mismatch);
+            } else {
+              DiagnosticList list;
+              list.push_back(mismatch);
+              auto keyPair = std::make_pair(refKind,result.isolation);
+              refErrors.insert(std::make_pair(keyPair, list));
+            }
+          } else {
+            ctx.Diags.diagnose(
+                               loc, diag::actor_isolated_non_self_reference,
+                               decl,
+                               useKind,
+                               refKind + 1, refGlobalActor,
+                               result.isolation)
+            .warnUntilSwiftVersionIf(preconcurrencyContext, 6);
+
+            noteIsolatedActorMember(decl, context);
+            if (result.isolation.isGlobalActor()) {
+              missingGlobalActorOnContext(
+                                       const_cast<DeclContext *>(getDeclContext()),
+                                       result.isolation.getGlobalActor(), DiagnosticBehavior::Note);
+            }
+          }
         return true;
       }
     }
@@ -3731,9 +3848,11 @@ void swift::checkFunctionActorIsolation(AbstractFunctionDecl *decl) {
   if (decl->getAttrs().hasAttribute<LLDBDebuggerFunctionAttr>())
     return;
 
+  auto &ctx = decl->getASTContext();
   ActorIsolationChecker checker(decl);
   if (auto body = decl->getBody()) {
     body->walk(checker);
+    if(ctx.LangOpts.hasFeature(Feature::GroupActorErrors)){ checker.diagnoseIsolationErrors(); }
   }
   if (auto ctor = dyn_cast<ConstructorDecl>(decl)) {
     if (auto superInit = ctor->getSuperInitCall())
diff --git a/lib/Sema/TypeCheckConcurrency.h b/lib/Sema/TypeCheckConcurrency.h
diff --git a/test/Concurrency/grouped_actor_isolation_diagnostics.swift b/test/Concurrency/grouped_actor_isolation_diagnostics.swift
