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

[mgehre]

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

[mgehre]

Include link to corresponding clang-tidy patch

[aaron.ballman]

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

No new undocumented attributes, please.

lib/Sema/SemaDeclAttr.cpp

3851

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

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

[rsmith]

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.

[aaron.ballman]

In D24886#552859, @rsmith wrote:

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.

[mgehre]

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?

2. 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.

3. 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?)

4. 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.

5. 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.

6. 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?)

[aaron.ballman]

In D24886#554130, @mgehre wrote:

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.

2. 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.

3. 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.

4. 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!

5. 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!

6. 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.

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.

[malcolm.parsons]

In D24886#554130, @mgehre wrote:

2. 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.

[mgehre]

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]].

[mgehre]

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]

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

[aaron.ballman]

include/clang/Basic/Attr.td

1456

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]

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]

include/clang/Basic/AttrDocs.td

2509 ↗

(On Diff #72981)

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

[mgehre]

Update for review comments

[aaron.ballman]

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

1455

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

3848

You can get rid of LiteralLock and just pass nullptr.

3851

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.

lib/Sema/SemaStmtAttr.cpp

72

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

[mgehre]

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

[mgehre]

Added test to misc; minor wording

[aaron.ballman]

LGTM!

[alexfh]

cfe/trunk/include/clang/Basic/AttrDocs.td

2792 ↗

(On Diff #93168)

Should this be gsl::suppress as well?

[roccoor]

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.

include/clang/Basic/Attr.td

def SwiftErrorResult : ParameterABIAttr {
   let Documentation = [SwiftErrorResultDocs];
 }
 
 def SwiftIndirectResult : ParameterABIAttr {
   let Spellings = [GCC<"swift_indirect_result">];
   let Documentation = [SwiftIndirectResultDocs];
 }
 
+def Suppress : StmtAttr {
+  let Spellings = [CXX11<"clang", "suppress">];
+  let Args = [VariadicStringArgument<"Rules">];
+  let Documentation = [Undocumented];
+}
+
 def SysVABI : InheritableAttr {
   let Spellings = [GCC<"sysv_abi">];
 //  let Subjects = [Function, ObjCMethod];
   let Documentation = [Undocumented];
 }
 
 def ThisCall : InheritableAttr {
   let Spellings = [GCC<"thiscall">, Keyword<"__thiscall">,

lib/Sema/SemaDeclAttr.cpp

case AttributeList::AT_PreserveAll:
     D->addAttr(::new (S.Context) PreserveAllAttr(
         Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
     return;
   default:
     llvm_unreachable("unexpected attribute kind");
   }
 }
 
+static void handleSuppressAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+  std::vector<StringRef> Rules;
+
+  for (unsigned I = 0, E = Attr.getNumArgs(); I != E; ++I) {
+    StringRef RuleName;
+    SourceLocation LiteralLoc;
+
+    if (!S.checkStringLiteralArgumentAttr(Attr, I, RuleName, &LiteralLoc))
+      return;
+
+    Rules.push_back(RuleName);
+  }
+  D->addAttr(::new (S.Context) SuppressAttr(
+      Attr.getRange(), S.Context, Rules.data(), Rules.size(),
+      Attr.getAttributeSpellingListIndex()));
+}
+
+
 bool Sema::CheckCallingConvAttr(const AttributeList &attr, CallingConv &CC, 
                                 const FunctionDecl *FD) {
   if (attr.isInvalid())
     return true;
 
   if (attr.hasProcessingCache()) {
     CC = (CallingConv) attr.getProcessingCache();
     return false;
static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
   case AttributeList::AT_MSABI:
   case AttributeList::AT_SysVABI:
   case AttributeList::AT_Pcs:
   case AttributeList::AT_IntelOclBicc:
   case AttributeList::AT_PreserveMost:
   case AttributeList::AT_PreserveAll:
     handleCallConvAttr(S, D, Attr);
     break;
+  case AttributeList::AT_Suppress:
+    handleSuppressAttr(S, D, Attr);
+    break;
   case AttributeList::AT_OpenCLKernel:
     handleSimpleAttribute<OpenCLKernelAttr>(S, D, Attr);
     break;
   case AttributeList::AT_OpenCLAccess:
     handleOpenCLAccessAttr(S, D, Attr);
     break;
   case AttributeList::AT_OpenCLNoSVM:
     handleOpenCLNoSVMAttr(S, D, Attr);

lib/Sema/SemaStmtAttr.cpp

static Attr *handleFallThroughAttr(Sema &S, Stmt *St, const AttributeList &A,
   if (!S.getLangOpts().CPlusPlus1z && A.isCXX11Attribute() &&
       !A.getScopeName())
     S.Diag(A.getLoc(), diag::ext_cxx1z_attr) << A.getName();
 
   FnScope->setHasFallthroughStmt();
   return ::new (S.Context) auto(Attr);
 }
 
+static Attr *handleSuppressAttr(Sema &S, Stmt *St, const AttributeList &A,
+                                SourceRange Range) {
+  std::vector<StringRef> Rules;
+
+  for (unsigned I = 0, E = A.getNumArgs(); I != E; ++I) {
+    StringRef RuleName;
+    SourceLocation LiteralLoc;
+
+    if (!S.checkStringLiteralArgumentAttr(A, I, RuleName, &LiteralLoc))
+      return nullptr;
+
+    Rules.push_back(RuleName);
+  }
+
+  SuppressAttr Attr(A.getRange(), S.Context, Rules.data(), Rules.size(),
+                    A.getAttributeSpellingListIndex());
+  return ::new (S.Context) auto(Attr);
+}
+
 static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
                                 SourceRange) {
   IdentifierLoc *PragmaNameLoc = A.getArgAsIdent(0);
   IdentifierLoc *OptionLoc = A.getArgAsIdent(1);
   IdentifierLoc *StateLoc = A.getArgAsIdent(2);
   Expr *ValueExpr = A.getArgAsExpr(3);
 
   bool PragmaUnroll = PragmaNameLoc->Ident->getName() == "unroll";
S.Diag(A.getLoc(), A.isDeclspecAttribute() ?
            diag::warn_unknown_attribute_ignored) << A.getName();
     return nullptr;
   case AttributeList::AT_FallThrough:
     return handleFallThroughAttr(S, St, A, Range);
   case AttributeList::AT_LoopHint:
     return handleLoopHintAttr(S, St, A, Range);
   case AttributeList::AT_OpenCLUnrollHint:
     return handleOpenCLUnrollHint(S, St, A, Range);
+  case AttributeList::AT_Suppress:
+    return handleSuppressAttr(S, St, A, Range);
   default:
     // if we're here, then we parsed a known attribute, but didn't recognize
     // it as a statement attribute => it is declaration attribute
     S.Diag(A.getRange().getBegin(), diag::err_decl_attribute_invalid_on_stmt)
         << A.getName() << St->getLocStart();
     return nullptr;
   }
 }