Merge pull request swiftlang#30735 from owenv/verifier-edu-notes

[DiagnosticVerifier] Support asserting edu notes are emitted

Author: CodaFi

docs/Diagnostics.md

@@ -150,3 +150,5 @@ An expected diagnostic is denoted by a comment which begins with `expected-error
 - (Required) The expected error message. The message should be enclosed in double curly braces and should not include the `error:`/`warning:`/`note:`/`remark:` prefix. For example, `// expected-error {{invalid redeclaration of 'y'}}` would match an error with that message on the same line. The expected message does not need to match the emitted message verbatim. As long as the expected message is a substring of the original message, they will match.
 
 - (Optional) Expected fix-its. These are each enclosed in double curly braces and appear after the expected message. An expected fix-it consists of a column range followed by the text it's expected to be replaced with. For example, `let r : Int i = j // expected-error{{consecutive statements}} {{12-12=;}}` will match a fix-it attached to the consecutive statements error which inserts a semicolon at column 12, just after the 't' in 'Int'. The special {{none}} specifier is also supported, which will cause the diagnostic match to fail if unexpected fix-its are produced.
+
+- (Optional) Expected educational notes. These appear as a comma separated list after the expected message, enclosed in double curly braces and prefixed by 'educational-notes='. For example, `{{educational-notes=some-note,some-other-note}}` will verify the educational notes with filenames `some-note` and `some-other-note` appear. Do not include the file extension when specifying note names.

include/swift/Frontend/DiagnosticVerifier.h

@@ -46,14 +46,19 @@ struct CapturedDiagnosticInfo {
   unsigned Line;
   unsigned Column;
   SmallVector<DiagnosticInfo::FixIt, 2> FixIts;
+  SmallVector<std::string, 1> EducationalNotes;
 
   CapturedDiagnosticInfo(llvm::SmallString<128> Message,
                          llvm::SmallString<32> FileName,
                          DiagnosticKind Classification, SourceLoc Loc,
                          unsigned Line, unsigned Column,
-                         SmallVector<DiagnosticInfo::FixIt, 2> FixIts)
+                         SmallVector<DiagnosticInfo::FixIt, 2> FixIts,
+                         SmallVector<std::string, 1> EducationalNotes)
       : Message(Message), FileName(FileName), Classification(Classification),
-        Loc(Loc), Line(Line), Column(Column), FixIts(FixIts) {}
+        Loc(Loc), Line(Line), Column(Column), FixIts(FixIts),
+        EducationalNotes(EducationalNotes) {
+    std::sort(EducationalNotes.begin(), EducationalNotes.end());
+  }
 };
 /// This class implements support for -verify mode in the compiler.  It
 /// buffers up diagnostics produced during compilation, then checks them

lib/Frontend/DiagnosticVerifier.cpp

@@ -17,9 +17,11 @@
 #include "swift/Frontend/DiagnosticVerifier.h"
 #include "swift/Basic/SourceManager.h"
 #include "swift/Parse/Lexer.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace swift;
@@ -36,6 +38,7 @@ struct ExpectedFixIt {
 namespace {
 
 static constexpr StringLiteral fixitExpectationNoneString("none");
+static constexpr StringLiteral educationalNotesSpecifier("educational-notes=");
 
 struct ExpectedDiagnosticInfo {
   // This specifies the full range of the "expected-foo {{}}" specifier.
@@ -65,6 +68,17 @@ struct ExpectedDiagnosticInfo {
   // Loc of {{none}}
   const char *noneMarkerStartLoc = nullptr;
 
+  /// Represents a specifier of the form '{{educational-notes=note1,note2}}'.
+  struct ExpectedEducationalNotes {
+    const char *StartLoc, *EndLoc; // The loc of the {{ and }}'s.
+    llvm::SmallVector<StringRef, 1> Names; // Names of expected notes.
+
+    ExpectedEducationalNotes(const char *StartLoc, const char *EndLoc,
+                             llvm::SmallVector<StringRef, 1> Names)
+        : StartLoc(StartLoc), EndLoc(EndLoc), Names(Names) {}
+  };
+  Optional<ExpectedEducationalNotes> EducationalNotes;
+
   ExpectedDiagnosticInfo(const char *ExpectedStart,
                          DiagnosticKind Classification)
       : ExpectedStart(ExpectedStart), Classification(Classification) {}
@@ -85,6 +99,18 @@ static std::string getDiagKindString(DiagnosticKind Kind) {
   llvm_unreachable("Unhandled DiagKind in switch.");
 }
 
+/// Render the verifier syntax for a given set of educational notes.
+static std::string
+renderEducationalNotes(llvm::SmallVectorImpl<std::string> &EducationalNotes) {
+  std::string Result;
+  llvm::raw_string_ostream OS(Result);
+  OS << "{{" << educationalNotesSpecifier;
+  interleave(EducationalNotes, [&](const auto &Note) { OS << Note; },
+             [&] { OS << ','; });
+  OS << "}}";
+  return OS.str();
+}
+
 /// If we find the specified diagnostic in the list, return it.
 /// Otherwise return CapturedDiagnostics.end().
 static std::vector<CapturedDiagnosticInfo>::iterator
@@ -439,38 +465,67 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
     StringRef ExtraChecks = MatchStart.substr(End+2).ltrim(" \t");
     while (ExtraChecks.startswith("{{")) {
       // First make sure we have a closing "}}".
-      size_t EndLoc = ExtraChecks.find("}}");
-      if (EndLoc == StringRef::npos) {
+      size_t EndIndex = ExtraChecks.find("}}");
+      if (EndIndex == StringRef::npos) {
         addError(ExtraChecks.data(),
-                 "didn't find '}}' to match '{{' in fix-it verification");
+                 "didn't find '}}' to match '{{' in diagnostic verification");
         break;
       }
-      
+
       // Allow for close braces to appear in the replacement text.
-      while (EndLoc+2 < ExtraChecks.size() && ExtraChecks[EndLoc+2] == '}')
-        ++EndLoc;
-      
-      StringRef FixItStr = ExtraChecks.slice(2, EndLoc);
+      while (EndIndex + 2 < ExtraChecks.size() &&
+             ExtraChecks[EndIndex + 2] == '}')
+        ++EndIndex;
+
+      const char *OpenLoc = ExtraChecks.data(); // Beginning of opening '{{'.
+      const char *CloseLoc =
+          ExtraChecks.data() + EndIndex + 2; // End of closing '}}'.
+
+      StringRef CheckStr = ExtraChecks.slice(2, EndIndex);
       // Check for matching a later "}}" on a different line.
-      if (FixItStr.find_first_of("\r\n") != StringRef::npos) {
+      if (CheckStr.find_first_of("\r\n") != StringRef::npos) {
         addError(ExtraChecks.data(), "didn't find '}}' to match '{{' in "
-                 "fix-it verification");
+                                     "diagnostic verification");
         break;
       }
-      
+
       // Prepare for the next round of checks.
-      ExtraChecks = ExtraChecks.substr(EndLoc+2).ltrim();
-      
+      ExtraChecks = ExtraChecks.substr(EndIndex + 2).ltrim();
+
+      // If this check starts with 'educational-notes=', check for one or more
+      // educational notes instead of a fix-it.
+      if (CheckStr.startswith(educationalNotesSpecifier)) {
+        if (Expected.EducationalNotes.hasValue()) {
+          addError(CheckStr.data(),
+                   "each verified diagnostic may only have one "
+                   "{{educational-notes=<#notes#>}} declaration");
+          continue;
+        }
+        StringRef NotesStr = CheckStr.substr(
+            educationalNotesSpecifier.size()); // Trim 'educational-notes='.
+        llvm::SmallVector<StringRef, 1> names;
+        // Note names are comma-separated.
+        std::pair<StringRef, StringRef> split;
+        do {
+          split = NotesStr.split(',');
+          names.push_back(split.first);
+          NotesStr = split.second;
+        } while (!NotesStr.empty());
+        Expected.EducationalNotes.emplace(OpenLoc, CloseLoc, names);
+        continue;
+      }
+
+      // This wasn't an educational notes specifier, so it must be a fix-it.
       // Special case for specifying no fixits should appear.
-      if (FixItStr == fixitExpectationNoneString) {
+      if (CheckStr == fixitExpectationNoneString) {
         if (Expected.noneMarkerStartLoc) {
-          addError(FixItStr.data() - 2,
+          addError(CheckStr.data() - 2,
                    Twine("A second {{") + fixitExpectationNoneString +
                        "}} was found. It may only appear once in an expectation.");
           break;
         }
 
-        Expected.noneMarkerStartLoc = FixItStr.data() - 2;
+        Expected.noneMarkerStartLoc = CheckStr.data() - 2;
         continue;
       }
 
@@ -482,14 +537,14 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
       }
 
       // Parse the pieces of the fix-it.
-      size_t MinusLoc = FixItStr.find('-');
+      size_t MinusLoc = CheckStr.find('-');
       if (MinusLoc == StringRef::npos) {
-        addError(FixItStr.data(), "expected '-' in fix-it verification");
+        addError(CheckStr.data(), "expected '-' in fix-it verification");
         continue;
       }
-      StringRef StartColStr = FixItStr.slice(0, MinusLoc);
-      StringRef AfterMinus = FixItStr.substr(MinusLoc+1);
-      
+      StringRef StartColStr = CheckStr.slice(0, MinusLoc);
+      StringRef AfterMinus = CheckStr.substr(MinusLoc + 1);
+
       size_t EqualLoc = AfterMinus.find('=');
       if (EqualLoc == StringRef::npos) {
         addError(AfterMinus.data(),
@@ -500,8 +555,8 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
       StringRef AfterEqual = AfterMinus.substr(EqualLoc+1);
 
       ExpectedFixIt FixIt;
-      FixIt.StartLoc = StartColStr.data()-2;
-      FixIt.EndLoc = FixItStr.data()+EndLoc;
+      FixIt.StartLoc = OpenLoc;
+      FixIt.EndLoc = CloseLoc;
       if (StartColStr.getAsInteger(10, FixIt.StartCol)) {
         addError(StartColStr.data(),
                  "invalid column number in fix-it verification");
@@ -514,7 +569,7 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
       }
 
       // Translate literal "\\n" into '\n', inefficiently.
-      StringRef fixItText = AfterEqual.slice(0, EndLoc);
+      StringRef fixItText = AfterEqual.slice(0, EndIndex);
       for (const char *current = fixItText.begin(), *end = fixItText.end();
            current != end; /* in loop */) {
         if (*current == '\\' && current + 1 < end) {
@@ -681,6 +736,33 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
                       replEndLoc, actualFixits);
     }
 
+    if (auto expectedNotes = expected.EducationalNotes) {
+      // Verify educational notes
+      for (auto &foundName : FoundDiagnostic.EducationalNotes) {
+        llvm::erase_if(expectedNotes->Names,
+                       [&](std::string item) { return item == foundName; });
+      }
+
+      if (!expectedNotes->Names.empty()) {
+        if (FoundDiagnostic.EducationalNotes.empty()) {
+          addError(expectedNotes->StartLoc,
+                   "expected educational note(s) not seen");
+        } else {
+          // If we had an incorrect expected note, render it and produce a fixit
+          // of our own.
+          auto actual =
+              renderEducationalNotes(FoundDiagnostic.EducationalNotes);
+          auto replStartLoc = SMLoc::getFromPointer(expectedNotes->StartLoc);
+          auto replEndLoc = SMLoc::getFromPointer(expectedNotes->EndLoc);
+
+          llvm::SMFixIt fix(llvm::SMRange(replStartLoc, replEndLoc), actual);
+          addError(expectedNotes->StartLoc,
+                   "expected educational note(s) not seen; actual educational "
+                   "note(s): " + actual, fix);
+        }
+      }
+    }
+
     // Actually remove the diagnostic from the list, so we don't match it
     // again. We do have to do this after checking fix-its, though, because
     // the diagnostic owns its fix-its.
@@ -866,6 +948,11 @@ void DiagnosticVerifier::handleDiagnostic(SourceManager &SM,
   SmallVector<DiagnosticInfo::FixIt, 2> fixIts;
   std::copy(Info.FixIts.begin(), Info.FixIts.end(), std::back_inserter(fixIts));
 
+  llvm::SmallVector<std::string, 1> eduNotes;
+  for (auto &notePath : Info.EducationalNotePaths) {
+    eduNotes.push_back(llvm::sys::path::stem(notePath));
+  }
+
   llvm::SmallString<128> message;
   {
     llvm::raw_svector_ostream Out(message);
@@ -878,10 +965,10 @@ void DiagnosticVerifier::handleDiagnostic(SourceManager &SM,
     const auto fileName = SM.getDisplayNameForLoc(Info.Loc);
     CapturedDiagnostics.emplace_back(message, fileName, Info.Kind, Info.Loc,
                                      lineAndColumn.first, lineAndColumn.second,
-                                     fixIts);
+                                     fixIts, eduNotes);
   } else {
     CapturedDiagnostics.emplace_back(message, StringRef(), Info.Kind, Info.Loc,
-                                     0, 0, fixIts);
+                                     0, 0, fixIts, eduNotes);
   }
 }
 

test/Constraints/diagnostics.swift

@@ -161,7 +161,7 @@ public func myMap<T, U>(_ x: T?, _ f: (T) -> U) -> U? {
 
 // <rdar://problem/20142523>
 func rdar20142523() {
-  myMap(0..<10, { x in // expected-error{{unable to infer complex closure return type; add explicit type to disambiguate}} {{21-21=-> <#Result#> }}
+  myMap(0..<10, { x in // expected-error{{unable to infer complex closure return type; add explicit type to disambiguate}} {{21-21=-> <#Result#> }} {{educational-notes=complex-closure-inference}}
     ()
     return x
   })

test/attr/attr_dynamic_callable.swift

@@ -98,7 +98,7 @@ func testFunction(a: CallableReturningFunction) {
 //===----------------------------------------------------------------------===//
 
 // Arguments' type may not be variadic.
-// expected-error @+1 {{@dynamicCallable attribute requires 'Invalid1' to have either a valid 'dynamicallyCall(withArguments:)' method or 'dynamicallyCall(withKeywordArguments:)' method}}
+// expected-error @+1 {{@dynamicCallable attribute requires 'Invalid1' to have either a valid 'dynamicallyCall(withArguments:)' method or 'dynamicallyCall(withKeywordArguments:)' method}} {{educational-notes=dynamic-callable-requirements}}
 @dynamicCallable
 struct Invalid1 {
   func dynamicallyCall(withArguments arguments: [Int]...) -> Int {
@@ -107,7 +107,7 @@ struct Invalid1 {
 }
 
 // Keyword arguments' key type must be ExpressibleByStringLiteral.
-// expected-error @+1 {{@dynamicCallable attribute requires 'Invalid2' to have either a valid 'dynamicallyCall(withArguments:)' method or 'dynamicallyCall(withKeywordArguments:)' method}}
+// expected-error @+1 {{@dynamicCallable attribute requires 'Invalid2' to have either a valid 'dynamicallyCall(withArguments:)' method or 'dynamicallyCall(withKeywordArguments:)' method}} {{educational-notes=dynamic-callable-requirements}}
 @dynamicCallable
 struct Invalid2 {
   func dynamicallyCall(
@@ -128,8 +128,8 @@ protocol NoKeywordProtocol {
 }
 
 func testInvalidKeywordCall(x: NoKeyword, y: NoKeywordProtocol & AnyObject) {
-  x(a: 1, b: 2) // expected-error {{@dynamicCallable type 'NoKeyword' cannot be applied with keyword arguments; missing 'dynamicCall(withKeywordArguments:)' method}}
-  y(a: 1, b: 2) // expected-error {{@dynamicCallable type 'NoKeywordProtocol & AnyObject' cannot be applied with keyword arguments; missing 'dynamicCall(withKeywordArguments:)' method}}
+  x(a: 1, b: 2) // expected-error {{@dynamicCallable type 'NoKeyword' cannot be applied with keyword arguments; missing 'dynamicCall(withKeywordArguments:)' method}} {{educational-notes=dynamic-callable-requirements}}
+  y(a: 1, b: 2) // expected-error {{@dynamicCallable type 'NoKeywordProtocol & AnyObject' cannot be applied with keyword arguments; missing 'dynamicCall(withKeywordArguments:)' method}} {{educational-notes=dynamic-callable-requirements}}
 }
 
 // expected-error @+1 {{'@dynamicCallable' attribute cannot be applied to this declaration}}

test/decl/ext/extensions.swift

@@ -81,10 +81,10 @@ protocol P1 {}
 
 protocol P2 {}
 
-extension () {} // expected-error {{non-nominal type '()' cannot be extended}}
+extension () {} // expected-error {{non-nominal type '()' cannot be extended}} {{educational-notes=nominal-types}}
 
 typealias TupleAlias = (x: Int, y: Int)
-extension TupleAlias {} // expected-error{{non-nominal type 'TupleAlias' (aka '(x: Int, y: Int)') cannot be extended}}
+extension TupleAlias {} // expected-error{{non-nominal type 'TupleAlias' (aka '(x: Int, y: Int)') cannot be extended}} {{educational-notes=nominal-types}}
 
 // Test property accessors in extended types
 class C {}

test/decl/var/property_wrappers.swift

@@ -58,7 +58,7 @@ struct WrapperAcceptingAutoclosure<T> {
 
 @propertyWrapper
 struct MissingValue<T> { }
-// expected-error@-1{{property wrapper type 'MissingValue' does not contain a non-static property named 'wrappedValue'}}
+// expected-error@-1{{property wrapper type 'MissingValue' does not contain a non-static property named 'wrappedValue'}} {{educational-notes=property-wrapper-requirements}}
 
 @propertyWrapper
 struct StaticValue {
@@ -76,7 +76,7 @@ protocol CannotBeAWrapper {
 
 @propertyWrapper
 struct NonVisibleValueWrapper<Value> {
-  private var wrappedValue: Value // expected-error{{private property 'wrappedValue' cannot have more restrictive access than its enclosing property wrapper type 'NonVisibleValueWrapper' (which is internal)}}
+  private var wrappedValue: Value // expected-error{{private property 'wrappedValue' cannot have more restrictive access than its enclosing property wrapper type 'NonVisibleValueWrapper' (which is internal)}} {{educational-notes=property-wrapper-requirements}}
 }
 
 @propertyWrapper
@@ -92,7 +92,7 @@ struct NonVisibleInitWrapper<Value> {
 struct InitialValueTypeMismatch<Value> {
   var wrappedValue: Value // expected-note{{'wrappedValue' declared here}}
 
-  init(wrappedValue initialValue: Value?) { // expected-error{{'init(wrappedValue:)' parameter type ('Value?') must be the same as its 'wrappedValue' property type ('Value') or an @autoclosure thereof}}
+  init(wrappedValue initialValue: Value?) { // expected-error{{'init(wrappedValue:)' parameter type ('Value?') must be the same as its 'wrappedValue' property type ('Value') or an @autoclosure thereof}} {{educational-notes=property-wrapper-requirements}}
     self.wrappedValue = initialValue!
   }
 }
@@ -112,7 +112,7 @@ struct MultipleInitialValues<Value> {
 struct InitialValueFailable<Value> {
   var wrappedValue: Value
 
-  init?(wrappedValue initialValue: Value) { // expected-error{{property wrapper initializer 'init(wrappedValue:)' cannot be failable}}
+  init?(wrappedValue initialValue: Value) { // expected-error{{property wrapper initializer 'init(wrappedValue:)' cannot be failable}} {{educational-notes=property-wrapper-requirements}}
     return nil
   }
 }

test/diagnostics/verifier.swift

@@ -33,6 +33,15 @@ func b() {
 // CHECK: unexpected warning produced: initialization of immutable value 'c' was never used
 // CHECK-WARNINGS-AS-ERRORS: unexpected error produced: initialization of immutable value 'c' was never used
 
+extension (Int, Int) {} // expected-error {{non-nominal type '(Int, Int)' cannot be extended}} {{educational-notes=foo-bar-baz}}
+// CHECK: error: expected educational note(s) not seen; actual educational note(s): {{[{][{]}}educational-notes=nominal-types{{[}][}]}}
+
+extension (Bool, Int) {} // expected-error {{non-nominal type '(Bool, Int)' cannot be extended}} {{educational-notes=nominal-types}} {{educational-notes=nominal-types}}
+// CHECK: error: each verified diagnostic may only have one {{[{][{]}}educational-notes=<#notes#>{{[}][}]}} declaration
+
+extension (Bool, Bool) {} // expected-error {{non-nominal type '(Bool, Bool)' cannot be extended}} {{educational-notes=nominal-types,foo-bar-baz}}
+// CHECK: error: expected educational note(s) not seen; actual educational note(s): {{[{][{]}}educational-notes=nominal-types{{[}][}]}}
+
 // Verify the serialized diags have the right magic at the top.
 // CHECK-SERIALIZED: DIA