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(ClassDecl && "DefineImplicitDefaultConstructor - invalid constructor");
 
   SynthesizedFunctionScope Scope(*this, Constructor);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.

   // 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;
   }

   // 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.

   // 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;
   }

   assert(ClassDecl && "DefineImplicitDestructor - invalid destructor");
 
   SynthesizedFunctionScope Scope(*this, Destructor);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.

   MarkBaseAndMemberDestructorsReferenced(Destructor->getLocation(),
                                          Destructor->getParent());
 
-  if (CheckDestructor(Destructor)) {
+  if (CheckDestructor(Destructor) || Trap.hasErrorOccurred()) {
     Destructor->setInvalidDecl();
     return;
   }

   }
 
   SynthesizedFunctionScope Scope(*this, CopyAssignOperator);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.

     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) {

   checkMoveAssignmentForRepeatedMove(*this, ClassDecl, CurrentLocation);
 
   SynthesizedFunctionScope Scope(*this, MoveAssignOperator);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.

     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) {

   assert(ClassDecl && "DefineImplicitCopyConstructor - invalid constructor");
 
   SynthesizedFunctionScope Scope(*this, CopyConstructor);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.

   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()

   assert(ClassDecl && "DefineImplicitMoveConstructor - invalid constructor");
 
   SynthesizedFunctionScope Scope(*this, MoveConstructor);
+  DiagnosticErrorTrap Trap(Diags);
 
   // The exception specification is needed because we are defining the
   // function.

   // 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()