[Clang] Add note for bad conversion error when expression is of forward-declared type

[zequanwu]

Summary

I think one note to speficify the forward declaretion location should be enough. Two notes might be too noisy.

[hans]

Thanks! This looks promising.

In addition to updating existing tests, it would be good to add a test aimed explicitly at this. Maybe you can reduce something from the example in the bug report. Maybe looking at git blame for emitBadConversionNotes() will provide hints for where existing tests for such notes were added.

clang/lib/Sema/SemaInit.cpp

8710

Can the reverse situation happen, where it's destType that's forward declared?

8713

Is the getDeclKind() still necessary even though you're doing getPointeeCXXRecordDecl() above and fromDecl is os type CXXRecordDecl*?

8714

I think we could add a new note instead of just note_forward_declaration which is more helpful, explaining that maybe the conversion would be valid if the type was defined and not just forward declared.

[zequanwu]

clang/lib/Sema/SemaInit.cpp

8710

If we only consider normal class without template, I think it's not necessary.
Here is an example of forward-declared destType.

class B{};
class A;
B *b;
A *a= b;

Even if A is defined as class A: public B{};, A *a = b is still not valid, initializing pointer to derived class with pointer to base class.

8713

Because I was thinking about excluding class template. getPointeeCXXRecordDecl() might return a ClassTemplateSpecializationDecl or ClassTemplatePartialSpecializationDecl.
For following example, it triggers the same error. Should the new note also be emitted for this case?

template<class C> class A{};
A<int> *a1;
A<char> *a2 = a1;

And the following code compiles:

template<class C> class A{};
template<> class A<char>{};
template<> class A<int> : public A<char>{};
A<int> *a1;
A<char> *a2 = a1;

This seems like the same error as normal class, could resolved by inheritance.

[zequanwu]

clang/lib/Sema/SemaInit.cpp

8710

The reverse situation could happen in class template.

template<class C> class A{};
template<class C> class B; // template<class C> class B : public A<C>{}; compiles
B<int> *b;
A<int> *a = b;

[zequanwu]

Add new note and test case.
I think for the class template case, the new note might not be useful, since they might simply caused by mismatch of template arguments.

[hans]

Looks great, thanks! I put a suggestion for different wording of the note, but I'll leave it up to you to decide.

clang/test/SemaCXX/pointer-forward-declared-class-conversion.cpp

5

Thanks, this is a much better message!

Instead of "forward declaration of class" it would be good to use the class name there, for example:

"'B1' is not defined, but forward declared here; conversion would be valid if it's derived from 'A1'"

Or something like that. Finding the right words is hard :)

[zequanwu]

Update diag note.

[rsmith]

Please see llvm.org/PR52014 -- this patch is not properly handling cv-qualifiers. We should not produce the new note if the destination type has qualifiers that the source type does not possess, because in that case inheritance makes no difference to whether the conversion is valid.

[zequanwu]

In D85390#3031104, @rsmith wrote:

Please see llvm.org/PR52014 -- this patch is not properly handling cv-qualifiers. We should not produce the new note if the destination type has qualifiers that the source type does not possess, because in that case inheritance makes no difference to whether the conversion is valid.

Sent a patch to fix it: D110780

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_init_conversion_failed : Error<
   "%diff{of type $ with an %select{rvalue|lvalue}2 of type $|"
   "with an %select{rvalue|lvalue}2 of incompatible type}1,3"
   "%select{|: different classes%diff{ ($ vs $)|}5,6"
   "|: different number of parameters (%5 vs %6)"
   "|: type mismatch at %ordinal5 parameter%diff{ ($ vs $)|}6,7"
   "|: different return type%diff{ ($ vs $)|}5,6"
   "|: different qualifiers (%5 vs %6)"
   "|: different exception specifications}4">;
+def note_forward_class_conversion : Note<"%0 is not defined, but forward "
+  "declared here; conversion would be valid if it's derived from %1">;
 
 def err_lvalue_to_rvalue_ref : Error<"rvalue reference %diff{to type $ cannot "
   "bind to lvalue of type $|cannot bind to incompatible lvalue}0,1">;
 def err_lvalue_reference_bind_to_initlist : Error<
   "%select{non-const|volatile}0 lvalue reference to type %1 cannot bind to an "
   "initializer list temporary">;
 def err_lvalue_reference_bind_to_temporary : Error<
   "%select{non-const|volatile}0 lvalue reference %diff{to type $ cannot bind "

clang/lib/Sema/SemaInit.cpp

if (destType.getNonReferenceType()->isObjCObjectPointerType() &&
     // operand is a message send with a related result type.
     S.EmitRelatedResultTypeNote(op);
 
     // Emit a possible note about a return failing because we're
     // expecting a related result type.
     if (entity.getKind() == InitializedEntity::EK_Result)
       S.EmitRelatedResultTypeNoteForReturn(destType);
   }
+  QualType fromType = op->getType();
+  auto *fromDecl = fromType.getTypePtr()->getPointeeCXXRecordDecl();
+  auto *destDecl = destType.getTypePtr()->getPointeeCXXRecordDecl();
+  if (fromDecl && destDecl && fromDecl->getDeclKind() == Decl::CXXRecord &&
+      destDecl->getDeclKind() == Decl::CXXRecord &&
+      !fromDecl->isInvalidDecl() && !destDecl->isInvalidDecl() &&
+      !fromDecl->hasDefinition())
+    S.Diag(fromDecl->getLocation(), diag::note_forward_class_conversion)
+        << S.getASTContext().getTagDeclType(fromDecl)
+        << S.getASTContext().getTagDeclType(destDecl);
 }
 
 static void diagnoseListInit(Sema &S, const InitializedEntity &Entity,
                              InitListExpr *InitList) {
   QualType DestType = Entity.getType();
 
   QualType E;
   if (S.getLangOpts().CPlusPlus11 && S.isStdInitializerList(DestType, &E)) {

clang/test/Modules/namespaces.cpp

 // actually perform any merging.
 void testAnonymousNotMerged() {
   N11::consumeFoo(N11::getFoo()); // expected-error{{cannot initialize a parameter of type 'N11::(anonymous namespace)::Foo *' with an rvalue of type 'N11::(anonymous namespace)::Foo *'}}
   N12::consumeFoo(N12::getFoo()); // expected-error{{cannot initialize a parameter of type 'N12::(anonymous namespace)::Foo *' with an rvalue of type 'N12::(anonymous namespace)::Foo *'}}
 }
 
 // expected-note@Inputs/namespaces-right.h:60 {{passing argument to parameter here}}
 // expected-note@Inputs/namespaces-right.h:67 {{passing argument to parameter here}}
-
+// expected-note@Inputs/namespaces-left.h:63 {{'N11::(anonymous namespace)::Foo' is not defined, but forward declared here; conversion would be valid if it's derived from 'N11::(anonymous namespace)::Foo'}}
+// expected-note@Inputs/namespaces-left.h:70 {{'N12::(anonymous namespace)::Foo' is not defined, but forward declared here; conversion would be valid if it's derived from 'N12::(anonymous namespace)::Foo'}}
 // Test that bringing in one name from an overload set does not hide the rest.
 void testPartialImportOfOverloadSet() {
   void (*p)() = N13::p;
   p();
   N13::f(0);
 }

clang/test/SemaCXX/elaborated-type-specifier.cpp

namespace NS {
   public:
     void test_elab2(struct S4 *s4); // expected-note{{'NS::S4' declared here}}
   };
 
   void X::test_elab2(S4 *s4) { } // expected-note{{passing argument to parameter 's4' here}}
 }
 
 void test_X_elab(NS::X x) {
-  struct S4 *s4 = 0;
+  struct S4 *s4 = 0; // expected-note{{'S4' is not defined, but forward declared here; conversion would be valid if it's derived from 'NS::S4'}}
   x.test_elab2(s4); // expected-error{{cannot initialize a parameter of type 'NS::S4 *' with an lvalue of type 'struct S4 *'}}
 }
 
 namespace NS {
   S4 *get_S4();
 }
 
 void test_S5_scope() {

clang/test/SemaCXX/pointer-forward-declared-class-conversion.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class A1 {};
+class B1; // expected-note{{'B1' is not defined, but forward declared here; conversion would be valid if it's derived from 'A1'}}
+B1 *b1;
+A1 *a1 = b1; // expected-error{{cannot initialize a variable of type 'A1 *' with an lvalue of type 'B1 *'}}
+
+template <class C> class A2 {};
+template <class C> class B2;
+B2<int> *b2;
+A2<int> *a2 = b2; // expected-error{{cannot initialize a variable of type 'A2<int> *' with an lvalue of type 'B2<int> *'}}