[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 {
----------------
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?

================
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;
----------------
This seems a bit arbitrary. Could you add a `TemplateArgumentListInfo *` instead? (Maybe make `TemplateArgs` above a `PointerUnion`?)

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

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

================
Comment at: lib/Sema/SemaTemplate.cpp:7918
@@ +7917,3 @@
+        }
+        D << AliasDecl << CondRange;
+      }
----------------
Please also change the literal `'enable_if'` in `err_typename_nested_not_found_enable_if` to a `%1`.


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