Page MenuHomePhabricator

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

Authored by jlebar on Jan 30 2021, 7:05 PM.

Details

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.

Diff Detail

Event Timeline

jlebar requested review of this revision.Jan 30 2021, 7:05 PM
jlebar created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2021, 7:05 PM

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

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
2331

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?

Thank you for your comments, Aaron!

clang/lib/Sema/Sema.cpp
2331

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 added inline comments.Feb 15 2021, 2:43 PM
clang/lib/Sema/Sema.cpp
2331

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 added inline comments.Feb 15 2021, 10:57 PM
clang/lib/Sema/Sema.cpp
2331

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 added inline comments.Feb 16 2021, 12:37 PM
clang/lib/Sema/Sema.cpp
2331

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 updated this revision to Diff 325158.Feb 19 2021, 10:20 PM

Rename noteNumOverloadCandidatesShown -> overloadCandidatesShown.

I guess I was confused by the function name

Haha, two hardest problems in computer science. :)

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

aaronpuchert accepted this revision.Feb 20 2021, 5:05 AM

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

This revision is now accepted and ready to land.Feb 20 2021, 5:05 AM

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