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

[Richard Smith]

================
Comment at: include/clang/Basic/SourceLocation.h:217-218
@@ +216,4 @@
+
+  /// \brief Indicates whether
+  /// `getBegin() <= other.getBegin() && other.getEnd() <= getEnd()`.
+  bool contains(SourceRange other) const {
----------------
Stephan Tolksdorf wrote:
> Richard Smith wrote:
> > Rather than essentially duplicating the implementation in the body, how about an English description, "Indicates whether another source range is the same as, or contained within, this range."
> > 
> > Also, the implementation doesn't appear to be correct; `<` on `SourceLocation`s does not in general give source order (though it happens to if the locations are within the same `FileID`). Use `SourceManager::isBeforeInTranslationUnit` for this check (and it shouldn't be a member of `SourceRange` to avoid layering problems).
> > 
> > Maybe this should be a `SourceManager` member function?
> I used the short technical definition in the description because I wasn't sure how to describe the behaviour verbally in a terse and accurate way.
> 
> I didn't use `isBeforeInTranslationUnit` for the implementation because I wanted to test whether one source range is visually contained by the other in the original source file. However, I just tested the behaviour for macro expansions and noticed that clang does print both the spelling and the expansion location for macros, so using `isBeforeInTranslationUnit` indeed seems preferable here.
> 
> Do you maybe have a suggestion for the name of the new method in `SourceManager`? Maybe `rangeContains`?
> 
> 
How about `SourceManager::isWithinInTranslationUnit(SourceRange, SourceLocation)`?

================
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
----------------
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.)

================
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();
----------------
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.


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