Merge pull request #37285 from atrick/fix-self-capture

Fix an assert in exclusivity diagnostics when inouts escape.

Author: atrick

lib/SILOptimizer/Analysis/AccessSummaryAnalysis.cpp

@@ -143,10 +143,22 @@ void AccessSummaryAnalysis::processArgument(FunctionInfo *info,
 /// used by directly calling it or passing it as argument, but not using it as a
 /// partial_apply callee.
 ///
-/// FIXME: This needs to be checked in the SILVerifier.
+/// An error found in DiagnoseInvalidEscapingCaptures can indicate invalid SIL
+/// that is detected here but not in normal SIL verification. When the
+/// source-level closure captures an inout argument, it appears in SIL to be a
+/// non-escaping closure. The following verification then fails because the
+/// "nonescaping" closure actually escapes.
+///
+/// FIXME: This should be checked in the SILVerifier, with consideration for the
+/// caveat above where an inout has been captured be an escaping closure.
 static bool hasExpectedUsesOfNoEscapePartialApply(Operand *partialApplyUse) {
   SILInstruction *user = partialApplyUse->getUser();
 
+  // Bypass this verification when a diagnostic error is present. See comments
+  // on DiagnoseInvalidEscapingCaptures above.
+  if (user->getModule().getASTContext().hadError())
+    return true;
+
   // It is fine to call the partial apply
   switch (user->getKind()) {
   case SILInstructionKind::ApplyInst:

lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp

@@ -1054,25 +1054,39 @@ static void checkAccessedAddress(Operand *memOper, StorageMap &Accesses) {
 
 namespace {
 
-class DiagnoseStaticExclusivity : public SILFunctionTransform {
+/// TODO: This is currently a module transform to ensure that source-level
+/// diagnostics, like DiagnoseInvalidCaptures run on closures (in addition to
+/// other callees) before this pass processes their parent functions. Otherwise,
+/// AccessSummaryAnalysis may crash on invalid SIL. Fix the pass manager to
+/// ensure that closures are always diagnosed before their parent. Then add an
+/// SCC transform to ensure that the previous diagnostic pass runs on all
+/// functions in the SCC before the next diagnostic pass. This will handle
+/// closures that call back into their parent. Then this can be converted to an
+/// SCC transform.
+class DiagnoseStaticExclusivity : public SILModuleTransform {
 public:
   DiagnoseStaticExclusivity() {}
 
 private:
   void run() override {
-    // Don't rerun diagnostics on deserialized functions.
-    if (getFunction()->wasDeserializedCanonical())
-      return;
+    for (auto &function : *getModule()) {
+      if (!function.isDefinition())
+        continue;
 
-    SILFunction *Fn = getFunction();
-    // This is a staging flag. Eventually the ability to turn off static
-    // enforcement will be removed.
-    if (!Fn->getModule().getOptions().EnforceExclusivityStatic)
-      return;
+      // Don't rerun diagnostics on deserialized functions.
+      if (function.wasDeserializedCanonical())
+        continue;
 
-    PostOrderFunctionInfo *PO = getAnalysis<PostOrderAnalysis>()->get(Fn);
-    auto *ASA = getAnalysis<AccessSummaryAnalysis>();
-    checkStaticExclusivity(*Fn, PO, ASA);
+      // This is a staging flag. Eventually the ability to turn off static
+      // enforcement will be removed.
+      if (!function.getModule().getOptions().EnforceExclusivityStatic)
+        continue;
+
+      PostOrderFunctionInfo *PO =
+        getAnalysis<PostOrderAnalysis>()->get(&function);
+      auto *ASA = getAnalysis<AccessSummaryAnalysis>();
+      checkStaticExclusivity(function, PO, ASA);
+    }
   }
 };
 

test/SILOptimizer/invalid_escaping_captures.swift

@@ -255,4 +255,23 @@ func badNoEscapeCaptureThroughVar(_ fn: () -> ()) {
   takesEscaping { // expected-error {{escaping closure captures non-escaping value}}
     myFunc()
   }
-}
+}
+
+@inline(never)
+func takeNoEscapeReturnGetter(f: ()->()->Int64) -> ()->Int64 { return f() }
+
+// Test that invalid escaping capture diagnostics are run on nested
+// closures before exclusivity diagnostics are run. Exclusivity
+// diagnostics need to inspect all referenced closures that capture
+// inouts. Also ensure that exclusivity diagnostics does not crash
+// when verifying that a closure that captures inout does not escape
+// as long as a previous diagnostic error is present.
+struct TestInoutEscapeInClosure {
+  var someValue: Int64 = 0
+  mutating func testInoutEscapeInClosure() -> () -> Int64 {
+    return takeNoEscapeReturnGetter {
+      return { return someValue } // expected-error {{escaping closure captures mutating 'self' parameter}}
+      // expected-note@-1 {{captured here}}
+    }
+  }
+}