[Exclusivity] Suggest Fix-Its to replace swap() with swapAt()

Extend the static diagnostics for exclusivity violations to suggest replacing

  swap(&collection[index1], &collection[index2]

with

  collection.swapAt(index1, index2).

when 'collection' is a MutableCollection.

To do so, repurpose some vestigial code that was previously used to suppress all
exclusivity diagnostics for calls to swap() and add some additional syntactic,
semantic,  and textual pattern matching.

rdar://problem/31916085

include/swift/AST/KnownStdlibTypes.def

@@ -49,6 +49,7 @@ KNOWN_STDLIB_TYPE_DECL(Set, NominalTypeDecl, 1)
 KNOWN_STDLIB_TYPE_DECL(Sequence, NominalTypeDecl, 1)
 KNOWN_STDLIB_TYPE_DECL(Dictionary, NominalTypeDecl, 2)
 KNOWN_STDLIB_TYPE_DECL(AnyHashable, NominalTypeDecl, 0)
+KNOWN_STDLIB_TYPE_DECL(MutableCollection, ProtocolDecl, 1)
 
 KNOWN_STDLIB_TYPE_DECL(PartialKeyPath, NominalTypeDecl, 1)
 KNOWN_STDLIB_TYPE_DECL(KeyPath, NominalTypeDecl, 2)

include/swift/AST/SILOptions.h

@@ -143,10 +143,6 @@ public:
   /// Emit compile-time diagnostics when the law of exclusivity is violated.
   bool EnforceExclusivityStatic = true;
 
-  /// Suppress static diagnostics for violations of exclusive access for calls
-  /// to the Standard Library's swap() free function.
-  bool SuppressStaticExclusivitySwap = false;
-
   /// Emit checks to trap at run time when the law of exclusivity is violated.
   bool EnforceExclusivityDynamic = false;
 

lib/Frontend/CompilerInvocation.cpp

@@ -1378,8 +1378,6 @@ static bool ParseSILArgs(SILOptions &Opts, ArgList &Args,
     |= Args.hasArg(OPT_assume_parsing_unqualified_ownership_sil);
   Opts.EnableMandatorySemanticARCOpts |=
       !Args.hasArg(OPT_disable_mandatory_semantic_arc_opts);
-  Opts.SuppressStaticExclusivitySwap |=
-      Args.hasArg(OPT_suppress_static_exclusivity_swap);
 
   if (Args.hasArg(OPT_debug_on_sil)) {
     // Derive the name of the SIL file for debugging from

lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp

@@ -30,6 +30,7 @@
 #include "swift/AST/Expr.h"
 #include "swift/AST/Stmt.h"
 #include "swift/Basic/SourceLoc.h"
+#include "swift/Parse/Lexer.h"
 #include "swift/SIL/CFG.h"
 #include "swift/SIL/SILArgument.h"
 #include "swift/SIL/SILInstruction.h"
@@ -305,33 +306,48 @@ static ExclusiveOrShared_t getRequiredAccess(const BeginAccessInst *BAI) {
   return ExclusiveOrShared_t::ExclusiveAccess;
 }
 
-/// Perform a syntactic suppression that returns true when the accesses are for
-/// inout arguments to a call that corresponds to one of the passed-in
-/// 'apply' instructions.
-static bool
-isConflictOnInoutArgumentsToSuppressed(const BeginAccessInst *Access1,
+/// Extract the text for the given expression.
+static StringRef extractExprText(const Expr *E, SourceManager &SM) {
+  const auto CSR = Lexer::getCharSourceRangeFromSourceRange(SM,
+      E->getSourceRange());
+  return SM.extractText(CSR);
+}
+
+/// Do a sytactic pattern match to try to safely suggest a Fix-It to rewrite
+/// calls like swap(&collection[index1], &collection[index2]) to
+///
+/// This method takes an array of all the ApplyInsts for calls to swap()
+/// in the function to avoid need to construct a parent map over the AST
+/// to find the CallExpr for the inout accesses.
+static void
+tryFixItWithCallToCollectionSwapAt(const BeginAccessInst *Access1,
                                   const BeginAccessInst *Access2,
-                                       ArrayRef<ApplyInst *> CallsToSuppress) {
-  if (CallsToSuppress.empty())
-    return false;
+                                  ArrayRef<ApplyInst *> CallsToSwap,
+                                  ASTContext &Ctx,
+                                  InFlightDiagnostic &Diag) {
+  if (CallsToSwap.empty())
+    return;
 
   // Inout arguments must be modifications.
   if (Access1->getAccessKind() != SILAccessKind::Modify ||
       Access2->getAccessKind() != SILAccessKind::Modify) {
-    return false;
+    return;
   }
 
   SILLocation Loc1 = Access1->getLoc();
   SILLocation Loc2 = Access2->getLoc();
   if (Loc1.isNull() || Loc2.isNull())
-    return false;
+    return;
 
   auto *InOut1 = Loc1.getAsASTNode<InOutExpr>();
   auto *InOut2 = Loc2.getAsASTNode<InOutExpr>();
   if (!InOut1 || !InOut2)
-    return false;
+    return;
 
-  for (ApplyInst *AI : CallsToSuppress) {
+  CallExpr *FoundCall = nullptr;
+  // Look through all the calls to swap() recorded in the function to find
+  // which one we're diagnosing.
+  for (ApplyInst *AI : CallsToSwap) {
     SILLocation CallLoc = AI->getLoc();
     if (CallLoc.isNull())
       continue;
@@ -340,21 +356,89 @@ isConflictOnInoutArgumentsToSuppressed(const BeginAccessInst *Access1,
     if (!CE)
       continue;
 
-    bool FoundTarget = false;
-    CE->forEachChildExpr([=,&FoundTarget](Expr *Child) {
-      if (Child == InOut1) {
-        FoundTarget = true;
-        // Stops the traversal.
-        return (Expr *)nullptr;
+    assert(CE->getCalledValue() == Ctx.getSwap(nullptr));
+    // swap() takes two arguments.
+    auto *ArgTuple = cast<TupleExpr>(CE->getArg());
+    const Expr *Arg1 = ArgTuple->getElement(0);
+    const Expr *Arg2 = ArgTuple->getElement(1);
+    if ((Arg1 == InOut1 && Arg2 == InOut2)) {
+        FoundCall = CE;
+      break;
+    }
+  }
+  if (!FoundCall)
+    return;
+
+  // We found a call to swap(&e1, &e2). Now check to see whether it
+  // matches the form swap(&someCollection[index1], &someCollection[index2]).
+  auto *SE1 = dyn_cast<SubscriptExpr>(InOut1->getSubExpr());
+  if (!SE1)
+    return;
+  auto *SE2 = dyn_cast<SubscriptExpr>(InOut2->getSubExpr());
+  if (!SE2)
+    return;
+
+  // Do the two subscripts refer to the same subscript declaration?
+  auto *Decl1 = cast<SubscriptDecl>(SE1->getDecl().getDecl());
+  auto *Decl2 = cast<SubscriptDecl>(SE2->getDecl().getDecl());
+  if (Decl1 != Decl2)
+    return;
+
+  ProtocolDecl *MutableCollectionDecl = Ctx.getMutableCollectionDecl();
+
+  // Is the subcript either (1) on MutableCollection itself or (2) a
+  // a witness for a subscript on MutableCollection?
+  bool IsSubscriptOnMutableCollection = false;
+  ProtocolDecl *ProtocolForDecl =
+      Decl1->getDeclContext()->getAsProtocolOrProtocolExtensionContext();
+  if (ProtocolForDecl) {
+    IsSubscriptOnMutableCollection = (ProtocolForDecl == MutableCollectionDecl);
+  } else {
+    for (ValueDecl *Req : Decl1->getSatisfiedProtocolRequirements()) {
+      DeclContext *ReqDC = Req->getDeclContext();
+      ProtocolDecl *ReqProto = ReqDC->getAsProtocolOrProtocolExtensionContext();
+      assert(ReqProto && "Protocol requirement not in a protocol?");
+
+      if (ReqProto == MutableCollectionDecl) {
+        IsSubscriptOnMutableCollection = true;
+        break;
       }
-      return Child;
     }
-    );
-    if (FoundTarget)
-      return true;
   }
 
-  return false;
+  if (!IsSubscriptOnMutableCollection)
+    return;
+
+  // We're swapping two subscripts on mutable collections -- but are they
+  // the same collection? Approximate this by checking for textual
+  // equality on the base expressions. This is just an approximation,
+  // but is fine for a best-effort Fix-It.
+  SourceManager &SM = Ctx.SourceMgr;
+  StringRef Base1Text = extractExprText(SE1->getBase(), SM);
+  StringRef Base2Text = extractExprText(SE2->getBase(), SM);
+
+  if (Base1Text != Base2Text)
+    return;
+
+  auto *Index1 = dyn_cast<ParenExpr>(SE1->getIndex());
+  if (!Index1)
+    return;
+
+  auto *Index2 = dyn_cast<ParenExpr>(SE2->getIndex());
+  if (!Index2)
+    return;
+
+  StringRef Index1Text = extractExprText(Index1->getSubExpr(), SM);
+  StringRef Index2Text = extractExprText(Index2->getSubExpr(), SM);
+
+  // Suggest replacing with call with a call to swapAt().
+  SmallString<64> FixItText;
+  {
+    llvm::raw_svector_ostream Out(FixItText);
+    Out << Base1Text << ".swapAt(" << Index1Text << ", " << Index2Text << ")";
+  }
+
+  Diag.fixItReplace(FoundCall->getSourceRange(), FixItText);
 }
 
 /// Emits a diagnostic if beginning an access with the given in-progress
@@ -363,6 +447,7 @@ isConflictOnInoutArgumentsToSuppressed(const BeginAccessInst *Access1,
 static void diagnoseExclusivityViolation(const AccessedStorage &Storage,
                                          const BeginAccessInst *PriorAccess,
                                          const BeginAccessInst *NewAccess,
+                                         ArrayRef<ApplyInst *> CallsToSwap,
                                          ASTContext &Ctx) {
 
   DEBUG(llvm::dbgs() << "Conflict on " << *PriorAccess
@@ -385,25 +470,29 @@ static void diagnoseExclusivityViolation(const AccessedStorage &Storage,
 
   SourceRange rangeForMain =
     AccessForMainDiagnostic->getLoc().getSourceRange();
+  unsigned AccessKindForMain =
+      static_cast<unsigned>(AccessForMainDiagnostic->getAccessKind());
 
   if (const ValueDecl *VD = Storage.getStorageDecl()) {
     // We have a declaration, so mention the identifier in the diagnostic.
     auto DiagnosticID = (Ctx.LangOpts.isSwiftVersion3() ?
                          diag::exclusivity_access_required_swift3 :
                          diag::exclusivity_access_required);
-    diagnose(Ctx, AccessForMainDiagnostic->getLoc().getSourceLoc(),
+    auto D = diagnose(Ctx, AccessForMainDiagnostic->getLoc().getSourceLoc(),
                        DiagnosticID,
                        VD->getDescriptiveKind(),
                        VD->getName(),
-             static_cast<unsigned>(AccessForMainDiagnostic->getAccessKind()))
-        .highlight(rangeForMain);
+                       AccessKindForMain);
+    D.highlight(rangeForMain);
+    tryFixItWithCallToCollectionSwapAt(PriorAccess, NewAccess,
+                                       CallsToSwap, Ctx, D);
   } else {
     auto DiagnosticID = (Ctx.LangOpts.isSwiftVersion3() ?
                          diag::exclusivity_access_required_unknown_decl_swift3 :
                          diag::exclusivity_access_required_unknown_decl);
     diagnose(Ctx, AccessForMainDiagnostic->getLoc().getSourceLoc(),
              DiagnosticID,
-             static_cast<unsigned>(AccessForMainDiagnostic->getAccessKind()))
+             AccessKindForMain)
         .highlight(rangeForMain);
   }
   diagnose(Ctx, AccessForNote->getLoc().getSourceLoc(),
@@ -500,7 +589,8 @@ static AccessedStorage findAccessedStorage(SILValue Source) {
 }
 
 /// Returns true when the apply calls the Standard Library swap().
-/// Used for diagnostic suppression.
+/// Used for fix-its to suggest replacing with Collection.swapAt()
+/// on exclusivity violations.
 bool isCallToStandardLibrarySwap(ApplyInst *AI, ASTContext &Ctx) {
   SILFunction *SF = AI->getReferencedFunction();
   if (!SF)
@@ -535,9 +625,8 @@ static void checkStaticExclusivity(SILFunction &Fn, PostOrderFunctionInfo *PO) {
   if (Fn.empty())
     return;
 
-  // Collects calls to functions for which diagnostics about conflicting inout
-  // arguments should be suppressed.
-  llvm::SmallVector<ApplyInst *, 8> CallsToSuppress;
+  // Collects calls the Standard Library swap() for Fix-Its.
+  llvm::SmallVector<ApplyInst *, 8> CallsToSwap;
 
   // Stores the accesses that have been found to conflict. Used to defer
   // emitting diagnostics until we can determine whether they should
@@ -606,11 +695,9 @@ static void checkStaticExclusivity(SILFunction &Fn, PostOrderFunctionInfo *PO) {
       }
 
       if (auto *AI = dyn_cast<ApplyInst>(&I)) {
-        // Suppress for the arguments to the Standard Library's swap()
-        // function until we can recommend a safe alternative.
-        if (Fn.getModule().getOptions().SuppressStaticExclusivitySwap &&
-            isCallToStandardLibrarySwap(AI, Fn.getASTContext()))
-          CallsToSuppress.push_back(AI);
+        // Record calls to swap() for potential Fix-Its.
+        if (isCallToStandardLibrarySwap(AI, Fn.getASTContext()))
+          CallsToSwap.push_back(AI);
       }
 
       // Sanity check to make sure entries are properly removed.
@@ -624,12 +711,9 @@ static void checkStaticExclusivity(SILFunction &Fn, PostOrderFunctionInfo *PO) {
   for (auto &Violation : ConflictingAccesses) {
     const BeginAccessInst *PriorAccess = Violation.FirstAccess;
     const BeginAccessInst *NewAccess = Violation.SecondAccess;
-    if (isConflictOnInoutArgumentsToSuppressed(PriorAccess, NewAccess,
-                                               CallsToSuppress))
-      continue;
 
     diagnoseExclusivityViolation(Violation.Storage, PriorAccess, NewAccess,
-                                 Fn.getASTContext());
+                                 CallsToSwap, Fn.getASTContext());
   }
 }
 

test/SILOptimizer/exclusivity_static_diagnostics.swift

@@ -89,6 +89,7 @@ func violationWithGenericType<T>(_ p: T) {
   takesTwoInouts(&local, &local)
 }
 
+
 // Helper.
 struct StructWithTwoStoredProp {
   var f1: Int
@@ -106,3 +107,74 @@ func violationWithUnsafePointer(_ s: inout StructWithTwoStoredProp) {
     _ = s.f2
   }
 }
+// Tests for Fix-Its to replace swap(&collection[a], &collection[b]) with
+// collection.swapAt(a, b)
+
+struct StructWithField {
+  var f = 12
+}
+
+struct StructWithFixits {
+  var arrayProp: [Int] = [1, 2, 3]
+  var dictionaryProp: [Int : Int] = [0 : 10, 1 : 11]
+
+  mutating
+  func shouldHaveFixIts<T>(_ i: Int, _ j: Int, _ param: T, _ paramIndex: T.Index) where T : MutableCollection {
+    var array1 = [1, 2, 3]
+    // expected-error@+2{{simultaneous accesses}}{{5-41=array1.swapAt(i + 5, j - 2)}}
+    // expected-note@+1{{conflicting access is here}}
+    swap(&array1[i + 5], &array1[j - 2])
+
+    // expected-error@+2{{simultaneous accesses}}{{5-49=self.arrayProp.swapAt(i, j)}}
+    // expected-note@+1{{conflicting access is here}}
+    swap(&self.arrayProp[i], &self.arrayProp[j])
+
+    var localOfGenericType = param
+    // expected-error@+2{{simultaneous accesses}}{{5-75=localOfGenericType.swapAt(paramIndex, paramIndex)}}
+    // expected-note@+1{{conflicting access is here}}
+    swap(&localOfGenericType[paramIndex], &localOfGenericType[paramIndex])
+  }
+
+  mutating
+  func shouldHaveNoFixIts(_ i: Int, _ j: Int) {
+    var s = StructWithField()
+    // expected-error@+2{{simultaneous accesses}}{{none}}
+    // expected-note@+1{{conflicting access is here}}
+    swap(&s.f, &s.f)
+
+    var array1 = [1, 2, 3]
+    var array2 = [1, 2, 3]
+
+    // Swapping between different arrays should cannot have the
+    // Fix-It.
+    swap(&array1[i], &array2[j]) // no-warning no-fixit
+    swap(&array1[i], &self.arrayProp[j]) // no-warning no-fixit
+
+    // Dictionaries aren't MutableCollections so don't support swapAt().
+    // expected-error@+2{{simultaneous accesses}}{{none}}
+    // expected-note@+1{{conflicting access is here}}
+    swap(&dictionaryProp[i], &dictionaryProp[j])
+
+    // We could safely Fix-It this but don't now because the left and
+    // right bases are not textually identical.
+    // expected-error@+2{{simultaneous accesses}}{{none}}
+    // expected-note@+1{{conflicting access is here}}
+    swap(&self.arrayProp[i], &arrayProp[j])
+
+    // We could safely Fix-It this but we're not that heroic.
+    // We don't suppress when swap() is used as a value
+    let mySwap: (inout Int, inout Int) -> () = swap
+
+    // expected-error@+2{{simultaneous accesses}}{{none}}
+    // expected-note@+1{{conflicting access is here}}
+    mySwap(&array1[i], &array1[j])
+
+    func myOtherSwap<T>(_ a: inout T, _ b: inout T) {
+      swap(&a, &b) // no-warning
+    }
+
+    // expected-error@+2{{simultaneous accesses}}{{none}}
+    // expected-note@+1{{conflicting access is here}}
+    mySwap(&array1[i], &array1[j])
+  }
+}

test/SILOptimizer/exclusivity_suppress_swap.swift

@@ -1,39 +0,0 @@
-// RUN: %target-swift-frontend -enforce-exclusivity=checked -suppress-static-exclusivity-swap -emit-sil -primary-file %s -o /dev/null -verify
-
-import Swift
-
-func takesTwoInouts(_ p1: inout Int, _ p2: inout Int) { }
-
-func swapSuppression(_ i: Int, _ j: Int) {
-  var a: [Int] = [1, 2, 3]
-
-  swap(&a[i], &a[j]) // no-warning
-
-  // expected-warning@+2{{simultaneous accesses to var 'a', but modification requires exclusive access; consider copying to a local variable}}
-  // expected-note@+1{{conflicting access is here}}
-  takesTwoInouts(&a[i], &a[j])
-}
-
-func missedSwapSuppression(_ i: Int, _ j: Int) {
-  var a: [Int] = [1, 2, 3]
-
-  // We don't suppress when swap() is used as a value
-  let mySwap: (inout Int, inout Int) -> () = swap
-
-  // expected-warning@+2{{simultaneous accesses to var 'a', but modification requires exclusive access; consider copying to a local variable}}
-  // expected-note@+1{{conflicting access is here}}
-  mySwap(&a[i], &a[j])
-}
-
-func dontSuppressUserSwap(_ i: Int, _ j: Int) {
-  var a: [Int] = [1, 2, 3]
-
-  // Don't suppress on user-defined swap.
-  func swap<T>(_ p1: inout T, _ p2: inout T) {
-    return (p1, p2) = (p2, p1)
-  }
-
-  // expected-warning@+2{{simultaneous accesses to var 'a', but modification requires exclusive access; consider copying to a local variable}}
-  // expected-note@+1{{conflicting access is here}}
-  swap(&a[i], &a[j])
-}