[clang-format] Fix WhitespaceSensitiveMacros not being honoured when macro closing parenthesis is followed by a newline.

[curdeius]

Summary

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

This fixes regression introduced in https://github.com/llvm/llvm-project/commit/5e5efd8a91f2e340e79a73bedbc6ab66ad4a4281.

Before the culprit commit, macros in WhitespaceSensitiveMacros were correctly formatted even if their closing parenthesis weren't followed by semicolon (or, to be precise, when they were followed by a newline).
That commit changed the type of the macro token type from TT_UntouchableMacroFunc to TT_FunctionLikeOrFreestandingMacro.

Correct formatting (with WhitespaceSensitiveMacros = ['FOO']):

FOO(1+2)
FOO(1+2);

Regressed formatting:

FOO(1 + 2)
FOO(1+2);

[ksyx]

clang/lib/Format/FormatTokenLexer.h

117–120 ↗

(On Diff #422494)

Would making constructor of struct MacroTokenInfo having default parameter or overloading it help avoiding the change of adding , /*Finalized=*/false to the existing initializer lists?

[curdeius]

clang/lib/Format/FormatTokenLexer.h

117–120 ↗

(On Diff #422494)

I've thought about it, but it would mean that we have a non-explicit 1-arg ctor. I'm not a big fan of these as they trigger implicit conversions.
I can do though:

struct MacroTokenInfo {
  TokenType Type;
  bool Finalized{false};
};

but we'd still need adding braces in:

Macros.insert({Identifier, {TT_ForEachMacro}});

[ksyx]

clang/lib/Format/FormatTokenLexer.h

117–120 ↗

(On Diff #422494)

Yes they are both good point to consider and my start point is just that the finalized property is less frequently be true.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1791

Can we simply do this and leave FormatTokenLexer alone?

clang/unittests/Format/FormatTest.cpp

23611

Do we really need this test case?

[curdeius]

clang/lib/Format/UnwrappedLineParser.cpp

1791

We can too. It seemed hacky to me because we can miss TT_UntouchableMacroFunc in other places.
Setting the token type finalized in the lexer will avoid such problems in the future.
I'm okay however to just apply your suggestion.

clang/unittests/Format/FormatTest.cpp

23611

Not really. I just wrote it to cover both cases but it's covered by existing cases indeed. Will remove.

[owenpan]

clang/lib/Format/FormatTokenLexer.cpp

1034–1035

It seems we can simply do this and leave the rest of FormatTokenLexer alone.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1791

We can too. It seemed hacky to me because we can miss TT_UntouchableMacroFunc in other places.
Setting the token type finalized in the lexer will avoid such problems in the future.

Yeah. Please see my comment above though.

[HazardyKnusperkeks]

clang/lib/Format/FormatTokenLexer.cpp

1034–1035

+1

clang/lib/Format/FormatTokenLexer.h

117–120 ↗

(On Diff #422494)

I wouldn't add a CTor. And I also wouldn't add a default initializer. But the latter is better.

[curdeius]

Simplify. Address comments.

[curdeius]

clang/unittests/Format/FormatTest.cpp

23611

On a second thought, we don't have any other test with a semicolon and a newline, so I'd rather keep this test.

[owenpan]

LGTM

[ksyx]

Thanks!

[krasimir]

It looks like this regressed the following example by adding an unwanted level of indentation to the #elif B branch:

% ./clang-format --version
clang-format version 15.0.0 (https://github.com/llvm/llvm-project.git 50cd52d9357224cce66a9e00c9a0417c658a5655)
% cat test.cc             
#define MACRO_BEGIN

MACRO_BEGIN

namespace internal {

#if A
int f() { return 0; }
#elif B
int f() { return 1; }
#endif

}  // namespace internal
% ./clang-format test.cc
#define MACRO_BEGIN

MACRO_BEGIN

namespace internal {

#if A
int f() { return 0; }
#elif B
  int f() { return 1; }
#endif

} // namespace internal
%

@curdeius could you please take a look?

[curdeius]

In D123676#3515949, @krasimir wrote:

It looks like this regressed the following example by adding an unwanted level of indentation to the #elif B branch:

Sure, I'll have a look.
It seems that even this:

MACRO_BEGIN
#if A
int f();
#else
int f();
#endif

gets misindented:

MACRO_BEGIN
#if A
int f();
#else
    int
    f();
#endif

[krasimir]

We found another regression with this in wrongly indenting/not putting on its own line ObjC @interface:

% ./clang-format --version           
clang-format version 15.0.0 (https://github.com/llvm/llvm-project.git 50cd52d9357224cce66a9e00c9a0417c658a5655)
% cat test.m
NS_SWIFT_NAME(A)
@interface B : C
@property(readonly) D value;
@end

% ./clang-format test.m
NS_SWIFT_NAME(A) @interface B : C
@property(readonly) D value;
@end
%

[curdeius]

Reverted for now.

[owenpan]

Fixed in D132001.

clang/lib/Format/FormatTokenLexer.cpp

FormatToken *FormatTokenLexer::getNextToken() {
   }
 
   if (Style.isCpp()) {
     auto it = Macros.find(FormatTok->Tok.getIdentifierInfo());
     if (!(Tokens.size() > 0 && Tokens.back()->Tok.getIdentifierInfo() &&
           Tokens.back()->Tok.getIdentifierInfo()->getPPKeywordID() ==
               tok::pp_define) &&
         it != Macros.end()) {
+      if (it->second == TT_UntouchableMacroFunc)
+        FormatTok->setFinalizedType(TT_UntouchableMacroFunc);
+      else
         FormatTok->setType(it->second);
       if (it->second == TT_IfMacro) {
         // The lexer token currently has type tok::kw_unknown. However, for this
         // substitution to be treated correctly in the TokenAnnotator, faking
         // the tok value seems to be needed. Not sure if there's a more elegant
         // way.
         FormatTok->Tok.setKind(tok::kw_if);
       }
     } else if (FormatTok->is(tok::identifier)) {
       if (MacroBlockBeginRegex.match(Text))
         FormatTok->setType(TT_MacroBlockBegin);

clang/lib/Format/UnwrappedLineParser.cpp

case tok::identifier: {
           parseParens();
 
         bool FollowedByNewline =
             CommentsBeforeNextToken.empty()
                 ? FormatTok->NewlinesBefore > 0
                 : CommentsBeforeNextToken.front()->NewlinesBefore > 0;
 
         if (FollowedByNewline && (Text.size() >= 5 || FunctionLike) &&
-            tokenCanStartNewLine(*FormatTok) && Text == Text.upper()) {
+            tokenCanStartNewLine(*FormatTok) && Text == Text.upper() &&
+            !PreviousToken->isTypeFinalized()) {
           PreviousToken->setFinalizedType(TT_FunctionLikeOrFreestandingMacro);
           addUnwrappedLine();
           return;
         }
       }
       break;
     }
     case tok::equal:

clang/unittests/Format/FormatTest.cpp

 }
 
 TEST_F(FormatTest, WhitespaceSensitiveMacros) {
   FormatStyle Style = getLLVMStyle();
   Style.WhitespaceSensitiveMacros.push_back("FOO");
 
   // Don't use the helpers here, since 'mess up' will change the whitespace
   // and these are all whitespace sensitive by definition
+
+  // Newlines are important here.
+  EXPECT_EQ("FOO(1+2  );\n", format("FOO(1+2  );\n", Style));
+  EXPECT_EQ("FOO(1+2  )\n", format("FOO(1+2  )\n", Style));
+
   EXPECT_EQ("FOO(String-ized&Messy+But(: :Still)=Intentional);",
             format("FOO(String-ized&Messy+But(: :Still)=Intentional);", Style));
   EXPECT_EQ(
       "FOO(String-ized&Messy+But\\(: :Still)=Intentional);",
       format("FOO(String-ized&Messy+But\\(: :Still)=Intentional);", Style));
   EXPECT_EQ("FOO(String-ized&Messy+But,: :Still=Intentional);",
             format("FOO(String-ized&Messy+But,: :Still=Intentional);", Style));
   EXPECT_EQ("FOO(String-ized&Messy+But,: :\n"