[Exclusivity] Improve static enforcement diagnostics

Static diagnostics now refer to the identifier for the variable requiring
exclusive diagnostics. Additionally, when two accesses conflict we now always
emit the main diagnostic on the first modifying access and the note on either
the second modifying access or the read.

The diagnostics also now highlight the source range for the expression
beginning the access.

Author: devincoughlin

include/swift/AST/DiagnosticsSIL.def

@@ -87,11 +87,17 @@ NOTE(previous_inout_alias,none,
 // This is temporarily a warning during staging to make it easier to evaluate.
 // The intent is to change it to an error before turning it on by default.
 WARNING(exclusivity_access_required,none,
+        "simultaneous accesses to %0 %1; "
+        "%select{initialization|read|modification|deinitialization}2 requires "
+        "exclusive access", (DescriptiveDeclKind, Identifier, unsigned))
+
+WARNING(exclusivity_access_required_unknown_decl,none,
+        "simultaneous accesses; "
         "%select{initialization|read|modification|deinitialization}0 requires "
-        "%select{exclusive|shared}1 access", (unsigned, unsigned))
+        "exclusive access", (unsigned))
+
 NOTE(exclusivity_conflicting_access,none,
-     "conflicting %select{initialization|read|modification|deinitialization}0 "
-     "requires %select{exclusive|shared}1 access", (unsigned, unsigned))
+     "conflicting access is here", ())
 
 ERROR(unsupported_c_function_pointer_conversion,none,
       "C function pointer signature %0 is not compatible with expected type %1",

lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp

@@ -29,6 +29,7 @@
 #include "swift/AST/Decl.h"
 #include "swift/AST/Expr.h"
 #include "swift/AST/Stmt.h"
+#include "swift/Basic/SourceLoc.h"
 #include "swift/SIL/CFG.h"
 #include "swift/SIL/SILArgument.h"
 #include "swift/SIL/SILInstruction.h"
@@ -135,6 +136,29 @@ class AccessedStorage {
     assert(Kind == AccessedStorageKind::ClassProperty);
     return ObjProj;
   }
+
+  /// Returns the ValueDecl for the underlying storage, if it can be
+  /// determined. Otherwise returns null. For diagonostic purposes.
+  const ValueDecl *getStorageDecl() const {
+    switch(Kind) {
+    case AccessedStorageKind::GlobalVar:
+      return getGlobal()->getDecl();
+    case AccessedStorageKind::Value:
+      if (auto *Box = dyn_cast<AllocBoxInst>(getValue())) {
+        return Box->getLoc().getAsASTNode<VarDecl>();
+      }
+      if (auto *Arg = dyn_cast<SILFunctionArgument>(getValue())) {
+        return Arg->getDecl();
+      }
+      break;
+    case AccessedStorageKind::ClassProperty: {
+      const ObjectProjection &OP = getObjectProjection();
+      const Projection &P = OP.getProjection();
+      return P.getVarDecl(OP.getObject()->getType());
+    }
+    }
+    return nullptr;
+  }
 };
 
 /// Models the in-progress accesses for a single storage location.
@@ -218,6 +242,7 @@ using StorageMap = llvm::SmallDenseMap<AccessedStorage, AccessInfo, 4>;
 
 /// A pair of 'begin_access' instructions that conflict.
 struct ConflictingAccess {
+  AccessedStorage Storage;
   const BeginAccessInst *FirstAccess;
   const BeginAccessInst *SecondAccess;
 };
@@ -334,25 +359,44 @@ isConflictOnInoutArgumentsToSuppressed(const BeginAccessInst *Access1,
 /// Emits a diagnostic if beginning an access with the given in-progress
 /// accesses violates the law of exclusivity. Returns true when a
 /// diagnostic was emitted.
-static void diagnoseExclusivityViolation(const BeginAccessInst *PriorAccess,
+static void diagnoseExclusivityViolation(const AccessedStorage &Storage,
+                                         const BeginAccessInst *PriorAccess,
                                          const BeginAccessInst *NewAccess,
                                          ASTContext &Ctx) {
 
   // Can't have a conflict if both accesses are reads.
   assert(!(PriorAccess->getAccessKind() == SILAccessKind::Read &&
            NewAccess->getAccessKind() == SILAccessKind::Read));
 
-  ExclusiveOrShared_t NewRequires = getRequiredAccess(NewAccess);
   ExclusiveOrShared_t PriorRequires = getRequiredAccess(PriorAccess);
 
-  diagnose(Ctx, NewAccess->getLoc().getSourceLoc(),
-           diag::exclusivity_access_required,
-           static_cast<unsigned>(NewAccess->getAccessKind()),
-           static_cast<unsigned>(NewRequires));
-  diagnose(Ctx, PriorAccess->getLoc().getSourceLoc(),
-           diag::exclusivity_conflicting_access,
-           static_cast<unsigned>(PriorAccess->getAccessKind()),
-           static_cast<unsigned>(PriorRequires));
+  // Diagnose on the first access that requires exclusivity.
+  const BeginAccessInst *AccessForMainDiagnostic = PriorAccess;
+  const BeginAccessInst *AccessForNote = NewAccess;
+  if (PriorRequires != ExclusiveOrShared_t::ExclusiveAccess) {
+    AccessForMainDiagnostic = NewAccess;
+    AccessForNote = PriorAccess;
+  }
+
+  SourceRange rangeForMain =
+    AccessForMainDiagnostic->getLoc().getSourceRange();
+
+  if (const ValueDecl *VD = Storage.getStorageDecl()) {
+    diagnose(Ctx, AccessForMainDiagnostic->getLoc().getSourceLoc(),
+             diag::exclusivity_access_required,
+             VD->getDescriptiveKind(),
+             VD->getName(),
+             static_cast<unsigned>(AccessForMainDiagnostic->getAccessKind()))
+        .highlight(rangeForMain);
+  } else {
+    diagnose(Ctx, AccessForMainDiagnostic->getLoc().getSourceLoc(),
+             diag::exclusivity_access_required_unknown_decl,
+             static_cast<unsigned>(AccessForMainDiagnostic->getAccessKind()))
+        .highlight(rangeForMain);
+  }
+  diagnose(Ctx, AccessForNote->getLoc().getSourceLoc(),
+           diag::exclusivity_conflicting_access)
+      .highlight(AccessForNote->getLoc().getSourceRange());
 }
 
 /// Make a best effort to find the underlying object for the purpose
@@ -508,11 +552,12 @@ static void checkStaticExclusivity(SILFunction &Fn, PostOrderFunctionInfo *PO) {
       // Ending onr decrements the count.
       if (auto *BAI = dyn_cast<BeginAccessInst>(&I)) {
         SILAccessKind Kind = BAI->getAccessKind();
-        AccessInfo &Info = Accesses[findAccessedStorage(BAI->getSource())];
+        const AccessedStorage &Storage = findAccessedStorage(BAI->getSource());
+        AccessInfo &Info = Accesses[Storage];
         if (Info.conflictsWithAccess(Kind) && !Info.alreadyHadConflict()) {
           const BeginAccessInst *Conflict = Info.getFirstAccess();
           assert(Conflict && "Must already have had access to conflict!");
-          ConflictingAccesses.push_back({ Conflict, BAI });
+          ConflictingAccesses.push_back({ Storage, Conflict, BAI });
         }
 
         Info.beginAccess(BAI);
@@ -554,7 +599,8 @@ static void checkStaticExclusivity(SILFunction &Fn, PostOrderFunctionInfo *PO) {
                                                CallsToSuppress))
       continue;
 
-    diagnoseExclusivityViolation(PriorAccess, NewAccess, Fn.getASTContext());
+    diagnoseExclusivityViolation(Violation.Storage, PriorAccess, NewAccess,
+                                 Fn.getASTContext());
   }
 }
 

test/SILOptimizer/exclusivity_static_diagnostics.sil

@@ -38,8 +38,8 @@ bb0(%0 : $Int):
   %3 = project_box %2 : ${ var Int }, 0
   store %0 to [trivial] %3 : $*Int
   %4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
-  %5 = begin_access [modify] [unknown] %3 : $*Int  // expected-note {{conflicting modification requires exclusive access}}
-  %6 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{modification requires exclusive access}}
+  %5 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{simultaneous accesses; modification requires exclusive access}}
+  %6 = begin_access [modify] [unknown] %3 : $*Int  // expected-note {{conflicting access is here}}
   %7 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
   end_access %6 : $*Int
   end_access %5: $*Int
@@ -55,9 +55,9 @@ bb0(%0 : $Int):
   %3 = project_box %2 : ${ var Int }, 0
   store %0 to [trivial] %3 : $*Int
   %4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
-  %5 = begin_access [modify] [unknown] %3 : $*Int  // expected-note {{conflicting modification requires exclusive access}}
+  %5 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{simultaneous accesses; modification requires exclusive access}}
   %6 = begin_access [modify] [unknown] %5 : $*Int
-  %7 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{modification requires exclusive access}}
+  %7 = begin_access [modify] [unknown] %3 : $*Int  // expected-note {{conflicting access is here}}
   %8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
   end_access %7 : $*Int
   end_access %6 : $*Int
@@ -147,8 +147,8 @@ bb0(%0 : $Int, %1 : $Builtin.Int1):
   br bb1
 bb1:
   // Make sure we don't diagnose twice.
-  %4 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
-  %5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
+  %4 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
+  %5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
   end_access %5: $*Int
   end_access %4: $*Int
   cond_br %1, bb1, bb2
@@ -198,8 +198,8 @@ bb0(%0 : $Int):
   %1 = alloc_box ${ var Int }
   %2 = project_box %1 : ${ var Int }, 0
   store %0 to [trivial] %2 : $*Int
-  %4 = begin_access [read] [unknown] %2 : $*Int // expected-note {{conflicting read requires shared access}}
-  %5 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{modification requires exclusive access}}
+  %4 = begin_access [read] [unknown] %2 : $*Int // expected-note {{conflicting access is here}}
+  %5 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
   end_access %5 : $*Int
   end_access %4: $*Int
   destroy_value %1 : ${ var Int }
@@ -213,8 +213,8 @@ bb0(%0 : $Int):
   %1 = alloc_box ${ var Int }
   %2 = project_box %1 : ${ var Int }, 0
   store %0 to [trivial] %2 : $*Int
-  %4 = begin_access [modify] [unknown] %2 : $*Int // expected-note {{conflicting modification requires exclusive access}}
-  %5 = begin_access [read] [unknown] %2 : $*Int // expected-warning {{read requires shared access}}
+  %4 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
+  %5 = begin_access [read] [unknown] %2 : $*Int // expected-note {{conflicting access is here}}
   end_access %5 : $*Int
   end_access %4: $*Int
   destroy_value %1 : ${ var Int }
@@ -232,11 +232,11 @@ sil hidden @classStoredProperty : $@convention(thin) (ClassWithStoredProperty) -
 bb0(%0 : $ClassWithStoredProperty):
   %1 = ref_element_addr %0 : $ClassWithStoredProperty, #ClassWithStoredProperty.f
 
-  // expected-note@+1{{conflicting modification requires exclusive access}}
+  // expected-warning@+1{{simultaneous accesses to var 'f'; modification requires exclusive access}}
   %2 = begin_access [modify] [dynamic] %1 : $*Int
   %3 = ref_element_addr %0 : $ClassWithStoredProperty, #ClassWithStoredProperty.f
 
-  // expected-warning@+1{{modification requires exclusive access}}
+  // expected-note@+1{{conflicting access is here}}
   %4 = begin_access [modify] [dynamic] %3 : $*Int
   end_access %4 : $*Int
   end_access %2 : $*Int
@@ -252,11 +252,11 @@ bb0(%0 : $ClassWithStoredProperty):
   %2 = begin_borrow %0 : $ClassWithStoredProperty
   %3 = ref_element_addr %1 : $ClassWithStoredProperty, #ClassWithStoredProperty.f
 
-  // expected-note@+1{{conflicting modification requires exclusive access}}
+  // expected-warning@+1{{simultaneous accesses to var 'f'; modification requires exclusive access}}
   %4 = begin_access [modify] [dynamic] %3 : $*Int
   %5 = ref_element_addr %2 : $ClassWithStoredProperty, #ClassWithStoredProperty.f
 
-  // expected-warning@+1{{modification requires exclusive access}}
+  // expected-note@+1{{conflicting access is here}}
   %6 = begin_access [modify] [dynamic] %5 : $*Int
   end_access %6 : $*Int
   end_access %4 : $*Int
@@ -279,8 +279,8 @@ bb0(%0 : $Int):
   store %0 to [trivial] %3 : $*Int
   %4 = copy_value %2 : ${ var Int }
   %5 = project_box %4 : ${ var Int }, 0
-  %6 = begin_access [modify] [unknown] %3 : $*Int  // expected-note {{conflicting modification requires exclusive access}}
-  %7 = begin_access [modify] [unknown] %5 : $*Int  // expected-warning {{modification requires exclusive access}}
+  %6 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{simultaneous accesses; modification requires exclusive access}}
+  %7 = begin_access [modify] [unknown] %5 : $*Int  // expected-note {{conflicting access is here}}
   end_access %7 : $*Int
   end_access %6: $*Int
   destroy_value %2 : ${ var Int }
@@ -297,8 +297,8 @@ bb0(%0 : $Int):
   %3 = project_box %2 : ${ var Int }, 0
   store %0 to [trivial] %3 : $*Int
   %4 = project_box %2 : ${ var Int }, 0
-  %5 = begin_access [modify] [unknown] %3 : $*Int  // expected-note {{conflicting modification requires exclusive access}}
-  %6 = begin_access [modify] [unknown] %4 : $*Int  // expected-warning {{modification requires exclusive access}}
+  %5 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{simultaneous accesses; modification requires exclusive access}}
+  %6 = begin_access [modify] [unknown] %4 : $*Int  // expected-note {{conflicting access is here}}
   end_access %6 : $*Int
   end_access %5: $*Int
   destroy_value %2 : ${ var Int }
@@ -315,8 +315,8 @@ sil hidden @modifySameGlobal : $@convention(thin) (Int) -> () {
 bb0(%0 : $Int):
   %1 = global_addr @global1 :$*Int
   %2 = global_addr @global1 :$*Int
-  %3 = begin_access [modify] [unknown] %1 : $*Int  // expected-note {{conflicting modification requires exclusive access}}
-  %4 = begin_access [modify] [unknown] %2 : $*Int  // expected-warning {{modification requires exclusive access}}
+  %3 = begin_access [modify] [unknown] %1 : $*Int  // expected-warning {{simultaneous accesses; modification requires exclusive access}}
+  %4 = begin_access [modify] [unknown] %2 : $*Int  // expected-note {{conflicting access is here}}
   end_access %4 : $*Int
   end_access %3: $*Int
   %5 = tuple ()
@@ -347,8 +347,8 @@ bb0(%0 : $Int):
   %3 = project_box %2 : ${ var Int }, 0
   store %0 to [trivial] %3 : $*Int
   %4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
-  %5 = begin_access [read] [unknown] %3 : $*Int  // expected-note {{conflicting read requires shared access}}
-  %6 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{modification requires exclusive access}}
+  %5 = begin_access [read] [unknown] %3 : $*Int  // expected-note {{conflicting access is here}}
+  %6 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{simultaneous accesses; modification requires exclusive access}}
   %7 = begin_access [read] [unknown] %3 : $*Int // no-error
   %8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
   end_access %7 : $*Int
@@ -368,8 +368,8 @@ bb0(%0 : $Int):
   %3 = project_box %2 : ${ var Int }, 0
   store %0 to [trivial] %3 : $*Int
   %4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
-  %5 = begin_access [modify] [unknown] %3 : $*Int  // expected-note {{conflicting modification requires exclusive access}}
-  %6 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{modification requires exclusive access}}
+  %5 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{simultaneous accesses; modification requires exclusive access}}
+  %6 = begin_access [modify] [unknown] %3 : $*Int  // expected-note {{conflicting access is here}}
   %7 = begin_access [modify] [unknown] %3 : $*Int  // no-error
   %8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
   end_access %7 : $*Int
@@ -392,8 +392,8 @@ bb0(%0 : $Int):
   %4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
   %5 = begin_access [modify] [unknown] %3 : $*Int  // no-note
   end_access %5 : $*Int
-  %6 = begin_access [modify] [unknown] %3 : $*Int  // expected-note {{conflicting modification requires exclusive access}}
-  %7 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{modification requires exclusive access}}
+  %6 = begin_access [modify] [unknown] %3 : $*Int  // expected-warning {{simultaneous accesses; modification requires exclusive access}}
+  %7 = begin_access [modify] [unknown] %3 : $*Int  // expected-note {{conflicting access is here}}
   %8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
   end_access %7 : $*Int
   end_access %6: $*Int

test/SILOptimizer/exclusivity_static_diagnostics.swift

@@ -7,17 +7,22 @@ func takesTwoInouts<T>(_ p1: inout T, _ p2: inout T) { }
 func simpleInoutDiagnostic() {
   var i = 7
 
-  // expected-warning@+2{{modification requires exclusive access}}
-  // expected-note@+1{{conflicting modification requires exclusive access}}
+  // expected-warning@+2{{simultaneous accesses to var 'i'; modification requires exclusive access}}
+  // expected-note@+1{{conflicting access is here}}
   takesTwoInouts(&i, &i)
 }
 
+func inoutOnInoutParameter(p: inout Int) {
+  // expected-warning@+2{{simultaneous accesses to parameter 'p'; modification requires exclusive access}}
+  // expected-note@+1{{conflicting access is here}}
+  takesTwoInouts(&p, &p)
+}
 
 func swapNoSuppression(_ i: Int, _ j: Int) {
   var a: [Int] = [1, 2, 3]
 
-  // expected-warning@+2{{modification requires exclusive access}}
-  // expected-note@+1{{conflicting modification requires exclusive access}}
+  // expected-warning@+2{{simultaneous accesses to var 'a'; modification requires exclusive access}}
+  // expected-note@+1{{conflicting access is here}}
   swap(&a[i], &a[j]) // no-warning
 }
 
@@ -29,23 +34,23 @@ struct StructWithMutatingMethodThatTakesSelfInout {
   mutating func mutate(_ other: inout SomeClass) { }
 
   mutating func callMutatingMethodThatTakesSelfInout() {
-    // expected-warning@+2{{modification requires exclusive access}}
-    // expected-note@+1{{conflicting modification requires exclusive access}}
+    // expected-warning@+2{{simultaneous accesses to parameter 'self'; modification requires exclusive access}}
+    // expected-note@+1{{conflicting access is here}}
     mutate(&self)
   }
 
   mutating func callMutatingMethodThatTakesSelfStoredPropInout() {
-    // expected-warning@+2{{modification requires exclusive access}}
-    // expected-note@+1{{conflicting modification requires exclusive access}}
+    // expected-warning@+2{{simultaneous accesses to parameter 'self'; modification requires exclusive access}}
+    // expected-note@+1{{conflicting access is here}}
     mutate(&self.f)
   }
 }
 
-var global1 = StructWithMutatingMethodThatTakesSelfInout()
+var globalStruct1 = StructWithMutatingMethodThatTakesSelfInout()
 func callMutatingMethodThatTakesGlobalStoredPropInout() {
-  // expected-warning@+2{{modification requires exclusive access}}
-  // expected-note@+1{{conflicting modification requires exclusive access}}
-  global1.mutate(&global1.f)
+  // expected-warning@+2{{simultaneous accesses to var 'globalStruct1'; modification requires exclusive access}}
+  // expected-note@+1{{conflicting access is here}}
+  globalStruct1.mutate(&globalStruct1.f)
 }
 
 class ClassWithFinalStoredProp {
@@ -55,21 +60,21 @@ class ClassWithFinalStoredProp {
   func callMutatingMethodThatTakesClassStoredPropInout() {
     s1.mutate(&s2.f) // no-warning
 
-    // expected-warning@+2{{modification requires exclusive access}}
-    // expected-note@+1{{conflicting modification requires exclusive access}}
+    // expected-warning@+2{{simultaneous accesses to var 's1'; modification requires exclusive access}}
+    // expected-note@+1{{conflicting access is here}}
     s1.mutate(&s1.f)
 
     let local1 = self
 
-    // expected-warning@+2{{modification requires exclusive access}}
-    // expected-note@+1{{conflicting modification requires exclusive access}}
+    // expected-warning@+2{{simultaneous accesses to var 's1'; modification requires exclusive access}}
+    // expected-note@+1{{conflicting access is here}}
     local1.s1.mutate(&local1.s1.f)
   }
 }
 
 func violationWithGenericType<T>(_ p: T) {
   var local = p
-  // expected-warning@+2{{modification requires exclusive access}}
-  // expected-note@+1{{conflicting modification requires exclusive access}}
+  // expected-warning@+2{{simultaneous accesses to var 'local'; modification requires exclusive access}}
+  // expected-note@+1{{conflicting access is here}}
   takesTwoInouts(&local, &local)
 }