[Sema] Fix deleted function problem in implicitly movable test

[nullptr.cpp]

Summary

In implicitly movable test, a two-stage overload resolution is performed.
If the first overload resolution selects a deleted function, Clang directly
performs the second overload resolution, without checking whether the
deleted function matches the additional criteria.

This patch fixes the above problem.

[• Quuxplusone]

clang/lib/Sema/SemaStmt.cpp

3110

s/need/we need/

3111

s/success/succeeds/

3212

Naming nit: I would prefer to call this "NeedSecondStep" or "NeedSecondOverloadResolution" or "NeedSecondResolution". The way it is now, it sounds like it's saying we need to find a second overload, which isn't really the intended meaning.

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

5

The new behavior looks good to me (once the tests pass).
Why are there two different -verify= tags here, though? This behavior hasn't changed between C++11/14/17 and C++20.

(IIUC, the point of this patch is to fix the implementation divergence described in http://quuxplusone.github.io/draft/d2266-implicit-move-rvalue-ref.html#two-lookups , example thirteen.)

[nullptr.cpp]

Update

[nullptr.cpp]

Fix format

[nullptr.cpp]

Note: The failed test has nothing to do with this patch.

clang/lib/Sema/SemaInit.cpp

4146

I'll send another patch to make Clang report both deleted function error and the following error.

[nullptr.cpp]

Ping

[• Quuxplusone]

I don't fully understand the new control flow, but at least the new behavior (after applying this patch) looks like an improvement to me.
I recommend rebasing on top-of-tree, mainly so that the buildbots will run again and presumably be greener this time.

I still doubt that I have the authority to "accept" this patch, and am hoping to see someone like @zygoloid weigh in, even if it's only to say "meh." I am willing and able to physically land this patch, if someone says it's appropriate for me to do so.

clang/lib/Sema/SemaInit.cpp

4181

IIUC, this comment is explaining the motivation for repeating if (Result == OR_Deleted) // don't return quite yet twice in the code above — line 4123 and line 4146. It might be better to move the comment higher, then.

5285

Checking my understanding: If the first resolution selects a deleted function which is a constructor whose first parameter is an rvalue reference to T, then we don't perform the second resolution. If the first resolution selects a deleted function which is not a constructor, or whose parameter is of the wrong type, then (in C++11 through C++17 but not in C++20) we do perform the second resolution.

[nullptr.cpp]

rebase

[nullptr.cpp]

rebase

[aeubanks]

This broke

struct A {
  A();
  A(A&&)=delete;
 private:
  A(const A&);
 friend class B;
};

struct B {
  A foo() {
    A a;
    return a;
  }
};

with

/tmp/a.cc:12:12: error: call to deleted constructor of 'A'
    return a;
           ^
/tmp/a.cc:3:3: note: 'A' has been explicitly marked deleted here
  A(A&&)=delete;
  ^
1 error generated.

is this intentional?

[• Quuxplusone]

In D92936#2476091, @aeubanks wrote:

This broke [ https://godbolt.org/z/9EG7ev ]
is this intentional?

Yes, intentional. This brings Clang's C++14 conformance into line with GCC/EDG/MSVC, who all already (correctly) reject that code.

[aeubanks]

Thanks, and checking other compilers with godbolt is a good idea (I had tried gcc locally but it was likely too old)

[vitalybuka]

Something is not initialized http://lab.llvm.org:8011/#/builders/74/builds/1834/steps/9/logs/stdio

[• Quuxplusone]

In D92936#2476356, @vitalybuka wrote:

Something is not initialized
http://lab.llvm.org:8011/#/builders/74/builds/1834/steps/9/logs/stdio

Confirmed; @nullptr.cpp what do you want to do about this? I hypothesize that maybe you're not allowed to look at Seq.getFailedOverloadResult() (nor Seq.getFailedCandidateSet()) unless Seq.getFailureKind() is one of FK_ConstructorOverloadFailed, FK_UserConversionOverloadFailed, FK_ReferenceInitOverloadFailed, or FK_ListConstructorOverloadFailed. The ctor InitializationSequence::InitializationSequence member-initializes FailedCandidateSet but does not member-initialize FailedOverloadResult. Perhaps the appropriate fix would be

--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -5595,7 +5595,8 @@ InitializationSequence::InitializationSequence(Sema &S,
                                                MultiExprArg Args,
                                                bool TopLevelOfInitList,
                                                bool TreatUnavailableAsInvalid)
-    : FailedCandidateSet(Kind.getLocation(), OverloadCandidateSet::CSK_Normal) {
+    : FailedOverloadResult(OR_Success),
+      FailedCandidateSet(Kind.getLocation(), OverloadCandidateSet::CSK_Normal) {
   InitializeFrom(S, Entity, Kind, Args, TopLevelOfInitList,
                  TreatUnavailableAsInvalid);

I've tested this locally and it doesn't cause any new tests to fail (whew!), but I haven't rebuilt everything with MSAN to see if this satisfies Vitaly's buildbot (and in fact since I'm on OSX I don't think I can rebuild with MSAN because OSX doesn't support MSAN).

[dmajor]

Before the revert, our bots hit the following issue where we only error out when -Wall is given, so there's definitely something strange going on. Also happens with @Quuxplusone's suggested change applied.

$ cat test.cpp
template < class > class RefPtr {
public:
  ~RefPtr();
  template < typename d > RefPtr(d);
  operator int() &;
  operator int() && = delete;
};
class X;
bool e() {
  RefPtr< X > frame(0);
  return frame;
}

$ clang -cc1 -std=c++17 test.cpp
$ clang -cc1 -std=c++17 test.cpp -Wall
test.cpp:11:10: error: conversion function from 'RefPtr<X>' to 'bool' invokes a deleted function
  return frame;
         ^~~~~
test.cpp:6:3: note: 'operator int' has been explicitly marked deleted here
  operator int() && = delete;
  ^
1 error generated.

[• Quuxplusone]

In D92936#2477764, @dmajor wrote:

Before the revert, our bots hit the following issue where we only error out when -Wall is given, so there's definitely something strange going on.

https://godbolt.org/z/P1dv9f
Yeah, I see what's happening here. -Wall turns on -Wreturn-std-move, which in C++17 mode does a preliminary overload resolution "as if by std::move" just to see what would happen. This preliminary overload resolution finds a deleted function, and apparently this is a hard error — we're not doing whatever dance Clang requires in order to suppress/defer diagnostics during "speculative" compilation. (The difficulty of speculatively compiling things is one of my pet peeves with Clang.)

Here's an example using Clang trunk to produce a hard error from the guts of -Wreturn-std-move (left-hand pane) but no error if you don't enable that warning (right-hand pane).
https://godbolt.org/z/e6zPsb
However, this might be Clang-trunk-including-Yang's-patch, I'm not sure.

[nullptr.cpp]

Fix use-of-uninitialized-value and -Wreturn-std-move with delete function

[• Quuxplusone]

clang/lib/Sema/SemaInit.cpp

4020

Two comments you could act on if you want, or just ignore if they seem too scope-creepy:

(1) This function is crazy long and complicated. It would be great to break it down into named steps or subtasks.

(2) I'm still concerned about the repetition of if (Result == OR_Deleted but Kind == IK_Copy) keep going. IIUC, the intent is that this function should return the InitializationSequence that is found by overload resolution... but sometimes we can tell that overload resolution is going to find an inappropriate sequence (e.g. an explicit conversion, or a sequence that depends on a deleted function, or maybe some other situations) and in that case we want to short-circuit, because the caller is going to treat "No unique sequence" in the same way as "The unique sequence was inappropriate". Would it be better to have the caller pass in a new function parameter, bool ShortCircuitUnusableSequences, which is usually true but can be explicitly set to false in the three cases (return, co_return, throw) where we currently want to treat "No unique sequence" differently from "The unique sequence was inappropriate"?

Then the repeated check would be if (Result == OR_Deleted && ShortCircuitUnusableSequences) return; and it would be a little clearer what's going on.

(Sidenote: Default function arguments, especially trailing boolean ones, are the devil.)

[dmajor]

I tried the updated patch against our build and it was successful. Thanks!

clang/lib/Sema/SemaInit.cpp

 ///                       type as the entity?
 static void TryConstructorInitialization(Sema &S,
                                          const InitializedEntity &Entity,
                                          const InitializationKind &Kind,
                                          MultiExprArg Args, QualType DestType,
                                          QualType DestArrayType,
                                          InitializationSequence &Sequence,
                                          bool IsListInit = false,
                                          bool IsInitListCopy = false) {
   assert(((!IsListInit && !IsInitListCopy) ||
           (Args.size() == 1 && isa<InitListExpr>(Args[0]))) &&
          "IsListInit/IsInitListCopy must come with a single initializer list "
          "argument.");
   InitListExpr *ILE =
       (IsListInit || IsInitListCopy) ? cast<InitListExpr>(Args[0]) : nullptr;
   MultiExprArg UnwrappedArgs =
       ILE ? MultiExprArg(ILE->getInits(), ILE->getNumInits()) : Args;
Result = ResolveConstructorOverload(S, Kind.getLocation(), UnwrappedArgs,
                                         /*OnlyListConstructors=*/false,
                                         IsListInit);
   }
   if (Result) {
     Sequence.SetOverloadFailure(IsListInit ?
                       InitializationSequence::FK_ListConstructorOverloadFailed :
                       InitializationSequence::FK_ConstructorOverloadFailed,
                                 Result);
+
+    if (Result != OR_Deleted)
       return;
   }
 
   bool HadMultipleCandidates = (CandidateSet.size() > 1);
 
   // In C++17, ResolveConstructorOverload can select a conversion function
   // instead of a constructor.
   if (auto *CD = dyn_cast<CXXConversionDecl>(Best->Function)) {
     // Add the user-defined conversion step that calls the conversion function.
     QualType ConvType = CD->getConversionType();
     assert(S.Context.hasSameUnqualifiedType(ConvType, DestType) &&
            "should not have selected this conversion function");
     Sequence.AddUserConversionStep(CD, Best->FoundDecl, ConvType,
                                    HadMultipleCandidates);
     if (!S.Context.hasSameType(ConvType, DestType))
       Sequence.AddQualificationConversionStep(DestType, VK_RValue);
     if (IsListInit)
       Sequence.RewrapReferenceInitList(Entity.getType(), ILE);
     return;
   }
 
+  CXXConstructorDecl *CtorDecl = cast<CXXConstructorDecl>(Best->Function);
+  if (Result != OR_Deleted) { // TODO: Support for more than one failure.
     // C++11 [dcl.init]p6:
     //   If a program calls for the default initialization of an object
     //   of a const-qualified type T, T shall be a class type with a
     //   user-provided default constructor.
     // C++ core issue 253 proposal:
     //   If the implicit default constructor initializes all subobjects, no
     //   initializer should be required.
-  // The 253 proposal is for example needed to process libstdc++ headers in 5.x.
-  CXXConstructorDecl *CtorDecl = cast<CXXConstructorDecl>(Best->Function);
+    // The 253 proposal is for example needed to process libstdc++ headers
+    // in 5.x.
     if (Kind.getKind() == InitializationKind::IK_Default &&
         Entity.getType().isConstQualified()) {
       if (!CtorDecl->getParent()->allowConstDefaultInit()) {
         if (!maybeRecoverWithZeroInitialization(S, Sequence, Entity))
           Sequence.SetFailed(InitializationSequence::FK_DefaultInitOfConst);
         return;
       }
     }
 
     // C++11 [over.match.list]p1:
     //   In copy-list-initialization, if an explicit constructor is chosen, the
     //   initializer is ill-formed.
     if (IsListInit && !Kind.AllowExplicit() && CtorDecl->isExplicit()) {
       Sequence.SetFailed(InitializationSequence::FK_ExplicitConstructor);
       return;
     }
+  }
+
+  // [class.copy.elision]p3:
+  // In some copy-initialization contexts, a two-stage overload resolution
+  // is performed.
+  // If the first overload resolution selects a deleted function, we also
+  // need the initialization sequence to decide whether to perform the second
+  // overload resolution.
+  // For deleted functions in other contexts, there is no need to get the
+  // initialization sequence.
+  if (Result == OR_Deleted && Kind.getKind() != InitializationKind::IK_Copy)
+    return;
 
   // Add the constructor initialization step. Any cv-qualification conversion is
   // subsumed by the initialization.
   Sequence.AddConstructorInitializationStep(
       Best->FoundDecl, CtorDecl, DestArrayType, HadMultipleCandidates,
       IsListInit | IsInitListCopy, AsInitializerList);
 }
 
static void TryUserDefinedConversion(Sema &S,
 
   // Perform overload resolution. If it fails, return the failed result.
   OverloadCandidateSet::iterator Best;
   if (OverloadingResult Result
         = CandidateSet.BestViableFunction(S, DeclLoc, Best)) {
     Sequence.SetOverloadFailure(
                         InitializationSequence::FK_UserConversionOverloadFailed,
                                 Result);
+
+    // [class.copy.elision]p3:
+    // In some copy-initialization contexts, a two-stage overload resolution
+    // is performed.
+    // If the first overload resolution selects a deleted function, we also
+    // need the initialization sequence to decide whether to perform the second
+    // overload resolution.
+    if (!(Result == OR_Deleted &&
+          Kind.getKind() == InitializationKind::IK_Copy))
       return;
   }
 
   FunctionDecl *Function = Best->Function;
   Function->setReferenced();
   bool HadMultipleCandidates = (CandidateSet.size() > 1);
 
   if (isa<CXXConstructorDecl>(Function)) {
     // Add the user-defined conversion step. Any cv-qualification conversion is

clang/lib/Sema/SemaStmt.cpp

 /// resolutions that find non-constructors, such as derived-to-base conversions
 /// or `operator T()&&` member functions. If false, do consider such
 /// conversion sequences.
 ///
 /// \param Res We will fill this in if move-initialization was possible.
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
 /// invalid state.
-static void TryMoveInitialization(Sema& S,
-                                  const InitializedEntity &Entity,
+///
+/// \returns Whether we need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution succeeds but
+/// the selected constructor/operator doesn't match the additional criteria, we
+/// need to do the second overload resolution.
+static bool TryMoveInitialization(Sema &S, const InitializedEntity &Entity,
                                   const VarDecl *NRVOCandidate,
-                                  QualType ResultType,
-                                  Expr *&Value,
+                                  QualType ResultType, Expr *&Value,
                                   bool ConvertingConstructorsOnly,
                                   ExprResult &Res) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
                             CK_NoOp, Value, VK_XValue, FPOptionsOverride());
 
   Expr *InitExpr = &AsRvalue;
 
   InitializationKind Kind = InitializationKind::CreateCopy(
       Value->getBeginLoc(), Value->getBeginLoc());
 
   InitializationSequence Seq(S, Entity, Kind, InitExpr);
 
-  if (!Seq)
-    return;
+  bool NeedSecondOverloadResolution = true;
+  if (!Seq && Seq.getFailedOverloadResult() != OR_Deleted) {
+    return NeedSecondOverloadResolution;
+  }
 
   for (const InitializationSequence::Step &Step : Seq.steps()) {
     if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
         Step.Kind != InitializationSequence::SK_UserConversion)
       continue;
 
     FunctionDecl *FD = Step.Function.Function;
     if (ConvertingConstructorsOnly) {
if (ConvertingConstructorsOnly) {
         // taking an rvalue reference.
         if (cast<CXXMethodDecl>(FD)->getRefQualifier() != RQ_RValue)
           break;
       } else {
         continue;
       }
     }
 
+    NeedSecondOverloadResolution = false;
     // Promote "AsRvalue" to the heap, since we now need this
     // expression node to persist.
     Value =
         ImplicitCastExpr::Create(S.Context, Value->getType(), CK_NoOp, Value,
                                  nullptr, VK_XValue, FPOptionsOverride());
 
     // Complete type-checking the initialization of the return type
     // using the constructor we found.
     Res = Seq.Perform(S, Entity, Kind, Value);
   }
+
+  return NeedSecondOverloadResolution;
 }
 
 /// Perform the initialization of a potentially-movable value, which
 /// is the result of return value.
 ///
 /// This routine implements C++14 [class.copy]p32, which attempts to treat
 /// returned lvalues as rvalues in certain cases (to prefer move construction),
 /// then falls back to treating them as lvalues if that failed.
 ExprResult
 Sema::PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
                                       const VarDecl *NRVOCandidate,
                                       QualType ResultType,
                                       Expr *Value,
                                       bool AllowNRVO) {
   // C++14 [class.copy]p32:
   // When the criteria for elision of a copy/move operation are met, but not for
   // an exception-declaration, and the object to be copied is designated by an
   // lvalue, or when the expression in a return statement is a (possibly
   // parenthesized) id-expression that names an object with automatic storage
   // duration declared in the body or parameter-declaration-clause of the
   // innermost enclosing function or lambda-expression, overload resolution to
   // select the constructor for the copy is first performed as if the object
   // were designated by an rvalue.
   ExprResult Res = ExprError();
+  bool NeedSecondOverloadResolution = true;
 
   if (AllowNRVO) {
     bool AffectedByCWG1579 = false;
 
     if (!NRVOCandidate) {
       NRVOCandidate = getCopyElisionCandidate(ResultType, Value, CES_Default);
       if (NRVOCandidate &&
           !getDiagnostics().isIgnored(diag::warn_return_std_move_in_cxx11,
                                       Value->getExprLoc())) {
         const VarDecl *NRVOCandidateInCXX11 =
             getCopyElisionCandidate(ResultType, Value, CES_FormerDefault);
         AffectedByCWG1579 = (!NRVOCandidateInCXX11);
       }
     }
 
     if (NRVOCandidate) {
-      TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value,
-                            true, Res);
+      NeedSecondOverloadResolution = TryMoveInitialization(
+          *this, Entity, NRVOCandidate, ResultType, Value, true, Res);
     }
 
-    if (!Res.isInvalid() && AffectedByCWG1579) {
+    if (!NeedSecondOverloadResolution && AffectedByCWG1579) {
       QualType QT = NRVOCandidate->getType();
       if (QT.getNonReferenceType()
                      .getUnqualifiedType()
                      .isTriviallyCopyableType(Context)) {
         // Adding 'std::move' around a trivially copyable variable is probably
         // pointless. Don't suggest it.
       } else {
         // Common cases for this are returning unique_ptr<Derived> from a
         // function of return type unique_ptr<Base>, or returning T from a
         // function of return type Expected<T>. This is totally fine in a
         // post-CWG1579 world, but was not fine before.
         assert(!ResultType.isNull());
         SmallString<32> Str;
         Str += "std::move(";
         Str += NRVOCandidate->getDeclName().getAsString();
         Str += ")";
         Diag(Value->getExprLoc(), diag::warn_return_std_move_in_cxx11)
             << Value->getSourceRange()
             << NRVOCandidate->getDeclName() << ResultType << QT;
         Diag(Value->getExprLoc(), diag::note_add_std_move_in_cxx11)
             << FixItHint::CreateReplacement(Value->getSourceRange(), Str);
       }
-    } else if (Res.isInvalid() &&
+    } else if (NeedSecondOverloadResolution &&
                !getDiagnostics().isIgnored(diag::warn_return_std_move,
                                            Value->getExprLoc())) {
       const VarDecl *FakeNRVOCandidate =
           getCopyElisionCandidate(QualType(), Value, CES_AsIfByStdMove);
       if (FakeNRVOCandidate) {
         QualType QT = FakeNRVOCandidate->getType();
         if (QT->isLValueReferenceType()) {
           // Adding 'std::move' around an lvalue reference variable's name is
if (!NeedSecondOverloadResolution && AffectedByCWG1579) {
         }
       }
     }
   }
 
   // Either we didn't meet the criteria for treating an lvalue as an rvalue,
   // above, or overload resolution failed. Either way, we need to try
   // (again) now with the return value expression as written.
-  if (Res.isInvalid())
+  if (NeedSecondOverloadResolution)
     Res = PerformCopyInitialization(Entity, SourceLocation(), Value);
 
   return Res;
 }
 
 /// Determine whether the declared return type of the specified function
 /// contains 'auto'.
 static bool hasDeducedReturnType(FunctionDecl *FD) {

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

+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=expected %s
+
+namespace test_delete_function {
+struct A1 {
+  A1();
+  A1(const A1 &);
+  A1(A1 &&) = delete; // expected-note {{'A1' has been explicitly marked deleted here}}
+};
+A1 test1() {
+  A1 a;
+  return a; // expected-error {{call to deleted constructor of 'test_delete_function::A1'}}
+}
+
+struct A2 {
+  A2();
+  A2(const A2 &);
+
+private:
+  A2(A2 &&); // expected-note {{declared private here}}
+};
+A2 test2() {
+  A2 a;
+  return a; // expected-error {{calling a private constructor of class 'test_delete_function::A2'}}
+}
+
+struct C {};
+
+struct B1 {
+  B1(C &);
+  B1(C &&) = delete; // expected-note {{'B1' has been explicitly marked deleted here}}
+};
+B1 test3() {
+  C c;
+  return c; // expected-error {{conversion function from 'test_delete_function::C' to 'test_delete_function::B1' invokes a deleted function}}
+}
+
+struct B2 {
+  B2(C &);
+
+private:
+  B2(C &&); // expected-note {{declared private here}}
+};
+B2 test4() {
+  C c;
+  return c; // expected-error {{calling a private constructor of class 'test_delete_function::B2'}}
+}
+} // namespace test_delete_function