Improve access note error handling

The LLVM rebranch added an “AllowUnknownKeys” setting to llvm::yaml::Input, which lets us rip out a lot of marginal code and encourages a broader rework of access note error diagnosis:

• Access note warnings and errors are now diagnosed as they’re found during YAML parsing
• They now have proper SourceLocs inside the .accessnotes file
• They’re now tested using -verify-additional-file instead of FileCheck
• A lot of gross duct tape is now gone

Author: beccadax

include/swift/AST/AccessNotes.h

@@ -94,15 +94,10 @@ class AccessNotesFile {
   /// 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;
-
-  /// 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);
+  /// Load the access notes from \p buffer, or \c None if they cannot be loaded.
+  /// Diagnoses any parsing issues with the access notes file.
+  static llvm::Optional<AccessNotesFile>
+      load(ASTContext &ctx, const 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;

include/swift/AST/DiagnosticsFrontend.def

@@ -405,13 +405,15 @@ WARNING(module_incompatible_with_skip_function_bodies,none,
         "-experimental-skip-*-function-bodies* flags; they have "
         "been automatically disabled", (StringRef))
 
-WARNING(invalid_access_notes_file,none,
-        "access notes at '%0' will be ignored due to an error while loading "
-        "them: %1",
+WARNING(access_notes_file_io_error,none,
+        "ignored access notes file at '%0' because it cannot be read: %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))
+WARNING(error_in_access_notes_file,none,
+        "ignored access notes file because it cannot be parsed: %0",
+        (StringRef))
+REMARK(warning_in_access_notes_file,none,
+       "ignored invalid content in access notes file: %0",
+       (StringRef))
 
 
 #define UNDEFINE_DIAGNOSTIC_MACROS

lib/AST/AccessNotes.cpp

@@ -19,6 +19,7 @@
 #include "swift/AST/Attr.h"
 #include "swift/AST/Decl.h"
 #include "swift/AST/Module.h"     // DeclContext::isModuleScopeContext()
+#include "swift/AST/DiagnosticsFrontend.h"
 #include "swift/Parse/Parser.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
@@ -183,36 +184,36 @@ template <> struct llvm::yaml::MappingTraits<swift::AccessNote> {
 namespace swift {
 
 static void
-convertToErrorAndJoin(const llvm::SMDiagnostic &diag, void *Context) {
-  auto newError = llvm::createStringError(std::errc::invalid_argument,
-                                          "%s at line %d, column %d",
-                                          diag.getMessage().bytes_begin(),
-                                          diag.getLineNo(), diag.getColumnNo());
-
-  auto &errors = *(llvm::Error*)Context;
-  errors = llvm::joinErrors(std::move(errors), std::move(newError));
+convertToErrorAndJoin(const llvm::SMDiagnostic &diag, void *ctxPtr) {
+  ASTContext &ctx = *(ASTContext*)ctxPtr;
+
+  SourceLoc loc{diag.getLoc()};
+  assert(ctx.SourceMgr.isOwning(loc));
+
+  switch (diag.getKind()) {
+  case llvm::SourceMgr::DK_Error:
+    ctx.Diags.diagnose(loc, diag::error_in_access_notes_file,
+                       diag.getMessage());
+    break;
+
+  default:
+    ctx.Diags.diagnose(loc, diag::warning_in_access_notes_file,
+                       diag.getMessage());
+    break;
+  }
 }
 
-struct AccessNoteYAMLContext {
-  ASTContext &ctx;
-  std::set<std::string> unknownKeys;
-};
-
-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);
+llvm::Optional<AccessNotesFile>
+AccessNotesFile::load(ASTContext &ctx, const llvm::MemoryBuffer *buffer) {
+  llvm::yaml::Input yamlIn(llvm::MemoryBufferRef(*buffer), (void *)&ctx,
+                           convertToErrorAndJoin, &ctx);
+  yamlIn.setAllowUnknownKeys(true);
 
   AccessNotesFile notes;
   yamlIn >> notes;
 
-  if (errors)
-    return llvm::Expected<AccessNotesFile>(std::move(errors));
-
-  notes.unknownKeys = std::move(yamlCtx.unknownKeys);
+  if (yamlIn.error())
+    return None;
 
   return notes;
 }
@@ -224,7 +225,7 @@ namespace yaml {
 
 using AccessNote = swift::AccessNote;
 using AccessNotesFile = swift::AccessNotesFile;
-using AccessNoteYAMLContext = swift::AccessNoteYAMLContext;
+using ASTContext = swift::ASTContext;
 using AccessNoteDeclName = swift::AccessNoteDeclName;
 using ObjCSelector = swift::ObjCSelector;
 
@@ -235,9 +236,9 @@ output(const AccessNoteDeclName &name, void *ctxPtr, raw_ostream &os) {
 
 StringRef ScalarTraits<AccessNoteDeclName>::
 input(StringRef str, void *ctxPtr, AccessNoteDeclName &name) {
-  auto &yamlCtx = *static_cast<swift::AccessNoteYAMLContext *>(ctxPtr);
+  auto &ctx = *static_cast<swift::ASTContext *>(ctxPtr);
 
-  name = AccessNoteDeclName(yamlCtx.ctx, str);
+  name = AccessNoteDeclName(ctx, str);
   return name.empty() ? "invalid declaration name" : "";
 }
 
@@ -248,46 +249,17 @@ void ScalarTraits<ObjCSelector>::output(const ObjCSelector &selector,
 
 StringRef ScalarTraits<ObjCSelector>::input(StringRef str, void *ctxPtr,
                                             ObjCSelector &selector) {
-  auto &yamlCtx = *static_cast<swift::AccessNoteYAMLContext *>(ctxPtr);
+  auto &ctx = *static_cast<swift::ASTContext *>(ctxPtr);
 
-  if (auto sel = ObjCSelector::parse(yamlCtx.ctx, str)) {
+  if (auto sel = ObjCSelector::parse(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);
@@ -306,8 +278,6 @@ std::string MappingTraits<AccessNote>::validate(IO &io, AccessNote &note) {
 }
 
 void MappingTraits<AccessNotesFile>::mapping(IO &io, AccessNotesFile &notes) {
-  diagnoseUnknownKeys(io, { "Reason", "Notes" });
-
   io.mapRequired("Reason", notes.Reason);
   io.mapRequired("Notes", notes.Notes);
 }

lib/Frontend/Frontend.cpp

@@ -902,16 +902,6 @@ bool CompilerInstance::createFilesForMainModule(
   return false;
 }
 
-template<typename ExpectedOut, typename ExpectedIn>
-static llvm::Expected<ExpectedOut>
-flatMap(llvm::Expected<ExpectedIn> &&in,
-        llvm::function_ref<llvm::Expected<ExpectedOut>(ExpectedIn &&)> transform) {
-  if (!in)
-    return llvm::Expected<ExpectedOut>(in.takeError());
-
-  return transform(std::move(in.get()));
-}
-
 ModuleDecl *CompilerInstance::getMainModule() const {
   if (!MainModule) {
     Identifier ID = Context->getIdentifier(Invocation.getModuleName());
@@ -943,46 +933,6 @@ ModuleDecl *CompilerInstance::getMainModule() const {
       // into a partial module that failed to load.
       MainModule->setFailedToLoad();
     }
-
-    if (!Invocation.getFrontendOptions().AccessNotesPath.empty()) {
-      auto accessNotesPath = Invocation.getFrontendOptions().AccessNotesPath;
-
-      auto expectedBuffer = llvm::errorOrToExpected(
-          swift::vfs::getFileOrSTDIN(getFileSystem(), accessNotesPath));
-
-      auto expectedAccessNotes =
-          flatMap<AccessNotesFile, std::unique_ptr<llvm::MemoryBuffer>>(
-              std::move(expectedBuffer),
-              [&](auto &&buffer) -> auto {
-                return AccessNotesFile::load(*Context, buffer.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) {
-          Context->Diags.diagnose(SourceLoc(), diag::invalid_access_notes_file,
-                                  accessNotesPath, error.message());
-        });
-    }
   }
   return MainModule;
 }
@@ -996,8 +946,30 @@ void CompilerInstance::setMainModule(ModuleDecl *newMod) {
 bool CompilerInstance::performParseAndResolveImportsOnly() {
   FrontendStatsTracer tracer(getStatsReporter(), "parse-and-resolve-imports");
 
-  // Resolve imports for all the source files.
   auto *mainModule = getMainModule();
+
+  // Load access notes.
+  if (!Invocation.getFrontendOptions().AccessNotesPath.empty()) {
+    auto accessNotesPath = Invocation.getFrontendOptions().AccessNotesPath;
+
+    auto bufferOrError =
+        swift::vfs::getFileOrSTDIN(getFileSystem(), accessNotesPath);
+    if (bufferOrError) {
+      int sourceID =
+          SourceMgr.addNewSourceBuffer(std::move(bufferOrError.get()));
+      auto buffer =
+          SourceMgr.getLLVMSourceMgr().getMemoryBuffer(sourceID);
+
+      if (auto accessNotesFile = AccessNotesFile::load(*Context, buffer))
+        mainModule->getAccessNotes() = *accessNotesFile;
+    }
+    else {
+      Diagnostics.diagnose(SourceLoc(), diag::access_notes_file_io_error,
+                           accessNotesPath, bufferOrError.getError().message());
+    }
+  }
+
+  // Resolve imports for all the source files.
   for (auto *file : mainModule->getFiles()) {
     if (auto *SF = dyn_cast<SourceFile>(file))
       performImportResolution(*SF);

test/Sema/Inputs/bad.accessnotes

@@ -1 +1,2 @@
 Colourless green ideas sleep furiously.
+# expected-warning@-1 {{ignored access notes file because it cannot be parsed: not a mapping}}

test/Sema/Inputs/extra.accessnotes

@@ -1,5 +1,7 @@
 Reason: Access notes containing future, unknown syntax
 CorinthianLeather: 'rich'
+# expected-remark@-1 {{ignored invalid content in access notes file: unknown key 'CorinthianLeather'}}
 Notes:
 - Name: 'fn()'
   CorinthianLeather: 'rich'
+  # expected-remark@-1 {{ignored invalid content in access notes file: unknown key 'CorinthianLeather'}}

test/Sema/access-notes-invalid.swift

@@ -1,14 +1,9 @@
 // RUN: %target-swift-frontend -typecheck -primary-file %s -access-notes-path %S/Inputs/missing.accessnotes 2>&1 | %FileCheck --check-prefix=CHECK-MISSING --enable-windows-compatibility %s
-// CHECK-MISSING: <unknown>:0: warning: access notes at 'SOURCE_DIR/test/Sema/Inputs/missing.accessnotes' will be ignored due to an error while loading them: No such file or directory{{$}}
+// CHECK-MISSING: <unknown>:0: warning: ignored access notes file at 'SOURCE_DIR/test/Sema/Inputs/missing.accessnotes' because it cannot be read: No such file or directory{{$}}
 
-// RUN: %target-swift-frontend -typecheck -primary-file %s -access-notes-path %S/Inputs/bad.accessnotes 2>&1 | %FileCheck --check-prefix=CHECK-BAD --enable-windows-compatibility %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-typecheck-verify-swift -access-notes-path %S/Inputs/bad.accessnotes -verify-additional-file %S/Inputs/bad.accessnotes
 
-// 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 --enable-windows-compatibility %s <%t.txt
-// RUN: %FileCheck --check-prefix=NEGATIVE-EXTRA --enable-windows-compatibility %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
+// RUN: %target-typecheck-verify-swift -access-notes-path %S/Inputs/extra.accessnotes -verify-additional-file %S/Inputs/extra.accessnotes
 
 // FIXME: Should diagnose multiple access notes for the same decl