[C++20] [P1825] Fix bugs with implicit-move from variables of reference type

[• Quuxplusone]

Summary

Review D88220 turns out to have some pretty severe bugs, but I *think* this patch fixes them.

Paper P1825 is supposed to enable implicit move from "non-volatile objects
and rvalue references to non-volatile object types." Instead, what was committed
seems to have enabled implicit move from "non-volatile things of all kinds,
except that if they're rvalue references then they must also refer to non-volatile
things." In other words, D88220 accidentally enabled implicit move from
lvalue object references (super yikes!) and also from non-object references
(such as references to functions).

These two cases are now fixed and regression-tested.

(For better and worse, D88220 was present in trunk for more than a month before anyone noticed the (very major!) bug — my thanks to @sberg for reporting it! — so we may be able to take our time fixing it, but also it may be another month before someone spots the next bug. I have no strong opinion on the process here.)

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3095

A drive by fix here would be that we already have a VDType in this context, might as well use it even though original for some reason missed it in this part.

[• Quuxplusone]

Per @mizvekov, use VDType instead of VD->getType() wherever possible.

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3095

This whole block is also logically equivalent to the much simpler:

if (VDType.isReferenceType()) {
    if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
      return false;
    VDType = VDType.getNonReferenceType()
}
if (!VDType.isObjectType() || VDType.isVolatileQualified()) 
  return false;

But you do have to adjust the comments there and adjust the rest to use VDType consistently :)
Also, I think it might be possible to even remove the !VDType.isObjectType() || part from my suggestion above, because it might be the only option left if it is not a reference anyway.

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3095

bool isObjectType() const {
  // C++ [basic.types]p8:
  //   An object type is a (possibly cv-qualified) type that is not a
  //   function type, not a reference type, and not a void type.
  return !isReferenceType() && !isFunctionType() && !isVoidType();
}

So yeah I think you can just make my suggestion be:

if (VDType.isReferenceType()) {
    if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
      return false;
    VDType = VDType.getNonReferenceType()
}
if (VDType.isVolatileQualified()) 
  return false;

[• Quuxplusone]

clang/lib/Sema/SemaStmt.cpp

3095

Yeah but I reaally don't want to
(1) change the meaning of VDType in the middle of this function (mantra: "one name = one meaning")
(2) get "clever". I don't want to have to think about "Are there any types that are neither object types nor reference types?" (What about function types? What about block types? What about, I dunno, bit-fields?) I want the code to be obviously correct, and also to casewise match exactly what the standard says. It says object or rvalue reference type — let's write code that expresses that wording exactly.

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3095

How about:

QualType VDObjType = VDType;
if (!VDType.isObjectType()) {
    if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
      return false;
    VDObjType = VDType.getNonReferenceType();
}
if (VDObjType .isVolatileQualified()) 
  return false;

And then s/VDType/VDObjType/ from here on.
I think this expresses the meaning of the standard clearly.

[aaronpuchert]

clang/lib/Sema/SemaStmt.cpp

3095

That seems like a sensible simplification, the proposed code is indeed a bit repetitive. I'd go with the original suggestion plus the new variable:

QualType VDNonRefType = VDType;
if (VDType.isReferenceType()) {
    if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
      return false;
    VDNonRefType = VDType.getNonReferenceType()
}
if (!VDNonRefType.isObjectType() || VDNonRefType.isVolatileQualified()) 
  return false;

or whatever name you find appropriate. Actually it's the type of the DeclRefExpr, isn't it? So maybe DREType?

The initialization might be a bit misleading, an alternative would be to not initialize and have an assignment VDNonRefType = VDType in the else branch instead.

3102

This is probably over the line limit, maybe try to reflow this as suggested.

[mizvekov]

clang/lib/Sema/SemaStmt.cpp

3095

@aaronpuchert Yeah that combination is good to me also, and I liked the name you suggested better. So +1 :)

[• Quuxplusone]

clang/lib/Sema/SemaStmt.cpp

3095

Actually it's the type of the DeclRefExpr, isn't it? So maybe DREType?

The fact that we don't know what it is gives me pause, re introducing it. ;) If I were going to introduce a local synonym for VDType.getNonReferenceType() on lines 3105-3108, I guess VDReferencedType would be the best name for it; but I don't think there's any reason to introduce another name here.

[aaronpuchert]

clang/lib/Sema/SemaStmt.cpp

3095

I think it's just the expression type. It should be precisely the type of E in Sema::getCopyElisionCandidate. (Since moving doesn't even affect the expression type, just its value category, you can either think of the original expression type or the implicitly moved expression type.)

Why the expression type? Well, in the end we're still returning an expression and not a declaration, even if it's a special kind of expression. That's why we want to know if the expression has non-volatile object type. That's how I would look at it.

[sberg]

FWIW, LibreOffice make check (which started to consistently fail with D88220) succeeds with this change.

[• Quuxplusone]

Shrink the code by one line, by introducing another local named variable.
Still hoping for an "accept" here.

[aaronpuchert]

I thought maybe you wanted to follow @mizvekov's proposal to simplify, but I understand you want to stick close to the standard language.

So LGTM.

[mizvekov]

In D98971#2644747, @Quuxplusone wrote:

Shrink the code by one line, by introducing another local named variable.
Still hoping for an "accept" here.

My two cents:

Functionality-wise I think it is OK, but like I said before, I think the extra repetition / verbosity hurts more than helps.

I think the suggested shortening captures the intention of the standard better: The object itself should be non-volatile, no matter if we have it by value or rvalue reference.

It does not look like to me that the clang code is in general styled to spell the wording that precisely, or else those comments quoting the standard wording would not be of much use.

clang/lib/Sema/SemaStmt.cpp

bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
   if (!VD->hasLocalStorage()) return false;
 
   // Return false if VD is a __block variable. We don't want to implicitly move
   // out of a __block variable during a return because we cannot assume the
   // variable will no longer be used.
   if (VD->hasAttr<BlocksAttr>())
     return false;
 
-  // ...non-volatile...
-  if (VD->getType().isVolatileQualified())
+  if (VDType->isObjectType()) {
+    // C++17 [class.copy.elision]p3:
+    // ...non-volatile automatic object...
+    if (VDType.isVolatileQualified())
       return false;
-
+  } else if (VDType->isRValueReferenceType()) {
     // C++20 [class.copy.elision]p3:
-  // ...rvalue reference to a non-volatile...
-  if (VD->getType()->isRValueReferenceType() &&
-      (!(CESK & CES_AllowRValueReferenceType) ||
-       VD->getType().getNonReferenceType().isVolatileQualified()))
+    // ...either a non-volatile object or an rvalue reference to a non-volatile object type...
+    if (!(CESK & CES_AllowRValueReferenceType))
+      return false;
+    QualType VDReferencedType = VDType.getNonReferenceType();
+    if (VDReferencedType.isVolatileQualified() || !VDReferencedType->isObjectType())
       return false;
+  } else {
+    return false;
+  }
 
   if (CESK & CES_AllowDifferentTypes)
     return true;
 
   // Variables with higher required alignment than their type's ABI
   // alignment cannot use NRVO.
-  if (!VD->getType()->isDependentType() && VD->hasAttr<AlignedAttr>() &&
-      Context.getDeclAlign(VD) > Context.getTypeAlignInChars(VD->getType()))
+  if (!VDType->isDependentType() && VD->hasAttr<AlignedAttr>() &&
+      Context.getDeclAlign(VD) > Context.getTypeAlignInChars(VDType))
     return false;
 
   return true;
 }
 
 /// Try to perform the initialization of a potentially-movable value,
 /// which is the operand to a return or throw statement.
 ///

clang/test/CXX/class/class.init/class.copy.elision/p3.cpp

 }
 NeedValue test_4_3() {
   // not rvalue reference
   // not same type
   DerivedB2 b;
   return b; // cxx20-error {{calling a private constructor of class 'test_ctor_param_rvalue_ref::B2'}}
 }
 } // namespace test_ctor_param_rvalue_ref
+
+namespace test_lvalue_ref_is_not_moved_from {
+
+struct Target {};
+  // expected-note@-1 {{candidate constructor (the implicit copy constructor) not viable}}
+  // expected-note@-2 {{candidate constructor (the implicit move constructor) not viable}}
+  // cxx11_14_17-note@-3 {{candidate constructor (the implicit copy constructor) not viable}}
+  // cxx11_14_17-note@-4 {{candidate constructor (the implicit move constructor) not viable}}
+
+struct CopyOnly {
+  CopyOnly(CopyOnly&&) = delete; // cxx20-note {{has been explicitly marked deleted here}}
+  CopyOnly(CopyOnly&);
+  operator Target() && = delete; // cxx20-note {{has been explicitly marked deleted here}}
+  operator Target() &;
+};
+
+struct MoveOnly {
+  MoveOnly(MoveOnly&&); // expected-note {{copy constructor is implicitly deleted because}}
+    // cxx11_14_17-note@-1 {{copy constructor is implicitly deleted because}}
+  operator Target() &&; // expected-note {{candidate function not viable}}
+    // cxx11_14_17-note@-1 {{candidate function not viable}}
+};
+
+extern CopyOnly copyonly;
+extern MoveOnly moveonly;
+
+CopyOnly t1() {
+    CopyOnly& r = copyonly;
+    return r;
+}
+
+CopyOnly t2() {
+    CopyOnly&& r = static_cast<CopyOnly&&>(copyonly);
+    return r; // cxx20-error {{call to deleted constructor}}
+}
+
+MoveOnly t3() {
+    MoveOnly& r = moveonly;
+    return r; // expected-error {{call to implicitly-deleted copy constructor}}
+}
+
+MoveOnly t4() {
+    MoveOnly&& r = static_cast<MoveOnly&&>(moveonly);
+    return r; // cxx11_14_17-error {{call to implicitly-deleted copy constructor}}
+}
+
+Target t5() {
+    CopyOnly& r = copyonly;
+    return r;
+}
+
+Target t6() {
+    CopyOnly&& r = static_cast<CopyOnly&&>(copyonly);
+    return r; // cxx20-error {{invokes a deleted function}}
+}
+
+Target t7() {
+    MoveOnly& r = moveonly;
+    return r; // expected-error {{no viable conversion}}
+}
+
+Target t8() {
+    MoveOnly&& r = static_cast<MoveOnly&&>(moveonly);
+    return r; // cxx11_14_17-error {{no viable conversion}}
+}
+
+} // namespace test_lvalue_ref_is_not_moved_from
+
+namespace test_rvalue_ref_to_nonobject {
+
+struct CopyOnly {};
+struct MoveOnly {};
+
+struct Target {
+    Target(CopyOnly (&)());
+    Target(CopyOnly (&&)()) = delete;
+    Target(MoveOnly (&)()) = delete; // expected-note {{has been explicitly marked deleted here}}
+      // expected-note@-1 {{has been explicitly marked deleted here}}
+    Target(MoveOnly (&&)());
+};
+
+CopyOnly make_copyonly();
+MoveOnly make_moveonly();
+
+Target t1() {
+    CopyOnly (&r)() = make_copyonly;
+    return r;
+}
+
+Target t2() {
+    CopyOnly (&&r)() = static_cast<CopyOnly(&&)()>(make_copyonly);
+    return r; // OK in all modes; not subject to implicit move
+}
+
+Target t3() {
+    MoveOnly (&r)() = make_moveonly;
+    return r; // expected-error {{invokes a deleted function}}
+}
+
+Target t4() {
+    MoveOnly (&&r)() = static_cast<MoveOnly(&&)()>(make_moveonly);
+    return r; // expected-error {{invokes a deleted function}}
+}
+
+} // namespace test_rvalue_ref_to_nonobject