Rephrase all access note remarks

• There is now one access note success remark and fix-it per declaration, not per attribute/modifier.
• Failure remarks have been rephrased to better emphasize the cause of the failure.
• The wording of other access note remarks and notes have been changed to follow a similar formula.

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -5852,39 +5852,43 @@ ERROR(atomics_ordering_must_be_constant, none,
 // MARK: access notes
 //------------------------------------------------------------------------------
 
+#define WHICH_ACCESS_NOTE(reason) "specified by access note for %" #reason
+
 REMARK(attr_added_by_access_note, none,
-       "access note for %0 adds %select{attribute|modifier}1 '%2' to this %3",
-       (StringRef, bool, StringRef, DescriptiveDeclKind))
+       "implicitly added '%1' to this %2, as " WHICH_ACCESS_NOTE(0),
+       (StringRef, StringRef, DescriptiveDeclKind))
 NOTE(fixit_attr_added_by_access_note, none,
-     "add %select{attribute|modifier}0 explicitly to silence this warning",
-     (bool))
+     "add '%0' explicitly to silence this warning",
+     (StringRef))
 
 REMARK(attr_removed_by_access_note, none,
-       "access note for %0 removes %select{attribute|modifier}1 '%2' from "
-       "this %3",
-       (StringRef, bool, StringRef, DescriptiveDeclKind))
+       "implicitly removed '%1' from this %3, as " WHICH_ACCESS_NOTE(0),
+       (StringRef, StringRef, DescriptiveDeclKind))
 NOTE(fixit_attr_removed_by_access_note, none,
-     "remove %select{attribute|modifier}0 explicitly to silence this warning",
-     (bool))
+     "remove '%0' explicitly to silence this warning",
+     (StringRef))
 
 REMARK(attr_objc_name_changed_by_access_note, none,
-        "access note for %0 changes the '@objc' name of this %1 to %2",
-        (StringRef, DescriptiveDeclKind, ObjCSelector))
+       "implicitly changed Objective-C name of this %1 to %2, as "
+       WHICH_ACCESS_NOTE(0),
+       (StringRef, DescriptiveDeclKind, ObjCSelector))
 NOTE(fixit_attr_objc_name_changed_by_access_note, none,
-     "change '@objc' name in source code explicitly to silence this warning",
+     "change Objective-C name explicitly to silence this warning",
      ())
 
 // Bad access note diagnostics. These are usually emitted as remarks, but
 // '-Raccess-note=all-validate' emits them as errors.
 
 ERROR(attr_objc_name_conflicts_with_access_note, none,
-      "access note for %0 changes the '@objc' name of this %1 to %2, but "
-      "source code specifies %3; the access note will be ignored",
+      "ignored access note: did not change Objective-C name of this %1 from %2 "
+      "to %3, even though it was " WHICH_ACCESS_NOTE(0),
       (StringRef, DescriptiveDeclKind, ObjCSelector, ObjCSelector))
 ERROR(wrap_invalid_attr_added_by_access_note, none,
-      "access note for %1 failed to add invalid "
-      "%select{attribute|modifier}2 '%3': %0",
-      (DiagnosticInfo *, StringRef, bool, StringRef))
+      "ignored access note: %0; did not implicitly add '%2' to this %3, even "
+      "though it was " WHICH_ACCESS_NOTE(1),
+      (DiagnosticInfo *, StringRef, StringRef, DescriptiveDeclKind))
+
+#undef WHICH_ACCESS_NOTE
 
 #define UNDEFINE_DIAGNOSTIC_MACROS
 #include "DefineDiagnosticMacros.h"

include/swift/AST/SourceFile.h

@@ -246,11 +246,10 @@ class SourceFile final : public FileUnit {
   /// List of Objective-C member conflicts we have found during type checking.
   std::vector<ObjCMethodConflict> ObjCMethodConflicts;
 
-  using DeclAndAttr = std::tuple<ValueDecl *, DeclAttribute *>;
-
   /// List of attributes added by access notes, used to emit remarks for valid
   /// ones.
-  std::vector<DeclAndAttr> AttrsAddedByAccessNotes;
+  llvm::DenseMap<ValueDecl *, std::vector<DeclAttribute *>>
+      AttrsAddedByAccessNotes;
 
   /// Describes what kind of file this is, which can affect some type checking
   /// and other behavior.

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -1416,35 +1416,59 @@ void TypeChecker::diagnoseDuplicateCaptureVars(CaptureListExpr *expr) {
   diagnoseDuplicateDecls(captureListVars);
 }
 
-template<typename ...DiagIDAndArgs> InFlightDiagnostic
-diagnoseAtAttrOrDecl(DeclAttribute *attr, ValueDecl *VD,
-                     DiagIDAndArgs... idAndArgs) {
-  ASTContext &ctx = VD->getASTContext();
-  if (attr->getLocation().isValid())
-    return ctx.Diags.diagnose(attr->getLocation(), idAndArgs...);
-  else
-    return ctx.Diags.diagnose(VD, idAndArgs...);
+static StringRef prettyPrintAttrs(const ValueDecl *VD,
+                                  ArrayRef<const DeclAttribute *> attrs,
+                                  SmallVectorImpl<char> &out) {
+  llvm::raw_svector_ostream os(out);
+  StreamPrinter printer(os);
+
+  PrintOptions opts = PrintOptions::printEverything();
+  VD->getAttrs().print(printer, opts, attrs, VD);
+  return StringRef(out.begin(), out.size()).drop_back();
+}
+
+static void diagnoseChangesByAccessNote(
+    ValueDecl *VD,
+    ArrayRef<const DeclAttribute *> attrs,
+    Diag<StringRef, StringRef, DescriptiveDeclKind> diagID,
+    Diag<StringRef> fixItID,
+    llvm::function_ref<void(InFlightDiagnostic, StringRef)> addFixIts) {
+  if (!VD->getASTContext().LangOpts.shouldRemarkOnAccessNoteSuccess() ||
+      attrs.empty())
+    return;
+
+  // Generate string containing all attributes.
+  SmallString<64> attrString;
+  auto attrText = prettyPrintAttrs(VD, attrs, attrString);
+
+  SourceLoc fixItLoc;
+
+  auto reason = VD->getModuleContext()->getAccessNotes().Reason;
+  auto diag = VD->diagnose(diagID, reason, attrText, VD->getDescriptiveKind());
+  for (auto attr : attrs) {
+    diag.highlight(attr->getRangeWithAt());
+    if (fixItLoc.isInvalid())
+      fixItLoc = attr->getRangeWithAt().Start;
+  }
+  diag.flush();
+
+  if (!fixItLoc)
+    fixItLoc = VD->getAttributeInsertionLoc(true);
+
+  addFixIts(VD->getASTContext().Diags.diagnose(fixItLoc, fixItID, attrText),
+            attrString);
 }
 
 template <typename Attr>
 static void addOrRemoveAttr(ValueDecl *VD, const AccessNotesFile &notes,
                             Optional<bool> expected,
+                            SmallVectorImpl<DeclAttribute *> &removedAttrs,
                             llvm::function_ref<Attr*()> willCreate) {
   if (!expected) return;
 
   auto attr = VD->getAttrs().getAttribute<Attr>();
   if (*expected == (attr != nullptr)) return;
 
-  auto diagnoseChangeByAccessNote =
-      [&](Diag<StringRef, bool, StringRef, DescriptiveDeclKind> diagID,
-          Diag<bool> fixitID) -> InFlightDiagnostic {
-    bool isModifier = attr->isDeclModifier();
-
-    diagnoseAtAttrOrDecl(attr, VD, diagID, notes.Reason, isModifier,
-                          attr->getAttrName(), VD->getDescriptiveKind());
-    return diagnoseAtAttrOrDecl(attr, VD, fixitID, isModifier);
-  };
-
   if (*expected) {
     attr = willCreate();
     attr->setAddedByAccessNote();
@@ -1453,77 +1477,80 @@ static void addOrRemoveAttr(ValueDecl *VD, const AccessNotesFile &notes,
     // Arrange for us to emit a remark about this attribute after type checking
     // has ensured it's valid.
     if (auto SF = VD->getDeclContext()->getParentSourceFile())
-      SF->AttrsAddedByAccessNotes.emplace_back(VD, attr);
+      SF->AttrsAddedByAccessNotes[VD].push_back(attr);
   } else {
+    removedAttrs.push_back(attr);
     VD->getAttrs().removeAttribute(attr);
-    if (VD->getASTContext().LangOpts.shouldRemarkOnAccessNoteSuccess())
-      diagnoseChangeByAccessNote(diag::attr_removed_by_access_note,
-                                 diag::fixit_attr_removed_by_access_note)
-        .fixItRemove(attr->getRangeWithAt());
   }
 }
 
 InFlightDiagnostic
 swift::softenIfAccessNote(const Decl *D, const DeclAttribute *attr,
                           InFlightDiagnostic &diag) {
-  if (!attr || !attr->getAddedByAccessNote())
+  const ValueDecl *VD = dyn_cast<ValueDecl>(D);
+  if (!VD || !attr || !attr->getAddedByAccessNote())
     return std::move(diag);
 
-  auto behavior = D->getASTContext().LangOpts.getAccessNoteFailureLimit();
+  SmallString<32> attrString;
+  auto attrText = prettyPrintAttrs(VD, makeArrayRef(attr), attrString);
+
+  ASTContext &ctx = D->getASTContext();
+  auto behavior = ctx.LangOpts.getAccessNoteFailureLimit();
   return std::move(diag.wrapIn(diag::wrap_invalid_attr_added_by_access_note,
                                D->getModuleContext()->getAccessNotes().Reason,
-                               attr->isDeclModifier(), attr->getAttrName())
+                               ctx.AllocateCopy(attrText), D->getDescriptiveKind())
                         .limitBehavior(behavior));
 }
 
 static void applyAccessNote(ValueDecl *VD, const AccessNote &note,
                             const AccessNotesFile &notes) {
   ASTContext &ctx = VD->getASTContext();
+  SmallVector<DeclAttribute *, 2> removedAttrs;
 
-  addOrRemoveAttr<ObjCAttr>(VD, notes, note.ObjC, [&]{
+  addOrRemoveAttr<ObjCAttr>(VD, notes, note.ObjC, removedAttrs, [&]{
     return ObjCAttr::create(ctx, note.ObjCName, false);
   });
 
-  addOrRemoveAttr<DynamicAttr>(VD, notes, note.Dynamic, [&]{
+  addOrRemoveAttr<DynamicAttr>(VD, notes, note.Dynamic, removedAttrs, [&]{
     return new (ctx) DynamicAttr(true);
   });
 
+  // FIXME: If we ever have more attributes, we'll need to sort removedAttrs by
+  // SourceLoc. As it is, attrs are always before modifiers, so we're okay now.
+
+  diagnoseChangesByAccessNote(VD, removedAttrs,
+                              diag::attr_removed_by_access_note,
+                              diag::fixit_attr_removed_by_access_note,
+                              [&](InFlightDiagnostic diag, StringRef code) {
+    for (auto attr : llvm::reverse(removedAttrs))
+      diag.fixItRemove(attr->getRangeWithAt());
+  });
+
   if (note.ObjCName) {
     auto attr = VD->getAttrs().getAttribute<ObjCAttr>();
     assert(attr && "ObjCName set, but ObjCAttr not true or did not apply???");
 
     if (!attr->hasName()) {
+      auto oldName = attr->getName();
       attr->setName(*note.ObjCName, true);
 
       if (!ctx.LangOpts.shouldRemarkOnAccessNoteSuccess())
         return;
 
-      diagnoseAtAttrOrDecl(attr, VD,
-                           diag::attr_objc_name_changed_by_access_note,
-                           notes.Reason, VD->getDescriptiveKind(),
-                           *note.ObjCName);
-
-      SmallString<64> newNameString;
-      llvm::raw_svector_ostream os(newNameString);
-      os << "(";
-      os << *note.ObjCName;
-      os << ")";
-
-      auto note = diagnoseAtAttrOrDecl(attr, VD,
-                            diag::fixit_attr_objc_name_changed_by_access_note);
+      VD->diagnose(diag::attr_objc_name_changed_by_access_note,
+                   notes.Reason, VD->getDescriptiveKind(), *note.ObjCName);
 
-      if (attr->getLParenLoc().isValid())
-        note.fixItReplace({ attr->getLParenLoc(), attr->getRParenLoc() },
-                          newNameString);
-      else
-        note.fixItInsertAfter(attr->getLocation(), newNameString);
+      auto fixIt =
+          VD->diagnose(diag::fixit_attr_objc_name_changed_by_access_note);
+      fixDeclarationObjCName(fixIt, VD, oldName, *note.ObjCName);
     }
     else if (attr->getName() != *note.ObjCName) {
       auto behavior = ctx.LangOpts.getAccessNoteFailureLimit();
-      diagnoseAtAttrOrDecl(attr, VD,
-                           diag::attr_objc_name_conflicts_with_access_note,
-                           notes.Reason, VD->getDescriptiveKind(),
-                           *note.ObjCName, *attr->getName())
+
+      VD->diagnose(diag::attr_objc_name_conflicts_with_access_note,
+                   notes.Reason, VD->getDescriptiveKind(), *attr->getName(),
+                   *note.ObjCName)
+          .highlight(attr->getRangeWithAt())
           .limitBehavior(behavior);
     }
   }
@@ -1538,24 +1565,31 @@ void swift::diagnoseAttrsAddedByAccessNote(SourceFile &SF) {
   if (!SF.getASTContext().LangOpts.shouldRemarkOnAccessNoteSuccess())
     return;
 
-  for (auto declAndAttr : SF.AttrsAddedByAccessNotes) {
-    auto VD = std::get<0>(declAndAttr);
-    auto attr = std::get<1>(declAndAttr);
-
-    if (attr->isInvalid()) continue;
-    assert(attr->getAddedByAccessNote());
-
-    bool isModifier = attr->isDeclModifier();
-
-    auto reason = VD->getModuleContext()->getAccessNotes().Reason;
-    VD->diagnose(diag::attr_added_by_access_note, reason, isModifier,
-                 attr->getAttrName(), VD->getDescriptiveKind());
+  for (auto declAndAttrs : SF.AttrsAddedByAccessNotes) {
+    auto D = declAndAttrs.getFirst();
+    SmallVector<DeclAttribute *, 4> sortedAttrs;
+    llvm::append_range(sortedAttrs, declAndAttrs.getSecond());
+
+    // Filter out invalid attributes.
+    sortedAttrs.erase(
+      llvm::remove_if(sortedAttrs, [](DeclAttribute *attr) {
+        assert(attr->getAddedByAccessNote());
+        return attr->isInvalid();
+      }), sortedAttrs.end());
+    if (sortedAttrs.empty()) continue;
+
+    // Sort attributes by name.
+    llvm::sort(sortedAttrs, [](DeclAttribute * first, DeclAttribute * second) {
+      return first->getAttrName() < second->getAttrName();
+    });
+    sortedAttrs.erase(std::unique(sortedAttrs.begin(), sortedAttrs.end()),
+                      sortedAttrs.end());
 
-    SmallString<64> attrString;
-    llvm::raw_svector_ostream os(attrString);
-    attr->print(os, VD);
-    VD->diagnose(diag::fixit_attr_added_by_access_note, isModifier)
-        .fixItInsert(VD->getAttributeInsertionLoc(isModifier), attrString);
+    diagnoseChangesByAccessNote(D, sortedAttrs, diag::attr_added_by_access_note,
+                                diag::fixit_attr_added_by_access_note,
+                                [=](InFlightDiagnostic diag, StringRef code) {
+      diag.fixItInsert(D->getAttributeInsertionLoc(/*isModifier=*/true), code);
+    });
   }
 }