[NFC] Add InFlightDiagnostic::limitBehavior()

beccadax · beccadax · commit c58fe8984104 · 2021-03-03T15:32:52.000-08:00
This allows code to reduce the “severity” of a diagnostic at one particular emission site, so that (for instance) an error can be emitted as a warning in some situations. It also changes some code in TypeCheckConcurrency.cpp to use this new feature, allowing us to delete some redundant diagnostic definitions.
diff --git a/include/swift/AST/DiagnosticEngine.h b/include/swift/AST/DiagnosticEngine.h
@@ -344,15 +344,16 @@ namespace swift {
     }
   };
 
-  /// Describes the current behavior to take with a diagnostic
+  /// Describes the current behavior to take with a diagnostic.
+  /// Ordered from most severe to least.
   enum class DiagnosticBehavior : uint8_t {
-    Unspecified,
-    Ignore,
-    Note,
-    Remark,
-    Warning,
-    Error,
+    Unspecified = 0,
     Fatal,
+    Error,
+    Warning,
+    Remark,
+    Note,
+    Ignore,
   };
 
   struct DiagnosticFormatOptions {
@@ -405,6 +406,7 @@ namespace swift {
     SourceLoc Loc;
     bool IsChildNote = false;
     const swift::Decl *Decl = nullptr;
+    DiagnosticBehavior BehaviorLimit = DiagnosticBehavior::Unspecified;
 
     friend DiagnosticEngine;
 
@@ -432,10 +434,12 @@ namespace swift {
     bool isChildNote() const { return IsChildNote; }
     SourceLoc getLoc() const { return Loc; }
     const class Decl *getDecl() const { return Decl; }
+    DiagnosticBehavior getBehaviorLimit() const { return BehaviorLimit; }
 
     void setLoc(SourceLoc loc) { Loc = loc; }
     void setIsChildNote(bool isChildNote) { IsChildNote = isChildNote; }
     void setDecl(const class Decl *decl) { Decl = decl; }
+    void setBehaviorLimit(DiagnosticBehavior limit){ BehaviorLimit = limit; }
 
     /// Returns true if this object represents a particular diagnostic.
     ///
@@ -505,6 +509,11 @@ namespace swift {
     /// Flush the active diagnostic to the diagnostic output engine.
     void flush();
 
+    /// Prevent the diagnostic from behaving more severely than \p limit. For
+    /// instance, if \c DiagnosticBehavior::Warning is passed, an error will be
+    /// emitted as a warning, but a note will still be emitted as a note.
+    InFlightDiagnostic &limitBehavior(DiagnosticBehavior limit);
+
     /// Add a token-based range to the currently-active diagnostic.
     InFlightDiagnostic &highlight(SourceRange R);
 
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -4364,38 +4364,20 @@ ERROR(non_concurrent_type_member,none,
       "%select{stored property %1|associated value %1}0 of "
       "'ConcurrentValue'-conforming %2 %3 has non-concurrent-value type %4",
       (bool, DeclName, DescriptiveDeclKind, DeclName, Type))
-WARNING(non_concurrent_type_member_warn,none,
-      "%select{stored property %1|associated value %1}0 of "
-      "'ConcurrentValue'-conforming %2 %3 has non-concurrent-value type %4",
-      (bool, DeclName, DescriptiveDeclKind, DeclName, Type))
 ERROR(concurrent_value_class_mutable_property,none,
       "stored property %0 of 'ConcurrentValue'-conforming %1 %2 is mutable",
       (DeclName, DescriptiveDeclKind, DeclName))
-WARNING(concurrent_value_class_mutable_property_warn,none,
-      "stored property %0 of 'ConcurrentValue'-conforming %1 %2 is mutable",
-      (DeclName, DescriptiveDeclKind, DeclName))
 ERROR(concurrent_value_outside_source_file,none,
       "conformance to 'ConcurrentValue' must occur in the same source file as "
       "%0 %1; use 'UnsafeConcurrentValue' for retroactive conformance",
       (DescriptiveDeclKind, DeclName))
-WARNING(concurrent_value_outside_source_file_warn,none,
-      "conformance to 'ConcurrentValue' must occur in the same source file as "
-      "%0 %1; use 'UnsafeConcurrentValue' for retroactive conformance",
-      (DescriptiveDeclKind, DeclName))
 ERROR(concurrent_value_nonfinal_class,none,
       "non-final class %0 cannot conform to `ConcurrentValue`; "
       "use `UnsafeConcurrentValue`", (DeclName))
-WARNING(concurrent_value_nonfinal_class_warn,none,
-      "non-final class %0 cannot conform to `ConcurrentValue`; "
-      "use `UnsafeConcurrentValue`", (DeclName))
 ERROR(concurrent_value_inherit,none,
       "`ConcurrentValue` class %1 cannot inherit from another class"
       "%select{| other than 'NSObject'}0",
       (bool, DeclName))
-WARNING(concurrent_value_inherit_warn,none,
-        "`ConcurrentValue` class %1 cannot inherit from another class"
-        "%select{| other than 'NSObject'}0",
-        (bool, DeclName))
 
 ERROR(actorindependent_let,none,
       "'@actorIndependent' is meaningless on 'let' declarations because "
diff --git a/lib/AST/DiagnosticEngine.cpp b/lib/AST/DiagnosticEngine.cpp
@@ -289,6 +289,12 @@ InFlightDiagnostic &InFlightDiagnostic::fixItExchange(SourceRange R1,
   return *this;
 }
 
+InFlightDiagnostic &
+InFlightDiagnostic::limitBehavior(DiagnosticBehavior limit) {
+  Engine->getActiveDiagnostic().setBehaviorLimit(limit);
+  return *this;
+}
+
 void InFlightDiagnostic::flush() {
   if (!IsActive)
     return;
@@ -828,10 +834,11 @@ DiagnosticBehavior DiagnosticState::determineBehavior(const Diagnostic &diag) {
   //   2) If the user ignored this specific diagnostic, follow that
   //   3) If the user provided a behavior for this diagnostic's kind, follow
   //      that
-  //   4) Otherwise remap the diagnostic kind
+  //   4) Otherwise remap the diagnostic kind, applying the behaviorLimit
 
   auto diagInfo = storedDiagnosticInfos[(unsigned)diag.getID()];
-  bool isNote = diagInfo.kind == DiagnosticKind::Note;
+  bool isNote = diagInfo.kind == DiagnosticKind::Note
+                  || diag.getBehaviorLimit() == DiagnosticBehavior::Note;
 
   //   1) If current state dictates a certain behavior, follow that
 
@@ -850,27 +857,34 @@ DiagnosticBehavior DiagnosticState::determineBehavior(const Diagnostic &diag) {
 
   //   3) If the user provided a behavior for this diagnostic's kind, follow
   //      that
-  if (diagInfo.kind == DiagnosticKind::Warning) {
+  if (diagInfo.kind == DiagnosticKind::Warning
+      || (diag.getBehaviorLimit() == DiagnosticBehavior::Warning && !isNote)) {
     if (suppressWarnings)
       return set(DiagnosticBehavior::Ignore);
     if (warningsAsErrors)
       return set(DiagnosticBehavior::Error);
   }
 
-  //   4) Otherwise remap the diagnostic kind
+  //   4) Otherwise remap the diagnostic kind, applying the behaviorLimit
+  DiagnosticBehavior lvl = DiagnosticBehavior::Unspecified;
   switch (diagInfo.kind) {
   case DiagnosticKind::Note:
-    return set(DiagnosticBehavior::Note);
+    lvl = DiagnosticBehavior::Note;
+    break;
   case DiagnosticKind::Error:
-    return set(diagInfo.isFatal ? DiagnosticBehavior::Fatal
-                                : DiagnosticBehavior::Error);
+    lvl = diagInfo.isFatal ? DiagnosticBehavior::Fatal
+                           : DiagnosticBehavior::Error;
+    break;
   case DiagnosticKind::Warning:
-    return set(DiagnosticBehavior::Warning);
+    lvl = DiagnosticBehavior::Warning;
+    break;
   case DiagnosticKind::Remark:
-    return set(DiagnosticBehavior::Remark);
+    lvl = DiagnosticBehavior::Remark;
+    break;
   }
+  assert(lvl != DiagnosticBehavior::Unspecified);
 
-  llvm_unreachable("Unhandled DiagnosticKind in switch.");
+  return set(std::max(lvl, diag.getBehaviorLimit()));
 }
 
 void DiagnosticEngine::flushActiveDiagnostic() {
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -2371,14 +2371,16 @@ static bool shouldDiagnoseExistingDataRaces(const DeclContext *dc) {
   return false;
 }
 
-static bool shouldDiagnoseConcurrentValue(ConcurrentValueCheck check) {
+static DiagnosticBehavior toDiagnosticBehavior(ConcurrentValueCheck check,
+                                               bool diagnoseImplicit = false) {
   switch (check) {
   case ConcurrentValueCheck::ImpliedByStandardProtocol:
+    return DiagnosticBehavior::Warning;
   case ConcurrentValueCheck::Explicit:
-    return true;
-
+    return DiagnosticBehavior::Unspecified;
   case ConcurrentValueCheck::Implicit:
-    return false;
+    return diagnoseImplicit ? DiagnosticBehavior::Unspecified
+                            : DiagnosticBehavior::Ignore;
   }
 }
 
@@ -2388,34 +2390,31 @@ static bool checkConcurrentValueInstanceStorage(
     NominalTypeDecl *nominal, DeclContext *dc, ConcurrentValueCheck check) {
   // Stored properties of structs and classes must have
   // ConcurrentValue-conforming types.
-  bool asWarning = (check == ConcurrentValueCheck::ImpliedByStandardProtocol);
+  auto behavior = toDiagnosticBehavior(check);
   bool invalid = false;
   if (isa<StructDecl>(nominal) || isa<ClassDecl>(nominal)) {
     auto classDecl = dyn_cast<ClassDecl>(nominal);
     for (auto property : nominal->getStoredProperties()) {
       if (classDecl && property->supportsMutation()) {
-        if (!shouldDiagnoseConcurrentValue(check))
+        if (behavior == DiagnosticBehavior::Ignore)
           return true;
-
-        property->diagnose(
-            asWarning ? diag::concurrent_value_class_mutable_property_warn
-                      : diag::concurrent_value_class_mutable_property,
-            property->getName(), nominal->getDescriptiveKind(),
-            nominal->getName());
+        property->diagnose(diag::concurrent_value_class_mutable_property,
+                           property->getName(), nominal->getDescriptiveKind(),
+                           nominal->getName())
+            .limitBehavior(behavior);
         invalid = true;
         continue;
       }
 
       auto propertyType = dc->mapTypeIntoContext(property->getInterfaceType());
       if (!isConcurrentValueType(dc, propertyType)) {
-        if (!shouldDiagnoseConcurrentValue(check))
+        if (behavior == DiagnosticBehavior::Ignore)
           return true;
-
-        property->diagnose(
-            asWarning ? diag::non_concurrent_type_member_warn
-                      : diag::non_concurrent_type_member,
-            false, property->getName(),
-            nominal->getDescriptiveKind(), nominal->getName(), propertyType);
+        property->diagnose(diag::non_concurrent_type_member,
+                           false, property->getName(),
+                           nominal->getDescriptiveKind(), nominal->getName(),
+                           propertyType)
+            .limitBehavior(behavior);
         invalid = true;
         continue;
       }
@@ -2435,14 +2434,13 @@ static bool checkConcurrentValueInstanceStorage(
         auto elementType = dc->mapTypeIntoContext(
             element->getArgumentInterfaceType());
         if (!isConcurrentValueType(dc, elementType)) {
-          if (!shouldDiagnoseConcurrentValue(check))
+          if (behavior == DiagnosticBehavior::Ignore)
             return true;
-
-          element->diagnose(
-              asWarning ? diag::non_concurrent_type_member_warn
-                        : diag::non_concurrent_type_member,
-              true, element->getName(),
-              nominal->getDescriptiveKind(), nominal->getName(), elementType);
+          element->diagnose(diag::non_concurrent_type_member,
+                            true, element->getName(),
+                            nominal->getDescriptiveKind(), nominal->getName(),
+                            elementType)
+              .limitBehavior(behavior);
           invalid = true;
           continue;
         }
@@ -2469,28 +2467,26 @@ bool swift::checkConcurrentValueConformance(
 
   // ConcurrentValue can only be used in the same source file.
   auto conformanceDecl = conformanceDC->getAsDecl();
-  bool asWarning = (check == ConcurrentValueCheck::ImpliedByStandardProtocol);
+  auto behavior = toDiagnosticBehavior(check, /*diagnoseImplicit=*/true);
   if (!conformanceDC->getParentSourceFile() ||
       conformanceDC->getParentSourceFile() != nominal->getParentSourceFile()) {
-    conformanceDecl->diagnose(
-        asWarning
-          ? diag::concurrent_value_outside_source_file_warn
-          : diag::concurrent_value_outside_source_file,
-        nominal->getDescriptiveKind(), nominal->getName());
+    conformanceDecl->diagnose(diag::concurrent_value_outside_source_file,
+                              nominal->getDescriptiveKind(),
+                              nominal->getName())
+        .limitBehavior(behavior);
 
-    if (!asWarning)
+    if (behavior != DiagnosticBehavior::Warning)
       return true;
   }
 
   if (classDecl) {
     // An non-final class cannot conform to `ConcurrentValue`.
     if (!classDecl->isFinal()) {
-      classDecl->diagnose(
-          asWarning ? diag::concurrent_value_nonfinal_class_warn
-                    : diag::concurrent_value_nonfinal_class,
-          classDecl->getName());
+      classDecl->diagnose(diag::concurrent_value_nonfinal_class,
+                          classDecl->getName())
+          .limitBehavior(behavior);
 
-      if (!asWarning)
+      if (behavior != DiagnosticBehavior::Warning)
         return true;
     }
 
@@ -2500,12 +2496,12 @@ bool swift::checkConcurrentValueConformance(
       if (auto superclassDecl = classDecl->getSuperclassDecl()) {
         if (!superclassDecl->isNSObject()) {
           classDecl->diagnose(
-              asWarning ? diag::concurrent_value_inherit_warn
-                        : diag::concurrent_value_inherit,
+              diag::concurrent_value_inherit,
               nominal->getASTContext().LangOpts.EnableObjCInterop,
-              classDecl->getName());
+              classDecl->getName())
+              .limitBehavior(behavior);
 
-          if (!asWarning)
+          if (behavior != DiagnosticBehavior::Warning)
             return true;
         }
       }
