[NFC] Clean up access note code

• Rename properties for greater consistency with the YAML file
• Rename AccessNotes -> AccessNotesFile
• Add doc comments
• Miscellaneous comment/style improvements.

Author: beccadax

include/swift/AST/AccessNotes.h

@@ -32,47 +32,79 @@ namespace swift {
 class ASTContext;
 class ValueDecl;
 
+/// The name of the declaration an access note should be applied to.
+///
+/// Grammatically, this is equivalent to a \c ParsedDeclName, but supports a
+/// subset of that type's features.
 class AccessNoteDeclName {
 public:
+  /// The names of the parent/contextual declarations containing the declaration
+  /// the access note should apply to.
   std::vector<Identifier> parentNames;
+
+  /// The name of the declaration the access note should be applied to. (For
+  /// accessors, this is actually the name of the storage it's attached to.)
   DeclName name;
+
+  /// For accessors, the kind of accessor; for non-accessors, \c None.
   Optional<AccessorKind> accessorKind;
 
   AccessNoteDeclName(ASTContext &ctx, StringRef str);
   AccessNoteDeclName();
 
+  /// If true, an access note with this name should apply to \p VD.
   bool matches(ValueDecl *VD) const;
+
+  /// If true, the name is empty and invalid.
   bool empty() const;
 
   void print(llvm::raw_ostream &os) const;
   SWIFT_DEBUG_DUMP;
 };
 
+/// An individual access note specifying modifications to a declaration.
 class AccessNote {
 public:
-  AccessNoteDeclName name;
+  /// The name of the declaration to modify.
+  AccessNoteDeclName Name;
 
+  /// If \c true, add an @objc attribute; if \c false, delete an @objc
+  /// attribute; if \c None, do nothing.
   Optional<bool> ObjC;
+
+  /// If \c true, add a dynamic modifier; if \c false, delete a dynamic
+  /// modifier; if \c None, do nothing.
   Optional<bool> Dynamic;
+
+  /// If set, modify an @objc attribute to give it the specified \c ObjCName.
+  /// If \c ObjC would otherwise be \c None, it will be set to \c true.
   Optional<ObjCSelector> ObjCName;
 
   void dump(llvm::raw_ostream &os, int indent = 0) const;
   SWIFT_DEBUG_DUMP;
 };
 
-class AccessNotes {
+/// A set of access notes with module-wide metadata about them.
+class AccessNotesFile {
 public:
-  std::string reason;
-  std::vector<AccessNote> notes;
+  /// A human-readable string describing why the access notes are being applied.
+  /// Inserted into diagnostics about access notes in the file.
+  std::string Reason;
+
+  /// Access notes to apply to the module.
+  std::vector<AccessNote> Notes;
 
   /// Contains keys which were present in the JSON file but were not recognized
   /// by the compiler. Has no functional effect, but can be used for error
   /// handling.
   std::set<std::string> unknownKeys;
 
-  static llvm::Expected<AccessNotes> load(ASTContext &ctx,
-                                          llvm::MemoryBuffer *buffer);
+  /// Load the access notes from \p buffer, or return an error if they cannot
+  /// be loaded.
+  static llvm::Expected<AccessNotesFile> load(ASTContext &ctx,
+                                              llvm::MemoryBuffer *buffer);
 
+  /// Look up the access note in this file, if any, which applies to \p VD.
   NullablePtr<const AccessNote> lookup(ValueDecl *VD) const;
 
   void dump(llvm::raw_ostream &os) const;

include/swift/AST/Module.h

@@ -250,7 +250,7 @@ class ModuleDecl : public DeclContext, public TypeDecl {
   /// \see EntryPointInfoTy
   EntryPointInfoTy EntryPointInfo;
 
-  AccessNotes accessNotes;
+  AccessNotesFile accessNotes;
 
   ModuleDecl(Identifier name, ASTContext &ctx, ImplicitImportInfo importInfo);
 
@@ -282,8 +282,8 @@ class ModuleDecl : public DeclContext, public TypeDecl {
   /// imports.
   ImplicitImportList getImplicitImports() const;
 
-  AccessNotes &getAccessNotes() { return accessNotes; }
-  const AccessNotes &getAccessNotes() const { return accessNotes; }
+  AccessNotesFile &getAccessNotes() { return accessNotes; }
+  const AccessNotesFile &getAccessNotes() const { return accessNotes; }
 
   ArrayRef<FileUnit *> getFiles() {
     assert(!Files.empty() || failedToLoad());

lib/AST/AccessNotes.cpp

@@ -56,7 +56,8 @@ AccessNoteDeclName::AccessNoteDeclName(ASTContext &ctx, StringRef str) {
 }
 
 bool AccessNoteDeclName::matches(ValueDecl *VD) const {
-  // These are normally just `VD` and `name`, but not for accessors.
+  // The declaration whose name and context we ought to match. Usually `VD`, but
+  // when `VD` is an accessor, it becomes the accessor's storage instead.
   auto lookupVD = VD;
 
   // First, we check if the accessor-ness of `VD` matches the accessor-ness of
@@ -120,17 +121,17 @@ void AccessNoteDeclName::dump() const {
   llvm::errs() << '\n';
 }
 
-NullablePtr<const AccessNote> AccessNotes::lookup(ValueDecl *VD) const {
+NullablePtr<const AccessNote> AccessNotesFile::lookup(ValueDecl *VD) const {
   assert(VD != nullptr);
 
-  auto iter = llvm::find_if(notes, [&](const AccessNote &note) -> bool {
-    return note.name.matches(VD);
+  auto iter = llvm::find_if(Notes, [&](const AccessNote &note) -> bool {
+    return note.Name.matches(VD);
   });
 
-  return NullablePtr<const AccessNote>(iter == notes.end() ? nullptr : &*iter);
+  return NullablePtr<const AccessNote>(iter == Notes.end() ? nullptr : &*iter);
 }
 
-void AccessNotes::dump() const {
+void AccessNotesFile::dump() const {
   dump(llvm::dbgs());
   llvm::dbgs() << "\n";
 }
@@ -139,20 +140,21 @@ void AccessNote::dump() const {
   llvm::dbgs() << "\n";
 }
 
-void AccessNotes::dump(llvm::raw_ostream &os) const {
-  os << "(access_notes reason='" << reason << "'";
-  for (const auto &note : notes) {
+void AccessNotesFile::dump(llvm::raw_ostream &os) const {
+  os << "(access_notes reason='" << Reason << "'";
+  for (const auto &note : Notes) {
     os << "\n";
     note.dump(os, /*indent=*/2);
   }
+  os << ")";
 }
 
 void AccessNote::dump(llvm::raw_ostream &os, int indent) const {
   os.indent(indent) << "(note name='";
-  name.print(os);
+  Name.print(os);
   os << "'";
 
-  if (name.name.getBaseName().isSpecial())
+  if (Name.name.getBaseName().isSpecial())
     os << " is_special_name";
 
   if (ObjC)
@@ -170,7 +172,7 @@ void AccessNote::dump(llvm::raw_ostream &os, int indent) const {
 LLVM_YAML_DECLARE_SCALAR_TRAITS(swift::AccessNoteDeclName, QuotingType::Single);
 LLVM_YAML_DECLARE_SCALAR_TRAITS(swift::ObjCSelector, QuotingType::Single);
 LLVM_YAML_IS_SEQUENCE_VECTOR(swift::AccessNote)
-LLVM_YAML_DECLARE_MAPPING_TRAITS(swift::AccessNotes)
+LLVM_YAML_DECLARE_MAPPING_TRAITS(swift::AccessNotesFile)
 
 // Not using macro to avoid validation issues.
 template <> struct llvm::yaml::MappingTraits<swift::AccessNote> {
@@ -196,19 +198,19 @@ struct AccessNoteYAMLContext {
   std::set<std::string> unknownKeys;
 };
 
-llvm::Expected<AccessNotes>
-AccessNotes::load(ASTContext &ctx, llvm::MemoryBuffer *buffer) {
+llvm::Expected<AccessNotesFile>
+AccessNotesFile::load(ASTContext &ctx, llvm::MemoryBuffer *buffer) {
   llvm::Error errors = llvm::Error::success();
   AccessNoteYAMLContext yamlCtx = { ctx, {} };
 
   llvm::yaml::Input yamlIn(buffer->getBuffer(), (void *)&yamlCtx,
                            convertToErrorAndJoin, &errors);
 
-  AccessNotes notes;
+  AccessNotesFile notes;
   yamlIn >> notes;
 
   if (errors)
-    return llvm::Expected<AccessNotes>(std::move(errors));
+    return llvm::Expected<AccessNotesFile>(std::move(errors));
 
   notes.unknownKeys = std::move(yamlCtx.unknownKeys);
 
@@ -221,7 +223,7 @@ namespace llvm {
 namespace yaml {
 
 using AccessNote = swift::AccessNote;
-using AccessNotes = swift::AccessNotes;
+using AccessNotesFile = swift::AccessNotesFile;
 using AccessNoteYAMLContext = swift::AccessNoteYAMLContext;
 using AccessNoteDeclName = swift::AccessNoteDeclName;
 using ObjCSelector = swift::ObjCSelector;
@@ -286,7 +288,7 @@ static void diagnoseUnknownKeys(IO &io, ArrayRef<StringRef> expectedKeys) {
 void MappingTraits<AccessNote>::mapping(IO &io, AccessNote &note) {
   diagnoseUnknownKeys(io, { "Name", "ObjC", "Dynamic", "ObjCName" });
 
-  io.mapRequired("Name", note.name);
+  io.mapRequired("Name", note.Name);
   io.mapOptional("ObjC", note.ObjC);
   io.mapOptional("Dynamic", note.Dynamic);
   io.mapOptional("ObjCName", note.ObjCName);
@@ -303,11 +305,11 @@ StringRef MappingTraits<AccessNote>::validate(IO &io, AccessNote &note) {
   return "";
 }
 
-void MappingTraits<AccessNotes>::mapping(IO &io, AccessNotes &notes) {
+void MappingTraits<AccessNotesFile>::mapping(IO &io, AccessNotesFile &notes) {
   diagnoseUnknownKeys(io, { "Reason", "Notes" });
 
-  io.mapRequired("Reason", notes.reason);
-  io.mapRequired("Notes", notes.notes);
+  io.mapRequired("Reason", notes.Reason);
+  io.mapRequired("Notes", notes.Notes);
 }
 
 }

lib/Frontend/Frontend.cpp

@@ -951,10 +951,10 @@ ModuleDecl *CompilerInstance::getMainModule() const {
           swift::vfs::getFileOrSTDIN(getFileSystem(), accessNotesPath));
 
       auto expectedAccessNotes =
-          flatMap<AccessNotes, std::unique_ptr<llvm::MemoryBuffer>>(
+          flatMap<AccessNotesFile, std::unique_ptr<llvm::MemoryBuffer>>(
               std::move(expectedBuffer),
               [&](auto &&buffer) -> auto {
-                return AccessNotes::load(*Context, buffer.get());
+                return AccessNotesFile::load(*Context, buffer.get());
               });
 
       if (expectedAccessNotes) {

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -1447,8 +1447,8 @@ void TypeChecker::diagnoseDuplicateCaptureVars(CaptureListExpr *expr) {
 }
 
 template<typename ...DiagIDAndArgs> InFlightDiagnostic
-diagnoseForAccessNote(DeclAttribute *attr, ValueDecl *VD,
-                      DiagIDAndArgs... idAndArgs) {
+diagnoseAtAttrOrDecl(DeclAttribute *attr, ValueDecl *VD,
+                     DiagIDAndArgs... idAndArgs) {
   ASTContext &ctx = VD->getASTContext();
   if (attr->getLocation().isValid())
     return ctx.Diags.diagnose(attr->getLocation(), idAndArgs...);
@@ -1457,7 +1457,7 @@ diagnoseForAccessNote(DeclAttribute *attr, ValueDecl *VD,
 }
 
 template <typename Attr>
-static void addOrRemoveAttr(ValueDecl *VD, const AccessNotes &notes,
+static void addOrRemoveAttr(ValueDecl *VD, const AccessNotesFile &notes,
                             Optional<bool> expected,
                             llvm::function_ref<Attr*()> willCreate) {
   if (!expected) return;
@@ -1470,9 +1470,9 @@ static void addOrRemoveAttr(ValueDecl *VD, const AccessNotes &notes,
           Diag<bool> fixitID) -> InFlightDiagnostic {
     bool isModifier = attr->isDeclModifier();
 
-    diagnoseForAccessNote(attr, VD, diagID, notes.reason, isModifier,
+    diagnoseAtAttrOrDecl(attr, VD, diagID, notes.Reason, isModifier,
                           attr->getAttrName(), VD->getDescriptiveKind());
-    return diagnoseForAccessNote(attr, VD, fixitID, isModifier);
+    return diagnoseAtAttrOrDecl(attr, VD, fixitID, isModifier);
   };
 
   if (*expected) {
@@ -1496,10 +1496,10 @@ static void addOrRemoveAttr(ValueDecl *VD, const AccessNotes &notes,
 }
 
 static void applyAccessNote(ValueDecl *VD, const AccessNote &note,
-                            const AccessNotes &notes) {
+                            const AccessNotesFile &notes) {
   ASTContext &ctx = VD->getASTContext();
 
-  addOrRemoveAttr<ObjCAttr>(VD, notes, note.ObjC, [&]() {
+  addOrRemoveAttr<ObjCAttr>(VD, notes, note.ObjC, [&]{
     return ObjCAttr::create(ctx, note.ObjCName, false);
   });
 
@@ -1513,18 +1513,18 @@ static void applyAccessNote(ValueDecl *VD, const AccessNote &note,
 
     if (!attr->hasName()) {
       attr->setName(*note.ObjCName, true);
-      diagnoseForAccessNote(attr, VD,
-                            diag::attr_objc_name_changed_by_access_note,
-                            notes.reason, VD->getDescriptiveKind(),
-                            *note.ObjCName);
+      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 = diagnoseForAccessNote(attr, VD,
+      auto note = diagnoseAtAttrOrDecl(attr, VD,
                             diag::fixit_attr_objc_name_changed_by_access_note);
 
       if (attr->getLParenLoc().isValid())
@@ -1534,17 +1534,17 @@ static void applyAccessNote(ValueDecl *VD, const AccessNote &note,
         note.fixItInsertAfter(attr->getLocation(), newNameString);
     }
     else if (attr->getName() != *note.ObjCName) {
-      diagnoseForAccessNote(attr, VD,
-                            diag::attr_objc_name_conflicts_with_access_note,
-                            notes.reason, VD->getDescriptiveKind(),
-                            *note.ObjCName, *attr->getName());
+      diagnoseAtAttrOrDecl(attr, VD,
+                           diag::attr_objc_name_conflicts_with_access_note,
+                           notes.Reason, VD->getDescriptiveKind(),
+                           *note.ObjCName, *attr->getName());
     }
   }
 }
 
 evaluator::SideEffect
 ApplyAccessNoteRequest::evaluate(Evaluator &evaluator, ValueDecl *VD) const {
-  AccessNotes &notes = VD->getModuleContext()->getAccessNotes();
+  AccessNotesFile &notes = VD->getModuleContext()->getAccessNotes();
   if (auto note = notes.lookup(VD))
     applyAccessNote(VD, *note.get(), notes);
   return {};