[clang-format] Do not parse certain characters in pragma directives

[jhuber6]

Summary

Currently, we parse lines inside of a compiler #pragma the same way we
parse any other line. This is fine for some cases, like separating
expressions and adding proper spacing, but in others it causes some poor
results from miscategorizing some tokens.

For example, the OpenMP offloading uses certain clauses that contain
special characters like map(tofrom : A[0:N]). This will be formatted
poorly as it will be split between lines on the first colon.
Additionally the subscript notation will lead to poor spacing. This can
be seen in the OpenMP tests as the automatic clang formatting with
inevitably ruin the formatting.

For example, the following contrived example will be formatted poorly.

#pragma omp target teams distribute collapse(2) map(to: A[0 : M * K])  \
    map(to: B[0:K * N]) map(tofrom:C[0:M*N]) firstprivate(Alpha) \
    firstprivate(Beta) firstprivate(X) firstprivate(D) firstprivate(Y) \
    firstprivate(E) firstprivate(Z) firstprivate(F)

This results in this when formatted, which is far from ideal.

#pragma omp target teams distribute collapse(2) map(to                         \
                                                    : A [0:M * K])             \
    map(to                                                                     \
        : B [0:K * N]) map(tofrom                                              \
                           : C [0:M * N]) firstprivate(Alpha)                  \
        firstprivate(Beta) firstprivate(X) firstprivate(D) firstprivate(Y)     \
            firstprivate(E) firstprivate(Z) firstprivate(F)

This patch seeks to improve this by adding extra logic where the parsing goes
awry. This is primarily caused by the colon being parsed as an inline-asm
directive and the brackes an objective-C expressions. Also the line gets
indented every single time the line is dropped.

This doesn't implement true parsing handling for OpenMP statements.

[MyDeveloperDay]

Pretty interesting, it looks ok from what I can tell, let the others take a look

[jhuber6]

In D136100#3863427, @MyDeveloperDay wrote:

Pretty interesting, it looks ok from what I can tell, let the others take a look

Thanks, I was originally hoping I could avoid adding a new boolean for InPragma by asking something like Line.startswith(tok::pp_pragma) but that didn't seem to work.

[hans]

Chromium is seeing a formatting regression after this: https://github.com/llvm/llvm-project/issues/59473

[jhuber6]

In D136100#3988424, @hans wrote:

Chromium is seeing a formatting regression after this: https://github.com/llvm/llvm-project/issues/59473

My guess is from this line, we could be more specific on the type of pragma.

if (State.Line->InPragmaDirective)
  return CurrentState.Indent + Style.ContinuationIndentWidth;

[MyDeveloperDay]

In D136100#3988424, @hans wrote:

Chromium is seeing a formatting regression after this: https://github.com/llvm/llvm-project/issues/59473

Can we get more specific about what Chromium is seeing?

[thakis]

Can we get more specific about what Chromium is seeing?

https://github.com/llvm/llvm-project/issues/59473 has a standalone repro. What else would you like to see?

[HazardyKnusperkeks]

I think you need to parse the first paren as whole and do not indent it.

[owenpan]

Should we revert this patch?

clang/unittests/Format/FormatTest.cpp

5175

Why was this test case changed? It seemed to be related to the regression mentioned in D136100#3988574.

[jhuber6]

I would prefer that this doesn't get reverted, see the summary for the awful results for OpenMP without this patch. A potential solution would be to parse the next token and only add the indent if it's omp.

clang/unittests/Format/FormatTest.cpp

5175

It's definitely related, we want some indentation for successive OpenMP clauses pushed to a new line, see below.

clang/lib/Format/ContinuationIndenter.cpp

if (CurrentState.StartOfArraySubscripts != 0) {
       return CurrentState.StartOfArraySubscripts;
     } else if (Style.isCSharp()) { // C# allows `["key"] = value` inside object
                                    // initializers.
       return CurrentState.Indent;
     }
     return ContinuationIndent;
   }
 
+  if (State.Line->InPragmaDirective)
+    return CurrentState.Indent + Style.ContinuationIndentWidth;
+
   // This ensure that we correctly format ObjC methods calls without inputs,
   // i.e. where the last element isn't selector like: [callee method];
   if (NextNonComment->is(tok::identifier) && NextNonComment->FakeRParens == 0 &&
       NextNonComment->Next && NextNonComment->Next->is(TT_ObjCMethodExpr)) {
     return CurrentState.Indent;
   }
 
   if (NextNonComment->isOneOf(TT_StartOfName, TT_PointerOrReference) ||

clang/lib/Format/TokenAnnotator.h

 };
 
 class AnnotatedLine {
 public:
   AnnotatedLine(const UnwrappedLine &Line)
       : First(Line.Tokens.front().Tok), Level(Line.Level),
         MatchingOpeningBlockLineIndex(Line.MatchingOpeningBlockLineIndex),
         MatchingClosingBlockLineIndex(Line.MatchingClosingBlockLineIndex),
-        InPPDirective(Line.InPPDirective), InMacroBody(Line.InMacroBody),
+        InPPDirective(Line.InPPDirective),
+        InPragmaDirective(Line.InPragmaDirective),
+        InMacroBody(Line.InMacroBody),
         MustBeDeclaration(Line.MustBeDeclaration), MightBeFunctionDecl(false),
         IsMultiVariableDeclStmt(false), Affected(false),
         LeadingEmptyLinesAffected(false), ChildrenAffected(false),
         IsContinuation(Line.IsContinuation),
         FirstStartColumn(Line.FirstStartColumn) {
     assert(!Line.Tokens.empty());
 
     // Calculate Next and Previous for all tokens. Note that we must overwrite
public:
 
   SmallVector<AnnotatedLine *, 0> Children;
 
   LineType Type;
   unsigned Level;
   size_t MatchingOpeningBlockLineIndex;
   size_t MatchingClosingBlockLineIndex;
   bool InPPDirective;
+  bool InPragmaDirective;
   bool InMacroBody;
   bool MustBeDeclaration;
   bool MightBeFunctionDecl;
   bool IsMultiVariableDeclStmt;
 
   /// \c True if this line should be formatted, i.e. intersects directly or
   /// indirectly with one of the input ranges.
   bool Affected;

clang/lib/Format/TokenAnnotator.cpp

while (CurrentToken) {
         return false;
       if (CurrentToken->is(tok::colon)) {
         if (IsCpp11AttributeSpecifier &&
             CurrentToken->endsSequence(tok::colon, tok::identifier,
                                        tok::kw_using)) {
           // Remember that this is a [[using ns: foo]] C++ attribute, so we
           // don't add a space before the colon (unlike other colons).
           CurrentToken->setType(TT_AttributeColon);
-        } else if (!Style.isVerilog() &&
+        } else if (!Style.isVerilog() && !Line.InPragmaDirective &&
                    Left->isOneOf(TT_ArraySubscriptLSquare,
                                  TT_DesignatedInitializerLSquare)) {
           Left->setType(TT_ObjCMethodExpr);
           StartsObjCMethodExpr = true;
           Contexts.back().ColonIsObjCMethodExpr = true;
           if (Parent && Parent->is(tok::r_paren)) {
             // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
             Parent->setType(TT_CastRParen);
case tok::colon:
         }
       } else if (canBeObjCSelectorComponent(*Tok->Previous) && Tok->Next &&
                  (Tok->Next->isOneOf(tok::r_paren, tok::comma) ||
                   (canBeObjCSelectorComponent(*Tok->Next) && Tok->Next->Next &&
                    Tok->Next->Next->is(tok::colon)))) {
         // This handles a special macro in ObjC code where selectors including
         // the colon are passed as macro arguments.
         Tok->setType(TT_ObjCMethodExpr);
-      } else if (Contexts.back().ContextKind == tok::l_paren) {
+      } else if (Contexts.back().ContextKind == tok::l_paren &&
+                 !Line.InPragmaDirective) {
         Tok->setType(TT_InlineASMColon);
       }
       break;
     case tok::pipe:
     case tok::amp:
       // | and & in declarations/type expressions represent union and
       // intersection types, respectively.
       if (Style.isJavaScript() && !Contexts.back().IsExpression)

clang/lib/Format/UnwrappedLineParser.h

struct UnwrappedLine {
   /// The \c Tokens comprising this \c UnwrappedLine.
   std::list<UnwrappedLineNode> Tokens;
 
   /// The indent level of the \c UnwrappedLine.
   unsigned Level;
 
   /// Whether this \c UnwrappedLine is part of a preprocessor directive.
   bool InPPDirective;
+  /// Whether this \c UnwrappedLine is part of a pramga directive.
+  bool InPragmaDirective;
   /// Whether it is part of a macro body.
   bool InMacroBody;
 
   bool MustBeDeclaration;
 
   /// \c True if this line should be indented by ContinuationIndent in
   /// addition to the normal indention level.
   bool IsContinuation = false;
private:
   void parseChildBlock(bool CanContainBracedList = true,
                        TokenType NextLBracesType = TT_Unknown);
   void parsePPDirective();
   void parsePPDefine();
   void parsePPIf(bool IfDef);
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();
+  void parsePPPragma();
   void parsePPUnknown();
   void readTokenWithJavaScriptASI();
   void parseStructuralElement(bool IsTopLevel = false,
                               TokenType NextLBracesType = TT_Unknown,
                               IfStmtKind *IfKind = nullptr,
                               FormatToken **IfLeftBrace = nullptr,
                               bool *HasDoWhile = nullptr,
                               bool *HasLabel = nullptr);
struct UnwrappedLineNode {
   UnwrappedLineNode() : Tok(nullptr) {}
   UnwrappedLineNode(FormatToken *Tok) : Tok(Tok) {}
 
   FormatToken *Tok;
   SmallVector<UnwrappedLine, 0> Children;
 };
 
 inline UnwrappedLine::UnwrappedLine()
-    : Level(0), InPPDirective(false), InMacroBody(false),
-      MustBeDeclaration(false), MatchingOpeningBlockLineIndex(kInvalidIndex) {}
+    : Level(0), InPPDirective(false), InPragmaDirective(false),
+      InMacroBody(false), MustBeDeclaration(false),
+      MatchingOpeningBlockLineIndex(kInvalidIndex) {}
 
 } // end namespace format
 } // end namespace clang
 
 #endif

clang/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parsePPDirective() {
   case tok::pp_elifdef:
   case tok::pp_elifndef:
   case tok::pp_elif:
     parsePPElIf();
     break;
   case tok::pp_endif:
     parsePPEndIf();
     break;
+  case tok::pp_pragma:
+    parsePPPragma();
+    break;
   default:
     parsePPUnknown();
     break;
   }
 }
 
 void UnwrappedLineParser::conditionalCompilationCondition(bool Unreachable) {
   size_t Line = CurrentLines->size();
void UnwrappedLineParser::parsePPDefine() {
   // Errors during a preprocessor directive can only affect the layout of the
   // preprocessor directive, and thus we ignore them. An alternative approach
   // would be to use the same approach we use on the file level (no
   // re-indentation if there was a structural error) within the macro
   // definition.
   parseFile();
 }
 
+void UnwrappedLineParser::parsePPPragma() {
+  Line->InPragmaDirective = true;
+  parsePPUnknown();
+}
+
 void UnwrappedLineParser::parsePPUnknown() {
   do {
     nextToken();
   } while (!eof());
   if (Style.IndentPPDirectives != FormatStyle::PPDIS_None)
     Line->Level += PPBranchLevel + 1;
   addUnwrappedLine();
 }

clang/unittests/Format/FormatTest.cpp

verifyFormat("#define MACRO(a) \\\n"
                "    f();         \\\n"
                "  else           \\\n"
                "    g()",
                getLLVMStyleWithColumns(18));
   verifyFormat("#define A template <typename T>");
   verifyIncompleteFormat("#define STR(x) #x\n"
                          "f(STR(this_is_a_string_literal{));");
   verifyFormat("#pragma omp threadprivate( \\\n"
-               "    y)), // expected-warning",
+               "        y)), // expected-warning",
                getLLVMStyleWithColumns(28));
   verifyFormat("#d, = };");
   verifyFormat("#if \"a");
   verifyIncompleteFormat("({\n"
                          "#define b     \\\n"
                          "  }           \\\n"
                          "  a\n"
                          "a",
TEST_F(FormatTest, UnderstandsPragmas) {
   verifyFormat("#pragma omp reduction(+ : var)");
 
   EXPECT_EQ("#pragma mark Any non-hyphenated or hyphenated string "
             "(including parentheses).",
             format("#pragma    mark   Any non-hyphenated or hyphenated string "
                    "(including parentheses)."));
 }
 
+TEST_F(FormatTest, UnderstandsPragmaOmpTarget) {
+  verifyFormat("#pragma omp target map(to : var)");
+  verifyFormat("#pragma omp target map(to : var[ : N])");
+  verifyFormat("#pragma omp target map(to : var[0 : N])");
+  verifyFormat("#pragma omp target map(always, to : var[0 : N])");
+
+  EXPECT_EQ(
+      "#pragma omp target       \\\n"
+      "    reduction(+ : var)   \\\n"
+      "    map(to : A[0 : N])   \\\n"
+      "    map(to : B[0 : N])   \\\n"
+      "    map(from : C[0 : N]) \\\n"
+      "    firstprivate(i)      \\\n"
+      "    firstprivate(j)      \\\n"
+      "    firstprivate(k)",
+      format(
+          "#pragma omp target reduction(+:var) map(to:A[0:N]) map(to:B[0:N]) "
+          "map(from:C[0:N]) firstprivate(i) firstprivate(j) firstprivate(k)",
+          getLLVMStyleWithColumns(26)));
+}
+
 TEST_F(FormatTest, UnderstandPragmaOption) {
   verifyFormat("#pragma option -C -A");
 
   EXPECT_EQ("#pragma option -C -A", format("#pragma    option   -C   -A"));
 }
 
 TEST_F(FormatTest, UnderstandPragmaRegion) {
   auto Style = getLLVMStyleWithColumns(0);