[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

3122

s/need/we need/

3123

s/success/succeeds/

3225

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–4169

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.

5283

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!