Feature request: Don't warn for specified "unknown" attribute

Clang Frontend
#1

The following code will emit a warning with -Wattributes:

[[some_ns::some_attribute]]
void call_me();

:1:3: warning: unknown attribute 'some_attribute' ignored [-Wunknown-attributes] [[some_ns::some_attribute]] ^

However, this warning is very useful for detecting typos of standard attributes. AFAIK, there’s no way to turn off the warning just for one attribute, just all of them (-Wno-unknown-attributes).

To solve this, I propose that we add the ability to specify attributes to ignore for -Wunknown-attributes. In other words, something like this: ‘-Wignore-unknown-attribute=some_ns::some_attribute’.

There are some alternatives, such as only warning if the edit distance is close to a known attribute, but I think that specifying the known attribute in the build system like this is better, as it will also catch misspellings of [[some_ns::some_attribute]].

Thank you,
Justin Bassett

#2

The following code will emit a warning with -Wattributes:

[[some_ns::some_attribute]]
void call_me();

<source>:1:3: warning: unknown attribute 'some_attribute' ignored [-Wunknown-attributes]
[[some_ns::some_attribute]]
  ^

However, this warning is very useful for detecting typos of standard attributes. AFAIK, there's no way to turn off the warning just for one attribute, just all of them (-Wno-unknown-attributes).

That's correct, but it's useful beyond just detecting typos. Different
vendors support different sets of attributes and this diagnostic is
how a developer can tell whether Clang is ignoring a given attribute
or not. It can be very difficult to tell the difference between
"parsed and silently ignoring this attribute" and "parsed attribute
and it functions as designed" depending on the attribute without this
diagnostic. For example, in order to tell whether
__attribute__((deprecated)) is supported, you would have to alter code
to see that it's behaving by design; otherwise, the declarations are
just silently marked as deprecated.

To solve this, I propose that we add the ability to specify attributes to ignore for -Wunknown-attributes. In other words, something like this: '-Wignore-unknown-attribute=some_ns::some_attribute'.

What issue is being solved, though? If the user wishes to ignore a
specific attribute on Clang because it's not known to the
implementation, the better solution is to use the preprocessor to
provide decent fallback behavior for the attribute. This is why
__has_attribute and friends exist. Is there a different problem that
this new warning flag behavior solves?

~Aaron

#3

That’s correct, but it’s useful beyond just detecting typos. Different
vendors support different sets of attributes and this diagnostic is
how a developer can tell whether Clang is ignoring a given attribute
or not. It can be very difficult to tell the difference between
“parsed and silently ignoring this attribute” and “parsed attribute
and it functions as designed” depending on the attribute without this
diagnostic. For example, in order to tell whether
attribute((deprecated)) is supported, you would have to alter code
to see that it’s behaving by design; otherwise, the declarations are
just silently marked as deprecated.

Indeed, which is why I want the warning turned on by default (not that it’s a default compiler warning, but that it’s on for all attributes “by default” when the warning is turned on).

To solve this, I propose that we add the ability to specify attributes to ignore for -Wunknown-attributes. In other words, something like this: ‘-Wignore-unknown-attribute=some_ns::some_attribute’.

What issue is being solved, though? If the user wishes to ignore a
specific attribute on Clang because it’s not known to the
implementation, the better solution is to use the preprocessor to
provide decent fallback behavior for the attribute. This is why
__has_attribute and friends exist. Is there a different problem that
this new warning flag behavior solves?

~Aaron

No, this problem is not solved by the preprocessor. Yes, most C++ code bases today use the preprocessor to hack around this, but IMO, this is a broken solution. C++17 states that unknown attributes should be ignored, not warned on (although I believe the compiler is within its rights to warn). I strongly feel that users should be able to use attributes without macros, otherwise C++11 attributes lose a lot of their appeal, almost to the point where vendor-specific attribute syntaxes should have continued as the way forward.

In short, I would argue that the preprocessor is a worse solution, as requiring it blocks the perfectly valid syntax of using a C++11 attribute, and using it requires a lot more work to set up the macro.

Regards,
Justin

#4

That's correct, but it's useful beyond just detecting typos. Different
vendors support different sets of attributes and this diagnostic is
how a developer can tell whether Clang is ignoring a given attribute
or not. It can be very difficult to tell the difference between
"parsed and silently ignoring this attribute" and "parsed attribute
and it functions as designed" depending on the attribute without this
diagnostic. For example, in order to tell whether
__attribute__((deprecated)) is supported, you would have to alter code
to see that it's behaving by design; otherwise, the declarations are
just silently marked as deprecated.

Indeed, which is why I want the warning turned on by default (not that it's a default compiler warning, but that it's on for all attributes "by default" when the warning is turned on).

> To solve this, I propose that we add the ability to specify attributes to ignore for -Wunknown-attributes. In other words, something like this: '-Wignore-unknown-attribute=some_ns::some_attribute'.

What issue is being solved, though? If the user wishes to ignore a
specific attribute on Clang because it's not known to the
implementation, the better solution is to use the preprocessor to
provide decent fallback behavior for the attribute. This is why
__has_attribute and friends exist. Is there a different problem that
this new warning flag behavior solves?

~Aaron

No, this problem is not solved by the preprocessor. Yes, most C++ code bases today use the preprocessor to hack around this, but IMO, this is a broken solution. C++17 states that unknown attributes should be ignored, not warned on (although I believe the compiler is within its rights to warn). I strongly feel that users should be able to use attributes without macros, otherwise C++11 attributes lose a lot of their appeal, almost to the point where vendor-specific attribute syntaxes should have continued as the way forward.

I disagree that users should avoid the preprocessor here. If their
code is only compiled by Clang, there is not a lot of compelling
reason for them to pass -Wunknown-attributes=whatever in the first
place. If the code is compiled by more than just Clang, then the only
nonfragile cross-compiler solutions are to use the preprocessor or the
build system. In my experience, developers prefer writing code to
writing build scripts.

In short, I would argue that the preprocessor is a worse solution, as requiring it blocks the perfectly valid syntax of using a C++11 attribute, and using it requires a lot more work to set up the macro.

I'm not certain why you believe using the preprocessor blocks using a
C++11 attribute -- it doesn't. There's no difference between using
[[clang::foobar]] and a macro that expands to [[clang::foobar]] if the
attribute is supported and [[]] if it's not, except for the lack of a
warning about ignored attributes.

I am not convinced that hacking build scripts is less work than using
macros. It's arguable that the first time you need the macro is an
outsized amount of work (because you have to start adding boilerplate
at that point), but I believe the same is true the first time you need
to hack a build script to add the new warning flag + attribute list.
Given that (AFAIK) no other compiler gives you granularity to disable
unknown attribute warnings on a per-attribute basis, I don't think
forcing users to try to deal with this in build scripts does users a
service. This is especially true given that each compiler will require
a different build script change even if the flags are identical
between compilers, because different compilers support different lists
of attributes.

~Aaron

#5

I disagree that users should avoid the preprocessor here. If their
code is only compiled by Clang, there is not a lot of compelling
reason for them to pass -Wunknown-attributes=whatever in the first
place. If the code is compiled by more than just Clang, then the only
nonfragile cross-compiler solutions are to use the preprocessor or the
build system. In my experience, developers prefer writing code to
writing build scripts.

Why isn’t there a compelling reason to pass -Wunknown-attributes=whatever? Attributes are useful for all sorts of things, such as static analysis tools. And adding a flag to a build script is not “writing build scripts”. It’s the simplest of build-system logic that I’d expect any developer to know how to do, but not every developer needs to know how to do it, just the ones who add tools to the workflow or update standards, etc.

I’m not certain why you believe using the preprocessor blocks using a
C++11 attribute – it doesn’t. There’s no difference between using
[[clang::foobar]] and a macro that expands to [[clang::foobar]] if the
attribute is supported and [[]] if it’s not, except for the lack of a
warning about ignored attributes.

My point is this: if we use macros anyway, there was no need to standardize attributes in C++11. attribute((whatever)) and MSVC’s __decspec(…) is sufficient (custom attributes could be handled in a similar way, such as how Qt Moc does it). Yes, things like [[noreturn]] and [[nodiscard]] would need to be wrapped in macros, but that’s not too much work.

However, we standardized an attribute syntax for C++11, and in C++17 we clarified that this attribute syntax needs to allow arbitrary attributes, not just standardized or compiler-specific ones.

I am not convinced that hacking build scripts is less work than using
macros. It’s arguable that the first time you need the macro is an
outsized amount of work (because you have to start adding boilerplate
at that point), but I believe the same is true the first time you need
to hack a build script to add the new warning flag + attribute list.

True. However, macros obscure the code:

SOME_ATTRIBUTE
void call_me();

It may be clear that SOME_ATTRIBUTE is an attribute, but I have to chase down the definition of SOME_ATTRIBUTE to determine what it is.

Also consider that some users will prefer to pass -Wno-unknown-attributes than use these macros, meaning they get no typo detection. However, if they could whitelist allowed attributes to turn off the warning for, I’d imagine many of those users would do so. Why not have a fine-grained option for warning control?

Given that (AFAIK) no other compiler gives you granularity to disable
unknown attribute warnings on a per-attribute basis, I don’t think
forcing users to try to deal with this in build scripts does users a
service.

Just because no compiler does it yet doesn’t mean we shouldn’t do it. I’ve submitted a similar feature request on GCC, and it seems well-received. I intend to do the same for MSVC.

This is especially true given that each compiler will require
a different build script change even if the flags are identical
between compilers, because different compilers support different lists
of attributes.

If different compilers support different lists of attributes, that’s not a problem. A -Wignore-unknown-attribute=clang::some_attribute in GCC would be fine in Clang, because it does nothing in Clang. The flag may differ between compilers, though, but that’s not anything developers aren’t already familiar with.

#6

Rather than having to list all the attributes from other compilers you
choose to use - would it make sense to use the namespace/scoping rules
here? Diagnose any unknown attributes that are either unscoped or in
the clang:: scope, but ignore all others?

#7

Rather than having to list all the attributes from other compilers you
choose to use - would it make sense to use the namespace/scoping rules
here? Diagnose any unknown attributes that are either unscoped or in
the clang:: scope, but ignore all others?

IMO, no. Vendors are welcome to support attributes from other vendors
-- for instance, Clang supports some but not nearly all of the
attributes in the gnu namespace, so there would continue to be a
granularity issue. As for listing attributes individually, there are
other design questions that I think don't have clear answers: how to
handle attributes with multiple spellings and how to handle attributes
that are unknown to one target but not others. We could probably
figure something out for those, but I do not think listing out
attributes in a warning flag is a tenable option for this
functionality. As mentioned, there is already a standards-based
approach for solving this problem, which is to use __has_cpp_attribute
(and similar macros) and the preprocessor.

~Aaron

#8

>
> Rather than having to list all the attributes from other compilers you
> choose to use - would it make sense to use the namespace/scoping rules
> here? Diagnose any unknown attributes that are either unscoped or in
> the clang:: scope, but ignore all others?

IMO, no. Vendors are welcome to support attributes from other vendors
-- for instance, Clang supports some but not nearly all of the
attributes in the gnu namespace, so there would continue to be a
granularity issue. As for listing attributes individually, there are
other design questions that I think don't have clear answers: how to
handle attributes with multiple spellings and how to handle attributes
that are unknown to one target but not others. We could probably
figure something out for those, but I do not think listing out
attributes in a warning flag is a tenable option for this
functionality. As mentioned, there is already a standards-based
approach for solving this problem, which is to use __has_cpp_attribute
(and similar macros) and the preprocessor.

Perhaps, but I appreciate Justin's point - there seems limited value
in the syntax for attributes being standardized if there's no nice way
to use them without protecting them behind macros anyway. Not zero
value, but it does reduce the value, I think.

If we support attributes from other namespaces, then including those
in the set of accepted (& thus warned on) namespaces seems OK to me.
If someone's using clang and writes gnu::unsupported when they meant
gnu::supported (even though gnu::unsupported is supported by some
version of GCC but isn't supported by Clang) that seems like a nice
thing to warn on in this mode, while silently ignoring
"unsupported::anything" still.

#9

A view from another angle:

IMHO, we should not make decisions about how C/C++ programmers
structure their code. One project might choose do hide attributes
behind macros, while another one prefers to switch off the warning in
the compiler-specific part of their build script. Both have pros and
cons, and might be fine depending on the situation and therefore not a
decision for compiler developers to make on the user's behalf.
Similarly, as we allow to switch off specific warnings in case a
project, e.g. such as allowing GNU extensions. Even clang uses
-Wno-unused-parameter when compiling with clang.

However, we'd have to maintain it, but it it looks like a low-effort feature.

Michael

#10

>
> >
> > Rather than having to list all the attributes from other compilers you
> > choose to use - would it make sense to use the namespace/scoping rules
> > here? Diagnose any unknown attributes that are either unscoped or in
> > the clang:: scope, but ignore all others?
>
> IMO, no. Vendors are welcome to support attributes from other vendors
> -- for instance, Clang supports some but not nearly all of the
> attributes in the gnu namespace, so there would continue to be a
> granularity issue. As for listing attributes individually, there are
> other design questions that I think don't have clear answers: how to
> handle attributes with multiple spellings and how to handle attributes
> that are unknown to one target but not others. We could probably
> figure something out for those, but I do not think listing out
> attributes in a warning flag is a tenable option for this
> functionality. As mentioned, there is already a standards-based
> approach for solving this problem, which is to use __has_cpp_attribute
> (and similar macros) and the preprocessor.

Perhaps, but I appreciate Justin's point - there seems limited value
in the syntax for attributes being standardized if there's no nice way
to use them without protecting them behind macros anyway. Not zero
value, but it does reduce the value, I think.

I don't agree that this limits the value of the syntax -- it's a
common idiom that's well-established practice and has been for
numerous years.

That's not to say we shouldn't explore improving it, but I'm not
convinced pushing this into the build system is a good design in
practice. We support around 300 attributes currently, and this is a
fraction of the total attributes supported by all vendors and our list
of supported attributes (as well as other compiler's similar lists)
change with every release. I haven't seen any evidence that a list of
specific attributes to not diagnose is useful in the face of the sheer
number of possible attributes.

If we support attributes from other namespaces, then including those
in the set of accepted (& thus warned on) namespaces seems OK to me.
If someone's using clang and writes gnu::unsupported when they meant
gnu::supported (even though gnu::unsupported is supported by some
version of GCC but isn't supported by Clang) that seems like a nice
thing to warn on in this mode, while silently ignoring
"unsupported::anything" still.

I would be comfortable having two separate "unknown attribute" warning
flags: -Wunknown-attributes and -Wunknown-attribute-namespaces (or
something) where the distinction is the former warns on any unknown
attribute regardless of namespace, and the latter only diagnoses
unknown attributes in namespaces for which Clang does not support any
attributes. This seems like it would have a useful level of
granularity while not requiring the user to maintain a separate list
of things in the build system. It also nicely sidesteps problems like
attributes with multiple spellings or target-specific attributes.

~Aaron

#11

We support around 300 attributes currently, and this is a
fraction of the total attributes supported by all vendors and our list
of supported attributes (as well as other compiler’s similar lists)
change with every release. I haven’t seen any evidence that a list of
specific attributes to not diagnose is useful in the face of the sheer
number of possible attributes.

I severely underestimated the amount of attributes which may need to be listed through whitelisting. The 300 clang-supported attributes wouldn’t need to be whitelisted since they are already known, but it’s certainly conceivable that some unknown third party attributes could reach 30s to 100s or above, which is no longer that reasonable to list individually.

I would be comfortable having two separate “unknown attribute” warning
flags: -Wunknown-attributes and -Wunknown-attribute-namespaces (or
something) where the distinction is the former warns on any unknown
attribute regardless of namespace, and the latter only diagnoses
unknown attributes in namespaces for which Clang does not support any
attributes. This seems like it would have a useful level of
granularity while not requiring the user to maintain a separate list
of things in the build system. It also nicely sidesteps problems like
attributes with multiple spellings or target-specific attributes.

I agree with this option. I already thought was reasonable, but now that I realize how many attributes there are, it seems to be one of the better options.

Regards,
Justin

#12

We support around 300 attributes currently, and this is a
fraction of the total attributes supported by all vendors and our list
of supported attributes (as well as other compiler's similar lists)
change with every release. I haven't seen any evidence that a list of
specific attributes to not diagnose is useful in the face of the sheer
number of possible attributes.

I severely underestimated the amount of attributes which may need to be listed through whitelisting. The 300 clang-supported attributes wouldn't need to be whitelisted since they are already known, but it's certainly conceivable that some unknown third party attributes could reach 30s to 100s or above, which is no longer that reasonable to list individually.

I believe GCC currently supports even more attributes than Clang does,
so 100s is a pretty reasonable estimation. I'm very wary of
arbitrary-length lists of arbitrary identifiers in command line
arguments as it always tends to have weird issues. For instance, : is
a reserved character in some shells, so spelling something
-Wunknown-attributes=foo::bar would fail for some users and have to be
written as -Wunknown-attributes="foo::bar" instead. Command line
length limits are also a concern with arbitrary lists. Having an
option where the user doesn't have to spell out specific attributes
alleviates all of my concerns and I think the granularity will be
sufficiently useful.

I would be comfortable having two separate "unknown attribute" warning
flags: -Wunknown-attributes and -Wunknown-attribute-namespaces (or
something) where the distinction is the former warns on any unknown
attribute regardless of namespace, and the latter only diagnoses
unknown attributes in namespaces for which Clang does not support any
attributes. This seems like it would have a useful level of
granularity while not requiring the user to maintain a separate list
of things in the build system. It also nicely sidesteps problems like
attributes with multiple spellings or target-specific attributes.

I agree with this option. I already thought was reasonable, but now that I realize how many attributes there are, it seems to be one of the better options.

I'd like some time to think on this one a bit more, but it does seem
tenable to me -- I'm glad that it may work for your use case. I don't
think this would be too difficult to implement; we don't currently
have a list of supported attribute namespaces anywhere, but I think it
would be easy to generate one automatically, so there shouldn't be
much maintenance overhead to support something like
-Wunknown-attribute-namespaces.

~Aaron

#13

>>
>> We support around 300 attributes currently, and this is a
>> fraction of the total attributes supported by all vendors and our list
>> of supported attributes (as well as other compiler's similar lists)
>> change with every release. I haven't seen any evidence that a list of
>> specific attributes to not diagnose is useful in the face of the sheer
>> number of possible attributes.
>
>
> I severely underestimated the amount of attributes which may need to be listed through whitelisting. The 300 clang-supported attributes wouldn't need to be whitelisted since they are already known, but it's certainly conceivable that some unknown third party attributes could reach 30s to 100s or above, which is no longer that reasonable to list individually.

I believe GCC currently supports even more attributes than Clang does,
so 100s is a pretty reasonable estimation. I'm very wary of
arbitrary-length lists of arbitrary identifiers in command line
arguments as it always tends to have weird issues. For instance, : is
a reserved character in some shells, so spelling something
-Wunknown-attributes=foo::bar would fail for some users and have to be
written as -Wunknown-attributes="foo::bar" instead. Command line
length limits are also a concern with arbitrary lists. Having an
option where the user doesn't have to spell out specific attributes
alleviates all of my concerns and I think the granularity will be
sufficiently useful.

>> I would be comfortable having two separate "unknown attribute" warning
>> flags: -Wunknown-attributes and -Wunknown-attribute-namespaces (or
>> something) where the distinction is the former warns on any unknown
>> attribute regardless of namespace, and the latter only diagnoses
>> unknown attributes in namespaces for which Clang does not support any
>> attributes. This seems like it would have a useful level of
>> granularity while not requiring the user to maintain a separate list
>> of things in the build system. It also nicely sidesteps problems like
>> attributes with multiple spellings or target-specific attributes.
>
>
> I agree with this option. I already thought was reasonable, but now that I realize how many attributes there are, it seems to be one of the better options.

I'd like some time to think on this one a bit more, but it does seem
tenable to me -- I'm glad that it may work for your use case. I don't
think this would be too difficult to implement; we don't currently
have a list of supported attribute namespaces anywhere, but I think it
would be easy to generate one automatically, so there shouldn't be
much maintenance overhead to support something like
-Wunknown-attribute-namespaces.

FYI: I put together a patch to explore this in https://reviews.llvm.org/D60872

~Aaron

#14

I agree, I am playing around with using attributes to annotate my code for style checking and code gen for a test framework and have had to turn off attributes warnings entirely. It would be nice to be able to register custom attributes to the know set so clang can ignore them.

-Matt

#15

I agree, I am playing around with using attributes to annotate my code for style checking and code gen for a test framework and have had to turn off attributes warnings entirely. It would be nice to be able to register custom attributes to the know set so clang can ignore them.

Does the proposed patch/design not address your use case here? (a
warning that ignores all attributes in unknown namespaces) - your
style checker and code generator could/should/would use custom
namespaces for their attributes, unknown to Clang so those would be
ignored with this new warning category & there would be no need to
register custom attributes to the known set.

- Dave

#16

Why does an analysis tool have to use a custom namespace? Is it a requirement of the spec that the compiler only gets to use attributes in the root? I am not opposed to doing this, but that seems arbitrary to me.

-Matt

#17

Sorry I was missing half the conversation. Yes, I think adding ignore unknown namespace or register a known namespace works great for bigger projects that have many attributes. But I could see small one off solutions that create just one or two new attributes that should be able to have unqualified attributes if they want. Why not add both namespace level and individual attribute register flags?

-Matt