[PATCH] Fix PR18792 - Bad diagnostic messages for std::enable_if_t uses

[Stephan Tolksdorf]

================
Comment at: lib/Sema/SemaOverload.cpp:8905-8918
@@ +8904,16 @@
+      // a relevant part of the alias template instantiation.
+      assert(PD.getNumArgs() >= 1 && PDRanges.size() >= 1 &&
+             PD.getArgKind(0) == DiagnosticsEngine::ak_declcontext &&
+             (PD.getNumArgs() == 1 ||
+              (PD.getArgKind(1) == DiagnosticsEngine::ak_nameddecl &&
+               PDRanges.size() >= 2)) &&
+             "an err_typename_nested_not_found_enable_if diagnostic does"
+             " not contain the expected arguments and source ranges");
+      const bool IsAlias = PDRanges.size() > 1;
+      const NamedDecl *EnableIfOrAliasDecl =
+          IsAlias ? reinterpret_cast<NamedDecl *>(PD.getRawArg(1))
+                  : cast<ClassTemplateSpecializationDecl>(
+                        reinterpret_cast<DeclContext *>(PD.getRawArg(0)))
+                        ->getSpecializedTemplate();
+      SourceRange Range = PDRanges[IsAlias ? 1 : 0].getAsRange();
+      // If an alias template that contains an enable_if type lookup is
----------------
Richard Smith wrote:
> Stephan Tolksdorf wrote:
> > Richard Smith wrote:
> > > I don't like inspecting the source ranges on a `PartialDiagnostic` -- they're not part of the numbered argument set for the diagnostic, so someone emitting the diagnostic is within their rights to change the set of ranges they provide. More generally, I'm not really a fan of digging into the implementation of the `PartialDiagnostic` to find its arguments.
> > > 
> > > Instead, how about switching the diagnostic ID from err_typename_nested_not_found_enable_if to note_ovl_candidate_disabled_by_enable_if (with comments in the .td file saying that they're required to take the same arguments)?
> > > 
> > > Instead of using the `Range`, below, to determine the location for the diagnostic, you could use something like `TemplDecl->getSourceRange().contains(PDiag->first) ? PDiag->first : Templated->getLocation()`.
> > In my view a `PartialDiagnostic` (or `Diagnostic`) is not just the input to some formatting routine but a strongly typed container of an error id and additional pieces of information about the error. So, accessing the arguments to the diagnostic through a proper API (introduced by D3060) in order to generate a better error message doesn't seem that objectionable to me. Also, I interpreted your FIXME as a request to do exactly this.
> > 
> > I used a second source range and the `err_typename_nested_not_found_enable_if` id because I assumed that in `Sema::CheckTypenameType` we should always generate an error that would be appropriate if the error message is not suppressed by the SFINAE logic. If this is not the case, it might be cleaner to always prepare the enable_if diagnostic in `CheckTypenameType` and then just reemit it in `DiagnoseBadDeduction` (adding the missing `TemplateArgString`).
> > 
> > If you'd like this code to not fail in the hypothetical case where someone generates a `err_typename_nested_not_found_enable_if` diagnostic with unexpected contents, I could replace the assert with some always-on fallback logic.
> > 
> > 
> I don't think a `PartialDiagnostic` is even notionally strongly typed (you can use the same diagnostic with a string or a Decl for the same argument, for instance, and we do that sort of thing in some cases). The approach you're taking is fragile in the presence of refactoring or people reusing the same diagnostic message in other circumstances (where they may not provide the same SourceRanges). I'm also not particularly enthusiastic about a fallback codepath -- I'd much rather have a clean way to pass the necessary information through. I'm not yet sure what that mechanism is, though.
> 
> (Yes, the diagnostic produced in `CheckTypenameType` should be appropriate in the case where we're not in a SFINAE condition.)
What I meant with strongly typed is that you can access the diagnostic arguments in a type safe way, as the types of the arguments are recorded and can be queried. Documenting the extra arguments in the diagnostic .td file should minimize any fragility.

If I want to pass on additional information about an error, putting it into the diagnostic data structure still seems rather natural to me. In this case the only alternative I can think of is to put this as an additional field into the `TemplateDeductionInfo`, but I don't think that would be cleaner.






================
Comment at: lib/Sema/SemaTemplate.cpp:7887-7919
@@ -7844,1 +7886,35 @@
+               << Ctx << CondRange;
+      if (const Sema::ActiveTemplateInstantiation *ATI =
+              findOutermostAliasTemplateInstantiationInSFINAEContext(*this)) {
+        // For a failed enable_if lookup within an alias template instantiation
+        // we also store the source range of the instantiation and a pointer to
+        // the TypeAliasTemplateDecl in the PartialDiagnostic.
+        // This allows us is Sema::DiagnoseBadDeduction to highlight a source
+        // range that is actually *within* the declaration of the disabled
+        // overload (and not in the declaration of the alias template).
+        // We don't restrict this handling to alias templates named enable_if_t
+        // because if a failed 'typename enable_if<...>::type' lookup occurs
+        // within an alias template instantiation in a SFINAE context, it is
+        // highly likely that the alias is used similarly to std::enable_if_t.
+        auto *AliasDecl = cast<TypeAliasTemplateDecl>(ATI->Entity);
+        // Sema::CheckTemplateIdType stores the source range for the first
+        // template argument in ATI->InstantiationRange.
+        SourceRange Arg0Range = ATI->InstantiationRange;
+        auto *EnableIfII = AliasDecl->getDeclName().getAsIdentifierInfo();
+        TemplateParameterList *TPL = AliasDecl->getTemplateParameters();
+        if (Arg0Range.isInvalid() || TPL->size() == 0)
+          CondRange = ATI->PointOfInstantiation;
+        else if (TPL->size() == 1 || EnableIfII->isStr("enable_if_t"))
+          CondRange = Arg0Range;
+        else {
+          // If the alias template has more than one parameter and is not
+          // called enable_if_t, we can't be sure that the first argument is
+          // actually the one that caused the error. So instead of underlining
+          // the full argument, we only point to the beginning of it, which
+          // should avoid misunderstandings while still pointing (close) to the
+          // origin of the error.
+          CondRange = Arg0Range.getBegin();
+        }
+        D << AliasDecl << CondRange;
+      }
       return QualType();
----------------
Richard Smith wrote:
> Stephan Tolksdorf wrote:
> > Richard Smith wrote:
> > > Having reflected on this a bit more, I think we should *only* do this if the context surrounding the `enable_if` is an alias template named `enable_if_t` (and we should only walk through one such alias template). If we walk too far away from the `enable_if` expression itself, it will no longer be obvious what we're complaining about.
> > If we don't walk up all alias template instantiations in the current SFINAE context on the instantiation stack, the SFINAE diagnostic will only state ##no type named 'type' in 'enable_if<false>'## or ##disabled by 'enable_if'## without any indication which alias template instantiation caused this error, since the SFINAE diagnostic doesn't contain the stack of instantiated alias templates for the suppressed error. (Would it maybe be an alternative to store and print this stack when we don't have a straightforward enable_if case?)
> > 
> > In my opinion such a diagnostic is significantly worse than a diagnostic that states something like e.g. ##disabled by 'enable_if_is_callback'## or ##disabled by an error during the substitution of the alias template 'enable_if_is_callback'## and that points to the source location of the `enable_if_is_callback` instantiation in the function declaration triggering the error.
> > 
> > Could you maybe give an example for a situation where you'd find the proposed diagnostic confusing? Would rewording the diagnostic in that situation be an alternative to falling back to the basic SFINAE diagnostic?
> > 
> > 
> Hmm, suppose I have:
> 
>   template<typename T> void f(Iterator<T> iter);
> 
> with `Iterator` an alias template. If substitution fails for any reason within `Iterator<T>`, I don't want to be told `disabled by 'Iterator'`. I think in this case I actually want to produce two notes for the substitution failure: one indicating that the candidate was ignored due to a substitution failure (pointing at the candidate), and another pointing at the location within the alias template where we hit the problem.
> 
> I think we can do this without any alias template special-casing: when we come to emit a delayed 'substitution failure' diagnostic, check whether the location of the diagnostic is within the source range of the declaration of the candidate. If so, do what we do today, and if not, produce two notes (one pointing at the candidate and the other pointing at the location where we hit the error.
It would be really nice if we could tell the user that the substitution failure occurred within `Iterator<T>`. In this case there is no other possible place, but in more complicated situations it would make the error easier to understand, especially if the error occurred not directly within the `Iterator<T>` declaration but inside a nested alias template use. When you work with with an IDE that highlights the source ranges in an editor, this can be a real improvement. (And, of course, we can use a different wording than "disabled by xxx" for this and similar cases.)

Without any alias template special-casing the location of the original diagnostic for an error inside an alias template instantiation will never be within the source range of the declaration of the candidate and `enable_if_t` uses will generate arguably worse error messages than `enable_if` uses.






http://llvm-reviews.chandlerc.com/D3061