[clang] Deprecation warnings in system headers are silenced when the deprecated entity is used indirectly

[ldionne]

A while back, we marked std::char_traits<not-a-character-type> as deprecated (see #72694 and the related history). We then removed it in LLVM 19 after thinking we had provided users with a reasonable deprecation period of 6 months for such a small thing. And that was the case, with a major caveat.

Indeed, Clang <= 17 failed to surface deprecation warnings for things in system headers, so that the following would actually not issue any diagnostic:

#include <string> // assume char_traits<long> is deprecated in that header

int main() {
  std::char_traits<long> traits;
}

That was fixed in Clang 18 in #70353 (thanks to @cor3ntin!). In Clang 18 and above, the code above does warn. Based on that, we went ahead and removed std::char_traits<not-a-character-type> in LLVM 19, thinking we had provided one release of deprecation. However, the following code never warned and still doesn't warn:

#include <string> // assume char_traits<long> is deprecated in that header

int main() {
  std::basic_string<long> s;
}

Even though basic_string<long> instantiates char_traits<long>, I guess the issue is that the instantiation of char_traits<long> itself is happening from a system header, not user code, and as a result we don't warn on it.

IMO, that's broken. That means that we effectively shipped #72694 without giving any advance notice to people because nobody instantiates char_traits directly, everybody instantiates basic_string and basic_string_view. When I did my sanity testing before landing #72694, I must have used char_traits (because that's what we were deprecating and that's what didn't work before @cor3ntin 's fix), but that wasn't enough.

I'm not sure how to best fix that, but at the moment we seem to basically not have a good story for deprecating things located in system headers. I'm going to throw a few potential solutions out there:

• Actually allow full-on deprecation warnings in system headers -- don't attempt to suppress those. I'm not certain whether that's reasonable, but I know that libc++ makes sure not to use deprecated APIs (we run our test suite by including headers as not system headers to see everything, and we fix it). So I think that would be realistic as far as libc++ is concerned.
• Perhaps we can have a pragma that allows headers to change Clang's behavior diagnostics-wise? It always felt like a big hammer to disallow any and all warnings coming from system headers without giving them the choice to decide for themselves.
• Add another attribute that allows producing deprecation warnings from system headers no matter where the point of use came from. If we're OK with doing that, though, then I'm not certain why we wouldn't just allow all deprecation warnings in system headers.
• I'd also be willing to consider that perhaps the root cause of the problem is not that Clang didn't surface the deprecation warning, but that libc++ didn't mark the right APIs as deprecated. For example, one could argue that libc++ should have marked basic_string<long>, basic_string_view<long>, basic_istream<long> and the other 150 APIs that can end up instantiating char_traits<long> as deprecated. I think that would be really challenging and error prone, but I could imagine the argument being made. If that's the direction we want to take, then we'd need a way to easily mark specific specializations as deprecated. Today the only way to do this would be to provide an explicit instantiation of all the types that we want to deprecate (e.g. basic_istream<long>, basic_istream<unsigned char>, etc.) and duplicate their implementation while marking them as deprecated. So if we think that's philosophically the right way to go, we should try to find a way that makes it technically feasible.

I'm eager to see what other folks think!

CC @AaronBallman for awareness

[AaronBallman]

CC @erichkeane as well

[erichkeane]

This is a bit of a mess... We typically just suppress ALL warnings inside of system headers, so that you guys don't break folks with things they can't fix. This is obviously an exception to this for obvious reasons (where you wanted it to be shown).

Your last bullet sounds onerous/too much work to be valuable. But none of the others really solve the problem without harming others.

We could special case deprecated perhaps, but that seems awkward, as you could then never use 'deprecated' for internal APIs.

The pragma idea causing a 'special' verison of deprecated (vs a special version of deprecated) to be marked up isn't that bad, it could mark the DeprecatedAttr node to 'always' diagnose, but that requires some additional 'specialiness' in our diagnostics engine.

I'll have to think about this.. this is funky.

[firewave]

Not sure how revelant it is but GCC has -Wsystem-headers.

[ravikandhadai]

I wonder if there is a way in Clang to track whether an instantiation is trigged by user code or if it happens within a system header. E.g. whether std::char_traits<long> is instantiated within libcxx or is happening because someone did std::basic_string<long> and which intern caused std::char_traits<long> to happen. If so, that can be used to decided whether to surface a warning or suppress it.

[erichkeane]

I wonder if there is a way in Clang to track whether an instantiation is trigged by user code or if it happens within a system header. E.g. whether std::char_traits<long> is instantiated within libcxx or is happening because someone did std::basic_string<long> and which intern caused std::char_traits<long> to happen. If so, that can be used to decided whether to surface a warning or suppress it.

We don't necessarily have that because the instantiation might be caused by different code, and now just being re-referenced, so it would only be triggered on the FIRST instantiation (though, if none of the headers DO this referencing, we could perhaps diagnose).

That said, I suspect looking back to the whole template stack to figure out if we should diagnose could very well be a very unfortunate behavior, as a lot of 'deprecated, but only for internal use' kinda diagnostics would end up firing everywhere (or even, diagnostics you don't want happening in the header, like the REST of our warnings).

[ahatanak]

Can we mark the forward declaration of char_traits in libcxx/include/__fwd/string.h as deprecated too? If I do that, clang warns that char_traits<long> is deprecated.

It looks like clang tries to see where the template instantiation originated from in DoEmitAvailabilityWarning and emits a warning if it originated in a non-system header.

  // 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);

[ahatanak]

This reproduces the problem:

$ cat fw.h

#pragma clang system_header
template <class charT>
struct
#ifdef DEPRECATE1
__attribute__((deprecated)) char_traits;
#endif
char_traits;

template<class charT, class traits = char_traits<charT>>
class basic_string;

$ cat char_traits.h

#pragma clang system_header
template <class _CharT>
struct char_traits;

template <class charT>
struct __attribute__((deprecated)) char_traits
{
};

$ cat string.h

#pragma clang system_header
#include "fw.h"
#include "char_traits.h"

template<class charT, class traits>
class basic_string {
  typedef traits traits_type;
};

$ cat string.cpp

#include "string.h"

void f() {
  basic_string<long> bs;
}

$ clang++ -std=c++20 string.cpp -c
(no warnings)
$ clang++ -std=c++20 string.cpp -c -DDEPRECATE1
In file included from string.cpp:1:
In file included from ./string.h:2:
./fw.h:9:38: warning: 'char_traits' is deprecated [-Wdeprecated-declarations]
9 | template<class charT, class traits = char_traits>
| ^
string.cpp:4:3: note: in instantiation of default argument for 'basic_string' required here
4 | basic_string bs;
| ^~~~~~~~~~~~~~~~~~
./fw.h:5:16: note: 'char_traits' has been explicitly marked deprecated here
5 | attribute((deprecated)) char_traits;
| ^
1 warning generated.