[clang-format] Fix short functions being considered as inline inside an indented namespace.

[curdeius]

Summary

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

With config:

AllowShortFunctionsOnASingleLine: Inline
NamespaceIndentation: All

The code:

namespace Test
{
    void f()
    {
        return;
    }
}

was incorrectly formatted to:

namespace Test
{
    void f() { return; }
}

since the function f was considered being inside a class/struct/record.
That's because the check was simplistic and only checked for a non-zero indentation level of the line starting f.

[curdeius]

Still WIP but I'd like to have some feedback.

clang/lib/Format/UnwrappedLineFormatter.cpp

283

Because of this I consider this patch as a WIP still.
I'm all ears if anyone has an idea how to efficiently find the "parent" line of the current one, i.e. a line with level smaller by one.
There are places where we use a stack for similar purposes. I'm not sure if there's already something existing I could use.

As a last resort, I'll add a stack of Lines that start a new level in LevelIndentTracker and update it in nextLine().

[curdeius]

clang/lib/Format/UnwrappedLineFormatter.cpp

278

TODO: typo

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineFormatter.cpp

297

Is union used in another language and behaves then differently?

304

Is there a test for that?

[curdeius]

clang/lib/Format/UnwrappedLineFormatter.cpp

297

At least in C#, union is not a keyword and can be e.g. a name of a namespace. There's a test for this.

304

Yes, there were existing tests in https://github.com/llvm/llvm-project/blob/main/clang/unittests/Format/FormatTestJS.cpp#L993 at least.

[MyDeveloperDay]

LGTM

[curdeius]

Minor clean up.

[curdeius]

Rebase to fix Windows CI.

clang/lib/Format/UnwrappedLineFormatter.cpp

if (TheLine->Last->is(tok::l_brace) && TheLine->First == TheLine->Last &&
                    : 0;
 
       if (Tok && Tok->is(tok::kw_template) &&
           Style.BraceWrapping.SplitEmptyRecord && EmptyBlock) {
         return 0;
       }
     }
 
-    // FIXME: TheLine->Level != 0 might or might not be the right check to do.
-    // If necessary, change to something smarter.
-    bool MergeShortFunctions =
-        Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All ||
-        (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
-         I[1]->First->is(tok::r_brace)) ||
-        (Style.AllowShortFunctionsOnASingleLine & FormatStyle::SFS_InlineOnly &&
-         TheLine->Level != 0);
+    auto ShouldMergeShortFunctions =
+        [this, B = AnnotatedLines.begin()](
+            SmallVectorImpl<AnnotatedLine *>::const_iterator I) {
+          if (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All)
+            return true;
+          if (Style.AllowShortFunctionsOnASingleLine >=
+                  FormatStyle::SFS_Empty &&
+              I[1]->First->is(tok::r_brace))
+            return true;
+
+          if (Style.AllowShortFunctionsOnASingleLine &
+              FormatStyle::SFS_InlineOnly) {
+            // Just checking TheLine->Level != 0 is not enough, because it
+            // provokes treating functions inside indented namespaces as short.
+            if ((*I)->Level != 0) {
+              if (I == B)
+                return false;
+
+              // TODO: Use IndentTracker to avoid loop?
+              // Find the last line with lower level.
+              auto J = I - 1;
+              for (; J != B; --J)
+                if ((*J)->Level < (*I)->Level)
+                  break;
+
+              // Check if the found line starts a record.
+              auto *RecordTok = (*J)->First;
+              while (RecordTok) {
+                // TODO: Refactor to isRecord(RecordTok).
+                if (RecordTok->isOneOf(tok::kw_class, tok::kw_struct))
+                  return true;
+                if (Style.isCpp() && RecordTok->is(tok::kw_union))
+                  return true;
+                if (Style.isCSharp() && RecordTok->is(Keywords.kw_interface))
+                  return true;
+                if (Style.Language == FormatStyle::LK_Java &&
+                    RecordTok->is(tok::kw_enum))
+                  return true;
+                if (Style.isJavaScript() && RecordTok->is(Keywords.kw_function))
+                  return true;
+
+                RecordTok = RecordTok->Next;
+              }
+
+              return false;
+            }
+          }
+
+          return false;
+        };
+
+    bool MergeShortFunctions = ShouldMergeShortFunctions(I);
 
     if (Style.CompactNamespaces) {
       if (auto nsToken = TheLine->First->getNamespaceToken()) {
         int i = 0;
         unsigned closingLine = TheLine->MatchingClosingBlockLineIndex - 1;
         for (; I + 1 + i != E &&
                nsToken->TokenText == getNamespaceTokenText(I[i + 1]) &&
                closingLine == I[i + 1]->MatchingClosingBlockLineIndex &&

clang/unittests/Format/FormatTest.cpp

EXPECT_EQ("namespace out {\n"
             format("namespace out {\n"
                    "int i;\n"
                    "namespace in {\n"
                    "int i;\n"
                    "} // namespace in\n"
                    "} // namespace out",
                    Style));
 
+  FormatStyle ShortInlineFunctions = getLLVMStyle();
+  ShortInlineFunctions.NamespaceIndentation = FormatStyle::NI_All;
+  ShortInlineFunctions.AllowShortFunctionsOnASingleLine =
+      FormatStyle::SFS_Inline;
+  verifyFormat("namespace {\n"
+               "  void f() {\n"
+               "    return;\n"
+               "  }\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+  verifyFormat("namespace {\n"
+               "  int some_int;\n"
+               "  void f() {\n"
+               "    return;\n"
+               "  }\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+  verifyFormat("namespace interface {\n"
+               "  void f() {\n"
+               "    return;\n"
+               "  }\n"
+               "} // namespace interface\n",
+               ShortInlineFunctions);
+  verifyFormat("namespace {\n"
+               "  class X {\n"
+               "    void f() { return; }\n"
+               "  };\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+  verifyFormat("namespace {\n"
+               "  struct X {\n"
+               "    void f() { return; }\n"
+               "  };\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+  verifyFormat("namespace {\n"
+               "  union X {\n"
+               "    void f() { return; }\n"
+               "  };\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+  verifyFormat("extern \"C\" {\n"
+               "void f() {\n"
+               "  return;\n"
+               "}\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+  verifyFormat("namespace {\n"
+               "  class X {\n"
+               "    void f() { return; }\n"
+               "  } x;\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+  verifyFormat("namespace {\n"
+               "  [[nodiscard]] class X {\n"
+               "    void f() { return; }\n"
+               "  };\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+  verifyFormat("namespace {\n"
+               "  static class X {\n"
+               "    void f() { return; }\n"
+               "  } x;\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+  verifyFormat("namespace {\n"
+               "  constexpr class X {\n"
+               "    void f() { return; }\n"
+               "  } x;\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+
+  ShortInlineFunctions.IndentExternBlock = FormatStyle::IEBS_Indent;
+  verifyFormat("extern \"C\" {\n"
+               "  void f() {\n"
+               "    return;\n"
+               "  }\n"
+               "} // namespace\n",
+               ShortInlineFunctions);
+
   Style.NamespaceIndentation = FormatStyle::NI_Inner;
   EXPECT_EQ("namespace out {\n"
             "int i;\n"
             "namespace in {\n"
             "  int i;\n"
             "} // namespace in\n"
             "} // namespace out",
             format("namespace out {\n"

clang/unittests/Format/FormatTestCSharp.cpp

verifyFormat("try {\n"
                Style);
 
   verifyFormat("try {\n"
                "  doA();\n"
                "} catch (Exception e) {}\n",
                Style);
 }
 
+TEST_F(FormatTestCSharp, ShortFunctions) {
+  FormatStyle Style = getLLVMStyle(FormatStyle::LK_CSharp);
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
+  verifyFormat("interface Interface {\n"
+               "  void f() { return; }\n"
+               "};",
+               Style);
+  verifyFormat("public interface Interface {\n"
+               "  void f() { return; }\n"
+               "};",
+               Style);
+  verifyFormat("namespace {\n"
+               "  void f() {\n"
+               "    return;\n"
+               "  }\n"
+               "};",
+               Style);
+  // "union" is not a keyword in C#.
+  verifyFormat("namespace union {\n"
+               "  void f() {\n"
+               "    return;\n"
+               "  }\n"
+               "};",
+               Style);
+}
+
 } // namespace format
 } // end namespace clang

clang/unittests/Format/FormatTestJava.cpp

verifyFormat("void f() {\n"
                "  a >>>= 1;\n"
                "}");
   verifyFormat("void f() {\n"
                "  int a = 1;\n"
                "  a = a >>> 1;\n"
                "}");
 }
 
+TEST_F(FormatTestJava, ShortFunctions) {
+  FormatStyle Style = getLLVMStyle(FormatStyle::LK_Java);
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
+  verifyFormat("enum Enum {\n"
+               "  E1,\n"
+               "  E2;\n"
+               "  void f() { return; }\n"
+               "}",
+               Style);
+}
+
 } // namespace format
 } // end namespace clang