Compiler no longer accepting certain form of deprecated attribute after commit 8c7b64b5ae2a

[dyung]

One of our internal tests started giving a compile error on a deprecated attribute lately and I bisected it back to commit 8c7b64b. Consider the following code:

[[deprecated( "use bar instead" )]]
extern "C" int foo();

When compiled using a compiler built prior to 8c7b64b, this code would compile cleanly. However, once we try a compiler built that includes 8c7b64b, we get an error:

$ ~/src/upstream/8c7b64b5ae2a-linux/bin/clang -c test.cpp
test.cpp:1:1: error: an attribute list cannot appear here
[[deprecated( "use bar instead" )]]
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

I believe this should be a valid form of the deprecated attribute and the compiler seems to be parsing it incorrectly.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[dyung]

@martinboehme This was your change, can you take a look?

[hvdijk]

Interesting.

I believe this should be a valid form of the deprecated attribute and the compiler seems to be parsing it incorrectly.

I do not believe it is: the C++ grammar says

declaration:
linkage-specification

linkage-specification:
extern string-literal { declaration-seq opt }
extern string-literal declaration

And no other references to linkage-specification. This means extern "C" can only appear at the start of a declaration. GCC ("error: expected unqualified-id before string constant"), MSVC ("error C2059: syntax error: 'string'"), and Intel ("error: attributes are not allowed here") all reject it too.

But as this was previously silently accepted without so much as a warning, and the commit you reference specifically tries to not emit errors for code that was previously silently accepted, I do think the fact that this now results in an error may be unintended.

[martinboehme]

@hvdijk is correct: The C++ standard does not allow placing attributes in this position, and other compilers reject it. IOW, this was an unintentional language extension.

The correct placement is this:

extern "C"
[[deprecated( "use bar instead" )]] int foo();

Are you able to change the code accordingly (i.e. is this a codebase that you control)?

But as this was previously silently accepted without so much as a warning, and the commit you reference specifically tries to not emit errors for code that was previously silently accepted, I do think the fact that this now results in an error may be unintended.

Disallowing C++11 attributes in this position was an intended effect of https://reviews.llvm.org/D126061 (though not the main motivation for the change). @zygoloid pointed out to me during the review that C++11 attributes are not allowed before extern "C" and that disallowing attributes in this position would reduce the amount of additional "plumbing" that D126061 would need to add.

I've added a comment to the relevant discussion on D126061. I think it's worth discussing whether we should reinstate support for this (incorrect) usage temporarily, with a deprecation warning rather than an error? I think this depends on how easy it is for people to fix this incorrect usage if it occurs in their compiles. I would generally expect this kind of error to occur in first-party code (that is easily fixed) rather than third-party libraries, because the latter usually compile on GCC, and GCC disallows this usage. So I think maybe we're fine not reinstating support for the incorrect usage, but I'd like to hear others' opinions on this.

[AaronBallman]

Agreed that this is not a defect -- we fixed a bug in Clang where we would silently accept (and drop) the attribute in this position. As mentioned on the review, I'm not certain I see the value in downgrading this to a warning given that attributes written in this position had no semantic effect anyway.

[hvdijk]

Agreed that this is not a defect -- we fixed a bug in Clang where we would silently accept (and drop) the attribute in this position. As mentioned on the review, I'm not certain I see the value in downgrading this to a warning given that attributes written in this position had no semantic effect anyway.

This reads as if the attributes were previously silently accepted but ignored. I am not sure if that is what you mean, but they weren't, clang 14 does produce a deprecation warning for the following.

[[deprecated("foo")]] extern "C" void foo() {}
void bar() { foo(); }

[martinboehme]

This reads as if the attributes were previously silently accepted but ignored. I am not sure if that is what you mean, but they weren't, clang 14 does produce a deprecation warning for the following.

[[deprecated("foo")]] extern "C" void foo() {}
void bar() { foo(); }

Confirmed:

https://godbolt.org/z/zE1E378Gb

(See also the DeprecatedAttr in the AST.)

It was my recollection too that the attribute was actually having an effect in this position.

@AaronBallman

Agreed that this is not a defect -- we fixed a bug in Clang where we would silently accept (and drop) the attribute in this position. As mentioned on the review, I'm not certain I see the value in downgrading this to a warning given that attributes written in this position had no semantic effect anyway.

It appears from the above that the situation is slightly different -- the attributes were having an effect. (Maybe you are remembering the discussion around [[clang::noderef]] in the same patch, which in fact does not have an effect when specified in C++11 syntax?)

[AaronBallman]

We drop it on the compound form of the linkage spec, but not the direct form: https://godbolt.org/z/1oxGMWW9q

[martinboehme]

Thanks for the followup!

[AaronBallman]

That it was useful on the direct form is interesting, but it actually leans me more towards being happy with this being an error in some ways given that it's not consistent within the same syntactic construct. But, then again, it did something useful before and that's worth something too.

So I guess it still boils down to the situation this shows up in. Is it happening in hard/impossible-to-change code like system headers, or is it mostly limited to application code that can be updated? (This looks like one that could potentially even be updated with a sufficiently bad regex since it only worked on the direct form.)

[pogo59]

Looking at @dyung 's internal bug report, it's a test case (for something else entirely) that seems to be originally derived from LUA. The declarations of interest are in a header that's doing things like redirecting uses of str* function to something safer. It's a handful of declarations, under 10.
We can easily modify our test case but it's not something we produced ourselves.

[AaronBallman]

Thank you for the information @pogo59! That doesn't sound like a sufficient reason to make adjustments here, so my recommendation is to close the report as being not a bug, but if we see more breakage from this, we may want to reconsider.