Merge pull request #74869 from gottesmm/rdar130915737

[region-isolation] Improve async let errors to always use a new style error.

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -994,6 +994,10 @@ NOTE(regionbasedisolation_named_transfer_non_transferrable, none,
 NOTE(regionbasedisolation_named_transfer_non_transferrable_callee, none,
       "sending %1%0 to %2 %3 %4 risks causing data races between %2 and %5 uses",
       (Identifier, StringRef, ActorIsolation, DescriptiveDeclKind, DeclName, StringRef))
+NOTE(regionbasedisolation_named_nonisolated_asynclet_name, none,
+      "sending %0 into async let risks causing data races between async let uses and local uses",
+      (Identifier))
+
 NOTE(regionbasedisolation_named_transfer_into_sending_param, none,
      "%0%1 is passed as a 'sending' parameter; Uses in callee may race with later %0uses",
      (StringRef, Identifier))

include/swift/SILOptimizer/Analysis/RegionAnalysis.h

@@ -28,6 +28,12 @@ class RegionAnalysisValueMap;
 
 namespace regionanalysisimpl {
 
+#ifndef NDEBUG
+/// Global bool set only when asserts are enabled to ease debugging by causing
+/// unknown pattern errors to cause an assert so we drop into the debugger.
+extern bool AbortOnUnknownPatternMatchError;
+#endif
+
 using TransferringOperandSetFactory = Partition::TransferringOperandSetFactory;
 using Element = PartitionPrimitives::Element;
 using Region = PartitionPrimitives::Region;

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -48,11 +48,19 @@ using namespace swift::PartitionPrimitives;
 using namespace swift::PatternMatch;
 using namespace swift::regionanalysisimpl;
 
-static llvm::cl::opt<bool> AbortOnUnknownPatternMatchError(
+#ifndef NDEBUG
+
+bool swift::regionanalysisimpl::AbortOnUnknownPatternMatchError = false;
+
+static llvm::cl::opt<bool, true> AbortOnUnknownPatternMatchErrorCmdLine(
     "sil-region-isolation-assert-on-unknown-pattern",
     llvm::cl::desc("Abort if SIL region isolation detects an unknown pattern. "
                    "Intended only to be used when debugging the compiler!"),
-    llvm::cl::init(false), llvm::cl::Hidden);
+    llvm::cl::Hidden,
+    llvm::cl::location(
+        swift::regionanalysisimpl::AbortOnUnknownPatternMatchError));
+
+#endif
 
 //===----------------------------------------------------------------------===//
 //                              MARK: Utilities

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -575,6 +575,19 @@ class UseAfterTransferDiagnosticEmitter {
     emitRequireInstDiagnostics();
   }
 
+  void emitNamedAsyncLetNoIsolationCrossingError(SILLocation loc,
+                                                 Identifier name) {
+    // Emit the short error.
+    diagnoseError(loc, diag::regionbasedisolation_named_transfer_yields_race,
+                  name)
+        .highlight(loc.getSourceRange())
+        .limitBehaviorIf(getBehaviorLimit());
+
+    diagnoseNote(
+        loc, diag::regionbasedisolation_named_nonisolated_asynclet_name, name);
+    emitRequireInstDiagnostics();
+  }
+
   void emitTypedIsolationCrossing(SILLocation loc, Type inferredType,
                                   ApplyIsolationCrossing isolationCrossing) {
     diagnoseError(
@@ -665,6 +678,11 @@ class UseAfterTransferDiagnosticEmitter {
   }
 
   void emitUnknownPatternError() {
+    if (AbortOnUnknownPatternMatchError) {
+      llvm::report_fatal_error(
+          "RegionIsolation: Aborting on unknown pattern match error");
+    }
+
     diagnoseError(transferOp->getUser(),
                   diag::regionbasedisolation_unknown_pattern)
         .limitBehaviorIf(getBehaviorLimit());
@@ -839,7 +857,7 @@ struct UseAfterTransferDiagnosticInferrer::AutoClosureWalker : ASTWalker {
       : foundTypeInfo(foundTypeInfo), targetDecl(targetDecl),
         targetDeclIsolationInfo(targetDeclIsolationInfo) {}
 
-  Expr *lookThroughExpr(Expr *expr) {
+  Expr *lookThroughArgExpr(Expr *expr) {
     while (true) {
       if (auto *memberRefExpr = dyn_cast<MemberRefExpr>(expr)) {
         expr = memberRefExpr->getBase();
@@ -867,67 +885,85 @@ struct UseAfterTransferDiagnosticInferrer::AutoClosureWalker : ASTWalker {
 
   PreWalkResult<Expr *> walkToExprPre(Expr *expr) override {
     if (auto *declRef = dyn_cast<DeclRefExpr>(expr)) {
-      // If this decl ref expr was not visited as part of a callExpr, add it as
-      // something without isolation crossing.
+      // If this decl ref expr was not visited as part of a callExpr and is our
+      // target decl... emit a simple async let error.
       if (!visitedCallExprDeclRefExprs.count(declRef)) {
         if (declRef->getDecl() == targetDecl) {
-          visitedCallExprDeclRefExprs.insert(declRef);
           foundTypeInfo.diagnosticEmitter
-              .emitTypedRaceWithUnknownIsolationCrossing(
-                  foundTypeInfo.baseLoc, declRef->findOriginalType());
+              .emitNamedAsyncLetNoIsolationCrossingError(
+                  foundTypeInfo.baseLoc, targetDecl->getBaseIdentifier());
           return Action::Continue(expr);
         }
       }
     }
 
+    // If we have a call expr, see if any of its arguments will cause our sent
+    // value to be transferred into another isolation domain.
     if (auto *callExpr = dyn_cast<CallExpr>(expr)) {
-      if (auto isolationCrossing = callExpr->getIsolationCrossing()) {
-        // Search callExpr's arguments to see if we have our targetDecl.
-        auto *argList = callExpr->getArgs();
-        for (auto pair : llvm::enumerate(argList->getArgExprs())) {
-          auto *arg = lookThroughExpr(pair.value());
-          if (auto *declRef = dyn_cast<DeclRefExpr>(arg)) {
-            if (declRef->getDecl() == targetDecl) {
-              // Found our target!
-              visitedCallExprDeclRefExprs.insert(declRef);
-
-              // See if we can find a valueDecl/name for our callee so we can
-              // emit a nicer error.
-              ConcreteDeclRef concreteDecl =
-                  callExpr->getDirectCallee()->getReferencedDecl();
-
-              // If we do not find a direct one, see if we are calling a method
-              // on a nominal type.
-              if (!concreteDecl) {
-                if (auto *dot = dyn_cast<DotSyntaxCallExpr>(
-                        callExpr->getDirectCallee())) {
-                  concreteDecl = dot->getSemanticFn()->getReferencedDecl();
-                }
-              }
-
-              if (concreteDecl) {
-                auto *valueDecl = concreteDecl.getDecl();
-                assert(valueDecl &&
-                       "Should be non-null if concreteDecl is valid");
-                if (valueDecl->hasName()) {
-                  foundTypeInfo.diagnosticEmitter
-                      .emitNamedIsolationCrossingError(
-                          foundTypeInfo.baseLoc,
-                          targetDecl->getBaseIdentifier(),
-                          targetDeclIsolationInfo, *isolationCrossing,
-                          valueDecl->getName(),
-                          valueDecl->getDescriptiveKind());
-                  return Action::Continue(expr);
-                }
-              }
-
-              // Otherwise default back to the "callee" error.
-              foundTypeInfo.diagnosticEmitter.emitNamedIsolationCrossingError(
-                  foundTypeInfo.baseLoc, targetDecl->getBaseIdentifier(),
-                  targetDeclIsolationInfo, *isolationCrossing);
-              return Action::Continue(expr);
-            }
+      // Search callExpr's arguments to see if we have our targetDecl.
+      auto *argList = callExpr->getArgs();
+      for (auto pair : llvm::enumerate(argList->getArgExprs())) {
+        auto *arg = lookThroughArgExpr(pair.value());
+        auto *declRef = dyn_cast<DeclRefExpr>(arg);
+        if (!declRef)
+          continue;
+
+        if (declRef->getDecl() != targetDecl)
+          continue;
+
+        // Found our target!
+        visitedCallExprDeclRefExprs.insert(declRef);
+
+        auto isolationCrossing = callExpr->getIsolationCrossing();
+
+        // If we do not have an isolation crossing, then we must be just sending
+        // a value in a nonisolated fashion into an async let. So emit the
+        // simple async let error.
+        if (!isolationCrossing) {
+          foundTypeInfo.diagnosticEmitter
+              .emitNamedAsyncLetNoIsolationCrossingError(
+                  foundTypeInfo.baseLoc, targetDecl->getBaseIdentifier());
+          continue;
+        }
+
+        // Otherwise, we are calling an actor isolated function in the async
+        // let. Emit a better error.
+
+        // See if we can find a valueDecl/name for our callee so we can
+        // emit a nicer error.
+        ConcreteDeclRef concreteDecl =
+            callExpr->getDirectCallee()->getReferencedDecl();
+
+        // If we do not find a direct one, see if we are calling a method
+        // on a nominal type.
+        if (!concreteDecl) {
+          if (auto *dot =
+                  dyn_cast<DotSyntaxCallExpr>(callExpr->getDirectCallee())) {
+            concreteDecl = dot->getSemanticFn()->getReferencedDecl();
+          }
+        }
+
+        if (!concreteDecl)
+          continue;
+
+        auto *valueDecl = concreteDecl.getDecl();
+        assert(valueDecl && "Should be non-null if concreteDecl is valid");
+
+        if (auto isolationCrossing = callExpr->getIsolationCrossing()) {
+          // If we have an isolation crossing, use that information.
+          if (valueDecl->hasName()) {
+            foundTypeInfo.diagnosticEmitter.emitNamedIsolationCrossingError(
+                foundTypeInfo.baseLoc, targetDecl->getBaseIdentifier(),
+                targetDeclIsolationInfo, *isolationCrossing,
+                valueDecl->getName(), valueDecl->getDescriptiveKind());
+            continue;
           }
+
+          // Otherwise default back to the "callee" error.
+          foundTypeInfo.diagnosticEmitter.emitNamedIsolationCrossingError(
+              foundTypeInfo.baseLoc, targetDecl->getBaseIdentifier(),
+              targetDeclIsolationInfo, *isolationCrossing);
+          continue;
         }
       }
     }
@@ -1059,6 +1095,11 @@ void TransferNonSendableImpl::emitUseAfterTransferDiagnostics() {
     // tells the user to file a bug. This importantly ensures that we can
     // guarantee that we always find the require if we successfully compile.
     if (!didEmitRequireNote) {
+      if (AbortOnUnknownPatternMatchError) {
+        llvm::report_fatal_error(
+            "RegionIsolation: Aborting on unknown pattern match error");
+      }
+
       diagnoseError(transferOp, diag::regionbasedisolation_unknown_pattern);
       continue;
     }
@@ -1117,6 +1158,11 @@ class TransferNonTransferrableDiagnosticEmitter {
   }
 
   void emitUnknownPatternError() {
+    if (AbortOnUnknownPatternMatchError) {
+      llvm::report_fatal_error(
+          "RegionIsolation: Aborting on unknown pattern match error");
+    }
+
     diagnoseError(getOperand()->getUser(),
                   diag::regionbasedisolation_unknown_pattern)
         .limitBehaviorIf(getBehaviorLimit());
@@ -1602,6 +1648,11 @@ struct DiagnosticEvaluator final
   }
 
   void handleUnknownCodePattern(const PartitionOp &op) const {
+    if (AbortOnUnknownPatternMatchError) {
+      llvm::report_fatal_error(
+          "RegionIsolation: Aborting on unknown pattern match error");
+    }
+
     diagnoseError(op.getSourceInst(),
                   diag::regionbasedisolation_unknown_pattern);
   }