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

[Stephan Tolksdorf]

  Thanks for the review!


================
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 {
----------------
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`?



================
Comment at: include/clang/Sema/Sema.h:6055-6056
@@ -6054,1 +6054,4 @@
+    ///
+    /// For alias template instantiations this member is used to store the
+    /// source range of the first template argument.
     SourceRange InstantiationRange;
----------------
Richard Smith wrote:
> This seems a bit arbitrary. Could you add a `TemplateArgumentListInfo *` instead? (Maybe make `TemplateArgs` above a `PointerUnion`?)
Using the `InstantiationRange` for storing the source range seemed like the minimally invasive way to make the source information for the enable_if diagnostic available.

I could store a `TemplateArgumentListInfo` pointer instead, but that would also require a new `InstantiatingTemplate` constructor.


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



================
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:
> 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?




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