[Serialization] Show the side-effect of a failure as main diagnostic

xymus · xymus · commit fbacdeaf4ba0 · 2023-06-08T14:40:40.000-07:00
Use the diagnostic of the effect on the API as main diagnostic about
deserialization failures. For example:

Mod.swiftmodule:1:1: error: could not deserialize type for 'foo()'
Mod.swiftmodule:1:1: note: reference to type 'MyType' broken by a
context change; 'MyType' was expected to be in 'A', but now a candidate
is found only in 'B'
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -922,12 +922,12 @@ NOTE(modularization_issue_related_modules,none,
      "clang preprocessor macros may affect headers shared between these modules",
      (bool, DeclName))
 
-NOTE(modularization_issue_side_effect_extension_error,none,
-     "could not deserialize extension",
-     ())
-NOTE(modularization_issue_side_effect_type_error,none,
-     "could not deserialize type for %0",
-     (DeclName))
+ERROR(modularization_issue_side_effect_extension_error,none,
+      "could not deserialize extension",
+      ())
+ERROR(modularization_issue_side_effect_type_error,none,
+      "could not deserialize type for %0",
+      (DeclName))
 
 NOTE(modularization_issue_worked_around,none,
      "attempting forced recovery enabled by "
diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp
@@ -338,41 +338,52 @@ ModularizationError::diagnose(const ModuleFile *MF,
   }
 }
 
-void TypeError::diagnose(const ModuleFile *MF) const {
-  MF->getContext().Diags.diagnose(MF->getSourceLoc(),
-                                  diag::modularization_issue_side_effect_type_error,
-                                  name);
+void TypeError::diagnose(const ModuleFile *MF,
+                         DiagnosticBehavior limit) const {
+  auto &diags = MF->getContext().Diags;
+  auto inFlight = diags.diagnose(MF->getSourceLoc(),
+                             diag::modularization_issue_side_effect_type_error,
+                             name);
+  inFlight.limitBehavior(limit);
 }
 
-void ExtensionError::diagnose(const ModuleFile *MF) const {
-  MF->getContext().Diags.diagnose(MF->getSourceLoc(),
+void ExtensionError::diagnose(const ModuleFile *MF,
+                              DiagnosticBehavior limit) const {
+  auto &diags = MF->getContext().Diags;
+  auto inFlight = diags.diagnose(MF->getSourceLoc(),
                        diag::modularization_issue_side_effect_extension_error);
+  inFlight.limitBehavior(limit);
 }
 
 llvm::Error
 ModuleFile::diagnoseModularizationError(llvm::Error error,
                                         DiagnosticBehavior limit) const {
-  auto handleModularizationError =
+  auto handleModularizationCause =
     [&](const ModularizationError &modularError) -> llvm::Error {
-      modularError.diagnose(this, limit);
+      modularError.diagnose(this, DiagnosticBehavior::Note);
       return llvm::Error::success();
     };
   llvm::Error outError = llvm::handleErrors(std::move(error),
-        handleModularizationError,
-        [&](TypeError &typeError) -> llvm::Error {
-          if (typeError.diagnoseUnderlyingReason(handleModularizationError)) {
-            typeError.diagnose(this);
-            return llvm::Error::success();
-          }
-          return llvm::make_error<TypeError>(std::move(typeError));
-        },
-        [&](ExtensionError &extError) -> llvm::Error {
-          if (extError.diagnoseUnderlyingReason(handleModularizationError)) {
-            extError.diagnose(this);
-            return llvm::Error::success();
-          }
-          return llvm::make_error<ExtensionError>(std::move(extError));
-        });
+    [&](const ModularizationError &modularError) -> llvm::Error {
+      modularError.diagnose(this, limit);
+      return llvm::Error::success();
+    },
+    [&](TypeError &typeError) -> llvm::Error {
+      if (typeError.shouldDiagnoseUnderlyingReason(handleModularizationCause)) {
+        typeError.diagnose(this, limit);
+        typeError.diagnoseUnderlyingReason(handleModularizationCause);
+        return llvm::Error::success();
+      }
+      return llvm::make_error<TypeError>(std::move(typeError));
+    },
+    [&](ExtensionError &extError) -> llvm::Error {
+      if (extError.shouldDiagnoseUnderlyingReason(handleModularizationCause)) {
+        extError.diagnose(this, limit);
+        extError.diagnoseUnderlyingReason(handleModularizationCause);
+        return llvm::Error::success();
+      }
+      return llvm::make_error<ExtensionError>(std::move(extError));
+    });
 
   return outError;
 }
diff --git a/lib/Serialization/DeserializationErrors.h b/lib/Serialization/DeserializationErrors.h
@@ -461,23 +461,22 @@ class ErrorWithUnderlyingReason {
     }
   }
 
-  // Returns \c true if the error was diagnosed.
   template <typename HandlerT>
-  bool diagnoseUnderlyingReason(HandlerT &&handler) {
+  bool shouldDiagnoseUnderlyingReason(HandlerT &&handler) {
+    return underlyingReason &&
+           llvm::ErrorHandlerTraits<HandlerT>::appliesTo(*underlyingReason);
+  }
+
+  template <typename HandlerT>
+  void diagnoseUnderlyingReason(HandlerT &&handler) {
     if (underlyingReason &&
         llvm::ErrorHandlerTraits<HandlerT>::appliesTo(*underlyingReason)) {
       auto error = llvm::ErrorHandlerTraits<HandlerT>::apply(
                                                std::forward<HandlerT>(handler),
                                                std::move(underlyingReason));
-      if (!error) {
-        // The underlying reason was diagnosed.
-        return true;
-      } else {
+      if (error)
         underlyingReason = takeErrorInfo(std::move(error));
-        return false;
-      }
     }
-    return false;
   }
 };
 
@@ -496,7 +495,7 @@ class TypeError : public llvm::ErrorInfo<TypeError, DeclDeserializationError>,
     this->numVTableEntries = numVTableEntries;
   }
 
-  void diagnose(const ModuleFile *MF) const;
+  void diagnose(const ModuleFile *MF, DiagnosticBehavior limit) const;
 
   void log(raw_ostream &OS) const override {
     OS << "Could not deserialize type for '" << name << "'";
@@ -518,7 +517,7 @@ class ExtensionError : public llvm::ErrorInfo<ExtensionError>,
   explicit ExtensionError(std::unique_ptr<ErrorInfoBase> reason)
       : ErrorWithUnderlyingReason(std::move(reason)) {}
 
-  void diagnose(const ModuleFile *MF) const;
+  void diagnose(const ModuleFile *MF, DiagnosticBehavior limit) const;
 
   void log(raw_ostream &OS) const override {
     OS << "could not deserialize extension";
diff --git a/test/Serialization/Recovery/module-recovery-remarks.swift b/test/Serialization/Recovery/module-recovery-remarks.swift
@@ -14,7 +14,8 @@
 // RUN:   | %FileCheck --check-prefixes CHECK-MOVED %s
 
 /// Main error downgraded to a remark.
-// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: remark: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'A_related'
+// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: remark: could not deserialize type for 'foo()'
+// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: note: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'A_related'
 
 /// Contextual notes about the modules involved.
 // CHECK-MOVED: <unknown>:0: note: the type was expected to be found in module 'A' at '
@@ -33,7 +34,6 @@
 // CHECK-MOVED: <unknown>:0: note: declarations in the underlying clang module 'A' may be hidden by clang preprocessor macros
 // CHECK-MOVED: <unknown>:0: note: the distributed module 'LibWithXRef' refers to the local module 'A'; this may be caused by header maps or search paths
 // CHECK-MOVED: <unknown>:0: note: the type 'MyType' moved between related modules; clang preprocessor macros may affect headers shared between these modules
-// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: note: could not deserialize type for 'foo()'
 // CHECK-MOVED: error: cannot find 'foo' in scope
 
 /// Move A to the SDK, triggering a different note about layering.
diff --git a/test/Serialization/modularization-error.swift b/test/Serialization/modularization-error.swift
@@ -12,7 +12,8 @@
 // RUN: %target-swift-frontend %t/LibOriginal.swift -emit-module-path %t/B.swiftmodule -module-name B
 // RUN: not %target-swift-frontend -emit-sil %t/LibWithXRef.swiftmodule -module-name LibWithXRef -I %t 2>&1 \
 // RUN:   | %FileCheck --check-prefixes CHECK,CHECK-MOVED %s
-// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: error: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'B'
+// CHECK: LibWithXRef.swiftmodule:1:1: error: could not deserialize extension
+// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: note: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'B'
 
 /// Force working around the broken modularization to get a result and no errors.
 // RUN: %target-swift-frontend -emit-sil %t/LibWithXRef.swiftmodule -module-name LibWithXRef -I %t \
@@ -33,23 +34,22 @@
 // RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/B.swiftmodule -module-name B
 // RUN: not %target-swift-frontend -emit-sil %t/LibWithXRef.swiftmodule -module-name LibWithXRef -I %t 2>&1 \
 // RUN:   | %FileCheck --check-prefixes CHECK,CHECK-KIND-CHANGED %s
-// CHECK-KIND-CHANGED: LibWithXRef.swiftmodule:1:1: error: reference to type 'MyType' broken by a context change; the declaration kind of 'MyType' from 'A' changed since building 'LibWithXRef'
+// CHECK-KIND-CHANGED: LibWithXRef.swiftmodule:1:1: note: reference to type 'MyType' broken by a context change; the declaration kind of 'MyType' from 'A' changed since building 'LibWithXRef'
 
 /// Change MyType into a function and move it.
 // RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/A.swiftmodule -module-name A
 // RUN: %target-swift-frontend %t/LibTypeChanged.swift -emit-module-path %t/B.swiftmodule -module-name B
 // RUN: not %target-swift-frontend -emit-sil %t/LibWithXRef.swiftmodule -module-name LibWithXRef -I %t 2>&1 \
 // RUN:   | %FileCheck --check-prefixes CHECK,CHECK-KIND-CHANGED-AND-MOVED %s
-// CHECK-KIND-CHANGED-AND-MOVED: LibWithXRef.swiftmodule:1:1: error: reference to type 'MyType' broken by a context change; the declaration kind of 'MyType' changed since building 'LibWithXRef', it was in 'A' and is now a candidate is found only in 'B'
+// CHECK-KIND-CHANGED-AND-MOVED: LibWithXRef.swiftmodule:1:1: note: reference to type 'MyType' broken by a context change; the declaration kind of 'MyType' changed since building 'LibWithXRef', it was in 'A' and is now a candidate is found only in 'B'
 
 /// Remove MyType from all imported modules.
 // RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/A.swiftmodule -module-name A
 // RUN: %target-swift-frontend %t/Empty.swift -emit-module-path %t/B.swiftmodule -module-name B
 // RUN: not %target-swift-frontend -emit-sil %t/LibWithXRef.swiftmodule -module-name LibWithXRef -I %t 2>&1 \
 // RUN:   | %FileCheck --check-prefixes CHECK,CHECK-NOT-FOUND %s
-// CHECK-NOT-FOUND: LibWithXRef.swiftmodule:1:1: error: reference to type 'MyType' broken by a context change; 'MyType' is not found, it was expected to be in 'A'
+// CHECK-NOT-FOUND: LibWithXRef.swiftmodule:1:1: note: reference to type 'MyType' broken by a context change; 'MyType' is not found, it was expected to be in 'A'
 
-// CHECK: LibWithXRef.swiftmodule:1:1: note: could not deserialize extension
 
 //--- Empty.swift
 //--- LibOriginal.swift
