Add new warning for compound punctuation tokens that are split across macro expansions or split by whitespace.

[rsmith]

Summary

For example:

#define FOO(x) (x)
FOO({});

... forms a statement-expression after macro expansion. This warning
applies to '({' and '})' delimiting statement-expressions, '[[' and ']]'
delimiting attributes, and '::*' introducing a pointer-to-member.

The warning for forming these compound tokens across macro expansions
(or across files!) is enabled by default; the warning for whitespace
within the tokens is not, but is included in -Wall.

[sbenza]

clang/test/Parser/compound-token-split.cpp

31

Does this count as a "different" source file?
Or is it just an implementation detail of how clang uses FileIDs ?

[rsmith]

clang/test/Parser/compound-token-split.cpp

31

:) Yes, this is a consequence of how Clang uses FileIDs.

I guess it's debatable whether the includer and included file are "different", but I'm hoping no-one will be confused by the diagnostic. (I think it's one of those "if you ever see this diagnostic in practice you win a small prize" cases...)

clang/include/clang/Basic/DiagnosticGroups.td

 def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
 def BinaryLiteral : DiagGroup<"binary-literal", [CXX14BinaryLiteral,
                                                  CXXPre14CompatBinaryLiteral,
                                                  GNUBinaryLiteral]>;
 def GNUCompoundLiteralInitializer : DiagGroup<"gnu-compound-literal-initializer">;
 def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion">;
 def BitFieldEnumConversion : DiagGroup<"bitfield-enum-conversion">;
 def BitFieldWidth : DiagGroup<"bitfield-width">;
+def CompoundTokenSplitByMacro : DiagGroup<"compound-token-split-by-macro">;
+def CompoundTokenSplitBySpace : DiagGroup<"compound-token-split-by-space">;
+def CompoundTokenSplit : DiagGroup<"compound-token-split",
+                                   [CompoundTokenSplitByMacro,
+                                    CompoundTokenSplitBySpace]>;
 def CoroutineMissingUnhandledException :
   DiagGroup<"coroutine-missing-unhandled-exception">;
 def Coroutine : DiagGroup<"coroutine", [CoroutineMissingUnhandledException]>;
 def ObjCBoolConstantConversion : DiagGroup<"objc-bool-constant-conversion">;
 def ConstantConversion : DiagGroup<"constant-conversion",
                                    [BitFieldConstantConversion,
                                     ObjCBoolConstantConversion]>;
 def LiteralConversion : DiagGroup<"literal-conversion">;

 def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
 
 // Uniqueness Analysis warnings
 def Consumed       : DiagGroup<"consumed">;
 
 // Note that putting warnings in -Wall will not disable them by default. If a
 // warning should be active _only_ when -Wall is passed in, mark it as
 // DefaultIgnore in addition to putting it here.
-def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool, MisleadingIndentation]>;
+def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool,
+                            MisleadingIndentation, CompoundTokenSplit]>;
 
 // Warnings that should be in clang-cl /w4.
 def : DiagGroup<"CL4", [All, Extra]>;
 
 // Warnings enabled by -pedantic.  This is magically filled in by TableGen.
 def Pedantic : DiagGroup<"pedantic">;
 
 // Aliases.

clang/include/clang/Basic/DiagnosticParseKinds.td

 
 def warn_misleading_indentation : Warning<
   "misleading indentation; statement is not part of "
   "the previous '%select{if|else|for|while}0'">,
   InGroup<MisleadingIndentation>, DefaultIgnore;
 def note_previous_statement : Note<
   "previous statement is here">;
 
+def subst_compound_token_kind : TextSubstitution<
+  "%select{%1 and |}0%2 tokens "
+  "%select{introducing statement expression|terminating statement expression|"
+  "introducing attribute|terminating attribute|"
+  "forming pointer to member type}3">;
+def warn_compound_token_split_by_macro : Warning<
+  "%sub{subst_compound_token_kind}0,1,2,3 appear in different "
+  "macro expansion contexts">, InGroup<CompoundTokenSplitByMacro>;
+def warn_compound_token_split_by_file : Warning<
+  "%sub{subst_compound_token_kind}0,1,2,3 appear in different source files">,
+  InGroup<CompoundTokenSplit>;
+def note_compound_token_split_second_token_here : Note<
+  "%select{|second }0%1 token is here">;
+def warn_compound_token_split_by_whitespace : Warning<
+  "%sub{subst_compound_token_kind}0,1,2,3 are separated by whitespace">,
+  InGroup<CompoundTokenSplitBySpace>, DefaultIgnore;
+
 def ext_thread_before : Extension<"'__thread' before '%0'">;
 def ext_keyword_as_ident : ExtWarn<
   "keyword '%0' will be made available as an identifier "
   "%select{here|for the remainder of the translation unit}1">,
   InGroup<KeywordCompat>;
 
 def ext_nullability : Extension<
   "type nullability specifier %0 is a Clang extension">,

clang/include/clang/Parse/Parser.h

private:
   /// Return false if the next token is an identifier. An 'expected identifier'
   /// error is emitted otherwise.
   ///
   /// The parser tries to recover from the error by checking if the next token
   /// is a C++ keyword when parsing Objective-C++. Return false if the recovery
   /// was successful.
   bool expectIdentifier();
 
+  /// Kinds of compound pseudo-tokens formed by a sequence of two real tokens.
+  enum class CompoundToken {
+    /// A '(' '{' beginning a statement-expression.
+    StmtExprBegin,
+    /// A '}' ')' ending a statement-expression.
+    StmtExprEnd,
+    /// A '[' '[' beginning a C++11 or C2x attribute.
+    AttrBegin,
+    /// A ']' ']' ending a C++11 or C2x attribute.
+    AttrEnd,
+    /// A '::' '*' forming a C++ pointer-to-member declaration.
+    MemberPtr,
+  };
+
+  /// Check that a compound operator was written in a "sensible" way, and warn
+  /// if not.
+  void checkCompoundToken(SourceLocation FirstTokLoc,
+                          tok::TokenKind FirstTokKind, CompoundToken Op);
+
 public:
   //===--------------------------------------------------------------------===//
   // Scope manipulation
 
   /// ParseScope - Introduces a new scope for parsing. The kind of
   /// scope is determined by ScopeFlags. Objects of this type should
   /// be created on the stack to coincide with the position where the
   /// parser enters the new scope, and this object's constructor will

clang/lib/Headers/altivec.h

 /* vec_dssall */
 
 static __inline__ void __attribute__((__always_inline__)) vec_dssall(void) {
   __builtin_altivec_dssall();
 }
 
 /* vec_dst */
 #define vec_dst(__PTR, __CW, __STR) \
-  __extension__(                    \
-      { __builtin_altivec_dst((const void *)(__PTR), (__CW), (__STR)); })
+  __builtin_altivec_dst((const void *)(__PTR), (__CW), (__STR))
 
 /* vec_dstst */
 #define vec_dstst(__PTR, __CW, __STR) \
-  __extension__(                      \
-      { __builtin_altivec_dstst((const void *)(__PTR), (__CW), (__STR)); })
+  __builtin_altivec_dstst((const void *)(__PTR), (__CW), (__STR))
 
 /* vec_dststt */
 #define vec_dststt(__PTR, __CW, __STR) \
-  __extension__(                       \
-      { __builtin_altivec_dststt((const void *)(__PTR), (__CW), (__STR)); })
+  __builtin_altivec_dststt((const void *)(__PTR), (__CW), (__STR))
 
 /* vec_dstt */
 #define vec_dstt(__PTR, __CW, __STR) \
-  __extension__(                     \
-      { __builtin_altivec_dstt((const void *)(__PTR), (__CW), (__STR)); })
+  __builtin_altivec_dstt((const void *)(__PTR), (__CW), (__STR))
 
 /* vec_eqv */
 
 #ifdef __POWER8_VECTOR__
 static __inline__ vector signed char __ATTRS_o_ai
 vec_eqv(vector signed char __a, vector signed char __b) {
   return (vector signed char)__builtin_vsx_xxleqv((vector unsigned int)__a,
                                                   (vector unsigned int)__b);

clang/lib/Parse/ParseDecl.cpp

if (SS.isNotEmpty()) {
         else
           AnnotateScopeToken(SS, true);
 
         if (DirectDeclParser)
           (this->*DirectDeclParser)(D);
         return;
       }
 
+      if (SS.isValid()) {
+        checkCompoundToken(SS.getEndLoc(), tok::coloncolon,
+                           CompoundToken::MemberPtr);
+      }
+
       SourceLocation StarLoc = ConsumeToken();
       D.SetRangeEnd(StarLoc);
       DeclSpec DS(AttrFactory);
       ParseTypeQualifierListOpt(DS);
       D.ExtendWithDeclSpec(DS);
 
       // Recurse to parse whatever is left.
       ParseDeclaratorInternal(D, DirectDeclParser);

clang/lib/Parse/ParseDeclCXX.cpp

if (Tok.is(tok::kw_alignas)) {
     Diag(Tok.getLocation(), diag::warn_cxx98_compat_alignas);
     ParseAlignmentSpecifier(attrs, endLoc);
     return;
   }
 
   assert(Tok.is(tok::l_square) && NextToken().is(tok::l_square) &&
          "Not a double square bracket attribute list");
 
-  Diag(Tok.getLocation(), diag::warn_cxx98_compat_attribute);
+  SourceLocation OpenLoc = Tok.getLocation();
+  Diag(OpenLoc, diag::warn_cxx98_compat_attribute);
 
   ConsumeBracket();
+  checkCompoundToken(OpenLoc, tok::l_square, CompoundToken::AttrBegin);
   ConsumeBracket();
 
   SourceLocation CommonScopeLoc;
   IdentifierInfo *CommonScopeName = nullptr;
   if (Tok.is(tok::kw_using)) {
     Diag(Tok.getLocation(), getLangOpts().CPlusPlus17
                                 ? diag::warn_cxx14_compat_using_attribute_ns
                                 : diag::ext_using_attribute_ns);
if (!AttrParsed)
           ScopeName, ScopeLoc, nullptr, 0,
           getLangOpts().CPlusPlus ? ParsedAttr::AS_CXX11 : ParsedAttr::AS_C2x);
 
     if (TryConsumeToken(tok::ellipsis))
       Diag(Tok, diag::err_cxx11_attribute_forbids_ellipsis)
         << AttrName;
   }
 
+  SourceLocation CloseLoc = Tok.getLocation();
   if (ExpectAndConsume(tok::r_square))
     SkipUntil(tok::r_square);
+  else if (Tok.is(tok::r_square))
+    checkCompoundToken(CloseLoc, tok::r_square, CompoundToken::AttrEnd);
   if (endLoc)
     *endLoc = Tok.getLocation();
   if (ExpectAndConsume(tok::r_square))
     SkipUntil(tok::r_square);
 }
 
 /// ParseCXX11Attributes - Parse a C++11 or C2x attribute-specifier-seq.
 ///

clang/lib/Parse/ParseExpr.cpp

if (BridgeCast && !getLangOpts().ObjCAutoRefCount) {
     BridgeCast = false;
   }
 
   // None of these cases should fall through with an invalid Result
   // unless they've already reported an error.
   if (ExprType >= CompoundStmt && Tok.is(tok::l_brace)) {
     Diag(Tok, diag::ext_gnu_statement_expr);
 
+    checkCompoundToken(OpenLoc, tok::l_paren, CompoundToken::StmtExprBegin);
+
     if (!getCurScope()->getFnParent() && !getCurScope()->getBlockParent()) {
       Result = ExprError(Diag(OpenLoc, diag::err_stmtexpr_file_scope));
     } else {
       // Find the nearest non-record decl context. Variables declared in a
       // statement expression behave as if they were declared in the enclosing
       // function, block, or other code construct.
       DeclContext *CodeDC = Actions.CurContext;
       while (CodeDC->isRecord() || isa<EnumDecl>(CodeDC)) {

clang/lib/Parse/ParseStmt.cpp

while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
 
     if (R.isUsable())
       Stmts.push_back(R.get());
   }
 
   SourceLocation CloseLoc = Tok.getLocation();
 
   // We broke out of the while loop because we found a '}' or EOF.
-  if (!T.consumeClose())
+  if (!T.consumeClose()) {
+    // If this is the '})' of a statement expression, check that it's written
+    // in a sensible way.
+    if (isStmtExpr && Tok.is(tok::r_paren))
+      checkCompoundToken(CloseLoc, tok::r_brace, CompoundToken::StmtExprEnd);
+  } else {
     // Recover by creating a compound statement with what we parsed so far,
-    // instead of dropping everything and returning StmtError();
+    // instead of dropping everything and returning StmtError().
+  }
+
+  if (T.getCloseLocation().isValid())
     CloseLoc = T.getCloseLocation();
 
   return Actions.ActOnCompoundStmt(T.getOpenLocation(), CloseLoc,
                                    Stmts, isStmtExpr);
 }
 
 /// ParseParenExprOrCondition:
 /// [C  ]     '(' expression ')'

clang/lib/Parse/Parser.cpp

if (II->isCPlusPlusKeyword(getLangOpts())) {
       // Objective-C++: Recover by treating this keyword as a valid identifier.
       return false;
     }
   }
   Diag(Tok, diag::err_expected) << tok::identifier;
   return true;
 }
 
+void Parser::checkCompoundToken(SourceLocation FirstTokLoc,
+                                tok::TokenKind FirstTokKind, CompoundToken Op) {
+  if (FirstTokLoc.isInvalid())
+    return;
+  SourceLocation SecondTokLoc = Tok.getLocation();
+
+  // We expect both tokens to come from the same FileID.
+  FileID FirstID = PP.getSourceManager().getFileID(FirstTokLoc);
+  FileID SecondID = PP.getSourceManager().getFileID(SecondTokLoc);
+  if (FirstID != SecondID) {
+    Diag(FirstTokLoc, (FirstTokLoc.isMacroID() || SecondTokLoc.isMacroID())
+                          ? diag::warn_compound_token_split_by_macro
+                          : diag::warn_compound_token_split_by_file)
+        << (FirstTokKind == Tok.getKind()) << FirstTokKind << Tok.getKind()
+        << static_cast<int>(Op) << SourceRange(FirstTokLoc);
+    Diag(SecondTokLoc, diag::note_compound_token_split_second_token_here)
+        << (FirstTokKind == Tok.getKind()) << Tok.getKind()
+        << SourceRange(SecondTokLoc);
+    return;
+  }
+
+  // We expect the tokens to abut.
+  if (Tok.hasLeadingSpace()) {
+    SourceLocation SpaceLoc = PP.getLocForEndOfToken(FirstTokLoc);
+    if (SpaceLoc.isInvalid())
+      SpaceLoc = FirstTokLoc;
+    Diag(SpaceLoc, diag::warn_compound_token_split_by_whitespace)
+        << (FirstTokKind == Tok.getKind()) << FirstTokKind << Tok.getKind()
+        << static_cast<int>(Op) << SourceRange(FirstTokLoc, SecondTokLoc);
+    return;
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // Error recovery.
 //===----------------------------------------------------------------------===//
 
 static bool HasFlagsSet(Parser::SkipUntilFlags L, Parser::SkipUntilFlags R) {
   return (static_cast<unsigned>(L) & static_cast<unsigned>(R)) != 0;
 }
 

clang/test/Misc/warning-wall.c

 CHECK-NEXT:    -Wbitwise-op-parentheses
 CHECK-NEXT:    -Wshift-op-parentheses
 CHECK-NEXT:    -Woverloaded-shift-op-parentheses
 CHECK-NEXT:    -Wparentheses-equality
 CHECK-NEXT:    -Wdangling-else
 CHECK-NEXT:  -Wswitch
 CHECK-NEXT:  -Wswitch-bool
 CHECK-NEXT:  -Wmisleading-indentation
+CHECK-NEXT:  -Wcompound-token-split
+CHECK-NEXT:    -Wcompound-token-split-by-macro
+CHECK-NEXT:    -Wcompound-token-split-by-space
 
 
 CHECK-NOT:-W

clang/test/Parser/compound-token-split.cpp

+// RUN: %clang_cc1 %s -verify
+// RUN: %clang_cc1 %s -verify=expected,space -Wcompound-token-split
+// RUN: %clang_cc1 %s -verify=expected,space -Wall
+
+#ifdef LSQUARE
+[ // expected-note {{second '[' token is here}}
+#else
+
+#define VAR(type, name, init) type name = (init)
+
+void f() {
+  VAR(int, x, {}); // #1
+  // expected-warning@#1 {{'(' and '{' tokens introducing statement expression appear in different macro expansion contexts}}
+  // expected-note-re@#1 {{{{^}}'{' token is here}}
+  //
+  // FIXME: It would be nice to suppress this when we already warned about the opening '({'.
+  // expected-warning@#1 {{'}' and ')' tokens terminating statement expression appear in different macro expansion contexts}}
+  // expected-note-re@#1 {{{{^}}')' token is here}}
+  //
+  // expected-error@#1 {{cannot initialize a variable of type 'int' with an rvalue of type 'void'}}
+}
+
+#define RPAREN )
+
+int f2() {
+  int n = ({ 1; }RPAREN; // expected-warning {{'}' and ')' tokens terminating statement expression appear in different macro expansion contexts}} expected-note {{')' token is here}}
+  return n;
+}
+
+[ // expected-warning-re {{{{^}}'[' tokens introducing attribute appear in different source files}}
+#define LSQUARE
+#include __FILE__
+  noreturn ]]  void g();
+
+[[noreturn] ] void h(); // space-warning-re {{{{^}}']' tokens terminating attribute are separated by whitespace}}
+
+struct X {};
+int X:: *p; // space-warning {{'::' and '*' tokens forming pointer to member type are separated by whitespace}}
+
+#endif