[clang] Print 32 candidates on the first failure, with -fshow-overloads=best.

[jlebar]

Summary

Previously, -fshow-overloads=best always showed 4 candidates. The
problem is, when this isn't enough, you're kind of up a creek; the only
option available is to recompile with different flags. This can be
quite expensive!

With this change, we try to strike a compromise. The *first* error with
more than 4 candidates will show up to 32 candidates. All further
errors continue to show only 4 candidates.

The hope is that this way, users will have *some chance* of making
forward progress, without facing unbounded amounts of error spam.

[jlebar]

Not sure who can review this, but looking through blame it seems like maybe @aaronpuchert?

[aaronpuchert]

In D95754#2561849, @jlebar wrote:

Not sure who can review this, but looking through blame it seems like maybe @aaronpuchert?

I'm by no means an expert on overloading, but this seems more like a usability question.

I like the idea. Let's give @rsmith a chance to chime in, otherwise I'll approve it after a while.

clang/lib/Sema/Sema.cpp

2310

Why not in the following if? I assume we want to show a long list not necessarily once, but only if it comes with the first error?

[jlebar]

Thank you for your comments, Aaron!

clang/lib/Sema/Sema.cpp

2310

My intent was to show the long list once, even if it's not the very first error. My thought process:

All things being equal, it's better to show more information to the user than less. The problem is, at some point, the amount of information we show becomes overwhelming and spammy. Particularly problematic are multiline errors, because then you get O(nm) error lines across the whole TU. We prevent the O(nm) overwhelm by limiting the number of lines a particular error can produce (using the mechanism in question here, or the template backtrace limit, etc), and then also limiting the total number of individual errors before we stop printing those.

With this change, we display the full(ish) error the first time it occurs and then the truncated error every other time. So in total it's O(n + m) rather than O(nm).

[aaronpuchert]

clang/lib/Sema/Sema.cpp

2310

Got it, but currently you'd not be exhausting the option to print a long list once: when the first overload resolution error has fewer candidates than the limit you'd still say we stop printing long lists of notes from now on. That confused me because you're calling noteNumOverloadCandidatesShown but you might not actually have printed that note.

If you're going by the O(n+m) argument, you could put this call into the if (SuppressedOverloads) and still stay under that limit.

[jlebar]

clang/lib/Sema/Sema.cpp

2310

currently you'd not be exhausting the option to print a long list once: when the first overload resolution error has fewer candidates than the limit you'd still say we stop printing long lists of notes from now on.

But if we print <= 4 overloads then noteNumOverloadCandidatesShown has no effect?

OTOH if we move it into the if then imagine a case where NumOverloads starts at 32 and we print 16 overloads. In that case we don't suppress any overloads. But the *next* time we still want to limit it to only 4. So moving it into the if would do the wrong thing.

Unless I'm misunderstanding?

[aaronpuchert]

clang/lib/Sema/Sema.cpp

2310

You're absolutely right. I guess I was confused by the function name noteNumOverloadCandidatesShown, thinking that calling it would indicate that we've shown the note below.

Perhaps we can just drop the note or find another verb? Or you could explain how the name is intended to be read.

[jlebar]

Rename noteNumOverloadCandidatesShown -> overloadCandidatesShown.

[jlebar]

I guess I was confused by the function name

Haha, two hardest problems in computer science. :)

Changed it to S.Diags.overloadCandidatesShown(ShownOverloads);

[aaronpuchert]

Thanks, this looks good to me. Maybe wait a few days whether @rsmith has a comment before you land it.

[jlebar]

Thank you for the review!

I'll put a note in my cal to land this in a few days if I don't hear from @rsmith

clang/include/clang/Basic/Diagnostic.h

private:
   bool PrintTemplateTree = false;
 
   // Color printing is enabled.
   bool ShowColors = false;
 
   // Which overload candidates to show.
   OverloadsShown ShowOverloads = Ovl_All;
 
+  // With Ovl_Best, the number of overload candidates to show when we encounter
+  // an error.
+  //
+  // The value here is the number of candidates to show in the first nontrivial
+  // error.  Future errors may show a different number of candidates.
+  unsigned NumOverloadsToShow = 32;
+
   // Cap of # errors emitted, 0 -> no limit.
   unsigned ErrorLimit = 0;
 
   // Cap on depth of template backtrace stack, 0 -> no limit.
   unsigned TemplateBacktraceLimit = 0;
 
   // Cap on depth of constexpr evaluation backtrace stack, 0 -> no limit.
   unsigned ConstexprBacktraceLimit = 0;
public:
   /// fails.
   ///
   /// By default, we show all candidates.
   void setShowOverloads(OverloadsShown Val) {
     ShowOverloads = Val;
   }
   OverloadsShown getShowOverloads() const { return ShowOverloads; }
 
+  /// When a call or operator fails, print out up to this many candidate
+  /// overloads as suggestions.
+  ///
+  /// With Ovl_Best, we set a high limit for the first nontrivial overload set
+  /// we print, and a lower limit for later sets.  This way the user has a
+  /// chance of diagnosing at least one callsite in their program without
+  /// having to recompile with -fshow-overloads=all.
+  unsigned getNumOverloadCandidatesToShow() const {
+    switch (getShowOverloads()) {
+    case Ovl_All:
+      // INT_MAX rather than UINT_MAX so that we don't have to think about the
+      // effect of implicit conversions on this value. In practice we'll never
+      // hit 2^31 candidates anyway.
+      return std::numeric_limits<int>::max();
+    case Ovl_Best:
+      return NumOverloadsToShow;
+    }
+  }
+
+  /// Call this after showing N overload candidates.  This influences the value
+  /// returned by later calls to getNumOverloadCandidatesToShow().
+  void noteNumOverloadCandidatesShown(unsigned N) {
+    // Current heuristic: Start out with a large value for NumOverloadsToShow,
+    // and then once we print one nontrivially-large overload set, decrease it
+    // for future calls.
+    if (N > 4) {
+      NumOverloadsToShow = 4;
+    }
+  }
+
   /// Pretend that the last diagnostic issued was ignored, so any
   /// subsequent notes will be suppressed, or restore a prior ignoring
   /// state after ignoring some diagnostics and their notes, possibly in
   /// the middle of another diagnostic.
   ///
   /// This can be used by clients who suppress diagnostics themselves.
   void setLastDiagnosticIgnored(bool Ignored) {
     if (LastDiagLevel == DiagnosticIDs::Fatal)

clang/lib/Sema/Sema.cpp

 ///  many candidates. Typically this will be the location of the original
 ///  ill-formed expression.
 static void noteOverloads(Sema &S, const UnresolvedSetImpl &Overloads,
                           const SourceLocation FinalNoteLoc) {
   int ShownOverloads = 0;
   int SuppressedOverloads = 0;
   for (UnresolvedSetImpl::iterator It = Overloads.begin(),
        DeclsEnd = Overloads.end(); It != DeclsEnd; ++It) {
-    // FIXME: Magic number for max shown overloads stolen from
-    // OverloadCandidateSet::NoteCandidates.
-    if (ShownOverloads >= 4 && S.Diags.getShowOverloads() == Ovl_Best) {
+    if (ShownOverloads >= S.Diags.getNumOverloadCandidatesToShow()) {
       ++SuppressedOverloads;
       continue;
     }
 
     NamedDecl *Fn = (*It)->getUnderlyingDecl();
     // Don't print overloads for non-default multiversioned functions.
     if (const auto *FD = Fn->getAsFunction()) {
       if (FD->isMultiVersion() && FD->hasAttr<TargetAttr>() &&
           !FD->getAttr<TargetAttr>()->isDefaultVersion())
         continue;
     }
     S.Diag(Fn->getLocation(), diag::note_possible_target_of_call);
     ++ShownOverloads;
   }
 
+  S.Diags.noteNumOverloadCandidatesShown(ShownOverloads);
+
   if (SuppressedOverloads)
     S.Diag(FinalNoteLoc, diag::note_ovl_too_many_candidates)
       << SuppressedOverloads;
 }
 
 static void notePlausibleOverloads(Sema &S, SourceLocation Loc,
                                    const UnresolvedSetImpl &Overloads,
                                    bool (*IsPlausibleResult)(QualType)) {

clang/lib/Sema/SemaOverload.cpp

 /// "lead" diagnostic; it will be given two arguments, the source and
 /// target types of the conversion.
 void ImplicitConversionSequence::DiagnoseAmbiguousConversion(
                                  Sema &S,
                                  SourceLocation CaretLoc,
                                  const PartialDiagnostic &PDiag) const {
   S.Diag(CaretLoc, PDiag)
     << Ambiguous.getFromType() << Ambiguous.getToType();
-  // FIXME: The note limiting machinery is borrowed from
-  // OverloadCandidateSet::NoteCandidates; there's an opportunity for
-  // refactoring here.
-  const OverloadsShown ShowOverloads = S.Diags.getShowOverloads();
   unsigned CandsShown = 0;
   AmbiguousConversionSequence::const_iterator I, E;
   for (I = Ambiguous.begin(), E = Ambiguous.end(); I != E; ++I) {
-    if (CandsShown >= 4 && ShowOverloads == Ovl_Best)
+    if (CandsShown >= S.Diags.getNumOverloadCandidatesToShow())
       break;
     ++CandsShown;
     S.NoteOverloadCandidate(I->first, I->second);
   }
+  S.Diags.noteNumOverloadCandidatesShown(CandsShown);
   if (I != E)
     S.Diag(SourceLocation(), diag::note_ovl_too_many_candidates) << int(E - I);
 }
 
 static void DiagnoseBadConversion(Sema &S, OverloadCandidate *Cand,
                                   unsigned I, bool TakingCandidateAddress) {
   const ImplicitConversionSequence &Conv = Cand->Conversions[I];
   assert(Conv.isBad());
if (S.getLangOpts().CUDA && S.getLangOpts().GPUDeferDiag) {
     // host device candidates.
     auto WrongSidedCands =
         CompleteCandidates(S, OCD_AllCandidates, Args, OpLoc, [](auto &Cand) {
           return (Cand.Viable == false &&
                   Cand.FailureKind == ovl_fail_bad_target) ||
                  (Cand.Function->template hasAttr<CUDAHostAttr>() &&
                   Cand.Function->template hasAttr<CUDADeviceAttr>());
         });
-    DeferHint = WrongSidedCands.size();
+    DeferHint = !WrongSidedCands.empty();
   }
   return DeferHint;
 }
 
 /// When overload resolution fails, prints diagnostic messages containing the
 /// candidates in the candidate set.
 void OverloadCandidateSet::NoteCandidates(
     PartialDiagnosticAt PD, Sema &S, OverloadCandidateDisplayKind OCD,
void OverloadCandidateSet::NoteCandidates(Sema &S, ArrayRef<Expr *> Args,
   bool ReportedAmbiguousConversions = false;
 
   const OverloadsShown ShowOverloads = S.Diags.getShowOverloads();
   unsigned CandsShown = 0;
   auto I = Cands.begin(), E = Cands.end();
   for (; I != E; ++I) {
     OverloadCandidate *Cand = *I;
 
-    // Set an arbitrary limit on the number of candidate functions we'll spam
-    // the user with.  FIXME: This limit should depend on details of the
-    // candidate list.
-    if (CandsShown >= 4 && ShowOverloads == Ovl_Best) {
+    if (CandsShown >= S.Diags.getNumOverloadCandidatesToShow() &&
+        ShowOverloads == Ovl_Best) {
       break;
     }
     ++CandsShown;
 
     if (Cand->Function)
       NoteFunctionCandidate(S, Cand, Args.size(),
                             /*TakingCandidateAddress=*/false, DestAS);
     else if (Cand->IsSurrogate)
else {
         ReportedAmbiguousConversions = true;
       }
 
       // If this is a viable builtin, print it.
       NoteBuiltinOperatorCandidate(S, Opc, OpLoc, Cand);
     }
   }
 
+  // Inform S.Diags that we've shown an overload set with N elements.  This may
+  // inform the future value of S.Diags.getNumOverloadCandidatesToShow().
+  S.Diags.noteNumOverloadCandidatesShown(CandsShown);
+
   if (I != E)
     S.Diag(OpLoc, diag::note_ovl_too_many_candidates,
            shouldDeferDiags(S, Args, OpLoc))
         << int(E - I);
 }
 
 static SourceLocation
 GetLocationForCandidate(const TemplateSpecCandidate *Cand) {

clang/test/SemaCXX/ambiguous-conversion-show-overload.cpp

 // RUN: not %clang_cc1 -fsyntax-only -fshow-overloads=best -fno-caret-diagnostics %s 2>&1 | FileCheck %s
 struct S {
   S(void*);
   S(char*);
   S(unsigned char*);
   S(signed char*);
   S(unsigned short*);
   S(signed short*);
   S(unsigned int*);
   S(signed int*);
 };
 void f(const S& s);
-void g() {
-  f(0);
-}
+
+// First call to f emits all candidates.  Second call emits just the first 4.
+void g() { f(0); }
+// CHECK: {{conversion from 'int' to 'const S' is ambiguous}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+// CHECK-NEXT: {{candidate constructor}}
+
+void h() { f(0); }
 // CHECK: {{conversion from 'int' to 'const S' is ambiguous}}
 // CHECK-NEXT: {{candidate constructor}}
 // CHECK-NEXT: {{candidate constructor}}
 // CHECK-NEXT: {{candidate constructor}}
 // CHECK-NEXT: {{candidate constructor}}
 // CHECK-NEXT: {{remaining 4 candidates omitted; pass -fshow-overloads=all to show them}}

clang/test/SemaCXX/overloaded-builtin-operators.cpp

 
 // DR425
 struct A {
   template< typename T > operator T() const;
 };
 
 void test_dr425(A a) {
   (void)(1.0f * a); // expected-error{{ambiguous}} \
-                    // expected-note 4{{candidate}} \
-                    // expected-note {{remaining 8 candidates omitted; pass -fshow-overloads=all to show them}}
+                    // expected-note 12{{candidate}}
 }
 
 // pr5432
 enum e {X};
 
 const int a[][2] = {{1}};
 
 int test_pr5432() {