“Wrap” invalid access note diagnostics

This shows up better in Xcode, which unfortunately doesn’t really display notes very clearly.

Author: beccadax

include/swift/AST/DiagnosticEngine.h

@@ -115,6 +115,7 @@ namespace swift {
     VersionTuple,
     LayoutConstraint,
     ActorIsolation,
+    Diagnostic
   };
 
   namespace diag {
@@ -146,6 +147,7 @@ namespace swift {
       llvm::VersionTuple VersionVal;
       LayoutConstraint LayoutConstraintVal;
       ActorIsolation ActorIsolationVal;
+      DiagnosticInfo *DiagnosticVal;
     };
 
   public:
@@ -243,6 +245,11 @@ namespace swift {
         ActorIsolationVal(AI) {
     }
 
+    DiagnosticArgument(DiagnosticInfo *D)
+      : Kind(DiagnosticArgumentKind::Diagnostic),
+        DiagnosticVal(D) {
+    }
+
     /// Initializes a diagnostic argument using the underlying type of the
     /// given enum.
     template<
@@ -343,6 +350,11 @@ namespace swift {
       assert(Kind == DiagnosticArgumentKind::ActorIsolation);
       return ActorIsolationVal;
     }
+
+    DiagnosticInfo *getAsDiagnostic() const {
+      assert(Kind == DiagnosticArgumentKind::Diagnostic);
+      return DiagnosticVal;
+    }
   };
 
   /// Describes the current behavior to take with a diagnostic.
@@ -410,6 +422,7 @@ namespace swift {
     DiagnosticBehavior BehaviorLimit = DiagnosticBehavior::Unspecified;
 
     friend DiagnosticEngine;
+    friend class InFlightDiagnostic;
 
   public:
     // All constructors are intentionally implicit.
@@ -515,6 +528,42 @@ namespace swift {
     /// emitted as a warning, but a note will still be emitted as a note.
     InFlightDiagnostic &limitBehavior(DiagnosticBehavior limit);
 
+    /// Wraps this diagnostic in another diagnostic. That is, \p wrapper will be
+    /// emitted in place of the diagnostic that otherwise would have been
+    /// emitted.
+    ///
+    /// The first argument of \p wrapper must be of type 'Diagnostic *'.
+    ///
+    /// The emitted diagnostic will have:
+    ///
+    /// \li The ID, message, and behavior of \c wrapper.
+    /// \li The arguments of \c wrapper, with the last argument replaced by the
+    ///     diagnostic currently in \c *this.
+    /// \li The location, ranges, decl, fix-its, and behavior limit of the
+    ///     diagnostic currently in \c *this.
+    InFlightDiagnostic &wrapIn(const Diagnostic &wrapper);
+
+    /// Wraps this diagnostic in another diagnostic. That is, \p ID and
+    /// \p VArgs will be emitted in place of the diagnostic that otherwise would
+    /// have been emitted.
+    ///
+    /// The first argument of \p ID must be of type 'Diagnostic *'.
+    ///
+    /// The emitted diagnostic will have:
+    ///
+    /// \li The ID, message, and behavior of \c ID.
+    /// \li The arguments of \c VArgs, with an argument appended for the
+    ///     diagnostic currently in \c *this.
+    /// \li The location, ranges, decl, fix-its, and behavior limit of the
+    ///     diagnostic currently in \c *this.
+    template<typename ...ArgTypes>
+    InFlightDiagnostic &
+    wrapIn(Diag<DiagnosticInfo *, ArgTypes...> ID,
+           typename detail::PassArgument<ArgTypes>::type... VArgs) {
+      Diagnostic wrapper{ID, nullptr, std::move(VArgs)...};
+      return wrapIn(wrapper);
+    }
+
     /// Add a token-based range to the currently-active diagnostic.
     InFlightDiagnostic &highlight(SourceRange R);
 
@@ -678,6 +727,16 @@ namespace swift {
       ignoredDiagnostics[(unsigned)id] = ignored;
     }
 
+    void swap(DiagnosticState &other) {
+      std::swap(showDiagnosticsAfterFatalError, other.showDiagnosticsAfterFatalError);
+      std::swap(suppressWarnings, other.suppressWarnings);
+      std::swap(warningsAsErrors, other.warningsAsErrors);
+      std::swap(fatalErrorOccurred, other.fatalErrorOccurred);
+      std::swap(anyErrorOccurred, other.anyErrorOccurred);
+      std::swap(previousBehavior, other.previousBehavior);
+      std::swap(ignoredDiagnostics, other.ignoredDiagnostics);
+    }
+
   private:
     // Make the state movable only
     DiagnosticState(const DiagnosticState &) = delete;
@@ -706,6 +765,10 @@ namespace swift {
     /// The currently active diagnostic, if there is one.
     Optional<Diagnostic> ActiveDiagnostic;
 
+    /// Diagnostics wrapped by ActiveDiagnostic, if any.
+    SmallVector<DiagnosticInfo, 2> WrappedDiagnostics;
+    SmallVector<std::vector<DiagnosticArgument>, 4> WrappedDiagnosticArgs;
+
     /// All diagnostics that have are no longer active but have not yet
     /// been emitted due to an open transaction.
     SmallVector<Diagnostic, 4> TentativeDiagnostics;

include/swift/AST/DiagnosticsSema.def

@@ -5879,9 +5879,10 @@ REMARK(attr_objc_name_conflicts_with_access_note, none,
        (StringRef, DescriptiveDeclKind, ObjCSelector, ObjCSelector))
 
 // Used on invalid @objc diagnostics
-NOTE(note_invalid_attr_added_by_access_note, none,
-     "%select{attribute|modifier}0 '%1' was added by access note for %2",
-     (bool, StringRef, StringRef))
+REMARK(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))
 
 #define UNDEFINE_DIAGNOSTIC_MACROS
 #include "DefineDiagnosticMacros.h"

lib/AST/DiagnosticEngine.cpp

@@ -319,6 +319,38 @@ InFlightDiagnostic::limitBehavior(DiagnosticBehavior limit) {
   return *this;
 }
 
+InFlightDiagnostic &
+InFlightDiagnostic::wrapIn(const Diagnostic &wrapper) {
+  // Save current active diagnostic into WrappedDiagnostics, ignoring state
+  // so we don't get a None return or influence future diagnostics.
+  DiagnosticState tempState;
+  Engine->state.swap(tempState);
+  llvm::SaveAndRestore<DiagnosticBehavior>
+      limit(Engine->getActiveDiagnostic().BehaviorLimit,
+            DiagnosticBehavior::Unspecified);
+
+  Engine->WrappedDiagnostics.push_back(
+       *Engine->diagnosticInfoForDiagnostic(Engine->getActiveDiagnostic()));
+
+  Engine->state.swap(tempState);
+
+  auto &wrapped = Engine->WrappedDiagnostics.back();
+
+  // Copy and update its arg list.
+  Engine->WrappedDiagnosticArgs.emplace_back(wrapped.FormatArgs);
+  wrapped.FormatArgs = Engine->WrappedDiagnosticArgs.back();
+
+  // Overwrite the ID and argument with those from the wrapper.
+  Engine->getActiveDiagnostic().ID = wrapper.ID;
+  Engine->getActiveDiagnostic().Args = wrapper.Args;
+
+  // Set the argument to the diagnostic being wrapped.
+  assert(wrapper.getArgs().front().getKind() == DiagnosticArgumentKind::Diagnostic);
+  Engine->getActiveDiagnostic().Args.front() = &wrapped;
+
+  return *this;
+}
+
 void InFlightDiagnostic::flush() {
   if (!IsActive)
     return;
@@ -525,7 +557,7 @@ static bool isMainActor(Type type) {
 
 /// Format a single diagnostic argument and write it to the given
 /// stream.
-static void formatDiagnosticArgument(StringRef Modifier, 
+static void formatDiagnosticArgument(StringRef Modifier,
                                      StringRef ModifierArguments,
                                      ArrayRef<DiagnosticArgument> Args,
                                      unsigned ArgIndex,
@@ -752,6 +784,7 @@ static void formatDiagnosticArgument(StringRef Modifier,
         << FormatOpts.ClosingQuotationMark;
     break;
   case DiagnosticArgumentKind::ActorIsolation:
+    assert(Modifier.empty() && "Improper modifier for ActorIsolation argument");
     switch (auto isolation = Arg.getAsActorIsolation()) {
     case ActorIsolation::ActorInstance:
       Out << "actor-isolated";
@@ -775,6 +808,15 @@ static void formatDiagnosticArgument(StringRef Modifier,
       Out << "nonisolated";
       break;
     }
+    break;
+
+  case DiagnosticArgumentKind::Diagnostic: {
+    assert(Modifier.empty() && "Improper modifier for Diagnostic argument");
+    auto diagArg = Arg.getAsDiagnostic();
+    DiagnosticEngine::formatDiagnosticText(Out, diagArg->FormatString,
+                                           diagArg->FormatArgs);
+    break;
+  }
   }
 }
 
@@ -953,6 +995,8 @@ void DiagnosticEngine::flushActiveDiagnostic() {
   assert(ActiveDiagnostic && "No active diagnostic to flush");
   if (TransactionCount == 0) {
     emitDiagnostic(*ActiveDiagnostic);
+    WrappedDiagnostics.clear();
+    WrappedDiagnosticArgs.clear();
   } else {
     onTentativeDiagnosticFlush(*ActiveDiagnostic);
     TentativeDiagnostics.emplace_back(std::move(*ActiveDiagnostic));
@@ -965,6 +1009,8 @@ void DiagnosticEngine::emitTentativeDiagnostics() {
     emitDiagnostic(diag);
   }
   TentativeDiagnostics.clear();
+  WrappedDiagnostics.clear();
+  WrappedDiagnosticArgs.clear();
 }
 
 /// Returns the access level of the least accessible PrettyPrintedDeclarations

lib/Sema/TypeCheckAttr.cpp

@@ -1016,10 +1016,11 @@ void AttributeChecker::visitObjCAttr(ObjCAttr *attr) {
         else {
           firstNameLoc = D->getLoc();
         }
-        diagnose(firstNameLoc, diag::objc_name_req_nullary,
-                 D->getDescriptiveKind())
-          .fixItRemoveChars(afterFirstNameLoc, attr->getRParenLoc())
-          .limitBehavior(behavior);
+        softenIfAccessNote(D, attr,
+          diagnose(firstNameLoc, diag::objc_name_req_nullary,
+                   D->getDescriptiveKind())
+            .fixItRemoveChars(afterFirstNameLoc, attr->getRParenLoc())
+            .limitBehavior(behavior));
         const_cast<ObjCAttr *>(attr)->setName(
           ObjCSelector(Ctx, 0, objcName->getSelectorPieces()[0]),
           /*implicit=*/false);
@@ -1028,11 +1029,12 @@ void AttributeChecker::visitObjCAttr(ObjCAttr *attr) {
       SourceLoc diagLoc = attr->getLParenLoc();
       if (diagLoc.isInvalid())
         diagLoc = D->getLoc();
-      diagnose(diagLoc,
-               isa<SubscriptDecl>(D)
-                 ? diag::objc_name_subscript
-                 : diag::objc_name_deinit)
-          .limitBehavior(behavior);
+      softenIfAccessNote(D, attr,
+        diagnose(diagLoc,
+                 isa<SubscriptDecl>(D)
+                   ? diag::objc_name_subscript
+                   : diag::objc_name_deinit)
+            .limitBehavior(behavior));
       const_cast<ObjCAttr *>(attr)->clearName();
     } else {
       auto func = cast<AbstractFunctionDecl>(D);
@@ -1063,15 +1065,16 @@ void AttributeChecker::visitObjCAttr(ObjCAttr *attr) {
         SourceLoc firstNameLoc = func->getLoc();
         if (!attr->getNameLocs().empty())
           firstNameLoc = attr->getNameLocs().front();
-        diagnose(firstNameLoc,
-                 diag::objc_name_func_mismatch,
-                 isa<FuncDecl>(func),
-                 numArgumentNames,
-                 numArgumentNames != 1,
-                 numParameters,
-                 numParameters != 1,
-                 func->hasThrows())
-            .limitBehavior(behavior);
+        softenIfAccessNote(D, attr,
+          diagnose(firstNameLoc,
+                   diag::objc_name_func_mismatch,
+                   isa<FuncDecl>(func),
+                   numArgumentNames,
+                   numArgumentNames != 1,
+                   numParameters,
+                   numParameters != 1,
+                   func->hasThrows())
+              .limitBehavior(behavior));
         D->getAttrs().add(
           ObjCAttr::createUnnamed(Ctx, attr->AtLoc,  attr->Range.Start));
         D->getAttrs().removeAttribute(attr);
@@ -1180,26 +1183,13 @@ void TypeChecker::checkDeclAttributes(Decl *D) {
     default: break;
     }
 
-    DiagnosticBehavior behavior = attr->getAddedByAccessNote()
-                                ? DiagnosticBehavior::Remark
-                                : DiagnosticBehavior::Unspecified;
-
     if (!OnlyKind.empty())
       Checker.diagnoseAndRemoveAttr(attr, diag::attr_only_one_decl_kind,
-                                    attr, OnlyKind)
-          .limitBehavior(behavior);
+                                    attr, OnlyKind);
     else if (attr->isDeclModifier())
-      Checker.diagnoseAndRemoveAttr(attr, diag::invalid_decl_modifier, attr)
-          .limitBehavior(behavior);
+      Checker.diagnoseAndRemoveAttr(attr, diag::invalid_decl_modifier, attr);
     else
-      Checker.diagnoseAndRemoveAttr(attr, diag::invalid_decl_attribute, attr)
-          .limitBehavior(behavior);
-
-    if (attr->getAddedByAccessNote()) {
-      auto notesFileReason = D->getModuleContext()->getAccessNotes().Reason;
-      D->diagnose(diag::note_invalid_attr_added_by_access_note,
-                  attr->isDeclModifier(), attr->getAttrName(), notesFileReason);
-    }
+      Checker.diagnoseAndRemoveAttr(attr, diag::invalid_decl_attribute, attr);
   }
   Checker.checkOriginalDefinedInAttrs(D, ODIAttrs);
 }