Page MenuHomePhabricator

Add [[clang::suppress(rule, ...)]] attribute
ClosedPublic

Authored by mgehre on Sep 23 2016, 2:39 PM.

Details

Summary

This patch implements parsing of [[clang::suppress(rule, ...)]]
attributes.

C++ Core Guidelines depend heavily on tool support for
rule enforcement. They also propose a way to suppress
warnings [1] which is by annotating any ancestor in AST
with the C++11 attribute [[suppress(rule1,...)]].
I understand that clang has a policy to put all non-standard
attributes in clang namespace, thus here it is named
[[clang::suppress(rule1, ...)]].

For example, to suppress the warning cppcoreguidelines-slicing,
one could do

[[clang::suppress("cppcoreguidelines-slicing")]]
void f() { ... code that does slicing ... }

or

void g() {
  Derived b;
  [[clang::suppress("cppcoreguidelines-slicing")]]
  Base a{b};
  [[clang::suppress("cppcoreguidelines-slicing")]] {
    doSomething();
    Base a2{b};
  }
}

This parsing can then be used by clang-tidy, which includes multiple
C++ Core Guidelines rules, to suppress warnings (see https://reviews.llvm.org/D24888).
For the exact naming of the rule in the attribute, there
are different possibilities, which will be defined in the
corresponding clang-tidy patch.

Currently, clang-tidy supports suppressing of warnings through "//
NOLINT" comments. There are some advantages that the attribute has:

  • Suppressing specific warnings instead of all warnings
  • Suppressing warnings in a block (namespace, function, compound statement)
  • Code formatting may split a statement into multiple lines, thus a "// NOLINT" comment may be on the wrong line

I could imagine to later extend this attribute further (if agreed)

  • for suppressing clang-tidy warnings in general
  • for suppressing clang warnings in general.

I'm looking forward to your comments!

[1] https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#inforce-enforcement

Diff Detail

Repository
rL LLVM

Event Timeline

mgehre updated this revision to Diff 72359.Sep 23 2016, 2:39 PM
mgehre retitled this revision from to Add [[clang::suppress(rule, ...)]] attribute.
mgehre updated this object.
mgehre added reviewers: alexfh, aaron.ballman, rsmith.
mgehre added a subscriber: cfe-commits.
mgehre updated this revision to Diff 72366.Sep 23 2016, 2:58 PM

Include link to corresponding clang-tidy patch

mgehre updated this object.Sep 23 2016, 2:58 PM
aaron.ballman requested changes to this revision.Sep 26 2016, 8:45 AM
aaron.ballman edited edge metadata.

I'm generally in favor of this patch, but it's missing some pieces, like test cases. Also, I suspect we will want this attribute to also be written on types, but there are no changes to SemaType.cpp to handle this. Is that something you are planning to support as well?

include/clang/Basic/Attr.td
1457 ↗(On Diff #72366)

No new undocumented attributes, please.

lib/Sema/SemaDeclAttr.cpp
3851 ↗(On Diff #72366)

Should we diagnose if asked to suppress a diagnostic that we don't support? e.g., someone does [[clang::suppress("this does not exist")]]

On the one hand, we want to be forwards-compatible, but on the other hand, I can easily see typos in long string literals. So perhaps we may want to have a diagnostic based on some heuristic. e.g., we diagnose if the spelling is slightly off and we have viable options which we can use typo correction to issue a fixit, or if it's way off base, we silently eat it under the assumption it's a forwards compatibility thing?

lib/Sema/SemaStmtAttr.cpp
72 ↗(On Diff #72366)

Please construct this directly instead of relying on a copy constructor. Also, same comments about typos apply here as above.

This revision now requires changes to proceed.Sep 26 2016, 8:45 AM
rsmith edited edge metadata.Sep 26 2016, 1:10 PM

Giving this the name [[clang::suppress(...)]] seems unhelpful. This attribute is specified by the C++ core guidelines, not by clang, and presumably we want code using this attribute to be portable across implementations of the C++ code guidelines. I'd suggest asking the editors of the core guidelines what attribute namespace they'd like used, and using that here rather than using clang.

Giving this the name [[clang::suppress(...)]] seems unhelpful. This attribute is specified by the C++ core guidelines, not by clang, and presumably we want code using this attribute to be portable across implementations of the C++ code guidelines. I'd suggest asking the editors of the core guidelines what attribute namespace they'd like used, and using that here rather than using clang.

I believe this attribute should be used to silence diagnostics for more than just the C++ Core Guidelines, so I don't think it makes sense to let them dictate what attribute namespace should be used. However, I would appreciate if they didn't suggest implementers stomp all over the attribute namespace used by standards-based attributes, either.

Thank your very much for your comments!
Let me try to give me reasoning for those points:

  1. But it's missing some pieces, like test cases

I though about how to test this, having no semantic meaning itself.
I could look at the AST dump, but it does not even show the
rules that were passed, only that a "SuppressAttr" exists. Would that be enough?

  1. Also, I suspect we will want this attribute to also be written on types

I was thinking about a case were that was useful, and didn't find any.
Which of course doesn't mean that there is none. I will add this.

  1. No new undocumented attributes, please.

I completely agree that it cannot be merged like this. This depends a bit
on how our discussion turns out: Will this be specific to C++ Core Guidelines, or
clang-tidy or both or none? Then, should the clang documentation mention clang-tidy?
(or does that violate layering?)

  1. Should we diagnose if asked to suppress a diagnostic that we don't support?

I image that the users workflow would be like this: Run clang-tidy (e.g. by build server);
get a warning; add [[suppress]], run clang-tidy again; see that the warning is gone. I don't see a big
risk in not diagnosing a wrongly spelled suppression, because the user will notice right away
that the warning is not suppressed. There is not other implicit side-effect.
As an ad-don, diagnosing if the spelling is just slightly off seems like a bonus to me, but I hope
that this could be deferred to another patch.

  1. I'd suggest asking the editors of the core guidelines what attribute namespace they'd like used.

I followed your advice and asked here:
https://github.com/isocpp/CppCoreGuidelines/issues/742
I will post updates to that issue here.

  1. I believe this attribute should be used to silence diagnostics for more than just the C++ Core Guidelines,

so I don't think it makes sense to let them dictate what attribute namespace should be used.
Maybe I wanted to much here. There are two conflicting goals:

  1. Suppress C++ Core Guidelines rules in a vendor-neutral way
  2. Suppress specific clang(-tidy) warnings

I'm getting the feeling that we cannot have both in the same attribute.
For example, the C++ Core Guidelines say that the "No reinterpret_cast" rules shall be suppressed either by
saying "type" (also suppresses all other type related rules) or by saying "type.1" or by saying
"type1-dont-use-reinterpret_cast".
When we want to suppress other clang(-tidy) warnings, it would make sense from a usability point of view
to take the warning ids that clang(-tidy) outputs. For that particular C++ Core Guideline rule, it would be
"cppcoreguidelines-pro-type-reinterpret-cast".
So even if we had the same attribute name for both goals, the rule names would have to differ.

What are your opinions on this point? (Should I put this on the mailing list?)

Thank your very much for your comments!
Let me try to give me reasoning for those points:

  1. But it's missing some pieces, like test cases I though about how to test this, having no semantic meaning itself. I could look at the AST dump, but it does not even show the rules that were passed, only that a "SuppressAttr" exists. Would that be enough?

That's a good start. Other tests that are required: the attribute appertains to the proper syntactic constructs and is diagnosed otherwise, attribute has the correct number and kind of arguments, the arguments are sane, etc.

  1. Also, I suspect we will want this attribute to also be written on types I was thinking about a case were that was useful, and didn't find any. Which of course doesn't mean that there is none. I will add this.

If there are no use cases for it, then I guess we don't need to support it.

  1. No new undocumented attributes, please. I completely agree that it cannot be merged like this. This depends a bit on how our discussion turns out: Will this be specific to C++ Core Guidelines, or clang-tidy or both or none? Then, should the clang documentation mention clang-tidy? (or does that violate layering?)

I agree, we want to make sure the docs reflect our intended design. I don't think it's a problem for the clang docs to mention clang-tidy. Certainly we have LLVM documentation that mentions clang.

  1. Should we diagnose if asked to suppress a diagnostic that we don't support? I image that the users workflow would be like this: Run clang-tidy (e.g. by build server); get a warning; add [[suppress]], run clang-tidy again; see that the warning is gone. I don't see a big risk in not diagnosing a wrongly spelled suppression, because the user will notice right away that the warning is not suppressed. There is not other implicit side-effect.

I think that's definitely a reasonable use case, but I don't think it's a compelling explanation of why we should not warn the user "I have no idea what you're talking about", either. The same could be said of many diagnostics we give -- the user will notice that their code doesn't work properly. However, I tend to be in the camp of "warn on suspicious activity" camp.

As an ad-don, diagnosing if the spelling is just slightly off seems like a bonus to me, but I hope
that this could be deferred to another patch.

Certainly!

  1. I'd suggest asking the editors of the core guidelines what attribute namespace they'd like used. I followed your advice and asked here: https://github.com/isocpp/CppCoreGuidelines/issues/742 I will post updates to that issue here.

Thanks!

  1. I believe this attribute should be used to silence diagnostics for more than just the C++ Core Guidelines, so I don't think it makes sense to let them dictate what attribute namespace should be used. Maybe I wanted to much here. There are two conflicting goals:
  2. Suppress C++ Core Guidelines rules in a vendor-neutral way
  3. Suppress specific clang(-tidy) warnings I'm getting the feeling that we cannot have both in the same attribute.

I think we do our users a major disservice by not trying to do both with the same attribute. As a user, I do not want to have to remember which way to spell an attribute to silence warnings. This is especially important were we ever to allow triggering clang-tidy diagnostics through the clang frontend.

For example, the C++ Core Guidelines say that the "No reinterpret_cast" rules shall be suppressed either by
saying "type" (also suppresses all other type related rules) or by saying "type.1" or by saying
"type1-dont-use-reinterpret_cast".
When we want to suppress other clang(-tidy) warnings, it would make sense from a usability point of view
to take the warning ids that clang(-tidy) outputs. For that particular C++ Core Guideline rule, it would be
"cppcoreguidelines-pro-type-reinterpret-cast".
So even if we had the same attribute name for both goals, the rule names would have to differ.

What are your opinions on this point? (Should I put this on the mailing list?)

I guess I fail to see what the technical issue is (and perhaps I'm just obtuse), but I think that getting a wider audience is not a bad idea.

  1. Also, I suspect we will want this attribute to also be written on types I was thinking about a case were that was useful, and didn't find any. Which of course doesn't mean that there is none. I will add this.

I would like to suppress cppcoreguidelines-pro-type-union-access for all member functions of a class.
It might be useful to suppress cppcoreguidelines-pro-type-member-init for all constructors of a class.

In the C++ Core Guidelines issue on namespaceing of the [[suppress]], it was state that Microsoft uses
[[gsl::suppress]] and it is the intent to update the C++ Core Guidelines to [[gsl::suppress]].

My main goal is to be able to suppress any clang-tidy warning through an attribute. Initially, I though we could reuse the C++ Core Guidelines attribute for that,
(because I though it was plain [[suppress]]). As a bonus (not more), we would have interoperability with other C++ Core Guideline checkers.

But now that I know that it is spelled [[gsl::suppress]], I fear that this is not a good name for a general clang-tidy suppression attribute.
For that, [[clang::suppress]] sounds more reasonable to me.

mgehre updated this revision to Diff 72981.Sep 29 2016, 10:37 PM
mgehre edited edge metadata.

Add additional gsl::suppress spelling, add test, add documentation

aaron.ballman added inline comments.Oct 11 2016, 10:14 AM
include/clang/Basic/Attr.td
1469 ↗(On Diff #72981)

Perhaps we should name this something other than "Rules" -- you don't suppress a rule, you're suppressing a diagnostic based on some identifier, so I'd go with DiagnosticIdentifiers or something along those lines.

include/clang/Basic/AttrDocs.td
2509 ↗(On Diff #72981)

This statement does not match the test cases -- it does not appear to be possible to attach the attribute to a type.

mgehre added inline comments.Oct 23 2016, 3:19 PM
include/clang/Basic/AttrDocs.td
2509 ↗(On Diff #72981)

It is true that you cannot attach the attribute to an existing type during declaration of a variable,
but the attribute can be attached when declaring a new type. (See line 22 of the test case, where the attribute is attached to the union type U)
I would propose to says "type declaration" instead of "type", ok?

aaron.ballman added inline comments.Oct 26 2016, 1:03 PM
include/clang/Basic/AttrDocs.td
2509 ↗(On Diff #72981)

I think that "type declaration" would be an improvement.

mgehre updated this revision to Diff 92211.Mar 17 2017, 3:19 PM
mgehre marked an inline comment as done.

Update for review comments

I'd also like to see some tests in Misc that confirm the attribute is being attached to the appropriate AST nodes for declarations, statements, and at namespace scope.

include/clang/Basic/Attr.td
1527 ↗(On Diff #92211)

Given that this is only intended to be used with the C++ Core Guidelines, I think we should drop the clang::suppress spelling for this for right now. If we decide to expand the scope of this attribute to include more suppression scenarios in the future, we can always bring it back.

include/clang/Basic/AttrDocs.td
2730 ↗(On Diff #92211)

missing the word "attribute" before "is".

2733 ↗(On Diff #92211)

s/on/at.

lib/Sema/SemaDeclAttr.cpp
3851 ↗(On Diff #72366)

I think this needs a FIXME comment. We should probably warn the user if the rule name is unknown, but that involves a tricky layering issue since clang-tidy checks aren't trivial to expose to the Sema layer.

4085 ↗(On Diff #92211)

You can get rid of LiteralLock and just pass nullptr.

lib/Sema/SemaStmtAttr.cpp
72 ↗(On Diff #72366)

Same comments about the typo FIXME applies here as well. Also, you can get rid of LiteralLock and just pass nullptr.

mgehre updated this revision to Diff 92923.Mar 24 2017, 4:55 AM

Thank you for the comments; all of them have been addressed.

mgehre updated this revision to Diff 93055.Mar 25 2017, 2:18 PM

Added test to misc; minor wording

This revision is now accepted and ready to land.Mar 26 2017, 7:20 AM
This revision was automatically updated to reflect the committed changes.
alexfh added inline comments.Apr 18 2017, 11:13 AM
cfe/trunk/include/clang/Basic/AttrDocs.td
2792

Should this be gsl::suppress as well?

roccoor added a subscriber: roccoor.EditedOct 29 2017, 1:42 AM

Microsoft supports:

[[gsl::suppress(bounds.3)]]

Clang requires:

[[gsl::suppress("bounds.3")]]

Why is this inconsistency?

Here is an example from CppCoreGuidelines

[[suppress(bounds)]] char* raw_find(char* p, int n, char x)    // find x in p[0]..p[n - 1]
{
    // ...
}

class Istream { [[gsl::suppress(lifetime)]]
public:
    enum Opt { from_line = 1 };

I think you shouldn't require the quotes.