[Clang] Warn on deprecated specializations used in system headers.

[cor3ntin]

When the top of the instantiation stack is in user code.

The goal of this PR is to allow deprecation of some char_traits specializations in libc++ as done in https://reviews.llvm.org/D157058 which was later reverted by
#66153 (comment) as Clang never emitted the libc++ warnings.

Because Clang likes to eagerly instantiate, we can look for the location of the top of the instantiation stack, and emit a warning if that location is in user code.

The warning emission is forced by temporarily instructing the diag engine not to silence warning in system headers.

[cor3ntin]

@ldionne @mordante Ping for visibility :)

[llvmbot]

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

When the top of the instantiation stack is in user code.

The goal of this PR is to allow deprecation of some char_traits specializations in libc++ as done in https://reviews.llvm.org/D157058 which was later reverted by
#66153 (comment) as Clang never emitted the libc++ warnings.

Because Clang likes to eagerly instantiate, we can look for the location of the top of the instantiation stack, and emit a warning if that location is in user code.

The warning emission is forced by temporarily instructing the diag engine not to silence warning in system headers.

---

Full diff: https://github.com/llvm/llvm-project/pull/70353.diff

5 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/include/clang/Sema/Sema.h (+2)
• (modified) clang/lib/Sema/SemaAvailability.cpp (+23)
• (modified) clang/lib/Sema/SemaTemplate.cpp (+22)
• (added) clang/test/SemaCXX/warn-deprecated-specializations-in-system-headers.cpp (+28)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7238386231e1a28..0c46e0a5912905e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -404,6 +404,8 @@ Improvements to Clang's diagnostics
 - ``-Wzero-as-null-pointer-constant`` diagnostic is no longer emitted when using ``__null``
   (or, more commonly, ``NULL`` when the platform defines it as ``__null``) to be more consistent
   with GCC.
+- Clang will warn on deprecated specializations used in system headers when their instantiation
+  is caused by user code.
 
 Bug Fixes in This Version
 -------------------------
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 18ac85011aa752a..1a75fff331add5a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -8476,6 +8476,8 @@ class Sema final {
       ArrayRef<TemplateArgument> SugaredConverted,
       ArrayRef<TemplateArgument> CanonicalConverted, bool &HasDefaultArg);
 
+  SourceLocation getTopMostPointOfInstantiation(const NamedDecl *) const;
+
   /// Specifies the context in which a particular template
   /// argument is being checked.
   enum CheckTemplateArgumentKind {
diff --git a/clang/lib/Sema/SemaAvailability.cpp b/clang/lib/Sema/SemaAvailability.cpp
index 84c06566387ccbe..846a31a79673096 100644
--- a/clang/lib/Sema/SemaAvailability.cpp
+++ b/clang/lib/Sema/SemaAvailability.cpp
@@ -536,6 +536,29 @@ static void DoEmitAvailabilityWarning(Sema &S, AvailabilityResult K,
     }
   }
 
+  // We emit deprecation warning for deprecated specializations
+  // when their instantiation stacks originate outside
+  // of a system header, even if the diagnostics is suppresed at the
+  // point of definition.
+  SourceLocation InstantiationLoc =
+      S.getTopMostPointOfInstantiation(ReferringDecl);
+  bool ShouldAllowWarningInSystemHeader =
+      InstantiationLoc != Loc &&
+      !S.getSourceManager().isInSystemHeader(InstantiationLoc);
+  struct AllowWarningInSystemHeaders {
+    AllowWarningInSystemHeaders(DiagnosticsEngine &E,
+                                bool AllowWarningInSystemHeaders)
+        : Engine(E), Prev(E.getSuppressSystemWarnings()) {
+      E.setSuppressSystemWarnings(!AllowWarningInSystemHeaders);
+    }
+    ~AllowWarningInSystemHeaders() { Engine.setSuppressSystemWarnings(Prev); }
+
+  private:
+    DiagnosticsEngine &Engine;
+    bool Prev;
+  } SystemWarningOverrideRAII(S.getDiagnostics(),
+                              ShouldAllowWarningInSystemHeader);
+
   if (!Message.empty()) {
     S.Diag(Loc, diag_message) << ReferringDecl << Message << FixIts;
     if (ObjCProperty)
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index c2477ec0063e418..61d034e13b0a292 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -11615,3 +11615,25 @@ void Sema::checkSpecializationReachability(SourceLocation Loc,
                                           Sema::AcceptableKind::Reachable)
       .check(Spec);
 }
+
+/// Returns the top most location responsible for the definition of \p N.
+/// If \p N is a a template specialization, this is the location
+/// of the top of the instantiation stack.
+/// Otherwise, the location of \p N is returned.
+SourceLocation Sema::getTopMostPointOfInstantiation(const NamedDecl *N) const {
+  if (!getLangOpts().CPlusPlus || CodeSynthesisContexts.empty())
+    return N->getLocation();
+  if (auto *FD = dyn_cast<FunctionDecl>(N)) {
+    if (!FD->isFunctionTemplateSpecialization())
+      return FD->getLocation();
+  } else if (!isa<ClassTemplateSpecializationDecl,
+                  VarTemplateSpecializationDecl>(N)) {
+    return N->getLocation();
+  }
+  for (const CodeSynthesisContext &CSC : CodeSynthesisContexts) {
+    if (!CSC.isInstantiationRecord() || CSC.PointOfInstantiation.isInvalid())
+      continue;
+    return CSC.PointOfInstantiation;
+  }
+  return N->getLocation();
+}
diff --git a/clang/test/SemaCXX/warn-deprecated-specializations-in-system-headers.cpp b/clang/test/SemaCXX/warn-deprecated-specializations-in-system-headers.cpp
new file mode 100644
index 000000000000000..270e4292bf9a47b
--- /dev/null
+++ b/clang/test/SemaCXX/warn-deprecated-specializations-in-system-headers.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#ifdef BE_THE_HEADER
+#pragma clang system_header
+
+template <typename T>
+struct traits;
+
+template <>
+struct [[ deprecated]] traits<int> {}; // expected-note {{'traits<int>' has been explicitly marked deprecated here}}
+
+template<typename T, typename Trait = traits<T>>  // expected-warning {{'traits<int>' is deprecated}}
+struct basic_string {};
+
+// should not warn, defined and used in system headers
+using __do_what_i_say_not_what_i_do  = traits<int> ;
+
+template<typename T, typename Trait = traits<double>>
+struct should_not_warn {};
+
+#else
+#define BE_THE_HEADER
+#include __FILE__
+
+basic_string<int> test1; // expected-note {{in instantiation of default argument for 'basic_string<int>' required here}}
+should_not_warn<int> test2;
+
+#endif

[tahonermann]

I like this very much so long as it doesn't turn out to be too noisy!

[ldionne]

@cor3ntin This is really nice, thanks for the patch! The CI should pass if you merge main into your branch -- you were the victim of a temporary CI issue that has been fixed since then.

[AaronBallman]

LGTM!

[AaronBallman]

Suggested change

	if (auto *FD = dyn_cast<FunctionDecl>(N)) {
	if (const auto *FD = dyn_cast<FunctionDecl>(N)) {

[AaronBallman]

Suggested change

	struct [[ deprecated]] traits<int> {}; // expected-note {{'traits<int>' has been explicitly marked deprecated here}}
	struct [[deprecated]] traits<int> {}; // expected-note {{'traits<int>' has been explicitly marked deprecated here}}

[kamaub]

This patch exposes the use of deprecated specializations in the third-party Google Test suite built via this buildbot https://lab.llvm.org/staging/#/builders/104/builds/106/steps/6/logs/stdio. The bot ordinality isn't in staging, it is there temporary while we test and update its configuration and intend to move it back to the main server once it builds libcxx and libcxxabi cleanly.

Can you provide any insight on how to resolve these warning? Would specifying-Wno-deprecated-declarations specified in the gtest CMakeList.txt appropriate to temporarily handle them until the third-party unittest can be updated?

[ldionne]

I'm not familiar with your exact setup, but I think you should probably report those warnings to Google Test and turn off deprecation warnings in your build with -Wno-deprecated until you're able to update to a version of Google Test that has fixed this issue?