Improve error reporting for SFINAE

[pfultz2]

Summary

This is to help improve error reporting with SFINAE as part of the bug report here:

http://llvm.org/bugs/show_bug.cgi?id=13309

So now when this program compiles:

template<class T> auto h(T x)->decltype(x.smurf()){return x.smurf();}
void h() {}
template<class T> auto g(T x)->decltype(h(x)){return h(x);}
template<class T> auto f(T x)->decltype(g(x)){return g(x);}
int main(){
  f(3);
}

Clang will report back this error with the true diagnostics:

smurf.cpp:5:3: error: no matching function for call to 'f'
  f(3);
  ^
smurf.cpp:1:42: note: candidate template ignored: substitution failure [with T = int]: member reference base type 'int' is not a structure or union
template<class T> auto h(T x)->decltype(x.smurf()){return x.smurf();}
                                         ^

Now this only does this when there is single overloads involved. If we change
the example to add an extra overload for h, like this:

template<class T> auto h(T x)->decltype(x.smurf()){return x.smurf();}
void h() {}
template<class T> auto g(T x)->decltype(h(x)){return h(x);}
template<class T> auto f(T x)->decltype(g(x)){return g(x);}
int main(){
  f(3);
}

Then we get an error like this instead:

smurf.cpp:6:3: error: no matching function for call to 'f'
  f(3);
  ^
smurf.cpp:3:41: note: candidate template ignored: substitution failure [with T = int]: no matching function for call to 'h'
template<class T> auto g(T x)->decltype(h(x)){return h(x);}
                                        ^

So clang basically stops when there is multiple overloads. If the user needs
to see more detail for this, we could look at adding an option for a full
backtrace in the future.

Either way, I think this is very useful to have, and improves the error
reporting in clang immensely. I would like to get this patch merged into
clang.

[majnemer]

This needs a testcase added to the test/ directory.

[pfultz2]

Added tests

[rsmith]

As noted on PR13309, this is not acceptable as-is -- note that the sample diagnostic:

smurf.cpp:6:3: error: no matching function for call to 'f'
  f(3);
  ^
smurf.cpp:3:41: note: candidate template ignored: substitution failure [with T = int]: no matching function for call to 'h'
template<class T> auto g(T x)->decltype(h(x)){return h(x);}

... gives the user no idea of how we got from a call to f into a call to g. If we produced a stack of 'in instantiation of' notes, this would be fine (note that's exactly what my patch in comment#1/comment#2 of that bug does).

[pfultz2]

gives the user no idea of how we got from a call to f into a call to g.

For the default case, its better to show the first and last in the trace, as most users don't care about the intermediate steps.

If we produced a stack of 'in instantiation of' notes, this would be fine

Yes, the full back trace can be added in the future(perhaps as a compiler flag). This is the first step towards that direction. There needs to be some refactoring to DeductionFailureInfo so it stores the entire diagnostics(rather than a single diagnostic) to make a full back trace possible.

note that's exactly what my patch in comment#1/comment#2 of that bug does

That seemed to be more of a hack then something to be used in production.

[pfultz2]

The difference is, the current diagnostics allow the user to figure out what
happened. This change does not -- there's no way to find out which 'f' caused
the problem, and how we got from 'f' to 'g'.

Yes, the user can still figure out how you got from f to g. The user can
either add an extra overload for g or comment out g(and continue so until
the user gets to f). So instead of working forwards from f to g, the
user just works backwards from g to f. However, its less likely a user
would need to do that since 90% of the time users want to get to g and not
to f.

Paul

include/clang/Sema/TemplateDeduction.h

TemplateArgumentList *take() {
     TemplateArgumentList *Result = Deduced;
     Deduced = nullptr;
     return Result;
   }
 
   /// \brief Take ownership of the SFINAE diagnostic.
   void takeSFINAEDiagnostic(PartialDiagnosticAt &PD) {
     assert(HasSFINAEDiagnostic);
-    PD.first = SuppressedDiagnostics.front().first;
-    PD.second.swap(SuppressedDiagnostics.front().second);
+    unsigned NumberOfDiagnostics = SuppressedDiagnostics.size();
+    PartialDiagnosticAt &PDiag = SuppressedDiagnostics[((NumberOfDiagnostics == 2) ? 1 : 0)];
+    PD.first = PDiag.first;
+    PD.second.swap(PDiag.second);
     SuppressedDiagnostics.clear();
     HasSFINAEDiagnostic = false;
   }
 
   /// \brief Provide a new template argument list that contains the
   /// results of template argument deduction.
   void reset(TemplateArgumentList *NewDeduced) {
     Deduced = NewDeduced;
void addSFINAEDiagnostic(SourceLocation Loc, PartialDiagnostic PD) {
     SuppressedDiagnostics.clear();
     SuppressedDiagnostics.emplace_back(Loc, std::move(PD));
     HasSFINAEDiagnostic = true;
   }
 
   /// \brief Add a new diagnostic to the set of diagnostics
   void addSuppressedDiagnostic(SourceLocation Loc,
                                PartialDiagnostic PD) {
-    if (HasSFINAEDiagnostic)
-      return;
     SuppressedDiagnostics.emplace_back(Loc, std::move(PD));
   }
 
   /// \brief Iterator over the set of suppressed diagnostics.
   typedef SmallVectorImpl<PartialDiagnosticAt>::const_iterator
     diag_iterator;
 
   /// \brief Returns an iterator at the beginning of the sequence of suppressed

lib/Sema/SemaOverload.cpp

if (PDiag && PDiag->second.getDiagID() ==
           diag::err_typename_nested_not_found_enable_if) {
       // FIXME: Use the source range of the condition, and the fully-qualified
       //        name of the enable_if template. These are both present in PDiag.
       S.Diag(PDiag->first, diag::note_ovl_candidate_disabled_by_enable_if)
         << "'enable_if'" << TemplateArgString;
       return;
     }
 
+    // If the diagnsotics is already for overload candidate substitution
+    // failure, then just emit the diagnostics as is.
+    if (PDiag && (PDiag->second.getDiagID() ==
+                      diag::note_ovl_candidate_substitution_failure ||
+                  PDiag->second.getDiagID() ==
+                      diag::note_ovl_candidate_disabled_by_enable_if)) {
+      S.Diag(PDiag->first, PDiag->second);
+      return;
+    }
+
     // Format the SFINAE diagnostic into the argument string.
     // FIXME: Add a general mechanism to include a PartialDiagnostic *'s
     //        formatted message in another diagnostic.
     SmallString<128> SFINAEArgString;
     SourceRange R;
     if (PDiag) {
       SFINAEArgString = ": ";
       R = SourceRange(PDiag->first, PDiag->first);
       PDiag->second.EmitToString(S.getDiagnostics(), SFINAEArgString);
     }
 
-    S.Diag(Templated->getLocation(),
+    S.Diag(PDiag->first,
            diag::note_ovl_candidate_substitution_failure)
         << TemplateArgString << SFINAEArgString << R;
     MaybeEmitInheritedConstructorNote(S, Templated);
     return;
   }
 
   case Sema::TDK_FailedOverloadResolution: {
     OverloadExpr::FindResult R = OverloadExpr::find(DeductionFailure.getExpr());