[Exclusivity] Don't suggest copying to a local when swapAt() is appropriate

Update static exclusivity diagnostics to not suggest copying to a local
when a call to swapAt() should be used instead. We already emit a FixIt in
this case, so don't suggest an an helpful fix in the diagnostic.

rdar://problem/32296784

Author: devincoughlin

include/swift/AST/DiagnosticsSIL.def

@@ -92,14 +92,18 @@ NOTE(previous_inout_alias,none,
 WARNING(exclusivity_access_required_swift3,none,
         "overlapping accesses to %0, but "
         "%select{initialization|read|modification|deinitialization}1 requires "
-        "exclusive access; consider copying to a local variable",
-        (StringRef, unsigned))
+        "exclusive access; "
+        "%select{consider copying to a local variable|"
+                "consider calling MutableCollection.swapAt(_:_:)}2",
+        (StringRef, unsigned, bool))
 
 ERROR(exclusivity_access_required,none,
       "overlapping accesses to %0, but "
       "%select{initialization|read|modification|deinitialization}1 requires "
-      "exclusive access; consider copying to a local variable",
-      (StringRef, unsigned))
+      "exclusive access; "
+      "%select{consider copying to a local variable|"
+              "consider calling MutableCollection.swapAt(_:_:)}2",
+      (StringRef, unsigned, bool))
 
 ERROR(exclusivity_access_required_unknown_decl,none,
         "overlapping accesses, but "

lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp

@@ -500,38 +500,45 @@ static bool isCallToStandardLibrarySwap(CallExpr *CE, ASTContext &Ctx) {
 }
 #endif
 
-/// Do a sytactic pattern match to try to safely suggest a Fix-It to rewrite
-/// calls like swap(&collection[index1], &collection[index2]) to
+/// Do a syntactic pattern match to determine whether the call is a call
+/// to swap(&base[index1], &base[index2]), which can
+/// be replaced with a call to MutableCollection.swapAt(_:_:) on base.
+///
+/// Returns true if the call can be replaced. Returns the call expression,
+/// the base expression, and the two indices as out expressions.
 ///
 /// 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
+/// in the function to avoid needing 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 *> CallsToSwap,
-                                  ASTContext &Ctx,
-                                  InFlightDiagnostic &Diag) {
+static bool
+canReplaceWithCallToCollectionSwapAt(const BeginAccessInst *Access1,
+                                     const BeginAccessInst *Access2,
+                                     ArrayRef<ApplyInst *> CallsToSwap,
+                                     ASTContext &Ctx,
+                                     CallExpr *&FoundCall,
+                                     Expr *&Base,
+                                     Expr *&Index1,
+                                     Expr *&Index2) {
   if (CallsToSwap.empty())
-    return;
+    return false;
 
   // Inout arguments must be modifications.
   if (Access1->getAccessKind() != SILAccessKind::Modify ||
       Access2->getAccessKind() != SILAccessKind::Modify) {
-    return;
+    return false;
   }
 
   SILLocation Loc1 = Access1->getLoc();
   SILLocation Loc2 = Access2->getLoc();
   if (Loc1.isNull() || Loc2.isNull())
-    return;
+    return false;
 
   auto *InOut1 = Loc1.getAsASTNode<InOutExpr>();
   auto *InOut2 = Loc2.getAsASTNode<InOutExpr>();
   if (!InOut1 || !InOut2)
-    return;
+    return false;
 
-  CallExpr *FoundCall = nullptr;
+  FoundCall = nullptr;
   // Look through all the calls to swap() recorded in the function to find
   // which one we're diagnosing.
   for (ApplyInst *AI : CallsToSwap) {
@@ -554,22 +561,22 @@ tryFixItWithCallToCollectionSwapAt(const BeginAccessInst *Access1,
     }
   }
   if (!FoundCall)
-    return;
+    return false;
 
   // 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;
+    return false;
   auto *SE2 = dyn_cast<SubscriptExpr>(InOut2->getSubExpr());
   if (!SE2)
-    return;
+    return false;
 
   // 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;
+    return false;
 
   ProtocolDecl *MutableCollectionDecl = Ctx.getMutableCollectionDecl();
 
@@ -594,7 +601,7 @@ tryFixItWithCallToCollectionSwapAt(const BeginAccessInst *Access1,
   }
 
   if (!IsSubscriptOnMutableCollection)
-    return;
+    return false;
 
   // We're swapping two subscripts on mutable collections -- but are they
   // the same collection? Approximate this by checking for textual
@@ -605,24 +612,33 @@ tryFixItWithCallToCollectionSwapAt(const BeginAccessInst *Access1,
   StringRef Base2Text = extractExprText(SE2->getBase(), SM);
 
   if (Base1Text != Base2Text)
-    return;
+    return false;
 
-  auto *Index1 = dyn_cast<ParenExpr>(SE1->getIndex());
-  if (!Index1)
-    return;
+  auto *Index1Paren = dyn_cast<ParenExpr>(SE1->getIndex());
+  if (!Index1Paren)
+    return false;
 
-  auto *Index2 = dyn_cast<ParenExpr>(SE2->getIndex());
-  if (!Index2)
-    return;
+  auto *Index2Paren = dyn_cast<ParenExpr>(SE2->getIndex());
+  if (!Index2Paren)
+    return false;
 
-  StringRef Index1Text = extractExprText(Index1->getSubExpr(), SM);
-  StringRef Index2Text = extractExprText(Index2->getSubExpr(), SM);
+  Base = SE1->getBase();
+  Index1 = Index1Paren->getSubExpr();
+  Index2 = Index2Paren->getSubExpr();
+  return true;
+}
 
-  // Suggest replacing with call with a call to swapAt().
+/// Suggest replacing with call with a call to swapAt().
+static void addSwapAtFixit(InFlightDiagnostic &Diag, CallExpr *&FoundCall,
+                           Expr *Base, Expr *&Index1, Expr *&Index2,
+                           SourceManager &SM) {
+  StringRef BaseText = extractExprText(Base, SM);
+  StringRef Index1Text = extractExprText(Index1, SM);
+  StringRef Index2Text = extractExprText(Index2, SM);
   SmallString<64> FixItText;
   {
     llvm::raw_svector_ostream Out(FixItText);
-    Out << Base1Text << ".swapAt(" << Index1Text << ", " << Index2Text << ")";
+    Out << BaseText << ".swapAt(" << Index1Text << ", " << Index2Text << ")";
   }
 
   Diag.fixItReplace(FoundCall->getSourceRange(), FixItText);
@@ -686,15 +702,27 @@ static void diagnoseExclusivityViolation(const ConflictingAccess &Violation,
     SILModule &M = FirstAccess.getInstruction()->getModule();
     std::string PathDescription = getPathDescription(
         VD->getBaseName(), BaseType, MainAccess.getSubPath(), M);
+
+    // Determine whether we can safely suggest replacing the violation with
+    // a call to MutableCollection.swapAt().
+    bool SuggestSwapAt = false;
+    CallExpr *CallToReplace = nullptr;
+    Expr *Base = nullptr;
+    Expr *SwapIndex1 = nullptr;
+    Expr *SwapIndex2 = nullptr;
+    if (SecondAccess.getRecordKind() == RecordedAccessKind::BeginInstruction) {
+        SuggestSwapAt = canReplaceWithCallToCollectionSwapAt(
+            FirstAccess.getInstruction(), SecondAccess.getInstruction(),
+            CallsToSwap, Ctx, CallToReplace, Base, SwapIndex1, SwapIndex2);
+    }
+
     auto D =
         diagnose(Ctx, MainAccess.getAccessLoc().getSourceLoc(), DiagnosticID,
-                 PathDescription, AccessKindForMain);
+                 PathDescription, AccessKindForMain, SuggestSwapAt);
     D.highlight(RangeForMain);
-    if (SecondAccess.getRecordKind() == RecordedAccessKind::BeginInstruction) {
-      tryFixItWithCallToCollectionSwapAt(FirstAccess.getInstruction(),
-                                         SecondAccess.getInstruction(),
-                                         CallsToSwap, Ctx, D);
-    }
+    if (SuggestSwapAt)
+      addSwapAtFixit(D, CallToReplace, Base, SwapIndex1, SwapIndex2,
+                     Ctx.SourceMgr);
   } else {
     auto DiagnosticID = (Ctx.LangOpts.isSwiftVersion3() ?
                          diag::exclusivity_access_required_unknown_decl_swift3 :

test/SILGen/polymorphic_inout_aliasing.swift

@@ -11,7 +11,7 @@ class Story {
   }
 
   func test() {
-    // expected-warning@+2 {{overlapping accesses to 'finalStored', but modification requires exclusive access; consider copying to a local variable}}
+    // expected-warning@+2 {{overlapping accesses to 'finalStored', but modification requires exclusive access; consider calling MutableCollection.swapAt(_:_:)}}
     // expected-note@+1 {{conflicting access is here}}
     swap(&self.finalStored[0], &self.finalStored[1])
     swap(&self.overridableStored[0], &self.overridableStored[1])
@@ -24,7 +24,7 @@ protocol Storied {
 }
 
 func testProtocol<T: Storied>(x: inout T) {
-  // expected-warning@+2 {{overlapping accesses to 'x', but modification requires exclusive access; consider copying to a local variable}}
+  // expected-warning@+2 {{overlapping accesses to 'x', but modification requires exclusive access; consider calling MutableCollection.swapAt(_:_:)}}
   // expected-note@+1 {{conflicting access is here}}
   swap(&x.protocolRequirement[0], &x.protocolRequirement[1])
 }

test/SILOptimizer/exclusivity_static_diagnostics.swift

@@ -27,9 +27,9 @@ func inoutOnInoutParameter(p: inout Int) {
 func swapNoSuppression(_ i: Int, _ j: Int) {
   var a: [Int] = [1, 2, 3]
 
-  // expected-error@+2{{overlapping accesses to 'a', but modification requires exclusive access; consider copying to a local variable}}
+  // expected-error@+2{{overlapping accesses to 'a', but modification requires exclusive access; consider calling MutableCollection.swapAt(_:_:)}}
   // expected-note@+1{{conflicting access is here}}
-  swap(&a[i], &a[j]) // no-warning
+  swap(&a[i], &a[j])
 }
 
 class SomeClass { }

test/SILOptimizer/exclusivity_static_diagnostics_swift3.swift

@@ -28,3 +28,11 @@ func diagnoseOnSameField() {
   // expected-note@+1{{conflicting access is here}}
   takesTwoInouts(&x.f, &x.f)
 }
+
+func diagnoseSwapOnMutableCollection(_ i: Int, _ j: Int) {
+  var a: [Int] = [1, 2, 3]
+
+  // expected-warning@+2{{overlapping accesses to 'a', but modification requires exclusive access; consider calling MutableCollection.swapAt(_:_:)}}
+  // expected-note@+1{{conflicting access is here}}
+  swap(&a[i], &a[j])
+}