[clang] Improve diagnostic note for implicit conversion sequences that would work if more than one implicit user-defined conversion were allowed.

[logan-5]

Summary

The current text of the 'note' diagnostic for bad conversions is confusing in the presence of user-defined conversion operations: https://godbolt.org/z/zrgeHH
For the first error, the conversion function is simply pointed at in a note without any further explanation. For the second error, the final note claims there is no conversion from X2 to Y2, even though it is plainly visible in the source code. The reason for the error is that only one implicit user-defined conversion may be applied in these circumstances, but the error message is misleading to those who may not know this rule.

This is determined by searching for these illegal conversions in CompleteNonViableCandidate after overload resolution fails.

Depends on D74234. Also depends on D74235.

[logan-5]

Pinging this and the patches it depends on. I figured it would need a rebase by now, but it still applies cleanly to trunk.

[rsmith]

How do you defend against attempting an infinite sequence of user-defined conversion? For example:

template<int N> struct X {
    operator X<N + 1>();
};
X<0> x0 = X<1>();

clang/lib/Sema/SemaOverload.cpp

7333

Conversions[0] is for the conversion of the object argument to the implicit object parameter. This should be stored in Candidate.FinalConversion instead.

11349

&& goes on the previous line (here and below).

[logan-5]

@rsmith the intention is to only speculatively search one level deep (e.g. search for chains like A->B, B->C, but no deeper), which should cover most of the cases that are surprising to users.

However, the implementation of this patch has some issues, and it doesn't work quite how I intended. I still like the idea and I'd like to revisit it, but pondering how to get it to work properly is more than I have time for at the moment, with all the craziness in the world. So in the meantime, I'd like to withdraw it--I'll mark it and its related patches as 'abandoned'. Thanks very much for your time considering it.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def note_ovl_candidate_bad_conv : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "no known conversion "
     "%diff{from $ to $|from argument type to parameter type}3,4 for "
     "%select{%ordinal6 argument|object argument}5"
     "%select{|; dereference the argument with *|"
     "; take the address of the argument with &|"
     "; remove *|"
     "; remove &}7">;
+def note_ovl_candidate_multiple_user_defined_conv : Note<
+    "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
+    "converting "
+    "%diff{from $ to $|from argument type to parameter type}3,4 "
+    "would apply more than one implicit user-defined conversion"
+    >;
 def note_ovl_candidate_bad_arc_conv : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "cannot implicitly convert argument "
     "%diff{of type $ to $|type to parameter type}3,4 for "
     "%select{%ordinal6 argument|object argument}5 under ARC">;
 def note_ovl_candidate_bad_lvalue : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "expects an l-value for "

clang/include/clang/Sema/Overload.h

class Sema;
   /// BadConversionSequence - Records information about an invalid
   /// conversion sequence.
   struct BadConversionSequence {
     enum FailureKind {
       no_conversion,
       unrelated_class,
       bad_qualifiers,
       lvalue_ref_to_rvalue,
-      rvalue_ref_to_lvalue
+      rvalue_ref_to_lvalue,
+      multiple_user_defined,
     };
 
     // This can be null, e.g. for implicit object arguments.
     Expr *FromExpr;
 
     FailureKind Kind;
 
   private:

clang/include/clang/Sema/Sema.h

bool IsOverload(FunctionDecl *New, FunctionDecl *Old, bool IsForUsingDecl,
                   bool ConsiderRequiresClauses = true);
   
   enum class UserDefinedConversions {
     /// Find user-defined conversions.
     Allow,
 
     /// Don't find user-defined conversions.
     Suppress,
+
+    /// Allow multiple user-defined conversions in one implicit conversion
+    /// sequence. Illegal in C++, but useful for finding and pointing out 
+    /// invalid sequences in diagnostics.
+    AllowMultiple,
   };
 
   enum class AllowedExplicit {
     /// Allow no explicit functions to be used.
     None,
     /// Allow explicit conversion functions but not explicit constructors.
     Conversions,
     /// Allow both explicit conversion functions and explicit constructors.

clang/lib/Sema/SemaOverload.cpp

static bool IsTransparentUnionStandardConversion(Sema &S, Expr* From,
                                                  bool InOverloadResolution,
                                                  StandardConversionSequence &SCS,
                                                  bool CStyle);
 static OverloadingResult
 IsUserDefinedConversion(Sema &S, Expr *From, QualType ToType,
                         UserDefinedConversionSequence& User,
                         OverloadCandidateSet& Conversions,
                         AllowedExplicit AllowExplicit,
-                        bool AllowObjCConversionOnExplicit);
+                        bool AllowObjCConversionOnExplicit,
+                        bool AllowMultipleUserDefined);
 
 static ImplicitConversionSequence::CompareKind
 CompareStandardConversionSequences(Sema &S, SourceLocation Loc,
                                    const StandardConversionSequence& SCS1,
                                    const StandardConversionSequence& SCS2);
 
 static ImplicitConversionSequence::CompareKind
 CompareQualificationConversions(Sema &S,
TryUserDefinedConversion(Sema &S, Expr *From, QualType ToType,
 
   if (UserConversions == Sema::UserDefinedConversions::Suppress) {
     // We're not in the case above, so there is no conversion that
     // we can perform.
     ICS.setBad(BadConversionSequence::no_conversion, From, ToType);
     return ICS;
   }
 
+  bool AllowMultipleUserDefined 
+      = UserConversions == Sema::UserDefinedConversions::AllowMultiple;
+
   // Attempt user-defined conversion.
   OverloadCandidateSet Conversions(From->getExprLoc(),
                                    OverloadCandidateSet::CSK_Normal);
   switch (IsUserDefinedConversion(S, From, ToType, ICS.UserDefined,
                                   Conversions, AllowExplicit,
-                                  AllowObjCConversionOnExplicit)) {
+                                  AllowObjCConversionOnExplicit,
+                                  AllowMultipleUserDefined)) {
   case OR_Success:
   case OR_Deleted:
     ICS.setUserDefined();
     // C++ [over.ics.user]p4:
     //   A conversion of an expression of class type to the same class
     //   type is given Exact Match rank, and a conversion of an
     //   expression of class type to a base class of that type is
     //   given Conversion rank, in spite of the fact that a copy

 /// \param AllowObjCConversionOnExplicit true if the conversion should
 /// allow an extra Objective-C pointer conversion on uses of explicit
 /// constructors. Requires \c AllowExplicit to also be set.
 static OverloadingResult
 IsUserDefinedConversion(Sema &S, Expr *From, QualType ToType,
                         UserDefinedConversionSequence &User,
                         OverloadCandidateSet &CandidateSet,
                         AllowedExplicit AllowExplicit,
-                        bool AllowObjCConversionOnExplicit) {
+                        bool AllowObjCConversionOnExplicit,
+                        bool AllowMultipleUserDefined) {
   assert(AllowExplicit != AllowedExplicit::None ||
          !AllowObjCConversionOnExplicit);
   CandidateSet.clear(OverloadCandidateSet::CSK_InitByUserDefinedConversion);
 
   // Whether we will only visit constructors.
   bool ConstructorsOnly = false;
 
   // If the type we are conversion to is a class type, enumerate its
if (!S.isCompleteType(From->getExprLoc(), ToType)) {
         if (!Info)
           continue;
 
         bool Usable = !Info.Constructor->isInvalidDecl();
         if (!ListInitializing)
           Usable = Usable && Info.Constructor->isConvertingConstructor(
                                  /*AllowExplicit*/ true);
         if (Usable) {
-          bool SuppressUserConversions = !ConstructorsOnly;
+          bool SuppressUserConversions 
+              = !AllowMultipleUserDefined && !ConstructorsOnly;
           if (SuppressUserConversions && ListInitializing) {
             SuppressUserConversions = false;
             if (NumArgs == 1) {
               // If the first argument is (a reference to) the target type,
               // suppress conversions.
               SuppressUserConversions = isFirstArgumentCompatibleWithType(
                   S.Context, Info.Constructor, ToType);
             }

 
 bool
 Sema::DiagnoseAmbiguousUserDefinedConversion(Expr *From, QualType ToType) {
   ImplicitConversionSequence ICS;
   OverloadCandidateSet CandidateSet(From->getExprLoc(),
                                     OverloadCandidateSet::CSK_Normal);
   OverloadingResult OvResult =
     IsUserDefinedConversion(*this, From, ToType, ICS.UserDefined,
-                            CandidateSet, AllowedExplicit::None, false);
+                            CandidateSet, AllowedExplicit::None, false,
+                            /*AllowMultipleUserDefined=*/false);
 
   if (!(OvResult == OR_Ambiguous ||
         (OvResult == OR_No_Viable_Function && !CandidateSet.empty())))
     return false;
 
   auto Cands = CandidateSet.CompleteCandidates(
       *this,
       OvResult == OR_Ambiguous ? OCD_AmbiguousCandidates : OCD_AllCandidates,
case BadConversionSequence::rvalue_ref_to_lvalue: {
           << IsRValueQualified;
       Diag(Method->getLocation(), diag::note_previous_decl)
         << Method->getDeclName();
       return ExprError();
     }
 
     case BadConversionSequence::no_conversion:
     case BadConversionSequence::unrelated_class:
+    case BadConversionSequence::multiple_user_defined:
       break;
     }
 
     return Diag(From->getBeginLoc(), diag::err_member_function_call_bad_type)
            << ImplicitParamRecordType << FromRecordType
            << From->getSourceRange();
   }
 
if (ToType->isRValueReferenceType() &&
         ICS.Standard.First == ICK_Lvalue_To_Rvalue) {
       Candidate.Viable = false;
       Candidate.FailureKind = ovl_fail_bad_final_conversion;
       return;
     }
     break;
 
   case ImplicitConversionSequence::BadConversion:
+    // Save the bad conversion in the candidate, for later use by diagnostics.
+    Candidate.Conversions[0].setBad(BadConversionSequence::no_conversion,
+                                    From, CallResultType);
     Candidate.Viable = false;
     Candidate.FailureKind = ovl_fail_bad_final_conversion;
     return;
 
   default:
     llvm_unreachable(
            "Can only end up with a standard conversion sequence or failure");
   }
static void DiagnoseBadConversion(Sema &S, OverloadCandidate *Cand,
   std::pair<OverloadCandidateKind, OverloadCandidateSelect> FnKindPair =
       ClassifyOverloadCandidate(S, Cand->FoundDecl, Fn, Cand->getRewriteKind(),
                                 FnDesc);
 
   Expr *FromExpr = Conv.Bad.FromExpr;
   QualType FromTy = Conv.Bad.getFromType();
   QualType ToTy = Conv.Bad.getToType();
 
+  if (Conv.Bad.Kind == BadConversionSequence::multiple_user_defined) {
+    S.Diag(Fn->getLocation(), 
+           diag::note_ovl_candidate_multiple_user_defined_conv)
+        << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second
+        << FnDesc << (FromExpr ? FromExpr->getSourceRange() : SourceRange())
+        << FromTy << ToTy;
+    return;
+  }
+
   if (FromTy == S.Context.OverloadTy) {
     assert(FromExpr && "overload set argument came from implicit argument?");
     Expr *E = FromExpr->IgnoreParens();
     if (isa<UnaryOperator>(E))
       E = cast<UnaryOperator>(E)->getSubExpr()->IgnoreParens();
     DeclarationName Name = cast<OverloadExpr>(E)->getName();
 
     S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_overload)
S.Diag(Fn->getLocation(),
         << QualsForPrinting;
     MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
     return;
   }
 
   case ovl_fail_trivial_conversion:
   case ovl_fail_bad_final_conversion:
   case ovl_fail_final_conversion_not_exact:
-    return S.NoteOverloadCandidate(Cand->FoundDecl, Fn, Cand->getRewriteKind());
-
   case ovl_fail_bad_conversion: {
     unsigned I = (Cand->IgnoreObjectArgument ? 1 : 0);
     for (unsigned N = Cand->Conversions.size(); I != N; ++I)
       if (Cand->Conversions[I].isBad())
         return DiagnoseBadConversion(S, Cand, I, TakingCandidateAddress);
 
     // FIXME: this currently happens when we're called from SemaInit
     // when user-conversion overload fails.  Figure out how to handle
bool operator()(const OverloadCandidate *L,
     if (LLoc.isInvalid()) return false;
     if (RLoc.isInvalid()) return true;
 
     return S.SourceMgr.isBeforeInTranslationUnit(LLoc, RLoc);
   }
 };
 }
 
+/// See if a conversion could have been completed if multiple implicit
+/// user-defined conversions were allowed.
+static Optional<ImplicitConversionSequence>
+TryMultipleUserDefinedConversions(Sema &S, 
+                                  const ImplicitConversionSequence& ICS) {
+  assert(ICS.isBad());
+
+  if (ICS.Bad.Kind != BadConversionSequence::no_conversion)
+    return None;
+
+  // Skip implicit object arguments.
+  if (!ICS.Bad.FromExpr)
+    return None;
+
+  ImplicitConversionSequence Multiple =
+      TryCopyInitialization(S, ICS.Bad.FromExpr, ICS.Bad.getToType(),
+                            Sema::UserDefinedConversions::AllowMultiple,
+                            /*InOverloadResolution=*/true,
+                            /*AllowObjCWritebackConversion=*/
+                            S.getLangOpts().ObjCAutoRefCount);
+
+  if (!Multiple.isBad()) {
+    Multiple.setBad(BadConversionSequence::multiple_user_defined, 
+                    ICS.Bad.getFromType(), ICS.Bad.getToType());
+    return Multiple;
+  }
+
+  return None;
+}
+
 /// CompleteNonViableCandidate - Normally, overload resolution only
 /// computes up to the first bad conversion. Produces the FixIt set if
 /// possible.
 static void
 CompleteNonViableCandidate(Sema &S, OverloadCandidate *Cand,
                            ArrayRef<Expr *> Args,
                            OverloadCandidateSet::CandidateSetKind CSK) {
   assert(!Cand->Viable);
 
-  // Don't do anything on failures other than bad conversion.
+  auto SetAsMultipleUserDefined = [&S](ImplicitConversionSequence& ICS) {
+    if (auto Multiple = TryMultipleUserDefinedConversions(S, ICS)) {
+      ICS = *Multiple;
+      return true;
+    }
+    return false;
+  };
+
+  // Conversion candidates may hold a bad conversion that could have been
+  // completed if multiple user-defined conversions were allowed.
+  if (Cand->FailureKind == ovl_fail_bad_final_conversion
+      && Cand->Conversions[0].isBad()) {
+    SetAsMultipleUserDefined(Cand->Conversions[0]);
+    return;
+  }
+
+  // Don't do anything else on failures other than bad conversion.
   if (Cand->FailureKind != ovl_fail_bad_conversion)
     return;
 
   // We only want the FixIts if all the arguments can be corrected.
   bool Unfixable = false;
   // Use a implicit copy initialization to check conversion fixes.
   Cand->Fix.setConversionChecker(TryCopyInitialization);
 
   // Attempt to fix the bad conversion.
   unsigned ConvCount = Cand->Conversions.size();
   for (unsigned ConvIdx = (Cand->IgnoreObjectArgument ? 1 : 0); /**/;
        ++ConvIdx) {
     assert(ConvIdx != ConvCount && "no bad conversion in candidate");
     if (Cand->Conversions[ConvIdx].isInitialized() &&
         Cand->Conversions[ConvIdx].isBad()) {
+      if (!SetAsMultipleUserDefined(Cand->Conversions[ConvIdx]))
         Unfixable = !Cand->TryToFixBadConversion(ConvIdx, S);
       break;
     }
   }
 
   unsigned ConvIdx = 0;
   unsigned ArgIdx = 0;
   ArrayRef<QualType> ParamTypes;
   bool Reversed = Cand->RewriteKind & CRK_Reversed;
if (Cand->Conversions[ConvIdx].isInitialized()) {
       else {
         Cand->Conversions[ConvIdx] =
             TryCopyInitialization(S, Args[ArgIdx], ParamTypes[ParamIdx],
                                   Sema::UserDefinedConversions::Allow,
                                   /*InOverloadResolution=*/true,
                                   /*AllowObjCWritebackConversion=*/
                                   S.getLangOpts().ObjCAutoRefCount);
         // Store the FixIt in the candidate if it exists.
-        if (!Unfixable && Cand->Conversions[ConvIdx].isBad())
+        if (Cand->Conversions[ConvIdx].isBad()) {
+          if (!SetAsMultipleUserDefined(Cand->Conversions[ConvIdx]) 
+              && !Unfixable)
             Unfixable = !Cand->TryToFixBadConversion(ConvIdx, S);
         }
+      }
     } else
       Cand->Conversions[ConvIdx].setEllipsis();
   }
 }
 
 SmallVector<OverloadCandidate *, 32> OverloadCandidateSet::CompleteCandidates(
     Sema &S, OverloadCandidateDisplayKind OCD, ArrayRef<Expr *> Args,
     SourceLocation OpLoc,

clang/test/CXX/drs/dr0xx.cpp

 }
 
 namespace dr84 { // dr84: yes
   struct B;
   struct A { operator B() const; };
   struct C {};
   struct B {
     B(B&); // expected-note 0-1{{candidate}}
-    B(C); // expected-note 0-1{{no known conversion from 'dr84::B' to 'dr84::C'}}
+    B(C); // expected-note 0-1{{'dr84::B' to 'dr84::C' would apply more than one implicit user-defined conversion}}
     operator C() const;
   };
   A a;
   // Cannot use B(C) / operator C() pair to construct the B from the B temporary
   // here. In C++17, we initialize the B object directly using 'A::operator B()'.
   B b = a;
 #if __cplusplus <= 201402L
   // expected-error@-2 {{no viable}}

clang/test/SemaCXX/address-space-ctor.cpp

 // RUN: %clang_cc1 %s -std=c++14 -triple=spir -verify -fsyntax-only
 // RUN: %clang_cc1 %s -std=c++17 -triple=spir -verify -fsyntax-only
 
 struct MyType {
   MyType(int i) : i(i) {}
   int i;
 };
 
-//expected-note@-5{{candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const MyType &' for 1st argument}}
-//expected-note@-6{{candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'MyType &&' for 1st argument}}
+//expected-note@-5{{candidate constructor (the implicit copy constructor) not viable: converting from 'int' to 'const MyType &' would apply more than one implicit user-defined conversion}}
+//expected-note@-6{{candidate constructor (the implicit move constructor) not viable: converting from 'int' to 'MyType &&' would apply more than one implicit user-defined conversion}}
 //expected-note@-6{{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(10)))'}}
 //expected-note@-8{{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(10)))'}}
 //expected-note@-9{{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(10)))'}}
 //expected-note@-9{{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(10)))'}}
 
 // FIXME: We can't implicitly convert between address spaces yet.
 MyType __attribute__((address_space(10))) m1 = 123; //expected-error{{no viable conversion from 'int' to '__attribute__((address_space(10))) MyType'}}
 MyType __attribute__((address_space(10))) m2(123);  //expected-error{{no matching constructor for initialization of '__attribute__((address_space(10))) MyType'}}

clang/test/SemaCXX/constructor-initializer.cpp

namespace PR14073 {
   struct S1 { union { int n; }; S1() : n(n) {} };  // expected-warning {{field 'n' is uninitialized when used here}}
   struct S2 { union { union { int n; }; char c; }; S2() : n(n) {} };  // expected-warning {{field 'n' is uninitialized when used here}}
   struct S3 { struct { int n; }; S3() : n(n) {} };  // expected-warning {{field 'n' is uninitialized when used here}}
 }
 
 namespace PR10758 {
 struct A;
 struct B {
-  B (A const &); // expected-note 2 {{candidate constructor not viable: no known conversion from 'const PR10758::B' to 'const PR10758::A &' for 1st argument}}
+  B (A const &); // expected-note 2 {{candidate constructor not viable: converting from 'const PR10758::B' to 'const PR10758::A &' would apply more than one implicit user-defined conversion}}
   B (B &); // expected-note 2 {{candidate constructor not viable: 1st argument ('const PR10758::B') would lose const qualifier}}
 };
 struct A {
   A (B); // expected-note 2 {{passing argument to parameter here}}
 };
 
 B f(B const &b) {
   return b; // expected-error {{no matching constructor for initialization of 'PR10758::B'}}
 }
 
 A f2(const B &b) {
   return b; // expected-error {{no matching constructor for initialization of 'PR10758::B'}}
 }
 }

clang/test/SemaCXX/user-defined-conversions.cpp

private:
     A(int i); // expected-note{{declared private here}}
   };
 
   void testA(A a) {
     int b = 10;
     a = b; // expected-error{{calling a private constructor of class 'rdar10202900::A'}}
   }
 }
+
+namespace more_than_one {
+
+namespace std_example {
+struct X {
+  operator int();
+};
+
+struct Y {
+  operator X(); // expected-note{{converting from 'more_than_one::std_example::Y' to 'more_than_one::std_example::X' would apply more than one implicit user-defined conversion}}
+};
+
+Y a;
+int b = a; // expected-error{{no viable conversion}}
+int c = X(a);
+}
+
+namespace ctors {
+struct X {};
+struct Y {
+  Y(X);
+};
+struct Z { // expected-note 2{{candidate constructor (the implicit}}
+  Z(Y);   // expected-note{{converting from 'more_than_one::ctors::X' to 'more_than_one::ctors::Y' would apply more than one implicit user-defined conversion}}
+};
+
+X x;
+Z z = x; // expected-error{{no viable conversion}}
+Z z2 = Y(x);
+}
+
+namespace ops {
+
+struct Y;
+struct X {
+  operator Y() const; // expected-note {{converting from 'more_than_one::ops::X' to 'more_than_one::ops::Y' would apply more than one implicit user-defined conversion}}
+};
+struct Y {};
+struct Z { // expected-note 2{{candidate constructor (the implicit}}
+  Z(Y);   // expected-note{{converting from 'more_than_one::ops::X' to 'more_than_one::ops::Y' would apply more than one implicit user-defined conversion}}
+};
+
+X x;
+Z z = x; // expected-error{{no viable conversion}}
+Z z2 = static_cast<Y>(x);
+}
+} // namespace more_than_one
+