[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

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

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

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

23545

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

23545

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

1031–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

1031–1035

+1

clang/lib/Format/FormatTokenLexer.h

117–120

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

23545

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.h

private:
   IdentifierTable &IdentTable;
   AdditionalKeywords Keywords;
   encoding::Encoding Encoding;
   llvm::SpecificBumpPtrAllocator<FormatToken> &Allocator;
   // Index (in 'Tokens') of the last token that starts a new line.
   unsigned FirstInLineIndex;
   SmallVector<FormatToken *, 16> Tokens;
 
-  llvm::SmallMapVector<IdentifierInfo *, TokenType, 8> Macros;
+  struct MacroTokenInfo {
+    TokenType Type;
+    bool Finalized;
+  };
+
+  llvm::SmallMapVector<IdentifierInfo *, MacroTokenInfo, 8> Macros;
 
   bool FormattingDisabled;
 
   llvm::Regex MacroBlockBeginRegex;
   llvm::Regex MacroBlockEndRegex;
 
   // Targets that may appear inside a C# attribute.
   static const llvm::StringSet<> CSharpAttributeTargets;

clang/lib/Format/FormatTokenLexer.cpp

: FormatTok(nullptr), IsFirstToken(true), StateStack({LexerState::NORMAL}),
       Encoding(Encoding), Allocator(Allocator), FirstInLineIndex(0),
       FormattingDisabled(false), MacroBlockBeginRegex(Style.MacroBlockBegin),
       MacroBlockEndRegex(Style.MacroBlockEnd) {
   Lex.reset(new Lexer(ID, SourceMgr.getBufferOrFake(ID), SourceMgr, LangOpts));
   Lex->SetKeepWhitespaceMode(true);
 
   for (const std::string &ForEachMacro : Style.ForEachMacros) {
     auto Identifier = &IdentTable.get(ForEachMacro);
-    Macros.insert({Identifier, TT_ForEachMacro});
+    Macros.insert({Identifier, {TT_ForEachMacro, /*Finalized=*/false}});
   }
   for (const std::string &IfMacro : Style.IfMacros) {
     auto Identifier = &IdentTable.get(IfMacro);
-    Macros.insert({Identifier, TT_IfMacro});
+    Macros.insert({Identifier, {TT_IfMacro, /*Finalized=*/false}});
   }
   for (const std::string &AttributeMacro : Style.AttributeMacros) {
     auto Identifier = &IdentTable.get(AttributeMacro);
-    Macros.insert({Identifier, TT_AttributeMacro});
+    Macros.insert({Identifier, {TT_AttributeMacro, /*Finalized=*/false}});
   }
   for (const std::string &StatementMacro : Style.StatementMacros) {
     auto Identifier = &IdentTable.get(StatementMacro);
-    Macros.insert({Identifier, TT_StatementMacro});
+    Macros.insert({Identifier, {TT_StatementMacro, /*Finalized=*/false}});
   }
   for (const std::string &TypenameMacro : Style.TypenameMacros) {
     auto Identifier = &IdentTable.get(TypenameMacro);
-    Macros.insert({Identifier, TT_TypenameMacro});
+    Macros.insert({Identifier, {TT_TypenameMacro, /*Finalized=*/false}});
   }
   for (const std::string &NamespaceMacro : Style.NamespaceMacros) {
     auto Identifier = &IdentTable.get(NamespaceMacro);
-    Macros.insert({Identifier, TT_NamespaceMacro});
+    Macros.insert({Identifier, {TT_NamespaceMacro, /*Finalized=*/false}});
   }
   for (const std::string &WhitespaceSensitiveMacro :
        Style.WhitespaceSensitiveMacros) {
     auto Identifier = &IdentTable.get(WhitespaceSensitiveMacro);
-    Macros.insert({Identifier, TT_UntouchableMacroFunc});
+    Macros.insert({Identifier, {TT_UntouchableMacroFunc, /*Finalized=*/true}});
   }
   for (const std::string &StatementAttributeLikeMacro :
        Style.StatementAttributeLikeMacros) {
     auto Identifier = &IdentTable.get(StatementAttributeLikeMacro);
-    Macros.insert({Identifier, TT_StatementAttributeLikeMacro});
+    Macros.insert(
+        {Identifier, {TT_StatementAttributeLikeMacro, /*Finalized=*/false}});
   }
 }
 
 ArrayRef<FormatToken *> FormatTokenLexer::lex() {
   assert(Tokens.empty());
   assert(FirstInLineIndex == 0);
   do {
     Tokens.push_back(getNextToken());
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()) {
-      FormatTok->setType(it->second);
-      if (it->second == TT_IfMacro) {
+      if (it->second.Finalized) {
+        FormatTok->setFinalizedType(it->second.Type);
+      } else {
+        FormatTok->setType(it->second.Type);
+      }
+      if (it->second.Type == 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))

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"