Turn Educational Notes On-By-Default (swiftlang#30583)

* [Diagnostics] Turn educational notes on-by-default

* [Diagnostics] Only include educational notes in printed output if -print-educational-notes is passed

* Make -print-educational-notes a driver option

* [Diagnostics] Issue a printed remark if educational notes are available, but disabled

* [docs] Update educational notes documentation and add a contributing guide

* [Diagnostics] Cleanup PrintingDiagnosticConsumer handling of edu notes

* Revert "[Diagnostics] Issue a printed remark if educational notes are available, but disabled"
For now, don't notify users if edu notes are available but disabled. This decision can be reevaluated later.

Author: owenv

docs/Diagnostics.md

@@ -92,23 +92,34 @@ Most diagnostics have no reason to change behavior under editor mode. An example
 
 ### Educational Notes ###
 
-**Note**: This feature is currently experimental. It can be enabled by passing the `-Xfrontend -enable-descriptive-diagnostics` flag.
+Educational notes are short-form documentation attached to a diagnostic which explain relevant language concepts. They are intended to further Swift's goal of progressive disclosure by providing a learning resource at the point of use when encountering an error message for the first time. In very limited circumstances, they also allow the main diagnostic message to use precise terminology (e.g. nominal types) which would otherwise be too unfriendly for beginners.
 
-Educational notes are small snippets of documentation attached to a diagnostic which explain relevant language concepts. They are intended to further Swift's goal of progressive disclosure by providing a learning resource at the point of use for users encountering a new error message for the first time. In very limited circumstances, they also allow the main diagnostic message to use more precise and correct terminology (e.g. nominal types) which would otherwise be too unfriendly for beginners.
+When outputting diagnostics on the command line, educational notes will be printed after the main diagnostic body if enabled using the `-print-educational-notes` driver option. When presented in an IDE, it's expected they will be collapsed under a disclosure arrow, info button, or similar to avoid cluttering output.
 
-When outputting diagnostics on the command line, educational notes will be printed after the main diagnostic body if descriptive diagnostics are enabled. When presented in an IDE, it's expected they will be collapsed under a disclosure arrow, info button, or similar to avoid cluttering output.
-
-Generally speaking, a diagnostic should try to provide educational notes for any concepts/terminology which is difficult to understand from context or is especially subtle. Educational notes should:
-- Explain a single language concept. This makes them easy to reuse across diagnostics and helps keep them clear, concise, and easy to understand.
-- Be written in unabbreviated English. These are longer form messages compared to the main diagnostic, so there is no need to omit needless words and punctuation.
-- Not generally exceed 3-4 paragraphs. Educational notes should be clear and easily digestible. Messages which are too long also have the potential to create diagnostics UX issues in some contexts.
+Educational notes should:
+- Explain a single language concept. This makes them easy to reuse across related diagnostics and helps keep them clear, concise, and easy to understand.
+- Be written in unabbreviated English. These are longer-form messages compared to the main diagnostic, so there's no need to omit needless words and punctuation.
+- Not generally exceed 3-4 paragraphs. Educational notes should be clear and easily digestible. Messages which are too long also have the potential to create UX issues on the command line.
 - Be accessible. Educational notes should be beginner friendly and avoid assuming unnecesary prior knowledge. The goal is not only to help users understand what a diagnostic is telling them, but also to turn errors and warnings into "teachable moments".
-- Include references to relevant chapters of _The Swift Programming Language_ if applicable.
-- Be written in Markdown, but avoid excessive markup to avoid impacting the terminal UX. 
+- Include references to relevant chapters of _The Swift Programming Language_.
+- Be written in Markdown, but avoid excessive markup which negatively impacts the terminal UX.
+
+### Quick-Start Guide for Contributing New Educational Notes ###
+
+Adding new educational notes is a great way to get familiar with the process of contributing to Swift, while also making a big impact!
 
 To add a new educational note:
-1. Add a new Markdown file in the `userdocs/diagnostics/` directory containing the contents of the note. 
-2. Associate the note with one or more diagnostics in EducationalNotes.def.
+1. Follow the [directions in the README](https://github.com/apple/swift#getting-sources-for-swift-and-related-projects) to checkout the Swift sources locally. Being able to build the Swift compiler is recommended, but not required, when contributing a new note.
+2. Identify a diagnostic to write an educational note for. To associate an educational note with a diagnostic name, you'll need to know its internal identifier. The easiest way to do this is to write a small program which triggers the diagnostic, and run it using the `-debug-diagnostic-names` compiler flag. This flag will cause the internal diagnostic identifier to be printed after the diagnostic message in square brackets.
+3. Find any closely related diagnostics. Sometimes, what appears to be one diagnostic from a user's perspective may have multiple variations internally. After determining a diagnostic's internal identifier, run a search for it in the compiler source. You should find:
+    - An entry in a `Diagnostics*.def` file describing the diagnostic. If there are any closely related diagnostics the note should also be attached to, they can usually be found nearby.
+    - Each point in the compiler source where the diagnostic is emitted. This can be helpful in determining the exact circumstances which cause it to be emitted.
+4. Add a new Markdown file in the `userdocs/diagnostics/` directory in the swift repository containing the contents of the note. When writing a note, keep the writing guidelines from the section above in mind. The existing notes in the directory are another useful guide.
+5. Associate the note with the appropriate diagnostics in `EducationalNotes.def`. An entry like `EDUCATIONAL_NOTES(property_wrapper_failable_init, "property-wrapper-requirements.md")` will associate the note with filename `property-wrapper-requirements.md` with the diagnostic having an internal identifier of `property_wrapper_failable_init`.
+6. If possible, rebuild the compiler and try recompiling your test program with `-print-educational-notes`. Your new note should appear after the diagnostic in the terminal.
+7. That's it! The new note is now ready to be submitted as a pull request on GitHub.
+
+If you run into any issues or have questions while following the steps above, feel free to post a question on the Swift forums or open a work-in-progress pull request on GitHub.
 
 ### Format Specifiers ###
 

include/swift/AST/DiagnosticEngine.h

@@ -676,9 +676,6 @@ namespace swift {
     /// Print diagnostic names after their messages
     bool printDiagnosticNames = false;
 
-    /// Use educational notes when available.
-    bool useEducationalNotes = false;
-
     /// Path to diagnostic documentation directory.
     std::string diagnosticDocumentationPath = "";
 
@@ -730,9 +727,6 @@ namespace swift {
       return printDiagnosticNames;
     }
 
-    void setUseEducationalNotes(bool val) { useEducationalNotes = val; }
-    bool getUseEducationalNotes() const { return useEducationalNotes; }
-
     void setDiagnosticDocumentationPath(std::string path) {
       diagnosticDocumentationPath = path;
     }

include/swift/Basic/DiagnosticOptions.h

@@ -55,9 +55,9 @@ class DiagnosticOptions {
   // When printing diagnostics, include the diagnostic name at the end
   bool PrintDiagnosticNames = false;
 
-  /// If set to true, display educational note content to the user if available.
+  /// If set to true, include educational notes in printed output if available.
   /// Educational notes are documentation which supplement diagnostics.
-  bool EnableEducationalNotes = false;
+  bool PrintEducationalNotes = false;
 
   // If set to true, use the more descriptive experimental formatting style for
   // diagnostics.

include/swift/Frontend/PrintingDiagnosticConsumer.h

@@ -31,6 +31,7 @@ class AnnotatedSourceSnippet;
 class PrintingDiagnosticConsumer : public DiagnosticConsumer {
   llvm::raw_ostream &Stream;
   bool ForceColors = false;
+  bool PrintEducationalNotes = false;
   bool DidErrorOccur = false;
   bool ExperimentalFormattingEnabled = false;
   // The current snippet used to display an error/warning/remark and the notes
@@ -59,6 +60,10 @@ class PrintingDiagnosticConsumer : public DiagnosticConsumer {
     llvm::sys::Process::UseANSIEscapeCodes(true);
   }
 
+  void setPrintEducationalNotes(bool ShouldPrint) {
+    PrintEducationalNotes = ShouldPrint;
+  }
+
   void enableExperimentalFormatting() { ExperimentalFormattingEnabled = true; }
 
   bool didErrorOccur() {

include/swift/Option/FrontendOptions.td

@@ -130,9 +130,6 @@ def enable_cross_import_overlays : Flag<["-"], "enable-cross-import-overlays">,
   HelpText<"Automatically import declared cross-import overlays.">;
 def disable_cross_import_overlays : Flag<["-"], "disable-cross-import-overlays">,
   HelpText<"Do not automatically import declared cross-import overlays.">;
-
-def enable_educational_notes : Flag<["-"], "enable-educational-notes">,
-  HelpText<"Show educational notes, if available.">;
 
 def enable_experimental_diagnostic_formatting :
   Flag<["-"], "enable-experimental-diagnostic-formatting">,

include/swift/Option/Options.td

@@ -372,6 +372,9 @@ def no_color_diagnostics : Flag<["-"], "no-color-diagnostics">,
 def debug_diagnostic_names : Flag<["-"], "debug-diagnostic-names">,
   Flags<[FrontendOption, DoesNotAffectIncrementalBuild, HelpHidden]>,
   HelpText<"Include diagnostic names when printing">;
+def print_educational_notes : Flag<["-"], "print-educational-notes">,
+  Flags<[FrontendOption, DoesNotAffectIncrementalBuild]>,
+  HelpText<"Include educational notes in printed diagnostic output, if available">;
 
 def module_cache_path : Separate<["-"], "module-cache-path">,
   Flags<[FrontendOption, DoesNotAffectIncrementalBuild, ArgumentIsPath]>,

lib/AST/DiagnosticEngine.cpp

@@ -984,16 +984,15 @@ void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
     info->ChildDiagnosticInfo = childInfoPtrs;
 
     SmallVector<std::string, 1> educationalNotePaths;
-    if (useEducationalNotes) {
-      auto associatedNotes = educationalNotes[(uint32_t)diagnostic.getID()];
-      while (associatedNotes && *associatedNotes) {
-        SmallString<128> notePath(getDiagnosticDocumentationPath());
-        llvm::sys::path::append(notePath, *associatedNotes);
-        educationalNotePaths.push_back(notePath.str().str());
-        associatedNotes++;
-      }
-      info->EducationalNotePaths = educationalNotePaths;
+
+    auto associatedNotes = educationalNotes[(uint32_t)diagnostic.getID()];
+    while (associatedNotes && *associatedNotes) {
+      SmallString<128> notePath(getDiagnosticDocumentationPath());
+      llvm::sys::path::append(notePath, *associatedNotes);
+      educationalNotePaths.push_back(notePath.str().str());
+      associatedNotes++;
     }
+    info->EducationalNotePaths = educationalNotePaths;
 
     for (auto &consumer : Consumers) {
       consumer->handleDiagnostic(SourceMgr, *info);

lib/Driver/ToolChains.cpp

@@ -249,6 +249,7 @@ static void addCommonFrontendArgs(const ToolChain &TC, const OutputInfo &OI,
   inputArgs.AddLastArg(arguments, options::OPT_package_description_version);
   inputArgs.AddLastArg(arguments, options::OPT_serialize_diagnostics_path);
   inputArgs.AddLastArg(arguments, options::OPT_debug_diagnostic_names);
+  inputArgs.AddLastArg(arguments, options::OPT_print_educational_notes);
   inputArgs.AddLastArg(arguments, options::OPT_enable_astscope_lookup);
   inputArgs.AddLastArg(arguments, options::OPT_disable_astscope_lookup);
   inputArgs.AddLastArg(arguments, options::OPT_disable_parser_lookup);

lib/Frontend/CompilerInvocation.cpp

@@ -821,7 +821,7 @@ static bool ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
   Opts.SuppressWarnings |= Args.hasArg(OPT_suppress_warnings);
   Opts.WarningsAsErrors |= Args.hasArg(OPT_warnings_as_errors);
   Opts.PrintDiagnosticNames |= Args.hasArg(OPT_debug_diagnostic_names);
-  Opts.EnableEducationalNotes |= Args.hasArg(OPT_enable_educational_notes);
+  Opts.PrintEducationalNotes |= Args.hasArg(OPT_print_educational_notes);
   Opts.EnableExperimentalFormatting |=
       Args.hasArg(OPT_enable_experimental_diagnostic_formatting);
   if (Arg *A = Args.getLastArg(OPT_diagnostic_documentation_path)) {

lib/Frontend/Frontend.cpp

@@ -410,9 +410,6 @@ void CompilerInstance::setUpDiagnosticOptions() {
   if (Invocation.getDiagnosticOptions().PrintDiagnosticNames) {
     Diagnostics.setPrintDiagnosticNames(true);
   }
-  if (Invocation.getDiagnosticOptions().EnableEducationalNotes) {
-    Diagnostics.setUseEducationalNotes(true);
-  }
   Diagnostics.setDiagnosticDocumentationPath(
       Invocation.getDiagnosticOptions().DiagnosticDocumentationPath);
 }