Allow unknown keys in access notes

LLVM’s YAML support wants to show a hard error if an unknown string appears in an access note. Instead, emit a remark but load the parts of the access note we do understand to allow for future expansion of access notes.

Author: beccadax

include/swift/AST/AccessNotes.h

@@ -24,6 +24,8 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/raw_ostream.h"
+#include <set>
+#include <string>
 #include <vector>
 
 namespace swift {
@@ -63,6 +65,11 @@ class AccessNotes {
   std::string reason;
   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);
 

include/swift/AST/DiagnosticsFrontend.def

@@ -409,6 +409,10 @@ WARNING(invalid_access_notes_file,none,
         "access notes at '%0' will be ignored due to an error while loading "
         "them: %1",
         (StringRef, StringRef))
+REMARK(unknown_keys_in_access_notes_file,none,
+       "compiler ignored unknown key%s0 '%1' in access notes at '%2'",
+       (bool, StringRef, StringRef))
+
 
 #define UNDEFINE_DIAGNOSTIC_MACROS
 #include "DefineDiagnosticMacros.h"

lib/AST/AccessNotes.cpp

@@ -191,11 +191,17 @@ convertToErrorAndJoin(const llvm::SMDiagnostic &diag, void *Context) {
   errors = llvm::joinErrors(std::move(errors), std::move(newError));
 }
 
+struct AccessNoteYAMLContext {
+  ASTContext &ctx;
+  std::set<std::string> unknownKeys;
+};
+
 llvm::Expected<AccessNotes>
 AccessNotes::load(ASTContext &ctx, llvm::MemoryBuffer *buffer) {
   llvm::Error errors = llvm::Error::success();
+  AccessNoteYAMLContext yamlCtx = { ctx, {} };
 
-  llvm::yaml::Input yamlIn(buffer->getBuffer(), (void *)&ctx,
+  llvm::yaml::Input yamlIn(buffer->getBuffer(), (void *)&yamlCtx,
                            convertToErrorAndJoin, &errors);
 
   AccessNotes notes;
@@ -204,6 +210,8 @@ AccessNotes::load(ASTContext &ctx, llvm::MemoryBuffer *buffer) {
   if (errors)
     return llvm::Expected<AccessNotes>(std::move(errors));
 
+  notes.unknownKeys = std::move(yamlCtx.unknownKeys);
+
   return notes;
 }
 }
@@ -214,7 +222,7 @@ namespace yaml {
 
 using AccessNote = swift::AccessNote;
 using AccessNotes = swift::AccessNotes;
-using ASTContext = swift::ASTContext;
+using AccessNoteYAMLContext = swift::AccessNoteYAMLContext;
 using AccessNoteDeclName = swift::AccessNoteDeclName;
 using ObjCSelector = swift::ObjCSelector;
 
@@ -225,8 +233,9 @@ output(const AccessNoteDeclName &name, void *ctxPtr, raw_ostream &os) {
 
 StringRef ScalarTraits<AccessNoteDeclName>::
 input(StringRef str, void *ctxPtr, AccessNoteDeclName &name) {
-  ASTContext &ctx = *static_cast<ASTContext *>(ctxPtr);
-  name = AccessNoteDeclName(ctx, str);
+  auto &yamlCtx = *static_cast<swift::AccessNoteYAMLContext *>(ctxPtr);
+
+  name = AccessNoteDeclName(yamlCtx.ctx, str);
   return name.empty() ? "invalid declaration name" : "";
 }
 
@@ -237,17 +246,46 @@ void ScalarTraits<ObjCSelector>::output(const ObjCSelector &selector,
 
 StringRef ScalarTraits<ObjCSelector>::input(StringRef str, void *ctxPtr,
                                             ObjCSelector &selector) {
-  ASTContext &ctx = *static_cast<ASTContext *>(ctxPtr);
+  auto &yamlCtx = *static_cast<swift::AccessNoteYAMLContext *>(ctxPtr);
 
-  if (auto sel = ObjCSelector::parse(ctx, str)) {
+  if (auto sel = ObjCSelector::parse(yamlCtx.ctx, str)) {
     selector = *sel;
     return "";
   }
 
   return "invalid selector";
 }
 
+/// If \p io is outputting, mark all keys in the current mapping as used.
+static void diagnoseUnknownKeys(IO &io, ArrayRef<StringRef> expectedKeys) {
+  if (io.outputting())
+    return;
+
+  auto &yamlCtx = *static_cast<swift::AccessNoteYAMLContext *>(io.getContext());
+
+  for (auto key : io.keys()) {
+    if (is_contained(expectedKeys, key))
+      continue;
+
+    // "Use" this key without actually doing anything with it to suppress the
+    // error llvm::yaml::Input will otherwise generate.
+    bool useDefault;
+    void *saveInfo;
+    if (io.preflightKey((const char *)key.bytes_begin(), /*required=*/false,
+                        /*sameAsDefault=*/false, useDefault, saveInfo)) {
+      // FIXME: We should diagnose these with locations, but llvm::yaml::Input
+      // encapsulates all of the necessary details. Instead, we are currently
+      // just building a list that the frontend will display.
+      yamlCtx.unknownKeys.insert(key.str());
+
+      io.postflightKey(saveInfo);
+    }
+  }
+}
+
 void MappingTraits<AccessNote>::mapping(IO &io, AccessNote &note) {
+  diagnoseUnknownKeys(io, { "Name", "ObjC", "Dynamic", "ObjCName" });
+
   io.mapRequired("Name", note.name);
   io.mapOptional("ObjC", note.ObjC);
   io.mapOptional("Dynamic", note.Dynamic);
@@ -266,6 +304,8 @@ StringRef MappingTraits<AccessNote>::validate(IO &io, AccessNote &note) {
 }
 
 void MappingTraits<AccessNotes>::mapping(IO &io, AccessNotes &notes) {
+  diagnoseUnknownKeys(io, { "Reason", "Notes" });
+
   io.mapRequired("Reason", notes.reason);
   io.mapRequired("Notes", notes.notes);
 }

lib/Frontend/Frontend.cpp

@@ -957,8 +957,25 @@ ModuleDecl *CompilerInstance::getMainModule() const {
                 return AccessNotes::load(*Context, buffer.get());
               });
 
-      if (expectedAccessNotes)
-        MainModule->getAccessNotes() = std::move(expectedAccessNotes).get();
+      if (expectedAccessNotes) {
+        auto &notes = std::move(expectedAccessNotes).get();
+
+        // If there were unknown keys in the file, diagnose that.
+        if (!notes.unknownKeys.empty()) {
+          // Create a string like "key1', 'key2', 'key3". The diagnostic itself
+          // provides the leading and trailing single quotes.
+          SmallString<64> scratch;
+          llvm::raw_svector_ostream scratchOS(scratch);
+          llvm::interleave(notes.unknownKeys, scratchOS, "', '");
+
+          Context->Diags.diagnose(SourceLoc(),
+                                  diag::unknown_keys_in_access_notes_file,
+                                  notes.unknownKeys.size(), scratch,
+                                  accessNotesPath);
+        }
+
+        MainModule->getAccessNotes() = std::move(notes);
+      }
       else
         llvm::handleAllErrors(expectedAccessNotes.takeError(),
                               [&](const llvm::ErrorInfoBase &error) {

test/Sema/Inputs/extra.accessnotes

@@ -0,0 +1,5 @@
+Reason: Access notes containing future, unknown syntax
+CorinthianLeather: 'rich'
+Notes:
+- Name: 'fn()'
+  CorinthianLeather: 'rich'

test/Sema/access-notes-invalid.swift

@@ -3,3 +3,9 @@
 
 // RUN: %target-swift-frontend -typecheck -primary-file %s -access-notes-path %S/Inputs/bad.accessnotes 2>&1 | %FileCheck --check-prefix=CHECK-BAD %s
 // CHECK-BAD: <unknown>:0: warning: access notes at 'SOURCE_DIR/test/Sema/Inputs/bad.accessnotes' will be ignored due to an error while loading them: not a mapping at line 1, column 0{{$}}
+
+// RUN: %target-swift-frontend -typecheck -primary-file %s -access-notes-path %S/Inputs/extra.accessnotes >%t.txt 2>&1 
+// RUN: %FileCheck --check-prefix=CHECK-EXTRA %s <%t.txt
+// RUN: %FileCheck --check-prefix=NEGATIVE-EXTRA %s <%t.txt
+// CHECK-EXTRA: <unknown>:0: remark: compiler ignored unknown key 'CorinthianLeather' in access notes at 'SOURCE_DIR/test/Sema/Inputs/extra.accessnotes'
+// NEGATIVE-EXTRA-NOT: <unknown>:0: warning: access notes at 'SOURCE_DIR/test/Sema/Inputs/extra.accessnotes' will be ignored due to an error while loading them