constexpr_union_active_member_change.txt

#Component
C++

#Version
Trunk

#Title
Incomplete implementation of the P1330R0 proposal / [expr.const]p5 and [class.union]p6 (N4892)

#Body
All of the references and quotes in this report are concerned with the changes in P1330R0 and the current working standard document, N4892:
 - P1330R0: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1330r0.pdf
 - N4892:   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/n4892.pdf

Affected versions:
 - trunk
 - 12.0.1
 - 12.0.0
 - 11.0.1
 - 11.0.0
 - 10.0.1
 - 10.0.0
 - 9.0.1
 - 9.0.0

Driver cmdline:
Note: For versions not supporting -std=c++20, -std=c++2a was used.
 clang++ -std=c++20

Consider the following examples:
 [Example A:
  struct A {};

  constexpr auto test() {
   union U {
    int i;
    A a;
   } a{.i = 0};
   u.a = {};
   return 0;
  }

  int main() {
   constexpr auto r = test();
   (void)r;
  }

 [Example B:
  struct A {
   int i;

   constexpr A() noexcept : i(0) {}
   constexpr A(int i) noexcept : i(i) {}

   constexpr A& operator=(A const& other) noexcept {
    i = other.i;
    return *this;
   }
  };

  constexpr auto test() {
   union U {
    int i;
    A a;
   } u{.i = 0};
   u.a = {123};
   return u.a.i;
  }

  int main() {
   constexpr auto r = test();
   (void)r;
  }

 According to [expr.constant]p5

  An expression E is a core constant expression unless the evaluation of E, following the rules of the abstract machine (6.9.1), would evaluate one of the following:
   ... an invocation of an implicitly-defined copy/move constructor or copy/move assignment operator for a union whose active member (if any) is mutable, unless the lifetime of the union object began within the evaluation of E ...

 , and [class.union]p6

  When the left operand of an assignment operator involves a member access expression (7.6.1.5) that nominates a union member, it may begin the lifetime of that union member ...

 , the lifetime of `u.a` ("Example A":8) should start at the invocation of the `A`'s implicitly defined copy-assignment operator, however, under all listed clang versions this is not the case as compilation yields the following error:

  "Example A":13:17: error: constexpr variable 'r' must be initialized by a constant expression
  constexpr auto r = test();
                 ^   ~~~~~~
  "Example A":8:6: note: member call on member 'a' of union with active member 'i' is not allowed in a constant expression
  u.a = {};
      ^
  "Example A":13:21: note: in call to 'test()'
  constexpr auto r = test();
                     ^
  1 error generated.

 Likewise, for Example B the lifetime of `u.a` ("Example B":18) should begin at the invocation of `A`'s user-defined constexpr copy-assignment operator, however compilation yields the same error.

Addressing the issue:
 The attached patch addresses this issue and is based on the `release/12.x` tag:

  commit fed41342a82f5a3a9201819a82bf7a48313e296b (grafted, HEAD -> release/12.x, tag: llvmorg-12.0.1-rc4, tag: llvmorg-12.0.1, origin/release/12.x)
  Author: Tom Stellard <[email protected]>
  Date:   Mon Jun 28 09:23:38 2021 -0700

      Revert "Revert "[Coverage] Fix branch coverage merging in FunctionCoverageSummary::get() for instantiation""

      This reverts commit 33d312b2d731507327252fd597bac1b738870330.

      The original patch was correct, so we need to restore it in the
      release branch.

Notes:
 - In testing both examples against other major compilers, such as MSVC and GCC, it would seem that they compile and behave as expected.

ExprConstant.cpp.patch

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 1bdad771a..c3ad37e12 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -3602,6 +3602,11 @@ findSubobject(EvalInfo &Info, const Expr *E, const CompleteObject &Obj,
     return handler.failed();
   }
 
+  bool IsAssignment = false;
+  if (auto *ASE = dyn_cast<CXXOperatorCallExpr>(E)) {
+    IsAssignment = ASE->isAssignmentOp();
+  }
+
   APValue *O = Obj.Value;
   QualType ObjType = Obj.Type;
   const FieldDecl *LastField = nullptr;
@@ -3610,9 +3615,9 @@ findSubobject(EvalInfo &Info, const Expr *E, const CompleteObject &Obj,
   // Walk the designator's path to find the subobject.
   for (unsigned I = 0, N = Sub.Entries.size(); /**/; ++I) {
     // Reading an indeterminate value is undefined, but assigning over one is OK.
-    if ((O->isAbsent() && !(handler.AccessKind == AK_Construct && I == N)) ||
-        (O->isIndeterminate() &&
-         !isValidIndeterminateAccess(handler.AccessKind))) {
+    if ((O->isAbsent() && !((handler.AccessKind == AK_Construct && I == N) ||
+        (handler.AccessKind == AK_MemberCall && I == N && IsAssignment))) ||
+        (O->isIndeterminate() && !isValidIndeterminateAccess(handler.AccessKind))) {
       if (!Info.checkingPotentialConstantExpression())
         Info.FFDiag(E, diag::note_constexpr_access_uninit)
             << handler.AccessKind << O->isIndeterminate();
@@ -3751,8 +3756,10 @@ findSubobject(EvalInfo &Info, const Expr *E, const CompleteObject &Obj,
         const FieldDecl *UnionField = O->getUnionField();
         if (!UnionField ||
             UnionField->getCanonicalDecl() != Field->getCanonicalDecl()) {
-          if (I == N - 1 && handler.AccessKind == AK_Construct) {
-            // Placement new onto an inactive union member makes it active.
+          if (I == N - 1 && (handler.AccessKind == AK_Construct ||
+              (handler.AccessKind == AK_MemberCall && IsAssignment))) {
+            // Placement new onto, or assignment to, an inactive union member
+            // makes it active.
             O->setUnion(Field, APValue());
           } else {
             // FIXME: If O->getUnionValue() is absent, report that there's no
@@ -5878,10 +5885,10 @@ struct StartLifetimeOfUnionMemberHandler {
 
 const AccessKinds StartLifetimeOfUnionMemberHandler::AccessKind;
 
-/// Handle a builtin simple-assignment or a call to a trivial assignment
-/// operator whose left-hand side might involve a union member access. If it
-/// does, implicitly start the lifetime of any accessed union elements per
-/// C++20 [class.union]5.
+/// Handle a builtin simple-assignment or a call to an assignment operator
+/// whose left-hand side might involve a union member access. If it does,
+/// implicitly start the lifetime of any accessed union elements per C++20
+/// [class.union]5.
 static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
                                           const LValue &LHS) {
   if (LHS.InvalidBase || LHS.Designator.Invalid)
@@ -5893,12 +5900,12 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
   unsigned PathLength = LHS.Designator.Entries.size();
   for (const Expr *E = LHSExpr; E != nullptr;) {
     //   -- If E is of the form A.B, S(E) contains the elements of S(A)...
-    if (auto *ME = dyn_cast<MemberExpr>(E)) {
+    const auto handleMemberExpr = [&](const MemberExpr *ME) -> bool {
       auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl());
       // Note that we can't implicitly start the lifetime of a reference,
       // so we don't need to proceed any further if we reach one.
       if (!FD || FD->getType()->isReferenceType())
-        break;
+        return false;
 
       //    ... and also contains A.B if B names a union member ...
       if (FD->getParent()->isUnion()) {
@@ -5907,7 +5914,12 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
         //    such types.
         auto *RD =
             FD->getType()->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
-        if (!RD || RD->hasTrivialDefaultConstructor())
+
+        // Note: Weaken the requirement of a trivial default constructor to a
+        // default constructor in order to allow for changing the active union
+        // member for a non-trivially constructible type, with a user-defined
+        // constexpr default constructor.
+        if (!RD || RD->hasDefaultConstructor())
           UnionPathLengths.push_back({PathLength - 1, FD});
       }
 
@@ -5916,6 +5928,12 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
       assert(declaresSameEntity(FD,
                                 LHS.Designator.Entries[PathLength]
                                     .getAsBaseOrMember().getPointer()));
+      return true;
+    };
+
+    if (auto *ME = dyn_cast<MemberExpr>(E)) {
+      if (!handleMemberExpr(ME))
+        break;
 
       //   -- If E is of the form A[B] and is interpreted as a built-in array
       //      subscripting operator, S(E) is [S(the array operand, if any)].
@@ -5945,6 +5963,20 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
                                       .getAsBaseOrMember().getPointer()));
       }
 
+      //   Handle non-trivial assignment operator invocation.
+    } else if (auto *ASE = dyn_cast<CXXOperatorCallExpr>(E)) {
+      if (ASE->isAssignmentOp()) {
+        assert(ASE->getNumArgs() > 0);
+        // Start a lifetime for the union member being assigned through a
+        // non-trivial assignment oeprator invocation
+        if (auto *ME = dyn_cast<MemberExpr>(ASE->getArg(0))) {
+          if (!handleMemberExpr(ME))
+            break;
+        }
+      } else {
+       break;
+      }
+
     //   -- Otherwise, S(E) is empty.
     } else {
       break;
@@ -6089,7 +6121,7 @@ static bool HandleFunctionCall(SourceLocation CallLoc,
     if (!handleTrivialCopy(Info, MD->getParamDecl(0), Args[0], RHSValue,
                            MD->getParent()->isUnion()))
       return false;
-    if (Info.getLangOpts().CPlusPlus20 && MD->isTrivial() &&
+    if (Info.getLangOpts().CPlusPlus20 && /*MD->isTrivial() &&*/
         !HandleUnionActiveMemberChange(Info, Args[0], *This))
       return false;
     if (!handleAssignment(Info, Args[0], *This, MD->getThisType(),
@@ -7666,11 +7698,14 @@ public:
         if (!FD)
           return false;
       } else {
+        // Handle union active member change before evaluating check
+        if (Info.getLangOpts().CPlusPlus20) {
+          HandleUnionActiveMemberChange(Info, E, *This);
+        }
+
         // Check that the 'this' pointer points to an object of the right type.
-        // FIXME: If this is an assignment operator call, we may need to change
-        // the active union member before we check this.
         if (!checkNonVirtualMemberCallThisPointer(Info, E, *This, NamedMember))
-          return false;
+         return false;
       }
     }