[cxx-interop] Emit diagnostic to mark C++ type as non-copyable

Author: susmonteiro

include/swift/AST/DiagnosticsIRGen.def

@@ -81,7 +81,7 @@ ERROR(unable_to_load_pass_plugin,none,
       "unable to load plugin '%0': '%1'", (StringRef, StringRef))
 
 ERROR(failed_emit_copy, none,
-      "failed to copy %0; did you want to import %0 as ~Copyable?",
+      "failed to copy %0; did you mean to import %0 as ~Copyable?",
       (const clang::NamedDecl *))
 
 NOTE(use_requires_expression, none,
@@ -101,8 +101,16 @@ NOTE(annotate_non_copyable, none,
      ())
 
 NOTE(maybe_missing_annotation, none,
-     "maybe one of the types that %0 depends on needs a %1",
-     (const clang::NamedDecl *, StringRef))
+     "one of the types that %0 depends on may need a 'requires' clause (since "
+     "C++20) in the copy constructor, a 'SWIFT_COPYABLE_IF' annotation or a "
+     "'SWIFT_NONCOPYABLE' annotation'",
+     (const clang::NamedDecl *))
+
+NOTE(maybe_missing_parameter, none,
+     "the %select{'requires' clause on the copy constructor "
+     "of|'SWIFT_COPYABLE_IF' annotation on}0 %1 may be missing a "
+     "%select{constraint|parameter}0",
+     (bool, const clang::NamedDecl *))
 
 #define UNDEFINE_DIAGNOSTIC_MACROS
 #include "DefineDiagnosticMacros.h"

lib/IRGen/GenStruct.cpp

@@ -656,17 +656,22 @@ namespace {
               return false;
             });
 
-        if (copyConstructor->getTrailingRequiresClause()) {
-          ctx.Diags.diagnose(copyConstructorLoc, diag::maybe_missing_annotation,
-                             recordDecl, "'requires' clause");
-        } else if (hasCopyableIfAttr) {
+        bool hasRequiresClause =
+            copyConstructor->getTrailingRequiresClause() != nullptr;
+
+        if (hasRequiresClause || hasCopyableIfAttr) {
           ctx.Diags.diagnose(copyConstructorLoc, diag::maybe_missing_annotation,
-                             recordDecl, "'SWIFT_COPYABLE_IF' annotation");
+                             recordDecl);
+          ctx.Diags.diagnose(copyConstructorLoc, diag::maybe_missing_parameter,
+                             hasCopyableIfAttr, recordDecl);
         } else {
           ctx.Diags.diagnose(copyConstructorLoc, diag::use_requires_expression);
           ctx.Diags.diagnose(copyConstructorLoc, diag::annotate_copyable_if);
         }
-        ctx.Diags.diagnose(copyConstructorLoc, diag::annotate_non_copyable);
+
+        if (!copyConstructor->isUserProvided()) {
+          ctx.Diags.diagnose(copyConstructorLoc, diag::annotate_non_copyable);
+        }
       }
 
       auto callee = cast<llvm::Function>(clangFnAddr->stripPointerCasts());

test/Interop/Cxx/class/noncopyable-irgen.swift

@@ -28,17 +28,16 @@ struct OwnsT {
     OwnsT(const OwnsT &other) : element(other.element) {} 
     // expected-error@-1 *{{call to deleted constructor of 'NonCopyable'}}
     // expected-note@-2 *{{in instantiation of member function 'OwnsT<NonCopyable>::OwnsT' requested here}}
-    // expected-TEST1-error@-3 {{failed to copy 'OwnsT<NonCopyable>'; did you want to import 'OwnsT<NonCopyable>' as ~Copyable?}}
+    // expected-TEST1-error@-3 {{failed to copy 'OwnsT<NonCopyable>'; did you mean to import 'OwnsT<NonCopyable>' as ~Copyable?}}
     // expected-TEST1-note@-4 {{use 'requires' (since C++20) to specify the constraints under which the copy constructor is available}}
     // expected-TEST1-note@-5 {{annotate a type with 'SWIFT_COPYABLE_IF(<T>)' in C++ to specify that the type is Copyable if <T> is Copyable}}
-    // expected-TEST1-note@-6 {{annotate a type with 'SWIFT_NONCOPYABLE' in C++ to import it as ~Copyable}}
     OwnsT(OwnsT&& other) {}
 };
 
 using OwnsNonCopyable = OwnsT<NonCopyable>;
 
 template <typename T> struct Derived : OwnsT<T> {
-    // expected-TEST2-error@-1 {{failed to copy 'Derived<NonCopyable>'; did you want to import 'Derived<NonCopyable>' as ~Copyable?}}
+    // expected-TEST2-error@-1 {{failed to copy 'Derived<NonCopyable>'; did you mean to import 'Derived<NonCopyable>' as ~Copyable?}}
     // expected-TEST2-note@-2 {{use 'requires' (since C++20) to specify the constraints under which the copy constructor is available}}
     // expected-TEST2-note@-3 {{annotate a type with 'SWIFT_COPYABLE_IF(<T>)' in C++ to specify that the type is Copyable if <T> is Copyable}}
     // expected-TEST2-note@-4 {{annotate a type with 'SWIFT_NONCOPYABLE' in C++ to import it as ~Copyable}}
@@ -50,9 +49,9 @@ template <typename T> struct SWIFT_COPYABLE_IF(T) Annotated {
     T element;
     Annotated() : element() {}
     Annotated(const Annotated &other) : element(other.element) {}
-    // expected-TEST3-error@-1 {{failed to copy 'Annotated<OwnsT<NonCopyable>>'; did you want to import 'Annotated<OwnsT<NonCopyable>>' as ~Copyable?}}
-    // expected-TEST3-note@-2 {{maybe one of the types that 'Annotated<OwnsT<NonCopyable>>' depends on needs a 'SWIFT_COPYABLE_IF' annotation}}
-    // expected-TEST3-note@-3 {{annotate a type with 'SWIFT_NONCOPYABLE' in C++ to import it as ~Copyable}}
+    // expected-TEST3-error@-1 {{failed to copy 'Annotated<OwnsT<NonCopyable>>'; did you mean to import 'Annotated<OwnsT<NonCopyable>>' as ~Copyable?}}
+    // expected-TEST3-note@-2 {{one of the types that 'Annotated<OwnsT<NonCopyable>>' depends on may need a 'requires' clause (since C++20) in the copy constructor, a 'SWIFT_COPYABLE_IF' annotation or a 'SWIFT_NONCOPYABLE' annotation'}}
+    // expected-TEST3-note@-3 {{the 'SWIFT_COPYABLE_IF' annotation on 'Annotated<OwnsT<NonCopyable>>' may be missing a parameter}}
 
     Annotated(Annotated &&) = default;
 };
@@ -64,9 +63,9 @@ template <typename T> struct Requires {
     T element;
     Requires() : element() {}
     Requires(const Requires &other) requires std::is_copy_constructible_v<T> : element(other.element) {}
-    // expected-TEST4-error@-1 {{failed to copy 'Requires<OwnsT<NonCopyable>>'; did you want to import 'Requires<OwnsT<NonCopyable>>' as ~Copyable?}}
-    // expected-TEST4-note@-2 {{maybe one of the types that 'Requires<OwnsT<NonCopyable>>' depends on needs a 'requires' clause}}
-    // expected-TEST4-note@-3 {{annotate a type with 'SWIFT_NONCOPYABLE' in C++ to import it as ~Copyable}}
+    // expected-TEST4-error@-1 {{failed to copy 'Requires<OwnsT<NonCopyable>>'; did you mean to import 'Requires<OwnsT<NonCopyable>>' as ~Copyable?}}
+    // expected-TEST4-note@-2 {{one of the types that 'Requires<OwnsT<NonCopyable>>' depends on may need a 'requires' clause (since C++20) in the copy constructor, a 'SWIFT_COPYABLE_IF' annotation or a 'SWIFT_NONCOPYABLE' annotation'}}
+    // expected-TEST4-note@-3 {{the 'requires' clause on the copy constructor of 'Requires<OwnsT<NonCopyable>>' may be missing a constraint}}
 
     Requires(Requires &&) = default;
 };

test/Interop/Cxx/class/noncopyable-typechecker.swift

@@ -3,6 +3,9 @@
 // RUN: %target-swift-frontend -cxx-interoperability-mode=default -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t/Inputs %t/test.swift
 // RUN: %target-swift-frontend -cxx-interoperability-mode=default -Xcc -std=c++20 -verify-additional-prefix cpp20- -D CPP20 -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t/Inputs %t/test.swift
 
+// This test uses -verify-additional-file, which do not work well on Windows.
+// UNSUPPORTED: OS=windows-msvc
+
 //--- Inputs/module.modulemap
 module Test {
     header "noncopyable.h"