Page MenuHomePhabricator

[Sema] Improve diagnostics for template arg deduction
Needs ReviewPublic

Authored by jtbandes on Nov 20 2017, 11:57 PM.

Details

Reviewers
rsmith
Summary

I noticed a relatively simple program produced a bad error message:

template<size_t N>
void foo(double (&arr)[N]) {}

void bar(double (&xs)[]) {
    foo(xs);
}
error: no matching function for call to 'foo'
    foo(xs);
    ^~~
note: candidate template ignored: could not match 'double' against 'double'
void foo(double (&arr)[N]) {}
     ^

This seems to happen because DeduceTemplateArgumentsByTypeMatch will sometimes do this

Info.FirstArg = TemplateArgument(ParamIn);
Info.SecondArg = TemplateArgument(ArgIn);

before it determines whether there's an error or not. Since the function is called recursively with the same Info, the FirstArg and SecondArg that actually cause the failure get overwritten.

My solution here is not very clean, although I tried to be minimally invasive; I'm open to suggestions for a better way to organize this. I considered llvm::SaveAndRestore but it seems to be unconditional and couldn't be "canceled" in the case of a real failure return value. I'm not familiar with any other such utilities, but maybe there is one that can make this nicer?

Diff Detail

Event Timeline

jtbandes created this revision.Nov 20 2017, 11:57 PM
jtbandes added a subscriber: rsmith.

Adding @rsmith for review based on the commit history of SemaTemplateDeduction.cpp :)

This looks correct, but I definitely agree that RAII would make this a lot nicer. Have you considered adding a CancelableSaveAndRestore or something to SaveAndRestore.h? It seems useful and generic enough to make it worthwhile. Otherwise, you could just write your own RAII object special-cased to handle this. A less intrusive way of doing this might be to wrap calls to this function with another that checks if the return value is TDK_Success, and if so restores these fields.

lib/Sema/SemaTemplateDeduction.cpp
1384

What if this return TDK_Success?

jtbandes updated this revision to Diff 124033.Nov 22 2017, 9:33 PM

@erik.pilkington Updated to use a wrapper function. This is definitely less invasive, but it could defeat some optimizations (any approach that checks the return value will defeat tail recursion, I suppose...)

I'm not confident enough about the effects of this patch with only one test; the last patch also passes the test. I hope I can understand the effects better.

I would prefer that we make the more-invasive fix, and make each error case within template argument deduction set all the deduction failure information. (Consider factoring out the error setup into helper functions too.) The current approach is a maintenance problem in addition to creating incorrect diagnostics, as it makes it very hard to see what parameters are actually intended to be provided with each of the different forms of deduction failure.

jtbandes planned changes to this revision.Jan 3 2018, 10:38 PM
jtbandes updated this revision to Diff 133440.Feb 8 2018, 9:37 AM

Using a slightly more invasive fix. I haven't come up with any other test cases that exhibit the problem, which makes me unsure this fix is needed in all these locations. Maybe someone with more knowledge of this function can advise.

jtbandes marked an inline comment as done.Feb 8 2018, 9:39 AM

Please take another look when you get a chance :)