[Sema] Improve the error diagnostic for dot destructor calls on pointer objects

[arphaman]

Summary

This patch improves the mismatched destructor type error by detecting when the destructor call has used a '.' instead of a '->' on a pointer to the destructed type.

For example, when given the code sample below:

struct Bar {
  ~Bar();
};
void bar(Bar *object) {
  object.~Bar();
}

Clang will now produce the following diagnostic: error: member reference type 'Bar *' is a pointer; did you mean to use '->'?.

[aaron.ballman]

LGTM

lib/Sema/SemaExprCXX.cpp

6433

You can elide the curly braces.

[arphaman]

The updated patch improves error handling and adds a test for the fixit.

If we issue a fixit we should recover as-if the code was written with the fixit in. Does this code do that? (can we test it? I know we test some fixits - not sure it's necessary/worthwhile to test them all, but maybe we have a good idiom for testing that the recovery is correct)

This code does perform recovery, but the constructed AST for the destructor calls is different from the AST that would have been constructed if the code was correct: we still end up building the pseudo destructor expression. I'm not sure how important is that though, so please let me know if I should try and make the ASTs the same.

[arphaman]

Fix a typo in the fixit test.

[rsmith]

In D25817#576276, @arphaman wrote:

This code does perform recovery, but the constructed AST for the destructor calls is different from the AST that would have been constructed if the code was correct: we still end up building the pseudo destructor expression. I'm not sure how important is that though, so please let me know if I should try and make the ASTs the same.

The intent is that the following diagnostics produced by the compile-with-fixit should be the same as the diagnostics that would be produced by the fixed code (specifically: we should not issue any "follow-on" diagnostics if the fix was correct, and if all errors have fixits then the code with fixits applied should compile cleanly). On that basis, it seems like it should be OK to produce a pseudo-destructor expression here, but you should check that the destructor of the base type is callable if it's a class type, and that it's a type for which it's valid to use a pseudo-dtor otherwise (see line 6258).

[arphaman]

The updated patch addresses Richard's comment by making sure the fixit isn't emitted when the destructor call is invalid.

[arphaman]

Are there any other comments for this patch? I would like to commit it in the next couple of days, as it was accepted and I believe I addressed Richard's concerns.

Thanks

lib/Sema/SemaExprCXX.cpp

if (const PointerType *Ptr = ObjectType->getAs<PointerType>()) {
 
       OpKind = tok::period;
     }
   }
 
   return false;
 }
 
+/// \brief Check if it's ok to try and recover dot pseudo destructor calls on
+/// pointer objects.
+static bool
+canRecoverDotPseudoDestructorCallsOnPointerObjects(Sema &SemaRef,
+                                                   QualType DestructedType) {
+  // If this is a record type, check if its destructor is callable.
+  if (auto *RD = DestructedType->getAsCXXRecordDecl()) {
+    if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
+      return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
+    return false;
+  }
+
+  // Otherwise, check if it's a type for which it's valid to use a pseudo-dtor.
+  return DestructedType->isDependentType() || DestructedType->isScalarType() ||
+         DestructedType->isVectorType();
+}
+
 ExprResult Sema::BuildPseudoDestructorExpr(Expr *Base,
                                            SourceLocation OpLoc,
                                            tok::TokenKind OpKind,
                                            const CXXScopeSpec &SS,
                                            TypeSourceInfo *ScopeTypeInfo,
                                            SourceLocation CCLoc,
                                            SourceLocation TildeLoc,
                                          PseudoDestructorTypeStorage Destructed) {
ExprResult Sema::BuildPseudoDestructorExpr(Expr *Base,
   //   [...] The cv-unqualified versions of the object type and of the type
   //   designated by the pseudo-destructor-name shall be the same type.
   if (DestructedTypeInfo) {
     QualType DestructedType = DestructedTypeInfo->getType();
     SourceLocation DestructedTypeStart
       = DestructedTypeInfo->getTypeLoc().getLocalSourceRange().getBegin();
     if (!DestructedType->isDependentType() && !ObjectType->isDependentType()) {
       if (!Context.hasSameUnqualifiedType(DestructedType, ObjectType)) {
+        // Detect dot pseudo destructor calls on pointer objects, e.g.:
+        //   Foo *foo;
+        //   foo.~Foo();
+        if (OpKind == tok::period && ObjectType->isPointerType() &&
+            Context.hasSameUnqualifiedType(DestructedType,
+                                           ObjectType->getPointeeType())) {
+          auto Diagnostic =
+              Diag(OpLoc, diag::err_typecheck_member_reference_suggestion)
+              << ObjectType << /*IsArrow=*/0 << Base->getSourceRange();
+
+          // Issue a fixit only when the destructor is valid.
+          if (canRecoverDotPseudoDestructorCallsOnPointerObjects(
+                  *this, DestructedType))
+            Diagnostic << FixItHint::CreateReplacement(OpLoc, "->");
+
+          // Recover by setting the object type to the destructed type and the
+          // operator to '->'.
+          ObjectType = DestructedType;
+          OpKind = tok::arrow;
+        } else {
           Diag(DestructedTypeStart, diag::err_pseudo_dtor_type_mismatch)
               << ObjectType << DestructedType << Base->getSourceRange()
               << DestructedTypeInfo->getTypeLoc().getLocalSourceRange();
 
           // Recover by setting the destructed type to the object type.
           DestructedType = ObjectType;
-        DestructedTypeInfo = Context.getTrivialTypeSourceInfo(ObjectType,
-                                                           DestructedTypeStart);
+          DestructedTypeInfo =
+              Context.getTrivialTypeSourceInfo(ObjectType, DestructedTypeStart);
           Destructed = PseudoDestructorTypeStorage(DestructedTypeInfo);
+        }
       } else if (DestructedType.getObjCLifetime() !=
                                                 ObjectType.getObjCLifetime()) {
 
         if (DestructedType.getObjCLifetime() == Qualifiers::OCL_None) {
           // Okay: just pretend that the user provided the correctly-qualified
           // type.
         } else {
           Diag(DestructedTypeStart, diag::err_arc_pseudo_dtor_inconstant_quals)

test/CXX/special/class.dtor/p10-0x.cpp

void a(const A *x, int i, int *pi) {
 
   typedef int *intp;
   i->~decltype(int())(); // expected-error{{member reference type 'int' is not a pointer; did you mean to use '.'?}}
   i.~decltype(int())();
   i->~decltype(intp())(); // expected-error{{member reference type 'int' is not a pointer; did you mean to use '.'?}} \
                              expected-error{{the type of object expression ('int') does not match the type being destroyed ('decltype(intp())' (aka 'int *')) in pseudo-destructor expression}}
   i.~decltype(intp())(); // expected-error{{the type of object expression ('int') does not match the type being destroyed ('decltype(intp())' (aka 'int *')) in pseudo-destructor expression}}
   pi->~decltype(int())();
-  pi.~decltype(int())(); // expected-error{{the type of object expression ('int *') does not match the type being destroyed ('decltype(int())' (aka 'int')) in pseudo-destructor expression}}
+  pi.~decltype(int())(); // expected-error{{member reference type 'int *' is a pointer; did you mean to use '->'?}}
   pi.~decltype(intp())();
   pi->~decltype(intp())(); // expected-error{{the type of object expression ('int') does not match the type being destroyed ('decltype(intp())' (aka 'int *')) in pseudo-destructor expression}}
 }

test/FixIt/fixit.cpp

 
 struct const_zero_init {
   int a;
 };
 const const_zero_init czi; // expected-error {{default initialization of an object of const type 'const const_zero_init'}}
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:26-[[@LINE-1]]:26}:"{}"
 int use_czi = czi.a;
 
+namespace dotPointerDestructor {
+
+struct Bar {
+  ~Bar();
+};
+
+void bar(Bar *o) {
+  o.~Bar(); // expected-error {{member reference type 'dotPointerDestructor::Bar *' is a pointer; did you mean to use '->'}}
+}  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:5}:"->"
+
+}

test/FixIt/no-fixit.cpp

 // RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -x c++ -std=c++11 %s 2>&1 | FileCheck %s
 
 // test that the diagnostics produced by this code do not include fixit hints
 
 // CHECK-NOT: fix-it:
 
 template<template<typename> +> void func();
 
 struct {
   void i() {
     (void)&i;
   }
 } x;
+
+namespace dotPointerDestructor {
+
+struct Bar {
+  ~Bar() = delete;
+};
+
+void bar(Bar *o) {
+  o.~Bar(); // no fixit
+}
+
+}

test/SemaCXX/pseudo-destructors.cpp

namespace PR11339 {
 
   template void destroy(int*); // expected-note{{in instantiation of function template specialization}}
 }
 
 template<typename T> using Id = T;
 void AliasTemplate(int *p) {
   p->~Id<int>();
 }
+
+namespace dotPointerAccess {
+struct Base {
+  virtual ~Base() {}
+};
+
+struct Derived : Base {
+  ~Derived() {}
+};
+
+void test() {
+  Derived d;
+  static_cast<Base *>(&d).~Base(); // expected-error {{member reference type 'dotPointerAccess::Base *' is a pointer; did you mean to use '->'}}
+  d->~Derived(); // expected-error {{member reference type 'dotPointerAccess::Derived' is not a pointer; did you mean to use '.'}}
+}
+
+typedef Derived *Foo;
+
+void test2(Foo d) {
+  d.~Foo(); // This is ok
+  d.~Derived(); // expected-error {{member reference type 'Foo' (aka 'dotPointerAccess::Derived *') is a pointer; did you mean to use '->'}}
+}
+}