Fix regression in special member definitions under SuppressAllDiagnostics

[brad.king]

Summary

In commit r303930 (Switch from using a DiagnosticTrap and a note...,
2017-05-25) use of DiagnosticErrorTrap was removed because it was no
longer needed to detect whether to add a diagnostic note. However, the
trap was also necessary to correctly detect and mark invalid special
members under Sema.getDiagnostics().setSuppressAllDiagnostics(true).
Restore use of the trap to fix semantic checking when diagnostics are
suppressed.

This is useful for external tools that need to detect the set of valid
special members without issuing diagnostics on the invalid ones.
Add a unit test that does this and verifies the results.

[lebedev.ri]

1. Please upload path with full context (-U9999)
2. Tests?

[brad.king]

Updated diff with full context.

[brad.king]

Tests?

make check didn't regress from this.

This behavior difference is not observable from within Clang itself, only in external applications that use SuppressAllDiagnostics while forcing definition of implicit members. Adding a test will require reproducing a chunk of the application code in the test suite to create the appropriate CompilerInstance. I'd be happy to do that to keep this working in the future, but where would be the appropriate place?

BTW, I'd like to see this integrated in the release_50 branch because this regression breaks my application that works with 3.6, 3.7, 3.8, 3.9, and 4.0.

[lebedev.ri]

In D37381#858800, @brad.king wrote:

Tests?

make check didn't regress from this.

Which is the problem.

Say this gets committed, and then someone else for some reason reverts part of the patch.
Normally, tests should catch that. But as you have stated yourself, no tests failed because of this...

This behavior difference is not observable from within Clang itself, only in external applications that use SuppressAllDiagnostics while forcing definition of implicit members. Adding a test will require reproducing a chunk of the application code in the test suite to create the appropriate CompilerInstance. I'd be happy to do that to keep this working in the future, but where would be the appropriate place?

BTW, I'd like to see this integrated in the release_50 branch because this regression breaks my application that works with 3.6, 3.7, 3.8, 3.9, and 4.0.

[brad.king]

Updated diff to add a test case.

[rsmith]

It's not reasonable for external users of Clang's AST to infer deep meaning from the "invalid" flag. It's simply a mechanism by which we (a) limit the amount of follow-on diagnostics, and (b) somewhat weaken our AST invariants in the presence of invalid code; it is an implementation detail and subject to change at any time. (If a node is not marked invalid, that does not imply the corresponding source code is valid.)

If you would like a system to hook into the start and end of creating a function (so you can install your own error trap from an external user of Sema), that would seem like something we could reasonably support.

[brad.king]

@rsmith thanks.

If a node is not marked invalid, that does not imply the corresponding source code is valid.

Okay, that clears up my understanding.

hook into the start and end of creating a function (so you can install your own error trap from an external user of Sema)

I only need a way to detect special members in C++98 that won't compile (since they won't be isDeleted), not trap bad code in general. I found that I can install my own error trap around my code that forces the definition of each special member.

Therefore we can abandon this change. Thanks!

lib/Sema/SemaDeclCXX.cpp

assert((Constructor->isDefaulted() && Constructor->isDefaultConstructor() &&
     "DefineImplicitDefaultConstructor - call it for implicit default ctor");
   if (Constructor->willHaveBody() || Constructor->isInvalidDecl())
     return;
 
   CXXRecordDecl *ClassDecl = Constructor->getParent();
   assert(ClassDecl && "DefineImplicitDefaultConstructor - invalid constructor");
 
   SynthesizedFunctionScope Scope(*this, Constructor);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.
   ResolveExceptionSpec(CurrentLocation,
                        Constructor->getType()->castAs<FunctionProtoType>());
   MarkVTableUsed(CurrentLocation, ClassDecl);
 
   // Add a context note for diagnostics produced after this point.
   Scope.addContextNote(CurrentLocation);
 
-  if (SetCtorInitializers(Constructor, /*AnyErrors=*/false)) {
+  if (SetCtorInitializers(Constructor, /*AnyErrors=*/false) ||
+      Trap.hasErrorOccurred()) {
     Constructor->setInvalidDecl();
     return;
   }
 
   SourceLocation Loc = Constructor->getLocEnd().isValid()
                            ? Constructor->getLocEnd()
                            : Constructor->getLocation();
   Constructor->setBody(new (Context) CompoundStmt(Loc));
assert(Constructor->getInheritedConstructor() &&
          !Constructor->doesThisDeclarationHaveABody() &&
          !Constructor->isDeleted());
   if (Constructor->willHaveBody() || Constructor->isInvalidDecl())
     return;
 
   // Initializations are performed "as if by a defaulted default constructor",
   // so enter the appropriate scope.
   SynthesizedFunctionScope Scope(*this, Constructor);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.
   ResolveExceptionSpec(CurrentLocation,
                        Constructor->getType()->castAs<FunctionProtoType>());
   MarkVTableUsed(CurrentLocation, ClassDecl);
 
   // Add a context note for diagnostics produced after this point.
for (CXXBaseSpecifier &B : VBase ? RD->vbases() : RD->bases()) {
           Context, TInfo, VBase, InitLoc, Init.get(), InitLoc,
           SourceLocation()));
     }
   }
 
   // We now proceed as if for a defaulted default constructor, with the relevant
   // initializers replaced.
 
-  if (SetCtorInitializers(Constructor, /*AnyErrors*/false, Inits)) {
+  if (SetCtorInitializers(Constructor, /*AnyErrors*/false, Inits) ||
+      Trap.hasErrorOccurred()) {
     Constructor->setInvalidDecl();
     return;
   }
 
   Constructor->setBody(new (Context) CompoundStmt(InitLoc));
   Constructor->markUsed(Context);
 
   if (ASTMutationListener *L = getASTMutationListener()) {
assert((Destructor->isDefaulted() &&
          "DefineImplicitDestructor - call it for implicit default dtor");
   if (Destructor->willHaveBody() || Destructor->isInvalidDecl())
     return;
 
   CXXRecordDecl *ClassDecl = Destructor->getParent();
   assert(ClassDecl && "DefineImplicitDestructor - invalid destructor");
 
   SynthesizedFunctionScope Scope(*this, Destructor);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.
   ResolveExceptionSpec(CurrentLocation,
                        Destructor->getType()->castAs<FunctionProtoType>());
   MarkVTableUsed(CurrentLocation, ClassDecl);
 
   // Add a context note for diagnostics produced after this point.
   Scope.addContextNote(CurrentLocation);
 
   MarkBaseAndMemberDestructorsReferenced(Destructor->getLocation(),
                                          Destructor->getParent());
 
-  if (CheckDestructor(Destructor)) {
+  if (CheckDestructor(Destructor) || Trap.hasErrorOccurred()) {
     Destructor->setInvalidDecl();
     return;
   }
 
   SourceLocation Loc = Destructor->getLocEnd().isValid()
                            ? Destructor->getLocEnd()
                            : Destructor->getLocation();
   Destructor->setBody(new (Context) CompoundStmt(Loc));
void Sema::DefineImplicitCopyAssignment(SourceLocation CurrentLocation,
 
   CXXRecordDecl *ClassDecl = CopyAssignOperator->getParent();
   if (ClassDecl->isInvalidDecl()) {
     CopyAssignOperator->setInvalidDecl();
     return;
   }
 
   SynthesizedFunctionScope Scope(*this, CopyAssignOperator);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.
   ResolveExceptionSpec(CurrentLocation,
                        CopyAssignOperator->getType()->castAs<FunctionProtoType>());
 
   // Add a context note for diagnostics produced after this point.
   Scope.addContextNote(CurrentLocation);
void Sema::DefineImplicitCopyAssignment(SourceLocation CurrentLocation,
 
   if (!Invalid) {
     // Add a "return *this;"
     ExprResult ThisObj = CreateBuiltinUnaryOp(Loc, UO_Deref, This.build(*this, Loc));
     
     StmtResult Return = BuildReturnStmt(Loc, ThisObj.get());
     if (Return.isInvalid())
       Invalid = true;
-    else
+    else {
       Statements.push_back(Return.getAs<Stmt>());
+      if (Trap.hasErrorOccurred())
+        Invalid = true;
+    }
   }
 
   if (Invalid) {
     CopyAssignOperator->setInvalidDecl();
     return;
   }
 
   StmtResult Body;
void Sema::DefineImplicitMoveAssignment(SourceLocation CurrentLocation,
   //   are assigned, in the order in which they were declared in the class
   //   definition.
 
   // Issue a warning if our implicit move assignment operator will move
   // from a virtual base more than once.
   checkMoveAssignmentForRepeatedMove(*this, ClassDecl, CurrentLocation);
 
   SynthesizedFunctionScope Scope(*this, MoveAssignOperator);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.
   ResolveExceptionSpec(CurrentLocation,
                        MoveAssignOperator->getType()->castAs<FunctionProtoType>());
 
   // Add a context note for diagnostics produced after this point.
   Scope.addContextNote(CurrentLocation);
void Sema::DefineImplicitMoveAssignment(SourceLocation CurrentLocation,
   if (!Invalid) {
     // Add a "return *this;"
     ExprResult ThisObj =
         CreateBuiltinUnaryOp(Loc, UO_Deref, This.build(*this, Loc));
 
     StmtResult Return = BuildReturnStmt(Loc, ThisObj.get());
     if (Return.isInvalid())
       Invalid = true;
-    else
+    else {
       Statements.push_back(Return.getAs<Stmt>());
+      if (Trap.hasErrorOccurred())
+        Invalid = true;
+    }
   }
 
   if (Invalid) {
     MoveAssignOperator->setInvalidDecl();
     return;
   }
 
   StmtResult Body;
assert((CopyConstructor->isDefaulted() &&
          "DefineImplicitCopyConstructor - call it for implicit copy ctor");
   if (CopyConstructor->willHaveBody() || CopyConstructor->isInvalidDecl())
     return;
 
   CXXRecordDecl *ClassDecl = CopyConstructor->getParent();
   assert(ClassDecl && "DefineImplicitCopyConstructor - invalid constructor");
 
   SynthesizedFunctionScope Scope(*this, CopyConstructor);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.
   ResolveExceptionSpec(CurrentLocation,
                        CopyConstructor->getType()->castAs<FunctionProtoType>());
   MarkVTableUsed(CurrentLocation, ClassDecl);
 
   // Add a context note for diagnostics produced after this point.
   Scope.addContextNote(CurrentLocation);
 
   // C++11 [class.copy]p7:
   //   The [definition of an implicitly declared copy constructor] is
   //   deprecated if the class has a user-declared copy assignment operator
   //   or a user-declared destructor.
   if (getLangOpts().CPlusPlus11 && CopyConstructor->isImplicit())
     diagnoseDeprecatedCopyOperation(*this, CopyConstructor);
 
-  if (SetCtorInitializers(CopyConstructor, /*AnyErrors=*/false)) {
+  if (SetCtorInitializers(CopyConstructor, /*AnyErrors=*/false) ||
+      Trap.hasErrorOccurred()) {
     CopyConstructor->setInvalidDecl();
   }  else {
     SourceLocation Loc = CopyConstructor->getLocEnd().isValid()
                              ? CopyConstructor->getLocEnd()
                              : CopyConstructor->getLocation();
     Sema::CompoundScopeRAII CompoundScope(*this);
     CopyConstructor->setBody(
         ActOnCompoundStmt(Loc, Loc, None, /*isStmtExpr=*/false).getAs<Stmt>());
assert((MoveConstructor->isDefaulted() &&
          "DefineImplicitMoveConstructor - call it for implicit move ctor");
   if (MoveConstructor->willHaveBody() || MoveConstructor->isInvalidDecl())
     return;
 
   CXXRecordDecl *ClassDecl = MoveConstructor->getParent();
   assert(ClassDecl && "DefineImplicitMoveConstructor - invalid constructor");
 
   SynthesizedFunctionScope Scope(*this, MoveConstructor);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.
   ResolveExceptionSpec(CurrentLocation,
                        MoveConstructor->getType()->castAs<FunctionProtoType>());
   MarkVTableUsed(CurrentLocation, ClassDecl);
 
   // Add a context note for diagnostics produced after this point.
   Scope.addContextNote(CurrentLocation);
 
-  if (SetCtorInitializers(MoveConstructor, /*AnyErrors=*/false)) {
+  if (SetCtorInitializers(MoveConstructor, /*AnyErrors=*/false) ||
+      Trap.hasErrorOccurred()) {
     MoveConstructor->setInvalidDecl();
   } else {
     SourceLocation Loc = MoveConstructor->getLocEnd().isValid()
                              ? MoveConstructor->getLocEnd()
                              : MoveConstructor->getLocation();
     Sema::CompoundScopeRAII CompoundScope(*this);
     MoveConstructor->setBody(ActOnCompoundStmt(
         Loc, Loc, None, /*isStmtExpr=*/ false).getAs<Stmt>());

unittests/Sema/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(SemaTests
   ExternalSemaSourceTest.cpp
+  SuppressAllDiagnosticsTest.cpp
   )
 
 target_link_libraries(SemaTests
   clangAST
   clangBasic
   clangFrontend
   clangParse
   clangSema
   clangTooling
   )

unittests/Sema/SuppressAllDiagnosticsTest.cpp

+//=== unittests/Sema/SuppressAllDiagnosticsTest.cpp -----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Sema/Sema.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+#include <queue>
+
+namespace {
+
+// \brief ASTConsumer that forces definition of special members.
+class ForceImplicitMembersASTConsumer : public clang::ASTConsumer {
+  clang::CompilerInstance &CI;
+  std::queue<clang::CXXRecordDecl *> Classes;
+
+public:
+  ForceImplicitMembersASTConsumer(clang::CompilerInstance &CI) : CI(CI) {}
+
+  void AddImplicitMembers(clang::CXXRecordDecl *RD) {
+    auto &Sema = CI.getSema();
+
+    // Declare implicit members.
+    Sema.ForceDeclarationOfImplicitMembers(RD);
+
+    // Force definition of implicit members.
+    for (auto *D : RD->decls()) {
+      auto *M = clang::dyn_cast<clang::CXXMethodDecl>(D);
+      if (M && !M->isDeleted() && !M->isInvalidDecl()) {
+        bool Mark = false;
+        auto *C = clang::dyn_cast<clang::CXXConstructorDecl>(M);
+        if (C) {
+          Mark = (C->isDefaultConstructor() || C->isCopyConstructor() ||
+                  C->isMoveConstructor());
+        } else if (clang::dyn_cast<clang::CXXDestructorDecl>(M)) {
+          Mark = true;
+        } else {
+          Mark =
+              (M->isCopyAssignmentOperator() || M->isMoveAssignmentOperator());
+        }
+        if (Mark) {
+          // Ensure the member is defined.
+          Sema.MarkFunctionReferenced(clang::SourceLocation(), M);
+          if (C && C->isDefaulted() && C->isDefaultConstructor() &&
+              C->isTrivial() && !C->isUsed(false)) {
+            // Clang does not build the definition of trivial constructors
+            // until they are used.  Force semantic checking.
+            Sema.DefineImplicitDefaultConstructor(clang::SourceLocation(), C);
+          }
+          // Finish implicitly instantiated member.
+          Sema.PerformPendingInstantiations();
+        }
+      }
+    }
+  }
+
+  void HandleTagDeclDefinition(clang::TagDecl *D) {
+    if (auto *RD = clang::dyn_cast<clang::CXXRecordDecl>(D)) {
+      Classes.push(RD);
+    }
+  }
+
+  void HandleTranslationUnit(clang::ASTContext &CTX) {
+    auto &Sema = CI.getSema();
+
+    // Finish the translation unit as written in the source.
+    Sema.PerformPendingInstantiations();
+
+    // Suppress diagnostics while we force more definitions.
+    Sema.getDiagnostics().setSuppressAllDiagnostics(true);
+
+    // Force addition of implicit members to classes.
+    while (!Classes.empty()) {
+      auto *RD = Classes.front();
+      Classes.pop();
+      AddImplicitMembers(RD);
+    }
+    Sema.ActOnEndOfTranslationUnit();
+
+    // Verify that the "derived" class has the implicit members we expect.
+    auto const *TU = CTX.getTranslationUnitDecl();
+    auto &Ids = CI.getPreprocessor().getIdentifierTable();
+    auto const &result =
+        TU->lookup(clang::DeclarationName(&Ids.get("derived")));
+    bool CheckedRecordDecl = false;
+    for (auto const *N : result) {
+      if (auto const *RD = clang::dyn_cast<clang::CXXRecordDecl const>(N)) {
+        CheckedRecordDecl = true;
+        CheckImplicitMembers(RD);
+      }
+    }
+    ASSERT_TRUE(CheckedRecordDecl);
+  }
+
+  void CheckImplicitMembers(clang::CXXRecordDecl const *RD) {
+    bool CheckedDefaultConstructor = false;
+    bool CheckedDestructor = false;
+    bool CheckedCopyConstructor = false;
+    bool CheckedMoveConstructor = false;
+    bool CheckedCopyAssignment = false;
+    bool CheckedMoveAssignment = false;
+    for (auto const *D : RD->decls()) {
+      if (auto const *M = clang::dyn_cast<clang::CXXMethodDecl const>(D)) {
+        if (auto const *C =
+                clang::dyn_cast<clang::CXXConstructorDecl const>(M)) {
+          if (C->isDefaultConstructor()) {
+            CheckedDefaultConstructor = true;
+            ASSERT_FALSE(C->isDeleted());
+            ASSERT_FALSE(C->isInvalidDecl());
+          } else if (C->isCopyConstructor()) {
+            CheckedCopyConstructor = true;
+            if (CI.getLangOpts().CPlusPlus11)
+              ASSERT_TRUE(C->isDeleted());
+            else
+              ASSERT_TRUE(C->isInvalidDecl());
+          } else if (C->isMoveConstructor()) {
+            CheckedMoveConstructor = true;
+            ASSERT_TRUE(C->isDeleted());
+          }
+        } else if (clang::dyn_cast<clang::CXXDestructorDecl>(M)) {
+          CheckedDestructor = true;
+          ASSERT_FALSE(M->isDeleted());
+          ASSERT_FALSE(M->isInvalidDecl());
+        } else if (M->isCopyAssignmentOperator()) {
+          CheckedCopyAssignment = true;
+          ASSERT_TRUE(M->isDeleted() || M->isInvalidDecl());
+        } else if (M->isMoveAssignmentOperator()) {
+          CheckedMoveAssignment = true;
+          if (CI.getLangOpts().CPlusPlus11)
+            ASSERT_TRUE(M->isDeleted());
+          else
+            ASSERT_TRUE(M->isInvalidDecl());
+        }
+      }
+    }
+    ASSERT_TRUE(CheckedDefaultConstructor);
+    ASSERT_TRUE(CheckedDestructor);
+    ASSERT_TRUE(CheckedCopyConstructor);
+    ASSERT_TRUE(CheckedCopyAssignment);
+    if (CI.getLangOpts().CPlusPlus11) {
+      ASSERT_TRUE(CheckedMoveConstructor);
+      ASSERT_TRUE(CheckedMoveAssignment);
+    }
+  }
+};
+
+// \brief Creates a ForceImplicitMembersASTConsumer.
+class ForceImplicitMembersAction : public clang::SyntaxOnlyAction {
+
+protected:
+  std::unique_ptr<clang::ASTConsumer>
+  CreateASTConsumer(clang::CompilerInstance &Compiler,
+                    llvm::StringRef /* dummy */) override {
+    return llvm::make_unique<ForceImplicitMembersASTConsumer>(Compiler);
+  }
+};
+
+// Test semantic checking of C++98 implicit members
+// when diagnostics are suppressed.
+TEST(SuppressAllDiagnostics, ForceImplicitMembers98) {
+  auto Action = llvm::make_unique<ForceImplicitMembersAction>();
+  std::vector<std::string> Args(1, "-std=c++98");
+  ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
+      Action.release(), "class base {\n"
+                        "protected:\n"
+                        "  base();\n"
+                        "  ~base();\n"
+                        "private:\n"
+                        "  base(base const&);\n"
+                        "  base& operator=(base const&);\n"
+                        "};\n"
+                        "class derived : public base {};\n",
+      Args));
+}
+
+// Test semantic checking of C++11 implicit members
+// when diagnostics are suppressed.
+TEST(SuppressAllDiagnostics, ForceImplicitMembers11) {
+  auto Action = llvm::make_unique<ForceImplicitMembersAction>();
+  std::vector<std::string> Args(1, "-std=c++11");
+  ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
+      Action.release(), "class base {\n"
+                        "protected:\n"
+                        "  base();\n"
+                        "  ~base();\n"
+                        "  base(base&&) = delete;\n"
+                        "  base(base const&) = delete;\n"
+                        "  base& operator=(base&&) = delete;\n"
+                        "  base& operator=(base const&) = delete;\n"
+                        "};\n"
+                        "class derived : public base {};\n",
+      Args));
+}
+
+} // anonymous namespace