Refine access note descriptions in diagnostics

include/swift/AST/Attr.h

@@ -854,6 +854,7 @@ class ObjCAttr final : public DeclAttribute,
   /// Determine whether the name associated with this attribute was
   /// implicit.
   bool isNameImplicit() const { return Bits.ObjCAttr.ImplicitName; }
+  void setNameImplicit(bool newValue) { Bits.ObjCAttr.ImplicitName = newValue; }
 
   /// Set the name of this entity.
   void setName(ObjCSelector name, bool implicit) {
@@ -882,11 +883,6 @@ class ObjCAttr final : public DeclAttribute,
     Bits.ObjCAttr.Swift3Inferred = inferred;
   }
 
-  /// Clear the name of this entity.
-  void clearName() {
-    NameData = nullptr;
-  }
-
   /// Retrieve the source locations for the names in a non-implicit
   /// nullary or selector attribute.
   ArrayRef<SourceLoc> getNameLocs() const;

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

@@ -5018,10 +5018,11 @@ NOTE(objc_declared_here,none,
      OBJC_DIAG_SELECT " declared here",
      (unsigned, DeclName))
 
+// %2 and %3 are unused to make signature match with diag::objc_redecl.
 ERROR(objc_redecl_same,none,
-      OBJC_DIAG_SELECT " with Objective-C selector %2 conflicts with "
+      OBJC_DIAG_SELECT " with Objective-C selector %4 conflicts with "
       "previous declaration with the same Objective-C selector",
-      (unsigned, DeclName, ObjCSelector))
+      (unsigned, DeclName, unsigned, DeclName, ObjCSelector))
 
 ERROR(objc_override_other,none,
       OBJC_DIAG_SELECT " with Objective-C selector %4 conflicts with "
@@ -5852,31 +5853,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",
      ())
-REMARK(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",
-       (StringRef, DescriptiveDeclKind, ObjCSelector, ObjCSelector))
+
+// 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,
+      "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,
+      "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

@@ -241,11 +241,18 @@ class SourceFile final : public FileUnit {
   /// unsatisfied, which might conflict with other Objective-C methods.
   std::vector<ObjCUnsatisfiedOptReq> ObjCUnsatisfiedOptReqs;
 
+  /// A selector that is used by two different declarations in the same class.
+  /// Fields: classDecl, selector, isInstanceMethod.
   using ObjCMethodConflict = std::tuple<ClassDecl *, ObjCSelector, bool>;
 
   /// List of Objective-C member conflicts we have found during type checking.
   std::vector<ObjCMethodConflict> ObjCMethodConflicts;
 
+  /// List of attributes added by access notes, used to emit remarks for valid
+  /// ones.
+  llvm::DenseMap<ValueDecl *, std::vector<DeclAttribute *>>
+      AttrsAddedByAccessNotes;
+
   /// Describes what kind of file this is, which can affect some type checking
   /// and other behavior.
   const SourceFileKind Kind;

include/swift/Basic/LangOptions.h

@@ -36,6 +36,8 @@
 
 namespace swift {
 
+  enum class DiagnosticBehavior : uint8_t;
+
   /// Kind of implicit platform conditions.
   enum class PlatformConditionKind {
 #define PLATFORM_CONDITION(LABEL, IDENTIFIER) LABEL,
@@ -69,6 +71,13 @@ namespace swift {
     Other
   };
 
+  enum class AccessNoteDiagnosticBehavior : uint8_t {
+    Ignore,
+    RemarkOnFailure,
+    RemarkOnFailureOrSuccess,
+    ErrorOnFailureRemarkOnSuccess
+  };
+
   /// A collection of options that affect the language dialect and
   /// provide compiler debugging facilities.
   class LangOptions final {
@@ -339,6 +348,17 @@ namespace swift {
     std::shared_ptr<llvm::Regex> OptimizationRemarkPassedPattern;
     std::shared_ptr<llvm::Regex> OptimizationRemarkMissedPattern;
 
+    /// How should we emit diagnostics about access notes?
+    AccessNoteDiagnosticBehavior AccessNoteBehavior =
+        AccessNoteDiagnosticBehavior::RemarkOnFailureOrSuccess;
+
+    DiagnosticBehavior getAccessNoteFailureLimit() const;
+
+    bool shouldRemarkOnAccessNoteSuccess() const {
+      return AccessNoteBehavior >=
+          AccessNoteDiagnosticBehavior::RemarkOnFailureOrSuccess;
+    }
+
     /// Whether collect tokens during parsing for syntax coloring.
     bool CollectParsedToken = false;
 

include/swift/Option/FrontendOptions.td

@@ -196,6 +196,12 @@ def enable_infer_public_concurrent_value : Flag<["-"], "enable-infer-public-send
 def disable_infer_public_concurrent_value : Flag<["-"], "disable-infer-public-sendable">,
     HelpText<"Disable inference of Sendable conformances for public structs and enums">;
 
+def Raccess_note : Separate<["-"], "Raccess-note">,
+    MetaVarName<"none|failures|all|all-validate">,
+    HelpText<"Control access note remarks (default: all)">;
+def Raccess_note_EQ : Joined<["-"], "Raccess-note=">,
+    Alias<Raccess_note>;
+
 } // end let Flags = [FrontendOption, NoDriverOption]
 
 def debug_crash_Group : OptionGroup<"<automatic crashing options>">;

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