[clang] [clang] Fix deprecation attribute being ignored when used inside other attributes that are already applied to a deprecated symbol (PR #222094)

[Erich Keane via cfe-commits]

erichkeane wrote:

> I'm surprised to hear this. It absolutely looks like a bug to me -- the `deprecated` attribute applies to the symbol(s) being declared (`f` in that example), not to everything inside that declaration, and especially not to symbols used inside attributes in the declaration. They are completely orthogonal. For example, if we have

Except the enable-if here is actually PART of that `f` declaration.  So this looks 'by design'.

> 
> ```
> int foo();
> __attribute__((deprecated)) decltype(foo()) bar();
> ```
> 
> then you wouldn't say `foo` is deprecated, right? Or if we have

No?  But it isn't part of the declaration being deprecated.

> 
> ```
> namespace ns {
> int foo(const void*);
> 
> struct S {
>     __attribute__((deprecated))
>     friend char foo(S*);
> };
> 
> template<class T>
> __attribute__((deprecated))
> auto bar(T& x) -> decltype(foo(&x));
> }
> 
> int main() {
>     ns::S s;
>     ns::bar(s);
> }
> ```
> 
> then surely users should observe a difference between `foo` being deprecated vs. not-deprecated?

I'm not sure I get the point of this example.  I wouldn't be surprised if we do a bad job merging those, but the reference to `foo` is a part of the `bar` declaration, right?  Or is trailing return type in the decl-context of the parent? 

> 
> I also don't understand the motivation: what is the logic for _not_ telling users a symbol that they're using is deprecated? How are they supposed to migrate off the symbol if they don't know it's deprecated...?

It isn't clear to me either, but it appears to be by-design: We are walking up the declaration contexts, and suppressing diagnostics if they are in a deprecated context.  Since attributes are in the `context` of the declaration they are on, it would make sense to suppress them.  That said, if we want to step back and figure out what `deprecated` means /when we suppress it, we should step back and do so, not just "fix" 1 component to it, making it less consistent.

> 
> P.S., there is apparently also an inconsistency here where removing `namespace { }` actually _does_ cause this to fire! So even the intended behavior seems to have a bug in its implementation, though I'm arguing the intended behavior itself is buggy.

I'm not surprised that there are bugs here.  I haven't seen this code in a long time...

https://github.com/llvm/llvm-project/pull/222094