[clang-format] [PR45198] deletes whitespace inside of a noexcept specifier

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=45198

The following:

template<
    typename T,
    enable_if_t<is_move_assignable<T>::value> = true,
    enable_if_t<is_move_constructible<T>::value> = true>
constexpr void
swap(T &lhs, T &rhs) noexcept(
    is_nothrow_move_constructible<T>::value && is_nothrow_move_assignable<T>::value)

Results in this:

template<
    typename T,
    enable_if_t<is_move_assignable<T>::value> = true,
    enable_if_t<is_move_constructible<T>::value> = true>
constexpr void
swap(T &lhs, T &rhs) noexcept(
    is_nothrow_move_constructible<T>::value &&is_nothrow_move_assignable<T>::value)

This is because the && in is_nothrow_move_constructible<T>::value &&is_nothrow_move_assignable<T>::value gets incorrectly determined to be a TT_PointerOrReference

This revision attempts to detect determine a cases where this cannot be true especially in a noexcept context where the result is expected to be boolean

[JakeMerdichAMD]

I mainly have concerns with the search for noexcept being too brittle and not looking far enough back. Implementing that may have performance implications though, so I have no issue ignoring that problem if it's proven sufficiently rare.

Also: should this do the same thing for throw()? It has pretty much the same constraints/context and would be trivial to add, although it's deprecated and/or removed in recent C++ versions. I'm not particularly inclined to implement that, but the question ought to be raised since it theoretically has the same problem.

clang/lib/Format/TokenAnnotator.cpp

1933

Doesn't this not handle nested templates? For example, noexcept(Foo<Bar<T>>::value && Baz) would not work here. Ditto on expressions like Foo<T1, T2>.

I'm having trouble thinking of a clean generic solution, but the most robust thing I can think of is to search the current line for the previous opening parens and check if noexcept is before it (if they use multiple sets of parens, the other heuristics seem to cover it).

clang/lib/Format/TokenAnnotator.cpp

TokenType determineStarAmpUsage(const FormatToken &Tok, bool IsExpression,
     // This catches some cases where evaluation order is used as control flow:
     //   aaa && aaa->f();
     if (NextToken->Tok.isAnyIdentifier()) {
       const FormatToken *NextNextToken = NextToken->getNextNonComment();
       if (NextNextToken && NextNextToken->is(tok::arrow))
         return TT_BinaryOperator;
     }
 
-    // It is very unlikely that we are going to find a pointer or reference type
-    // definition on the RHS of an assignment.
+    // Try to distinguish (A && B) from (A &&b), which is ambiguous without
+    // other semantic information
+    // However in a noexcept context where it is going to be a boolean
+    // operation noexcept(A && b) resulting in a binary operation
+    if (Tok.is(tok::ampamp) && PrevToken &&
+        PrevToken->isOneOf(tok::identifier, TT_TemplateCloser) && NextToken) {
+      const FormatToken *NextNextToken = NextToken->getNextNonComment();
+      const FormatToken *PrevPrevToken = PrevToken->getPreviousNonComment();
+      if (NextToken->is(tok::identifier)) {
+        if (NextNextToken && NextNextToken->isOneOf(tok::less, tok::coloncolon))
+          return TT_BinaryOperator;
+        if (PrevPrevToken) {
+          const FormatToken *PrevPrevPrevToken =
+              PrevPrevToken->getPreviousNonComment();
+          // We already know its `x y identifier && identifer z`
+          // just need to confirm x,y,z
+          if (PrevPrevPrevToken && PrevPrevPrevToken->is(tok::kw_noexcept) &&
+              PrevPrevToken->is(tok::l_paren) &&
+              NextNextToken->is(tok::r_paren))
+            return TT_BinaryOperator;
+        }
+      }
+    }
+
+    // It is very unlikely that we are going to find a pointer or reference
+    // type definition on the RHS of an assignment.
     if (IsExpression && !Contexts.back().CaretFound)
       return TT_BinaryOperator;
 
     return TT_PointerOrReference;
   }
 
   TokenType determinePlusMinusCaretUsage(const FormatToken &Tok) {
     const FormatToken *PrevToken = Tok.getPreviousNonComment();
private:
   SmallVector<Context, 8> Contexts;
 
   const FormatStyle &Style;
   AnnotatedLine &Line;
   FormatToken *CurrentToken;
   bool AutoFound;
   const AdditionalKeywords &Keywords;
 
-  // Set of "<" tokens that do not open a template parameter list. If parseAngle
-  // determines that a specific token can't be a template opener, it will make
-  // same decision irrespective of the decisions for tokens leading up to it.
-  // Store this information to prevent this from causing exponential runtime.
+  // Set of "<" tokens that do not open a template parameter list. If
+  // parseAngle determines that a specific token can't be a template opener,
+  // it will make same decision irrespective of the decisions for tokens
+  // leading up to it. Store this information to prevent this from causing
+  // exponential runtime.
   llvm::SmallPtrSet<FormatToken *, 16> NonTemplateLess;
-};
+}; // namespace
 
 static const int PrecedenceUnaryOperator = prec::PointerToMember + 1;
 static const int PrecedenceArrowAndPeriod = prec::PointerToMember + 2;
 
 /// Parses binary expressions by inserting fake parenthesis based on
 /// operator precedence.
 class ExpressionParser {
 public:
   ExpressionParser(const FormatStyle &Style, const AdditionalKeywords &Keywords,
                    AnnotatedLine &Line)
       : Style(Style), Keywords(Keywords), Current(Line.First) {}
 
   /// Parse expressions with the given operator precedence.
   void parse(int Precedence = 0) {
     // Skip 'return' and ObjC selector colons as they are not part of a binary
     // expression.
     while (Current && (Current->is(tok::kw_return) ||
                        (Current->is(tok::colon) &&
                         Current->isOneOf(TT_ObjCMethodExpr, TT_DictLiteral))))
       next();
 
     if (!Current || Precedence > PrecedenceArrowAndPeriod)
       return;
 
-    // Conditional expressions need to be parsed separately for proper nesting.
+    // Conditional expressions need to be parsed separately for proper
+    // nesting.
     if (Precedence == prec::Conditional) {
       parseConditionalExpr();
       return;
     }
 
     // Parse unary operators, which all have a higher precedence than binary
     // operators.
     if (Precedence == PrecedenceUnaryOperator) {
while (Current) {
           (CurrentPrecedence != -1 && CurrentPrecedence < Precedence) ||
           (CurrentPrecedence == prec::Conditional &&
            Precedence == prec::Assignment && Current->is(tok::colon))) {
         break;
       }
 
       // Consume scopes: (), [], <> and {}
       if (Current->opensScope()) {
-        // In fragment of a JavaScript template string can look like '}..${' and
-        // thus close a scope and open a new one at the same time.
+        // In fragment of a JavaScript template string can look like '}..${'
+        // and thus close a scope and open a new one at the same time.
         while (Current && (!Current->closesScope() || Current->opensScope())) {
           next();
           parse();
         }
         next();
       } else {
         // Operator found.
         if (CurrentPrecedence == Precedence) {
while (Current &&
       Current = Current->Next;
   }
 
   const FormatStyle &Style;
   const AdditionalKeywords &Keywords;
   FormatToken *Current;
 };
 
-} // end anonymous namespace
+} // namespace
 
 void TokenAnnotator::setCommentLineLevels(
     SmallVectorImpl<AnnotatedLine *> &Lines) {
   const AnnotatedLine *NextNonCommentLine = nullptr;
   for (SmallVectorImpl<AnnotatedLine *>::reverse_iterator I = Lines.rbegin(),
                                                           E = Lines.rend();
        I != E; ++I) {
     bool CommentLine = true;
bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   if ((Left.is(tok::l_brace) && Left.BlockKind != BK_Block) ||
       (Right.is(tok::r_brace) && Right.MatchingParen &&
        Right.MatchingParen->BlockKind != BK_Block))
     return Style.Cpp11BracedListStyle ? Style.SpacesInParentheses : true;
   if (Left.is(TT_BlockComment))
     // No whitespace in x(/*foo=*/1), except for JavaScript.
     return Style.Language == FormatStyle::LK_JavaScript ||
            !Left.TokenText.endswith("=*/");
+
+  // Space between template and attribute.
+  // e.g. template <typename T> [[nodiscard]] ...
+  if (Left.is(TT_TemplateCloser) && Right.is(TT_AttributeSquare))
+    return true;
   if (Right.is(tok::l_paren)) {
     if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
         (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
       return true;
     if (Style.SpaceBeforeParens ==
             FormatStyle::SBPO_ControlStatementsExceptForEachMacros &&
         Left.is(TT_ForEachMacro))
       return false;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsSquareAttributes) {
   verifyFormat("int a = std::vector<int>{1, 2, 3}[0];");
 
   // Make sure that we do not mistake Objective-C method inside array literals
   // as attributes, even if those method names are also keywords.
   verifyFormat("@[ [foo bar] ];");
   verifyFormat("@[ [NSArray class] ];");
   verifyFormat("@[ [foo enum] ];");
 
+  verifyFormat("template <typename T> [[nodiscard]] int a() { return 1; }");
+
   // Make sure we do not parse attributes as lambda introducers.
   FormatStyle MultiLineFunctions = getLLVMStyle();
   MultiLineFunctions.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
   verifyFormat("[[unused]] int b() {\n"
                "  return 42;\n"
                "}\n",
                MultiLineFunctions);
 }
TEST_F(FormatTest, UnderstandsRvalueReferences) {
   verifyFormat("template <bool B, bool C> class A {\n"
                "  static_assert(B && C, \"Something is wrong\");\n"
                "};");
   verifyGoogleFormat("#define IF(a, b, c) if (a && (b == c))");
   verifyGoogleFormat("#define WHILE(a, b, c) while (a && (b == c))");
   verifyFormat("#define A(a, b) (a && b)");
 }
 
+TEST_F(FormatTest, BinaryOperationsInANoExceptContext) {
+  verifyFormat("int f(int &&a) {}");
+  verifyFormat("template <typename T> void swap() noexcept(Bar<T> && Foo<T>);");
+  verifyFormat(
+      "template <typename T> void swap() noexcept(Bar<T>::value && Foo<T>);");
+  verifyFormat("template <typename T> void swap() noexcept(Bar<T>::value && "
+               "Foo<T>::value);");
+  verifyFormat("template <typename T> void foo(Bar<T>::value &&b);");
+  verifyFormat("template <typename T> void swap() noexcept(Bar<T>::value && "
+               "std::Foo<T>);");
+  verifyFormat(
+      "template <typename T> void foo(Bar<T>::value &&a, Foo<T> &&b);");
+  verifyFormat("template <typename T> void swap() noexcept(Bar<T> &&b);");
+  verifyFormat(
+      "template <typename T> void swap() noexcept(Bar<T> && std::Foo<T>);");
+  verifyFormat("template <typename T> void foo(Bar<T> &&a, Foo<T> &&b);");
+  verifyFormat("template <typename T> void swap() noexcept(Bar && b);");
+  verifyFormat(
+      "template <typename T> void swap() noexcept(Bar && std::Foo<T>);");
+  verifyFormat("template <typename T> void foo(Bar &&a, Foo<T> &&b);");
+  verifyFormat(
+      "template <typename T> void swap() noexcept(Bar<T>() && Foo<T>());");
+
+  verifyFormat("template <typename T> void swap() noexcept(a && b);");
+  verifyFormat("template <typename T> void swap() noexcept(/*A*/ a && b);");
+  verifyFormat("template <typename T> void swap() noexcept(a && b /*A*/);");
+  verifyFormat(
+      "template <typename T> void swap() noexcept(/*A*/ a && /*A*/ b);");
+  verifyFormat("template <typename T> void foo(Bar &&b);");
+}
+
 TEST_F(FormatTest, FormatsBinaryOperatorsPrecedingEquals) {
   verifyFormat("void f() {\n"
                "  x[aaaaaaaaa -\n"
                "    b] = 23;\n"
                "}",
                getLLVMStyleWithColumns(15));
 }