Fix an assert in exclusivity diagnostics when inouts escape.

An error found in DiagnoseInvalidEscapingCaptures can indicate invalid
SIL, which will cause DiagnoseStaticExclusivity to assert during SIL
verification. When the source-level closure captures an inout
argument, it appears in SIL to be a non-escaping closure. The SIL
verification then fails because the "nonescaping" closure actually
escapes.

Ensure that capture diagnostics run on closures before exclusivity
enforcement runs on the parent function. Bypass the SIL verification
assert if a diagnostic error was found.

Fixes rdar://75364904 (Crash with assertion `noescape partial_apply
has unexpected use`)

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}}
+    }
+  }
+}