[PATCH] D12375: [PATCH] Relax parse ordering rules for attributes

[Aaron Ballman via cfe-commits]

On Wed, Aug 26, 2015 at 4:30 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Wed, Aug 26, 2015 at 11:27 AM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> aaron.ballman created this revision.
>> aaron.ballman added reviewers: rsmith, hans.
>> aaron.ballman added a subscriber: cfe-commits.
>>
>> PR24559 brings up a long-standing issue in Clang where attribute order
>> with differing syntax impacts whether a parse will succeed or fail with poor
>> diagnostics. For instance:
>>
>> struct __attribute__((lockable)) __declspec(dllexport) S { }; // ok
>> struct __declspec(dllexport) __attribute__((lockable)) T { }; // error
>> about declaring anonymous structs and a warning about declarations not
>> declaring anything
>>
>> When you consider attributes as extraneous semantic information to attach
>> to entities, ordering of the attributes really should not matter between the
>> various attribute syntaxes we support -- they all serve the same purpose.
>>
>> This patch begins to address the issue by providing a
>> MaybeParseAttributes() function accepting a mask of what attribute syntaxes
>> are allowed and parses the various syntaxes in a loop.
>>
>> However, this patch is not complete, as the test cases for function
>> attributes currently fail for the C++11-style attributes.
>>
>> [[noreturn]] __attribute__((cdecl)) __declspec(dllexport) void f();
>>
>> In this case, the [[noreturn]] is parsed as part of the top-level
>> declaration, while the GNU and declspec attributes are parsed as part of the
>> declaration specifier, and everything is accepted.
>>
>> __attribute__((cdecl)) [[noreturn]] __declspec(dllexport) void f();
>>
>> In this case, nothing is parsed as part of the top-level declaration, and
>> everything is parsed as part of the declaration specifier. However, the C++
>> attribute is prohibited from appearing on a declaration specifier and so is
>> diagnosed.
>
> That seems like pedantically correct behavior to me. C++ attributes at the
> start of the declaration appertain to the declared entities; C++ attributes
> after a decl-specifier, declarator, or id-expression appertain to that
> decl-specifier, declarator, or entity. Conversely, a GNU attribute or MS
> attribute (in this position) is a decl-specifier.

I agree with that assessment, thank you for reminding me that C++
attributes can apply to decl-specifiers. I still find it unsatisfying
from a usability perspective at the same time. While the GNU or MS
attribute in that position is a decl-specifier, it's also an
attribute. Do we want users thinking about what position an attribute
needs to be in to not be a decl-specifier? Then again, any reordering
we allow now would be really painful to deal with for anyone
(including the standards body) who wants an attribute to appertain to
the decl-specifier.

> I don't think that these cases are fundamentally different from this
> perspective:
>
> [[noreturn]] inline void f();
> [[noreturn]] __attribute__((gnu_inline)) void f();
>
> inline [[noreturn]] void f(); // ill-formed
> __attribute__((gnu_inline)) [[noreturn]] void f(); // ill-formed?
>
> Rejecting the last of these cases may not seem especially user-friendly, but
> we should do everything we can to avoid C++11 attributes becoming the morass
> of syntactic soup that __attribute__ has become.

That's definitely true and compelling, and I agree that we need to
tread more lightly than I was. Perhaps a different solution isn't to
allow arbitrary orders, but to instead provide a fixit suggesting the
"correct" (whatever that means with all these vendor extensions being
used at once) ordering. However, such a mechanism would still require
us to be able to parse funky orders in order to slide things around to
where they should be (and would make it really hard to have a C++
attribute apply to a decl-specifier), so that idea may be problematic
and time consuming too.

> (Also, I'd note that allowing reordering of C++11 and GNU attributes is not
> GCC-compatible; our current permitted ordering is intended to be GCC
> compatible. If we go ahead with your patch, you should at least add a
> -Wgcc-compat warning for it.)

If I understand correctly, it seems like __attribute__ and __declspec
should always be interchangeable, while [[]] should never be (and I
have no ideas on Microsoft [] attributes as there's no grammar, but I
could research this)? If that's the case, I can provide a modified
patch and some good test cases with comments explaining what's going
on.

Thanks!

~Aaron

>
>> I think ParseDeclarationSpecifiers() needs to accept a
>> ParsedAttributesWithRange object representing the top-level declaration
>> attributes, and the parser needs to attach the C++ attributes to it up until
>> we the first non-attribute token is found. But I'm not 100% certain that's
>> the right approach.
>>
>> Before taking the process further, I was wondering about two things:
>>
>> 1) Is the direction with MaybeParseAttributes() an acceptable approach?
>> 2) If it is, is it reasonable to commit that, and work on the
>> ParseDeclarationSpecifiers() portion in a follow-up commit?
>>
>> Thanks!
>>
>> ~Aaron
>>
>> http://reviews.llvm.org/D12375
>>
>> Files:
>>   include/clang/Parse/Parser.h
>>   lib/Parse/ParseDecl.cpp
>>   lib/Parse/ParseDeclCXX.cpp
>>   lib/Parse/ParseExprCXX.cpp
>>   lib/Parse/Parser.cpp
>>   test/Parser/attr-order.cpp
>>
>