Merge pull request swiftlang#36995 from DougGregor/data-race-checking-flags-5.5

DougGregor · web-flow · commit 7eb29e31705e · 2021-04-21T17:36:07.000-07:00
[5.5] [SILGen] Put actor data-race checking behind a flag.
diff --git a/include/swift/AST/Attr.def b/include/swift/AST/Attr.def
@@ -647,6 +647,11 @@ CONTEXTUAL_SIMPLE_DECL_ATTR(_unsafeMainActor, UnsafeMainActor,
   ABIStableToAdd | ABIStableToRemove | APIStableToAdd | APIBreakingToRemove,
   114)
 
+SIMPLE_DECL_ATTR(_implicitSelfCapture, ImplicitSelfCapture,
+  OnParam | UserInaccessible |
+  ABIStableToAdd | ABIStableToRemove | APIStableToAdd | APIBreakingToRemove,
+  115)
+
 #undef TYPE_ATTR
 #undef DECL_ATTR_ALIAS
 #undef CONTEXTUAL_DECL_ATTR_ALIAS
diff --git a/include/swift/AST/Expr.h b/include/swift/AST/Expr.h
@@ -292,10 +292,14 @@ class alignas(8) Expr {
     Kind : 2
   );
 
-  SWIFT_INLINE_BITFIELD(ClosureExpr, AbstractClosureExpr, 1,
+  SWIFT_INLINE_BITFIELD(ClosureExpr, AbstractClosureExpr, 1+1,
     /// True if closure parameters were synthesized from anonymous closure
     /// variables.
-    HasAnonymousClosureVars : 1
+    HasAnonymousClosureVars : 1,
+
+    /// True if "self" can be captured implicitly without requiring "self."
+    /// on each member reference.
+    ImplicitSelfCapture : 1
   );
 
   SWIFT_INLINE_BITFIELD_FULL(BindOptionalExpr, Expr, 16,
@@ -3871,6 +3875,7 @@ class ClosureExpr : public AbstractClosureExpr {
       Body(nullptr) {
     setParameterList(params);
     Bits.ClosureExpr.HasAnonymousClosureVars = false;
+    Bits.ClosureExpr.ImplicitSelfCapture = false;
   }
 
   SourceRange getSourceRange() const;
@@ -3898,7 +3903,17 @@ class ClosureExpr : public AbstractClosureExpr {
   void setHasAnonymousClosureVars() {
     Bits.ClosureExpr.HasAnonymousClosureVars = true;
   }
-  
+
+  /// Whether this closure allows "self" to be implicitly captured without
+  /// required "self." on each reference.
+  bool allowsImplicitSelfCapture() const {
+    return Bits.ClosureExpr.ImplicitSelfCapture;
+  }
+
+  void setAllowsImplicitSelfCapture(bool value = true) {
+    Bits.ClosureExpr.ImplicitSelfCapture = value;
+  }
+
   /// Determine whether this closure expression has an
   /// explicitly-specified result type.
   bool hasExplicitResultType() const { return ArrowLoc.isValid(); }
diff --git a/include/swift/AST/SILOptions.h b/include/swift/AST/SILOptions.h
@@ -65,6 +65,9 @@ class SILOptions {
   /// It is turned off by default.
   bool EnableSpeculativeDevirtualization = false;
 
+  /// Controls whether to emit actor data-race checks.
+  bool EnableActorDataRaceChecks = false;
+
   /// Should we run any SIL performance optimizations
   ///
   /// Useful when you want to enable -O LLVM opts but not -O SIL opts.
diff --git a/include/swift/AST/Types.h b/include/swift/AST/Types.h
@@ -3382,6 +3382,7 @@ struct ParameterListInfo {
   SmallBitVector propertyWrappers;
   SmallBitVector unsafeSendable;
   SmallBitVector unsafeMainActor;
+  SmallBitVector implicitSelfCapture;
 
 public:
   ParameterListInfo() { }
@@ -3410,6 +3411,13 @@ struct ParameterListInfo {
   /// part of the type system.
   bool isUnsafeMainActor(unsigned paramIdx) const;
 
+  /// Whether the given parameter is a closure that should allow capture of
+  /// 'self' to be implicit, without requiring "self.".
+  bool isImplicitSelfCapture(unsigned paramIdx) const;
+
+  /// Whether there is any contextual information set on this parameter list.
+  bool anyContextualInfo() const;
+
   /// Retrieve the number of non-defaulted parameters.
   unsigned numNonDefaultedParameters() const {
     return defaultArguments.count();
diff --git a/include/swift/Option/Options.td b/include/swift/Option/Options.td
@@ -607,6 +607,16 @@ def warn_swift3_objc_inference_minimal :
   Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,
   HelpText<"Warn about deprecated @objc inference in Swift 3 based on direct uses of the Objective-C entrypoint">;
 
+def enable_actor_data_race_checks :
+  Flag<["-"], "enable-actor-data-race-checks">,
+  Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,
+  HelpText<"Emit runtime checks for actor data races">;
+
+def disable_actor_data_race_checks :
+  Flag<["-"], "disable-actor-data-race-checks">,
+  Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,
+  HelpText<"Disable runtime checks for actor data races">;
+
 def warn_implicit_overrides :
   Flag<["-"], "warn-implicit-overrides">,
   Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,
diff --git a/lib/AST/ASTDumper.cpp b/lib/AST/ASTDumper.cpp
@@ -2546,6 +2546,8 @@ class PrintExpr : public ExprVisitor<PrintExpr> {
     printClosure(E, "closure_expr");
     if (E->hasSingleExpressionBody())
       PrintWithColorRAII(OS, ClosureModifierColor) << " single-expression";
+    if (E->allowsImplicitSelfCapture())
+      PrintWithColorRAII(OS, ClosureModifierColor) << " implicit-self";
 
     if (E->getParameters()) {
       OS << '\n';
diff --git a/lib/AST/Type.cpp b/lib/AST/Type.cpp
@@ -970,6 +970,7 @@ ParameterListInfo::ParameterListInfo(
   propertyWrappers.resize(params.size());
   unsafeSendable.resize(params.size());
   unsafeMainActor.resize(params.size());
+  implicitSelfCapture.resize(params.size());
 
   // No parameter owner means no parameter list means no default arguments
   // - hand back the zeroed bitvector.
@@ -1029,6 +1030,10 @@ ParameterListInfo::ParameterListInfo(
     if (param->getAttrs().hasAttribute<UnsafeMainActorAttr>()) {
       unsafeMainActor.set(i);
     }
+
+    if (param->getAttrs().hasAttribute<ImplicitSelfCaptureAttr>()) {
+      implicitSelfCapture.set(i);
+    }
   }
 }
 
@@ -1059,6 +1064,17 @@ bool ParameterListInfo::isUnsafeMainActor(unsigned paramIdx) const {
       : false;
 }
 
+bool ParameterListInfo::isImplicitSelfCapture(unsigned paramIdx) const {
+  return paramIdx < implicitSelfCapture.size()
+      ? implicitSelfCapture[paramIdx]
+      : false;
+}
+
+bool ParameterListInfo::anyContextualInfo() const {
+  return unsafeSendable.any() || unsafeMainActor.any() ||
+      implicitSelfCapture.any();
+}
+
 /// Turn a param list into a symbolic and printable representation that does not
 /// include the types, something like (_:, b:, c:)
 std::string swift::getParamListAsString(ArrayRef<AnyFunctionType::Param> params) {
diff --git a/lib/Driver/ToolChains.cpp b/lib/Driver/ToolChains.cpp
@@ -217,6 +217,9 @@ void ToolChain::addCommonFrontendArgs(const OutputInfo &OI,
   inputArgs.AddLastArg(arguments,
                        options::OPT_warn_swift3_objc_inference_minimal,
                        options::OPT_warn_swift3_objc_inference_complete);
+  inputArgs.AddLastArg(arguments,
+                       options::OPT_enable_actor_data_race_checks,
+                       options::OPT_disable_actor_data_race_checks);
   inputArgs.AddLastArg(arguments, options::OPT_warn_concurrency);
   inputArgs.AddLastArg(arguments, options::OPT_warn_implicit_overrides);
   inputArgs.AddLastArg(arguments, options::OPT_typo_correction_limit);
diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp
@@ -1222,6 +1222,9 @@ static bool ParseSILArgs(SILOptions &Opts, ArgList &Args,
   Opts.EnableOSSAModules |= Args.hasArg(OPT_enable_ossa_modules);
   Opts.EnableOSSAOptimizations &= !Args.hasArg(OPT_disable_ossa_opts);
   Opts.EnableSpeculativeDevirtualization |= Args.hasArg(OPT_enable_spec_devirt);
+  Opts.EnableActorDataRaceChecks |= Args.hasFlag(
+      OPT_enable_actor_data_race_checks,
+      OPT_disable_actor_data_race_checks, /*default=*/false);
   Opts.DisableSILPerfOptimizations |= Args.hasArg(OPT_disable_sil_perf_optzns);
   Opts.CrossModuleOptimization |= Args.hasArg(OPT_CrossModuleOptimization);
   Opts.VerifyAll |= Args.hasArg(OPT_sil_verify_all);
diff --git a/lib/SILGen/SILGenProlog.cpp b/lib/SILGen/SILGenProlog.cpp
@@ -492,6 +492,10 @@ void SILGenFunction::emitProlog(CaptureInfo captureInfo,
 
   // Initialize ExpectedExecutor if the function is an actor-isolated
   // function or closure.
+  bool wantDataRaceChecks = getOptions().EnableActorDataRaceChecks &&
+      !F.isAsync() &&
+      !isInActorDestructor(FunctionDC);
+
   if (auto *funcDecl =
         dyn_cast_or_null<AbstractFunctionDecl>(FunctionDC->getAsDecl())) {
     auto actorIsolation = getActorIsolation(funcDecl);
@@ -508,7 +512,7 @@ void SILGenFunction::emitProlog(CaptureInfo captureInfo,
       // are prone to dynamic data races in code that does not enforce Sendable
       // completely.
       if (F.isAsync() ||
-          (funcDecl->isLocalCapture() && !isInActorDestructor(funcDecl))) {
+          (wantDataRaceChecks && funcDecl->isLocalCapture())) {
         auto loc = RegularLocation::getAutoGeneratedLocation(F.getLocation());
         ManagedValue selfArg = ManagedValue::forUnmanaged(F.getSelfArgument());
         ExpectedExecutor = emitLoadActorExecutor(loc, selfArg);
@@ -517,14 +521,17 @@ void SILGenFunction::emitProlog(CaptureInfo captureInfo,
     }
 
     case ActorIsolation::GlobalActor:
-      if (F.isAsync() || !isInActorDestructor(funcDecl)) {
+      if (F.isAsync() || wantDataRaceChecks) {
         ExpectedExecutor =
           emitLoadGlobalActorExecutor(actorIsolation.getGlobalActor());
       }
       break;
     }
   } else if (auto *closureExpr = dyn_cast<AbstractClosureExpr>(FunctionDC)) {
-    bool wantExecutor = F.isAsync() || !isInActorDestructor(closureExpr);
+    bool wantExecutor = F.isAsync() ||
+      (wantDataRaceChecks &&
+       !(isa<ClosureExpr>(closureExpr) &&
+         cast<ClosureExpr>(closureExpr)->isUnsafeMainActor()));
     auto actorIsolation = closureExpr->getActorIsolation();
     switch (actorIsolation.getKind()) {
     case ClosureActorIsolation::Independent:
diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp
@@ -5711,16 +5711,18 @@ static bool hasCurriedSelf(ConstraintSystem &cs, ConcreteDeclRef callee,
 }
 
 /// Apply the contextually Sendable flag to the given expression,
-static void applyUnsafeConcurrent(
-      Expr *expr, bool sendable, bool forMainActor) {
+static void applyContextualClosureFlags(
+      Expr *expr, bool sendable, bool forMainActor, bool implicitSelfCapture) {
   if (auto closure = dyn_cast<ClosureExpr>(expr)) {
     closure->setUnsafeConcurrent(sendable, forMainActor);
+    closure->setAllowsImplicitSelfCapture(implicitSelfCapture);
     return;
   }
 
   if (auto captureList = dyn_cast<CaptureListExpr>(expr)) {
-    applyUnsafeConcurrent(
-        captureList->getClosureBody(), sendable, forMainActor);
+    applyContextualClosureFlags(
+        captureList->getClosureBody(), sendable, forMainActor,
+        implicitSelfCapture);
   }
 }
 
@@ -5808,7 +5810,8 @@ Expr *ExprRewriter::coerceCallArguments(
 
   // Quickly test if any further fix-ups for the argument types are necessary.
   if (AnyFunctionType::equalParams(args, params) &&
-      !shouldInjectWrappedValuePlaceholder)
+      !shouldInjectWrappedValuePlaceholder &&
+      !paramInfo.anyContextualInfo())
     return arg;
 
   // Apply labels to arguments.
@@ -5979,9 +5982,10 @@ Expr *ExprRewriter::coerceCallArguments(
     bool isUnsafeSendable = paramInfo.isUnsafeSendable(paramIdx);
     bool isMainActor = paramInfo.isUnsafeMainActor(paramIdx) ||
         (isUnsafeSendable && apply && isMainDispatchQueue(apply->getFn()));
-    applyUnsafeConcurrent(
+    bool isImplicitSelfCapture = paramInfo.isImplicitSelfCapture(paramIdx);
+    applyContextualClosureFlags(
         arg, isUnsafeSendable && contextUsesConcurrencyFeatures(dc),
-        isMainActor);
+        isMainActor, isImplicitSelfCapture);
 
     // If the types exactly match, this is easy.
     auto paramType = param.getOldType();
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
@@ -1570,6 +1570,13 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
           return false;
       }
 
+      // If the closure was used in a context where it's explicitly stated
+      // that it does not need "self." qualification, don't require it.
+      if (auto closure = dyn_cast<ClosureExpr>(CE)) {
+        if (closure->allowsImplicitSelfCapture())
+          return false;
+      }
+
       return true;
     }
 
diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp
@@ -137,6 +137,7 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
   IGNORED_ATTR(AtReasync)
   IGNORED_ATTR(UnsafeSendable)
   IGNORED_ATTR(UnsafeMainActor)
+  IGNORED_ATTR(ImplicitSelfCapture)
 #undef IGNORED_ATTR
 
   void visitAlignmentAttr(AlignmentAttr *attr) {
diff --git a/lib/Sema/TypeCheckDeclOverride.cpp b/lib/Sema/TypeCheckDeclOverride.cpp
@@ -1546,7 +1546,7 @@ namespace  {
     UNINTERESTING_ATTR(Nonisolated)
     UNINTERESTING_ATTR(UnsafeSendable)
     UNINTERESTING_ATTR(UnsafeMainActor)
-
+    UNINTERESTING_ATTR(ImplicitSelfCapture)
 #undef UNINTERESTING_ATTR
 
     void visitAvailableAttr(AvailableAttr *attr) {
diff --git a/test/Concurrency/Runtime/data_race_detection.swift b/test/Concurrency/Runtime/data_race_detection.swift
@@ -1,4 +1,4 @@
-// RUN: %target-run-simple-swift(-Xfrontend -enable-experimental-concurrency  %import-libdispatch -parse-as-library) > %t.log 2>&1
+// RUN: %target-run-simple-swift(-Xfrontend -enable-experimental-concurrency  -enable-actor-data-race-checks %import-libdispatch -parse-as-library) > %t.log 2>&1
 // RUN: %FileCheck %s < %t.log
 
 // REQUIRES: executable_test
diff --git a/test/SILGen/check_executor.swift b/test/SILGen/check_executor.swift
@@ -1,5 +1,5 @@
-// RUN: %target-swift-frontend -emit-silgen %s -module-name test -swift-version 5 -enable-experimental-concurrency | %FileCheck --enable-var-scope %s --check-prefix=CHECK-RAW
-// RUN: %target-swift-frontend -emit-silgen %s -module-name test -swift-version 5 -enable-experimental-concurrency > %t.sil
+// RUN: %target-swift-frontend -emit-silgen %s -module-name test -swift-version 5 -enable-experimental-concurrency -enable-actor-data-race-checks | %FileCheck --enable-var-scope %s --check-prefix=CHECK-RAW
+// RUN: %target-swift-frontend -emit-silgen %s -module-name test -swift-version 5 -enable-experimental-concurrency -enable-actor-data-race-checks > %t.sil
 // RUN: %target-sil-opt -enable-sil-verify-all %t.sil -lower-hop-to-actor -enable-experimental-concurrency | %FileCheck --enable-var-scope %s --check-prefix=CHECK-CANONICAL
 // REQUIRES: concurrency
 
@@ -15,6 +15,8 @@ import _Concurrency
 
 func takeClosure(_ fn: @escaping () -> Int) { }
 
+func takeUnsafeMainActorClosure(@_unsafeMainActor _ fn: @escaping () -> Int) { }
+
 public actor MyActor {
   var counter = 0
 
@@ -36,4 +38,15 @@ public actor MyActor {
   deinit {
     takeClosure { self.counter }
   }
+
+  // CHECK-RAW-LABEL: sil private [ossa] @$s4test7MyActorC0A10UnsafeMainyyFSiycfU_
+  // CHECK-RAW-NOT: _checkExpectedExecutor
+  // CHECK-RAW: onMainActor
+  // CHECK-RAW: return
+  public func testUnsafeMain() {
+    takeUnsafeMainActorClosure {
+      onMainActor()
+      return 5
+    }
+  }
 }
diff --git a/test/attr/attr_implicitselfcapture.swift b/test/attr/attr_implicitselfcapture.swift
@@ -0,0 +1,24 @@
+// RUN: %target-typecheck-verify-swift
+
+func takeFn(@_implicitSelfCapture fn: @escaping () -> Int) { }
+func takeEscapingFn(fn: @escaping () -> Int) { }
+
+class C {
+  var property: Int = 0
+
+  func method() { }
+
+  func testMethod() {
+    takeFn { // no errors
+      method()
+      return property
+    }
+
+    takeEscapingFn { // expected-note 2 {{capture 'self' explicitly to enable implicit 'self' in this closure}}
+      method() // expected-error{{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}}
+      // expected-note@-1{{reference 'self.' explicitly}}
+      return property // expected-error{{reference to property 'property' in closure requires explicit use of 'self' to make capture semantics explicit}}
+      // expected-note@-1{{reference 'self.' explicitly}}
+    }
+  }
+}
