[clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay]

Summary

D80144: [clang-format] @lefticus just taught the world how to use [[unlikely]] but we forgot to teach clang-format introduce an ObjC regression

Only parse the [] if what follows is really an attribute

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

2348

This so makes me feel like we need a peekNextToken()

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

2348

which is like all the time I have to write

Tok->Next && Tok->Next->is(tok::XXX)
Tok->Previous && Tok->Previous ->is(tok::XXX)

would be so much smaller code if we had

Tok->isNext(tok::XXX)
Tok->isPrevious(tok::XXX)

[curdeius]

Minor remarks.

clang/lib/Format/UnwrappedLineParser.cpp

2346

Nit: shouldn't comments be "Full phrases."?

2348

peekNextToken() probably should be done in a separate revision, nope?
It would be good to have it IMO.

2351

Maybe add assert(Tok);. How can you be know for sure that there's a next token?

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

2348

yes I was just thinking out loud..

[MyDeveloperDay]

Check for null Tok
Fix up comments

[krasimir]

clang/lib/Format/UnwrappedLineParser.cpp

2348

I think this should be more strict and check for the sequence of 5 tokens:

tok::l_square, tok::l_square, tok::identifier, tok::r_square, tok::r_square

Unfortunately [[ may start a nested Objective-C-style method call, e.g.,

[[obj1 method1:arg1] method2:arg2]

(I'm not super familiar with Objective-C-syntax, please correct me if I'm wrong about that.)

(Only checking for the last ]], or a combination of [[ and ]] that doesn't examine the meat in-between would suffer from similar ambiguities.)

Of course, attributes can be more complicated and can have a rich internal structure (looking at https://en.cppreference.com/w/cpp/language/attributes). Consider renaming this to tryToParseSimpleAttribute to not give folks false hopes that this deals with the general problem for now (we can rename this later as we improve this C++ attribute parsing to handle more of the interesting cases).

2351

+1, but instead of assert-ing which may/will crash clang-format on weird inputs and may lead to UB below in release builds, just return false and move on.

[MyDeveloperDay]

Handle more complex nested ObjC calls
Rename function to tryParseSimpleAttributes (not supporting full capability as yet)
Use RAII object to reset the TokenPosition
utilize the fact that Objective calls shouldn't end with "]]" I think... this should allow Attributes at least of the form [[identifier::identifier]]
I feel if this isn't a good enough perhaps we also check for the ; after the second ]

[aaron.ballman]

In D80547#2054763, @MyDeveloperDay wrote:

Handle more complex nested ObjC calls
Rename function to tryParseSimpleAttributes (not supporting full capability as yet)
Use RAII object to reset the TokenPosition
utilize the fact that Objective calls shouldn't end with "]]" I think... this should allow Attributes at least of the form [[identifier::identifier]]
I feel if this isn't a good enough perhaps we also check for the ; after the second ]

I'm not an ObjC expert, but I am pretty sure you can do something like: [[foo bar] baz: i[0]]; and [[foo bar] baz: i[0]][1];

[MyDeveloperDay]

Add more tests and check if a ';' follows the final ]]

[rdwampler]

clang/lib/Format/UnwrappedLineParser.cpp

1965

From the peanut gallery, would checking TT_AttributeSquare here work (that should handle ambiguities around ObjC method calls)?

[krasimir]

clang/lib/Format/UnwrappedLineParser.cpp

2349

nit: I've seen similar RAII idioms in lib/Format use names like **Scoped**LineState - just wanted to mention that naming alternative.

2372

nit: check that has a double space

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1965

unfortunately the TT_AttributeSquare hasn't been determined at this stage

[MyDeveloperDay]

Address nits

[krasimir]

This is nice! Thank you!

Ran this through a mix of C++ and Objective-C sources and didn't spot any regressions from pre-D80144.

clang/lib/Format/UnwrappedLineParser.h

private:
   // Parse a C# generic type constraint: `where T : IComparable<T>`.
   // See:
   // https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/where-generic-type-constraint
   void parseCSharpGenericTypeConstraint();
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
+  bool tryToParseSimpleAttribute();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
   // token. For example:
   // - if the token is '{' and opens a block, LevelDifference is 1.
   // - if the token is '}' and closes a block, LevelDifference is -1.
   void nextToken(int LevelDifference = 0);
   void readToken(int LevelDifference = 0);

clang/lib/Format/UnwrappedLineParser.cpp

 void UnwrappedLineParser::parseIfThenElse() {
   assert(FormatTok->Tok.is(tok::kw_if) && "'if' expected");
   nextToken();
   if (FormatTok->Tok.isOneOf(tok::kw_constexpr, tok::identifier))
     nextToken();
   if (FormatTok->Tok.is(tok::l_paren))
     parseParens();
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseSimpleAttribute())
     parseSquare();
   bool NeedsUnwrappedLine = false;
   if (FormatTok->Tok.is(tok::l_brace)) {
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock(/*MustBeDeclaration=*/false);
     if (Style.BraceWrapping.BeforeElse)
       addUnwrappedLine();
     else
       NeedsUnwrappedLine = true;
   } else {
     addUnwrappedLine();
     ++Line->Level;
     parseStructuralElement();
     --Line->Level;
   }
   if (FormatTok->Tok.is(tok::kw_else)) {
     nextToken();
     // handle [[likely]] / [[unlikely]]
-    if (FormatTok->is(tok::l_square))
+    if (FormatTok->Tok.is(tok::l_square) && tryToParseSimpleAttribute())
       parseSquare();
     if (FormatTok->Tok.is(tok::l_brace)) {
       CompoundStatementIndenter Indenter(this, Style, Line->Level);
       parseBlock(/*MustBeDeclaration=*/false);
       addUnwrappedLine();
     } else if (FormatTok->Tok.is(tok::kw_if)) {
       parseIfThenElse();
     } else {
bool UnwrappedLineParser::parseEnum() {
   }
   return true;
 
   // There is no addUnwrappedLine() here so that we fall through to parsing a
   // structural element afterwards. Thus, in "enum A {} n, m;",
   // "} n, m;" will end up in one unwrapped line.
 }
 
+namespace {
+// A class used to set and restore the Token position when peeking
+// ahead in the token source.
+class ScopedTokenPosition {
+  unsigned StoredPosition;
+  FormatTokenSource *Tokens;
+
+public:
+  ScopedTokenPosition(FormatTokenSource *Tokens) : Tokens(Tokens) {
+    assert(Tokens && "Tokens expected to not be null");
+    StoredPosition = Tokens->getPosition();
+  }
+
+  ~ScopedTokenPosition() { Tokens->setPosition(StoredPosition); }
+};
+} // namespace
+
+// Look to see if we have [[ by looking ahead, if
+// its not then rewind to the original position.
+bool UnwrappedLineParser::tryToParseSimpleAttribute() {
+  ScopedTokenPosition AutoPosition(Tokens);
+  FormatToken *Tok = Tokens->getNextToken();
+  // We already read the first [ check for the second.
+  if (Tok && !Tok->is(tok::l_square)) {
+    return false;
+  }
+  // Double check that the attribute is just something
+  // fairly simple.
+  while (Tok) {
+    if (Tok->is(tok::r_square)) {
+      break;
+    }
+    Tok = Tokens->getNextToken();
+  }
+  Tok = Tokens->getNextToken();
+  if (Tok && !Tok->is(tok::r_square)) {
+    return false;
+  }
+  Tok = Tokens->getNextToken();
+  if (Tok && Tok->is(tok::semi)) {
+    return false;
+  }
+  return true;
+}
+
 void UnwrappedLineParser::parseJavaEnumBody() {
   // Determine whether the enum is simple, i.e. does not have a semicolon or
   // constants with class bodies. Simple enums can be formatted like braced
   // lists, contracted to a single line, etc.
   unsigned StoredPosition = Tokens->getPosition();
   bool IsSimple = true;
   FormatToken *Tok = Tokens->getNextToken();
   while (Tok) {

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, LikelyUnlikely) {
   verifyFormat("if (argc > 5) [[unlikely]] {\n"
                "  return 29;\n"
                "} else if (argc > 10) [[likely]] {\n"
                "  return 99;\n"
                "} else {\n"
                "  return 42;\n"
                "}\n",
                Style);
+
+  verifyFormat("if (argc > 5) [[gnu::unused]] {\n"
+               "  return 29;\n"
+               "}",
+               Style);
 }
 
 TEST_F(FormatTest, LLVMDefaultStyle) {
   FormatStyle Style = getLLVMStyle();
   verifyFormat("extern \"C\" {\n"
                "int foo();\n"
                "}",
                Style);

clang/unittests/Format/FormatTestObjC.cpp

TEST_F(FormatTestObjC, BreakLineBeforeNestedBlockParam) {
   Style.ColumnLimit = 80;
   verifyFormat(
       "[self.test_method a:self b:self\n"
       "           callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {\n"
       "             u = v;\n"
       "           }]");
 }
 
+TEST_F(FormatTestObjC, IfNotUnlikely) {
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
+
+  verifyFormat("if (argc < 5) [obj func:arg];");
+  verifyFormat("if (argc < 5) [[obj1 method1:arg1] method2:arg2];");
+  verifyFormat("if (argc < 5) [[foo bar] baz:i[0]];");
+  verifyFormat("if (argc < 5) [[foo bar] baz:i[0]][1];");
+
+  verifyFormat("if (argc < 5)\n"
+               "  [obj func:arg];\n"
+               "else\n"
+               "  [obj func:arg2];");
+
+  verifyFormat("if (argc < 5) [[unlikely]]\n"
+               "  [obj func:arg];\n"
+               "else [[likely]]\n"
+               "  [obj func:arg2];");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang