[clang-format] Fix crash on invalid requires expression

[HazardyKnusperkeks]

Summary

Fixes https://github.com/llvm/llvm-project/issues/54350

As driveby rename Left to OpeningParen, this gives a better overview what it is, and we have a loop in the function, where Left is not adjusted.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

424–426

This is the actual fix. It reset to many types, although the comment said what it should reset.

clang/unittests/Format/FormatTest.cpp

23894–23897

I know we don't like changing tests, but I think there was a bug before which were never hit in our tests but here.

Before

M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=130 Name=decltype L=42 PPK=2 FakeLParens=5/ FakeRParens=0 II=0xf136c40 Text='decltype'
M=0 C=0 T=Unknown S=0 F=0 B=1 BK=0 P=23 Name=l_paren L=43 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=1 T=LambdaLSquare S=0 F=0 B=0 BK=0 P=140 Name=l_square L=44 PPK=2 FakeLParens=1/ FakeRParens=0 II=0x0 Text='['
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=r_square L=45 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=l_paren L=46 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=160 Name=r_paren L=47 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
M=0 C=1 T=LambdaArrow S=1 F=0 B=0 BK=0 P=150 Name=arrow L=50 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=43 Name=identifier L=54 PPK=2 FakeLParens= FakeRParens=0 II=0xf130c60 Text='std'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=coloncolon L=56 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='::'
M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=540 Name=identifier L=65 PPK=2 FakeLParens= FakeRParens=0 II=0xf130c88 Text='true_type'
M=0 C=0 T=LambdaLBrace S=1 F=0 B=0 BK=1 P=43 Name=l_brace L=67 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'
M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=61 Name=r_brace L=80 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='}'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=l_paren L=81 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=160 Name=r_paren L=82 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text=')'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=83 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=coloncolon L=85 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='::'
M=0 C=1 T=TrailingAnnotation S=0 F=0 B=0 BK=0 P=520 Name=identifier L=90 PPK=2 FakeLParens= FakeRParens=0 II=0xf130ca8 Text='value'
M=0 C=1 T=BinaryOperator S=1 F=0 B=0 BK=0 P=25 Name=ampamp L=93 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='&&'

after

M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=130 Name=decltype L=42 PPK=2 FakeLParens=5/ FakeRParens=0 II=0xee66c40 Text='decltype'
M=0 C=0 T=TypeDeclarationParen S=0 F=0 B=1 BK=0 P=23 Name=l_paren L=43 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=0 T=LambdaLSquare S=0 F=0 B=0 BK=0 P=140 Name=l_square L=44 PPK=2 FakeLParens=1/ FakeRParens=0 II=0x0 Text='['
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=r_square L=45 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=l_paren L=46 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=160 Name=r_paren L=47 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
M=0 C=1 T=LambdaArrow S=1 F=0 B=0 BK=0 P=150 Name=arrow L=50 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=43 Name=identifier L=54 PPK=2 FakeLParens= FakeRParens=0 II=0xee60c60 Text='std'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=coloncolon L=56 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='::'
M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=540 Name=identifier L=65 PPK=2 FakeLParens= FakeRParens=0 II=0xee60c88 Text='true_type'
M=0 C=0 T=LambdaLBrace S=1 F=0 B=0 BK=1 P=43 Name=l_brace L=67 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'
M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=61 Name=r_brace L=80 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='}'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=l_paren L=81 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=160 Name=r_paren L=82 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text=')'
M=0 C=0 T=TypeDeclarationParen S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=83 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=coloncolon L=85 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='::'
M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=520 Name=identifier L=90 PPK=2 FakeLParens= FakeRParens=0 II=0xee60ca8 Text='value'
M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=25 Name=ampamp L=93 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='&&'

Because of the missing check it just reset all paren with a brace in it, in this case the TypeDeclarationParen. which results in a miss labeling of the value as an annotation.

[curdeius]

clang/lib/Format/TokenAnnotator.cpp

215

Maybe you can commit the rename as a NFC commit and rebase the patch to remove the noise?

clang/unittests/Format/FormatTest.cpp

23894–23897

Looks legit to me.

clang/unittests/Format/TokenAnnotatorTest.cpp

335

I'd prefer a valid C++ code (if it's not too complicated) if possible but that's acceptable.

[HazardyKnusperkeks]

Split rename and bug fix

clang/lib/Format/TokenAnnotator.cpp

215

Done in D121557

clang/unittests/Format/TokenAnnotatorTest.cpp

335

I thought I'd stick to the bug report. The only thing that comes to my mind does compile on gcc, but clang gives a specific error (which I support, without having looked into the standard). https://gcc.godbolt.org/z/WvT9861b6

[curdeius]

LGTM. And thanks for splitting the patch.

clang/unittests/Format/TokenAnnotatorTest.cpp

335

Ok, now I see that it's actual test code. It's perfectly fine the.

[owenpan]

After applying this patch, we got a different assertion failure on a bunch of files in clang/test, e.g.:

~/llvm-project/clang$ for f in $(find . -name \*.cpp -o -name \*.c) ; do clang-format $f > /dev/null ; done
Assertion failed: (OpeningParen.is(tok::l_paren)), function parseParens, file TokenAnnotator.cpp, line 216.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.	Program arguments: cf ./test/Driver/ignore-xcoff-visibility.cpp
...

clang/lib/Format/TokenAnnotator.cpp

216

This patch either uncovers or causes an assertion failure here on a bunch of files in clang/test.

[owenpan]

Nvm. See https://github.com/llvm/llvm-project/issues/54384.

clang/lib/Format/TokenAnnotator.cpp

216

I was wrong! Please ignore it.

[HazardyKnusperkeks]

In D121550#3381903, @owenpan wrote:

Nvm. See https://github.com/llvm/llvm-project/issues/54384.

Phew. I thought I needed a fix for the fix, for the fix, for the fix.

clang/lib/Format/TokenAnnotator.cpp

bool parseUntouchableParens() {
     }
     return false;
   }
 
   bool parseParens(bool LookForDecls = false) {
     if (!CurrentToken)
       return false;
     assert(CurrentToken->Previous && "Unknown previous token");
     FormatToken &OpeningParen = *CurrentToken->Previous;
     assert(OpeningParen.is(tok::l_paren));
     FormatToken *PrevNonComment = OpeningParen.getPreviousNonComment();
     OpeningParen.ParentBracket = Contexts.back().ContextKind;
     ScopedContextCreator ContextCreator(*this, tok::l_paren, 1);
 
     // FIXME: This is a bit of a hack. Do better.
     Contexts.back().ColonIsForRangeExpr =
         Contexts.size() == 2 && Contexts[0].ColonIsForRangeExpr;
 
while (CurrentToken) {
           OpeningParen.setPackingKind(PPK_OnePerLine);
 
         next();
         return true;
       }
       if (CurrentToken->isOneOf(tok::r_square, tok::r_brace))
         return false;
 
-      if (CurrentToken->is(tok::l_brace))
-        OpeningParen.setType(TT_Unknown); // Not TT_ObjCBlockLParen
+      if (CurrentToken->is(tok::l_brace) && OpeningParen.is(TT_ObjCBlockLParen))
+        OpeningParen.setType(TT_Unknown);
       if (CurrentToken->is(tok::comma) && CurrentToken->Next &&
           !CurrentToken->Next->HasUnescapedNewline &&
           !CurrentToken->Next->isTrailingComment())
         HasMultipleParametersOnALine = true;
       bool ProbablyFunctionTypeLParen =
           (CurrentToken->is(tok::l_paren) && CurrentToken->Next &&
            CurrentToken->Next->isOneOf(tok::star, tok::amp, tok::caret));
       if ((CurrentToken->Previous->isOneOf(tok::kw_const, tok::kw_auto) ||
            CurrentToken->Previous->isSimpleTypeSpecifier()) &&

clang/unittests/Format/FormatTest.cpp

verifyFormat("template <typename T> concept HasFoo = requires(T t) {\n"
                "                                       };",
                Style);
 
   verifyFormat("template <typename T> concept True = true;", Style);
 
   verifyFormat(
       "template <typename T> concept C = decltype([]() -> std::true_type {\n"
       "                                    return {};\n"
-      "                                  }())::value\n"
-      "                                  && requires(T t) { t.bar(); } &&\n"
-      "                                  sizeof(T) <= 8;",
+      "                                  }())::value &&\n"
+      "                                  requires(T t) { t.bar(); } && "
+      "sizeof(T) <= 8;",
       Style);
 
   verifyFormat("template <typename T> concept Semiregular =\n"
                "    DefaultConstructible<T> && CopyConstructible<T> && "
                "CopyAssignable<T> &&\n"
                "    requires(T a, std::size_t n) {\n"
                "      requires Same<T *, decltype(&a)>;\n"
                "      { a.~T() } noexcept;\n"
                "      requires Same<T *, decltype(new T)>;\n"

clang/unittests/Format/TokenAnnotatorTest.cpp

Tokens = annotate("template <typename T>\n"
                     "  requires Bar<T> && Foo<T>;\n"
                     "};");
   ASSERT_EQ(Tokens.size(), 43u) << Tokens;
   EXPECT_TOKEN(Tokens[8], tok::kw_requires, TT_RequiresExpression);
   EXPECT_TOKEN(Tokens[9], tok::l_paren, TT_RequiresExpressionLParen);
   EXPECT_TOKEN(Tokens[13], tok::l_brace, TT_RequiresExpressionLBrace);
   EXPECT_TOKEN(Tokens[29], tok::kw_requires,
                TT_RequiresClauseInARequiresExpression);
+
+  // Invalid Code, but we don't want to crash. See http://llvm.org/PR54350.
+  Tokens = annotate("bool r10 = requires (struct new_struct { int x; } s) { "
+                    "requires true; };");
+  ASSERT_EQ(Tokens.size(), 21u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::kw_requires, TT_RequiresExpression);
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_RequiresExpressionLParen);
+  EXPECT_TOKEN(Tokens[14], tok::l_brace, TT_RequiresExpressionLBrace);
 }
 
 TEST_F(TokenAnnotatorTest, RequiresDoesNotChangeParsingOfTheRest) {
   auto NumberOfAdditionalRequiresClauseTokens = 5u;
   auto NumberOfTokensBeforeRequires = 5u;
 
   auto BaseTokens = annotate("template<typename T>\n"
                              "T Pi = 3.14;");