[PrintAsClang] Warn about unstable decl order

PrintAsClang is supposed to emit declarations in the same order regardless of the compiler’s internal state, but we have repeatedly found that our current criteria are inadequate, resulting in non-functionality-affecting changes to generated header content. Add a diagnostic that’s emitted when this happens soliciting a bug report.

Since there *should* be no cases where the compiler fails to order declarations, this diagnostic is never actually emitted. Instead, we test this change by enabling `-verify` on nearly all PrintAsClang tests to make sure they are unaffected.

This did demonstrate a missing criterion that only mattered in C++ mode: extensions that varied only in their generic signature were not sorted stably. Add a sort criterion for this.

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -6345,6 +6345,17 @@ ERROR(invalid_objc_swift_rooted_class,none,
 NOTE(invalid_objc_swift_root_class_insert_nsobject,none,
      "inherit from 'NSObject' to silence this error", ())
 
+WARNING(objc_header_sorting_arbitrary,none,
+        "arbitrarily printing %0 before %1 in generated header; subsequent "
+        "compilations may print them in a different order",
+        (Decl *, Decl *))
+NOTE(objc_header_sorting_arbitrary_other,none,
+     "%1 will be printed after %0 during this compilation",
+     (Decl *, Decl *))
+NOTE(objc_header_sorting_arbitrary_please_report,none,
+     "please report this as a bug in the Swift compiler and describe both "
+     "declarations", ())
+
 ERROR(invalid_nonobjc_decl,none,
       "only class members and extensions of classes can be declared @nonobjc", ())
 ERROR(invalid_nonobjc_extension,none,

lib/PrintAsClang/ModuleContentsWriter.cpp

@@ -751,21 +751,45 @@ class ModuleWriter {
       if (result != 0)
         return result;
 
+      auto lastDitchSort = [&](bool suppressDiagnostic) -> int {
+        // With no other criteria, we'll sort by memory address.
+        if (*lhs < *rhs)
+          result = Ascending;
+        else if (*lhs > *rhs)
+          result = Descending;
+        else {
+          // Sorting with yourself shouldn't happen (but implement consistent
+          // behavior if this assert is disabled).
+          ASSERT(*lhs != *rhs && "sorting should not compare decl to itself");
+          result = Equivalent;
+        }
+
+        // Warn that this isn't stable across different compilations.
+        if (!suppressDiagnostic) {
+          auto earlier = (result == Ascending) ? *lhs : *rhs;
+          auto later = (result == Ascending) ? *rhs : *lhs;
+
+          earlier->diagnose(diag::objc_header_sorting_arbitrary,
+                            earlier, later);
+          later->diagnose(diag::objc_header_sorting_arbitrary_other,
+                          earlier, later);
+          earlier->diagnose(diag::objc_header_sorting_arbitrary_please_report);
+        }
+
+        return result;
+      };
+
       // Mangled names ought to distinguish all value decls, leaving only
       // extensions of the same nominal type beyond this point.
-      assert(isa<ExtensionDecl>(*lhs) && isa<ExtensionDecl>(*rhs));
+      if (!isa<ExtensionDecl>(*lhs) || !isa<ExtensionDecl>(*rhs))
+        return lastDitchSort(/*suppressDiagnostic=*/false);
 
       // Break ties in extensions by putting smaller extensions last (in reverse
       // order).
-      // FIXME: This will end up taking linear time.
       auto lhsMembers = cast<ExtensionDecl>(*lhs)->getAllMembers();
       auto rhsMembers = cast<ExtensionDecl>(*rhs)->getAllMembers();
-      unsigned numLHSMembers = std::distance(lhsMembers.begin(),
-                                             lhsMembers.end());
-      unsigned numRHSMembers = std::distance(rhsMembers.begin(),
-                                             rhsMembers.end());
-      if (numLHSMembers != numRHSMembers)
-        return numLHSMembers < numRHSMembers ? Descending : Ascending;
+      if (lhsMembers.size() != rhsMembers.size())
+        return lhsMembers.size() < rhsMembers.size() ? Descending : Ascending;
 
       // Or the extension with fewer protocols.
       auto lhsProtos = cast<ExtensionDecl>(*lhs)->getLocalProtocols();
@@ -784,7 +808,9 @@ class ModuleWriter {
         });
         if (mismatch.first != lhsProtos.end()) {
           StringRef lhsProtoName = (*mismatch.first)->getName().str();
-          return lhsProtoName.compare((*mismatch.second)->getName().str());
+          result = lhsProtoName.compare((*mismatch.second)->getName().str());
+          if (result != 0)
+            return result;
         }
       }
 
@@ -820,9 +846,23 @@ class ModuleWriter {
         }
       }
 
-      // Hopefully two extensions with identical conformances and member names
-      // will be interchangeable enough not to matter.
-      return Equivalent;
+      // Tough customer. Maybe they have different generic signatures?
+      {
+        auto lhsSig = cast<ExtensionDecl>(*lhs)->getGenericSignature()
+                                                    .getAsString();
+        auto rhsSig = cast<ExtensionDecl>(*rhs)->getGenericSignature()
+                                                    .getAsString();
+        result = rhsSig.compare(lhsSig);
+        if (result != 0)
+          return result;
+      }
+
+      // Nothing, sadly.
+      bool areEmptyExtensions = lhsMembers.size() == 0
+                                  && rhsMembers.size() == 0
+                                  && lhsProtos.size() == 0
+                                  && rhsProtos.size() == 0;
+      return lastDitchSort(/*suppressDiagnostic=*/areEmptyExtensions);
     });
 
     assert(declsToWrite.empty());

test/Interop/CxxToSwiftToCxx/allow-shared-frt-back-to-cxx.swift

@@ -1,7 +1,7 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %s %t
 
-// RUN: %target-swift-frontend -typecheck %t/use-cxx-types.swift -typecheck -module-name UseCxxTy -emit-clang-header-path %t/UseCxxTy.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -disable-availability-checking
+// RUN: %target-swift-frontend %t/use-cxx-types.swift -module-name UseCxxTy -typecheck -verify -emit-clang-header-path %t/UseCxxTy.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -disable-availability-checking
 
 // RUN: %FileCheck %s < %t/UseCxxTy.h
 

test/Interop/CxxToSwiftToCxx/bridge-cxx-struct-back-to-cxx.swift

@@ -1,11 +1,11 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %s %t
 
-// RUN: %target-swift-frontend -typecheck %t/use-cxx-types.swift -typecheck -module-name UseCxxTy -emit-clang-header-path %t/UseCxxTy.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -disable-availability-checking
+// RUN: %target-swift-frontend %t/use-cxx-types.swift -module-name UseCxxTy -typecheck -verify -emit-clang-header-path %t/UseCxxTy.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -disable-availability-checking
 
 // RUN: %FileCheck %s --input-file %t/UseCxxTy.h
 
-// RUN: %target-swift-frontend -typecheck %t/use-cxx-types.swift -typecheck -module-name UseCxxTy -emit-clang-header-path %t/UseCxxTyExposeOnly.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=has-expose-attr -disable-availability-checking
+// RUN: %target-swift-frontend %t/use-cxx-types.swift -module-name UseCxxTy -typecheck -verify -emit-clang-header-path %t/UseCxxTyExposeOnly.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=has-expose-attr -disable-availability-checking
 
 // RUN: %FileCheck %s --input-file %t/UseCxxTyExposeOnly.h
 

test/Interop/CxxToSwiftToCxx/bridge-cxx-struct-back-to-objcxx.swift

@@ -1,7 +1,7 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %S/bridge-cxx-struct-back-to-cxx.swift %t
 
-// RUN: %target-swift-frontend -typecheck %t/use-cxx-types.swift -typecheck -module-name UseCxxTy -emit-clang-header-path %t/UseCxxTy.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -disable-availability-checking
+// RUN: %target-swift-frontend %t/use-cxx-types.swift -module-name UseCxxTy -typecheck -verify -emit-clang-header-path %t/UseCxxTy.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -disable-availability-checking
 // RUN: echo "#include \"header.h\"" > %t/full-cxx-swift-cxx-bridging.h
 // RUN: cat %t/UseCxxTy.h >> %t/full-cxx-swift-cxx-bridging.h
 

test/Interop/CxxToSwiftToCxx/consuming-cxx-struct-parameter-back-to-cxx-execution.cpp

@@ -1,7 +1,7 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %s %t
 
-// RUN: %target-swift-frontend -typecheck %t/use-cxx-types.swift -typecheck -module-name UseCxx -emit-clang-header-path %t/UseCxx.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -disable-availability-checking
+// RUN: %target-swift-frontend %t/use-cxx-types.swift -module-name UseCxx -typecheck -verify -emit-clang-header-path %t/UseCxx.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -disable-availability-checking
 
 // RUN: %target-interop-build-clangxx -std=c++20 -c %t/use-swift-cxx-types.cpp -I %t -o %t/swift-cxx-execution.o
 // RUN: %target-interop-build-swift %t/use-cxx-types.swift -o %t/swift-cxx-execution -Xlinker %t/swift-cxx-execution.o -module-name UseCxx -Xfrontend -entry-point-function-name -Xfrontend swiftMain -I %t -O -Xfrontend -disable-availability-checking

test/Interop/CxxToSwiftToCxx/link-cxx-type-metadata-accessor.swift

@@ -1,7 +1,7 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %s %t
 
-// RUN: %target-swift-frontend -typecheck %t/use-cxx-types.swift -typecheck -module-name UseCxx -emit-clang-header-path %t/UseCxx.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public
+// RUN: %target-swift-frontend %t/use-cxx-types.swift -module-name UseCxx -typecheck -verify -emit-clang-header-path %t/UseCxx.h -I %t -enable-experimental-cxx-interop -clang-header-expose-decls=all-public
 
 // Use the 'DEBUG' flug to force llvm used
 // RUN: %target-interop-build-clangxx -std=c++20 -c %t/use-swift-cxx-types.cpp -I %t -o %t/swift-cxx-execution.o -DDEBUG

test/Interop/CxxToSwiftToCxx/simd-bridge-cxx-struct-back-to-cxx.swift

@@ -1,7 +1,7 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %s %t
 
-// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck %t/use-cxx-types.swift -typecheck -module-name UseCxxTy -emit-clang-header-path %t/UseCxxTy.h -I %t -enable-experimental-cxx-interop -disable-availability-checking -Xcc -DSIMD_NO_CODE
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) %t/use-cxx-types.swift -module-name UseCxxTy -typecheck -verify -emit-clang-header-path %t/UseCxxTy.h -I %t -enable-experimental-cxx-interop -disable-availability-checking -Xcc -DSIMD_NO_CODE
 
 // RUN: %FileCheck %s < %t/UseCxxTy.h
 

test/Interop/CxxToSwiftToCxx/span/span-execution.cpp

@@ -1,7 +1,7 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %s %t
 
-// RUN: %target-swift-frontend -typecheck %t/use-span.swift -typecheck -module-name UseSpan -emit-clang-header-path %t/UseSpan.h -I %t -enable-experimental-cxx-interop -Xcc -Xclang -Xcc -fmodule-format=raw -Xcc -std=c++20 -clang-header-expose-decls=all-public
+// RUN: %target-swift-frontend %t/use-span.swift -module-name UseSpan -typecheck -verify -emit-clang-header-path %t/UseSpan.h -I %t -enable-experimental-cxx-interop -Xcc -Xclang -Xcc -fmodule-format=raw -Xcc -std=c++20 -clang-header-expose-decls=all-public
 
 // RUN: %target-interop-build-clangxx -std=c++20 -c %t/use-span.cpp -I %t -o %t/swift-cxx-execution.o
 // RUN: %target-interop-build-swift %t/use-span.swift -o %t/swift-cxx-execution -Xlinker %t/swift-cxx-execution.o -module-name UseSpan -Xfrontend -entry-point-function-name -Xfrontend swiftMain -I %t -O -Xcc --std=c++20

test/Interop/CxxToSwiftToObjcxx/bridge-cxx-types-to-objcxx.swift

@@ -1,6 +1,6 @@
 // RUN: %empty-directory(%t)
 
-// RUN: %target-swift-frontend -typecheck %s -module-name UseCxxTy -emit-clang-header-path %t/UseCxxTy.h -I %t -I %S/Inputs -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -target arm64-apple-macosx13.3
+// RUN: %target-swift-frontend %s -module-name UseCxxTy -typecheck -verify -emit-clang-header-path %t/UseCxxTy.h -I %t -I %S/Inputs -enable-experimental-cxx-interop -clang-header-expose-decls=all-public -target arm64-apple-macosx13.3
 // RUN: %FileCheck %s < %t/UseCxxTy.h
 
 // REQUIRES: OS=macosx