[clang-format] Add support to remove unnecessary semicolons after function definition

[MyDeveloperDay]

Summary

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

This change is to remove extraneous and unnecessary ';' from after a function definition, its off by default and carries the same "code modification" warning as some of our other code manipulating changes.

int max(int a, int b)
{
    return a>b?a:b;
};

class Foo
{
    int getSomething() const { return something; };
};

At first, I thought this was a minor problem and not worth anything other than using -Wextra-semi/-Wextra-semi-stmt as pointed out in the issue comments. But clang-format is used by people who may not use the clang compiler, and not all compilers have this extra semi warning (AFAIK)

However, I ran this over the clang-codebase and realized (in clang and even the clang-format Tests!) how prevalent this is.

This is implemented very much on the same lines as @owenpan does for removing the {} with RemoveBracesLLVM, there is some repeated code that we could rationalize down if accepted. (I didn't want to be too invasive on that work)

This is definitely one of those changes that it would be nice for those of us that use "clang-format on save" could get without having to go through a compile phase in order to catch the warnings.

[lahwaacz]

clang/docs/ClangFormatStyleOptions.rst

3769

Nit: the note should probably not be part of the warning.

3779

There is an extra ; in the true case ;-)

[HazardyKnusperkeks]

Maybe even extend the option to other unnecessary semicolons like int x = 5;; or other noop statements, one just has to be careful not to remove the semicolon when it's the sole if/loop body.

clang/include/clang/Format/Format.h

3076

The name is a bit hard, just from that it's not clear that this option is harmless (modulo bugs). Maybe something like the said warning RemoveExtraSemiColons?

clang/unittests/Format/FormatTest.cpp

26755

What happens with 2 (or more) semicolons?

[owenpan]

@MyDeveloperDay Cool!

In D135466#3843792, @HazardyKnusperkeks wrote:

Maybe even extend the option to other unnecessary semicolons like int x = 5;; or other noop statements, one just has to be careful not to remove the semicolon when it's the sole if/loop body.

IMO we should keep it simple for now. We can always extend it to an enum or struct down the road, similar to what I plan to do with RemoveBracesLLVM.

clang/docs/ClangFormatStyleOptions.rst

3761–3762

Because "semicolon" is a single word. Also, FWIW I prefer the singular form here and would save the plural form for e.g. InsertBraces to mean a pair of braces.

3770

IMO we don't need the note as it goes without saying.

3779

If this file was generated from the Format.h header below, we have a bug in the dump_format_style.py script.

clang/include/clang/Format/Format.h

3069–3071

LLVM style.

3076

I'm ok with the short name, which makes the user to look it up in the documentation. The alternative would be something like RemoveSemicolonAfterFunctionBody.

clang/lib/Format/Format.cpp

1971

Nit.

clang/lib/Format/UnwrappedLineParser.cpp

843

We don't need to add the parameter. See below.

922–925

So that we don't need to add a parameter to parseBlock.

973

The while loop would address https://reviews.llvm.org/D135466#inline-1306331 but is optional IMO because it wouldn't distinguish };; and the following:

};
;

996–1000

This can be deleted. See above. Now we don't have to worry about if parseParens() and parseStructuralElement() would see a }; sequence.

[lahwaacz]

clang/lib/Format/UnwrappedLineParser.cpp

973

Does it matter? If you have struct foo {};; and format it with e.g. clang-format --style=LLVM, you'll get

struct foo {};
;

so IMO these _should_ be indistinguishible.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

973

It might matter when removing the semicolons. We can keep the loop if we have sufficient coverage in test cases. For example, we must ensure that

void foo() {}; //
; int bar;

is not formatted to

void foo() {} // int bar;

[MyDeveloperDay]

In D135466#3844287, @owenpan wrote:

@MyDeveloperDay Cool!

In D135466#3843792, @HazardyKnusperkeks wrote:

Maybe even extend the option to other unnecessary semicolons like int x = 5;; or other noop statements, one just has to be careful not to remove the semicolon when it's the sole if/loop body.

IMO we should keep it simple for now. We can always extend it to an enum or struct down the road, similar to what I plan to do with RemoveBracesLLVM.

Thanks, everyone for all the reviews, I'll work on those.

I tend to agree here, I was really only focused on the extraneous ; after a function, I sort of feel catching a double coding ;; error or even adding a missing ; after a functions ) as prettier/vs code seems to do are different problem statements.

I certainly don't mind adding the capability to fix those too (in subsequent commits), I don't like to add too much in a single commit. (Especially the initial one that carries a lot of the infrastructure for a new feature).

[MyDeveloperDay]

• Switch to the looping method and remove the need to change parseBlock (phew!) thanks @owenpan for the guidance
• Add tests for double ;;
• Change the name to RemoveSemicolon

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

3779

I wonder if I didn't do a final regeneration, lets check when I update

clang/include/clang/Format/Format.h

3076

I feel if we called it RemoveSemicolonAfterFunctionBody then adding extra use cases would then require a new option.. in the same way, RemoveBracesLLVM doesn't remove all Braces.. can we go with @owenpan suggestion of RemoveSemicolon I could go for

RemoveExtraSemicolon
RemoveExtraSemicolons

But I don't feel Extra adds anything extra, if you pardon the pune.

clang/lib/Format/UnwrappedLineParser.cpp

843

Oh! thank goodness, I hated having to add that..

922–925

I don't know why I didn't see this! much better approach

clang/unittests/Format/FormatTest.cpp

26755

I'll add the test but with @owenpan suggestion it handles it

[owenpan]

Since this option doesn't work for empty functions yet, we should either add a note in the documentation or qualify the scope with "non-empty" as suggested in my comments.

clang/docs/ReleaseNotes.rst

528

IMO "unnecessary" is unnecessary. Add "non-empty" if we want to be exact.

clang/include/clang/Format/Format.h

3058

Now I feel it's probably better to not add "redundant".

3068

Nit: unwrap the braces.

clang/lib/Format/UnwrappedLineParser.cpp

992–997

Move the comments to line 965 and elide the braces.

clang/unittests/Format/FormatTest.cpp

26773

Add the test case if it will pass.

26780–26786

And edit the comments above.

[MyDeveloperDay]

There is a simple fix to allow the empty functions in UnwrappedLineParser::calculateBraceTypes

by moving this clause out of the "isJavaScript()" clause this will ensure for foo() {} the { is considered a FunctionLBrace (as it will mark the {} as a BK_Block

This is having more of a knock on to other unit tests which I need to look more deeply into (as I feel they could be wrong)..

i.e.

should it be

auto i = decltype(x){}; be or auto i = decltype(x) {};

"return (a)(b){1, 2, 3};" it "return (a)(b) {1, 2, 3};"

Its possible these tests could be wrong, (some look so) by for decltype and typeof what would people expect?

Either way that change needs to be separate from this.

To work on that I have raised https://github.com/llvm/llvm-project/issues/58251

[MyDeveloperDay]

Minor documentation changes based on review feedback

[owenpan]

LGTM

clang/unittests/Format/FormatTest.cpp

26773

Add the test case if it will pass.

So the test case above fails?

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

26773

No that passes only those in the #if 0 fail below

clang/docs/ClangFormatStyleOptions.rst

.. code-block:: c++
     } else {                                   else if (c)
       if (c) {                                   d();
         d();                                   else
       } else {                                   e();
         e();
       }
     }
 
+**RemoveSemicolon** (``Boolean``) :versionbadge:`clang-format 16`
+  Remove redundant semicolons after the closing brace of a function
+
+  .. warning:: 
+
+   Setting this option to `true` could lead to incorrect code formatting due
+   to clang-format's lack of complete semantic information. As such, extra
+   care should be taken to review code changes made by this option.
+
+  .. code-block:: c++
+
+    false:                                     true:
+
+    int max(int a, int b)                      int max(int a, int b)
+    {                                          {
+      return a > b ? a : b;                            return a > b ? a : b;
+    };                                         }
+
 **RequiresClausePosition** (``RequiresClausePositionStyle``) :versionbadge:`clang-format 15`
   The position of the ``requires`` clause.
 
   Possible values:
 
   * ``RCPS_OwnLine`` (in configuration: ``OwnLine``)
     Always put the ``requires`` clause on its own line.
 
     .. code-block:: c++

clang/docs/ReleaseNotes.rst

 Build System Changes
 --------------------
 
 AST Matchers
 ------------
 
 clang-format
 ------------
+- Add `RemoveSemicolon` option for removing unnecessary `;` after a function definition.
 
 clang-extdef-mapping
 --------------------
 
 libclang
 --------
 - Introduced the new function ``clang_getUnqualifiedType``, which mimics
   the behavior of ``QualType::getUnqualifiedType`` for ``CXType``.

clang/include/clang/Format/Format.h

///    NS_TABLE_.*_END$"
   ///     } else {                                   e();
   ///       e();
   ///     }
   ///   }
   /// \endcode
   /// \version 14
   bool RemoveBracesLLVM;
 
+  /// Remove redundant semicolons after the closing brace of a function
+  /// \warning
+  ///  Setting this option to `true` could lead to incorrect code formatting due
+  ///  to clang-format's lack of complete semantic information. As such, extra
+  ///  care should be taken to review code changes made by this option.
+  /// \endwarning
+  /// \code
+  ///   false:                                     true:
+  ///
+  ///   int max(int a, int b)                      int max(int a, int b)
+  ///   {                                          {
+  ///     return a > b ? a : b;                            return a > b ? a : b;
+  ///   };                                         }
+  ///
+  /// \endcode
+  /// \version 16
+  bool RemoveSemicolon;
+
   /// \brief The possible positions for the requires clause. The
   /// ``IndentRequires`` option is only used if the ``requires`` is put on the
   /// start of a line.
   enum RequiresClausePositionStyle : int8_t {
     /// Always put the ``requires`` clause on its own line.
     /// \code
     ///   template <typename T>
     ///   requires C<T>
     ///   struct Foo {...
return AccessModifierOffset == R.AccessModifierOffset &&
            PenaltyBreakTemplateDeclaration ==
                R.PenaltyBreakTemplateDeclaration &&
            PointerAlignment == R.PointerAlignment &&
            QualifierAlignment == R.QualifierAlignment &&
            QualifierOrder == R.QualifierOrder &&
            RawStringFormats == R.RawStringFormats &&
            ReferenceAlignment == R.ReferenceAlignment &&
            RemoveBracesLLVM == R.RemoveBracesLLVM &&
+           RemoveSemicolon == R.RemoveSemicolon &&
            RequiresClausePosition == R.RequiresClausePosition &&
            SeparateDefinitionBlocks == R.SeparateDefinitionBlocks &&
            ShortNamespaceLines == R.ShortNamespaceLines &&
            SortIncludes == R.SortIncludes &&
            SortJavaStaticImport == R.SortJavaStaticImport &&
            SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
            SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
            SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("PenaltyIndentedWhitespace",
                    Style.PenaltyIndentedWhitespace);
     IO.mapOptional("PointerAlignment", Style.PointerAlignment);
     IO.mapOptional("PPIndentWidth", Style.PPIndentWidth);
     IO.mapOptional("RawStringFormats", Style.RawStringFormats);
     IO.mapOptional("ReferenceAlignment", Style.ReferenceAlignment);
     IO.mapOptional("ReflowComments", Style.ReflowComments);
     IO.mapOptional("RemoveBracesLLVM", Style.RemoveBracesLLVM);
+    IO.mapOptional("RemoveSemicolon", Style.RemoveSemicolon);
     IO.mapOptional("RequiresClausePosition", Style.RequiresClausePosition);
     IO.mapOptional("SeparateDefinitionBlocks", Style.SeparateDefinitionBlocks);
     IO.mapOptional("ShortNamespaceLines", Style.ShortNamespaceLines);
     IO.mapOptional("SortIncludes", Style.SortIncludes);
     IO.mapOptional("SortJavaStaticImport", Style.SortJavaStaticImport);
     IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations);
     IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
     IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Leave;
   LLVMStyle.ShortNamespaceLines = 1;
   LLVMStyle.SpacesBeforeTrailingComments = 1;
   LLVMStyle.Standard = FormatStyle::LS_Latest;
   LLVMStyle.UseCRLF = false;
   LLVMStyle.UseTab = FormatStyle::UT_Never;
   LLVMStyle.ReflowComments = true;
   LLVMStyle.RemoveBracesLLVM = false;
+  LLVMStyle.RemoveSemicolon = false;
   LLVMStyle.SpacesInParentheses = false;
   LLVMStyle.SpacesInSquareBrackets = false;
   LLVMStyle.SpaceInEmptyBlock = false;
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpacesInLineCommentPrefix = {/*Minimum=*/1, /*Maximum=*/-1u};
   LLVMStyle.SpaceAfterCStyleCast = false;
for (auto I = Lines.begin(); I != End; ++I) {
       removeBraces(Line->Children, Result);
       if (!Line->Affected)
         continue;
       const auto NextLine = I + 1 == End ? nullptr : I[1];
       for (auto Token = Line->First; Token && !Token->Finalized;
            Token = Token->Next) {
         if (!Token->Optional)
           continue;
-        assert(Token->isOneOf(tok::l_brace, tok::r_brace));
+        if (!Token->isOneOf(tok::l_brace, tok::r_brace))
+          continue;
+        auto Next = Token->Next;
+        assert(Next || Token == Line->Last);
+        if (!Next && NextLine)
+          Next = NextLine->First;
+        SourceLocation Start;
+        if (Next && Next->NewlinesBefore == 0 && Next->isNot(tok::eof)) {
+          Start = Token->Tok.getLocation();
+          Next->WhitespaceRange = Token->WhitespaceRange;
+        } else {
+          Start = Token->WhitespaceRange.getBegin();
+        }
+        const auto Range =
+            CharSourceRange::getCharRange(Start, Token->Tok.getEndLoc());
+        cantFail(Result.add(tooling::Replacement(SourceMgr, Range, "")));
+      }
+    }
+  }
+};
+
+class SemiRemover : public TokenAnalyzer {
+public:
+  SemiRemover(const Environment &Env, const FormatStyle &Style)
+      : TokenAnalyzer(Env, Style) {}
+
+  std::pair<tooling::Replacements, unsigned>
+  analyze(TokenAnnotator &Annotator,
+          SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+          FormatTokenLexer &Tokens) override {
+    AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
+    tooling::Replacements Result;
+    removeSemi(AnnotatedLines, Result);
+    return {Result, 0};
+  }
+
+private:
+  void removeSemi(SmallVectorImpl<AnnotatedLine *> &Lines,
+                  tooling::Replacements &Result) {
+    const auto &SourceMgr = Env.getSourceManager();
+    const auto End = Lines.end();
+    for (auto I = Lines.begin(); I != End; ++I) {
+      const auto Line = *I;
+      removeSemi(Line->Children, Result);
+      if (!Line->Affected)
+        continue;
+      const auto NextLine = I + 1 == End ? nullptr : I[1];
+      for (auto Token = Line->First; Token && !Token->Finalized;
+           Token = Token->Next) {
+        if (!Token->Optional)
+          continue;
+        if (Token->isNot(tok::semi))
+          continue;
         auto Next = Token->Next;
         assert(Next || Token == Line->Last);
         if (!Next && NextLine)
           Next = NextLine->First;
         SourceLocation Start;
         if (Next && Next->NewlinesBefore == 0 && Next->isNot(tok::eof)) {
           Start = Token->Tok.getLocation();
           Next->WhitespaceRange = Token->WhitespaceRange;
if (Style.isCpp()) {
     }
 
     if (Style.RemoveBracesLLVM) {
       Passes.emplace_back([&](const Environment &Env) {
         return BracesRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
       });
     }
 
+    if (Style.RemoveSemicolon) {
+      Passes.emplace_back([&](const Environment &Env) {
+        return SemiRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
+      });
+    }
+
     if (Style.FixNamespaceComments) {
       Passes.emplace_back([&](const Environment &Env) {
         return NamespaceEndCommentsFixer(Env, Expanded).process();
       });
     }
 
     if (Style.SortUsingDeclarations) {
       Passes.emplace_back([&](const Environment &Env) {

clang/lib/Format/UnwrappedLineParser.cpp

bool UnwrappedLineParser::mightFitOnOneLine(
 
   return Line.Level * Style.IndentWidth + Length <= ColumnLimit;
 }
 
 FormatToken *UnwrappedLineParser::parseBlock(
     bool MustBeDeclaration, unsigned AddLevels, bool MunchSemi, bool KeepBraces,
     IfStmtKind *IfKind, bool UnindentWhitesmithsBraces,
     bool CanContainBracedList, TokenType NextLBracesType) {
   auto HandleVerilogBlockLabel = [this]() {
     // ":" name
     if (Style.isVerilog() && FormatTok->is(tok::colon)) {
       nextToken();
       if (Keywords.isVerilogIdentifier(*FormatTok))
         nextToken();
     }
   };
 
if (eof())
     return IfLBrace;
 
   if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
                  : !FormatTok->is(tok::r_brace)) {
     Line->Level = InitialLevel;
     FormatTok->setBlockKind(BK_Block);
     return IfLBrace;
   }
 
+  const bool IsFunctionRBrace =
+      FormatTok->is(tok::r_brace) && Tok->is(TT_FunctionLBrace);
+
   auto RemoveBraces = [=]() mutable {
     if (!SimpleBlock)
       return false;
     assert(Tok->isOneOf(TT_ControlStatementLBrace, TT_ElseLBrace));
     assert(FormatTok->is(tok::r_brace));
     const bool WrappedOpeningBrace = !Tok->Previous;
     if (WrappedOpeningBrace && FollowedByComment)
       return false;
if (RemoveBraces()) {
     Tok->MatchingParen = FormatTok;
     FormatTok->MatchingParen = Tok;
   }
 
   size_t PPEndHash = computePPHash();
 
   // Munch the closing brace.
   nextToken(/*LevelDifference=*/-AddLevels);
+
+  if (Style.RemoveSemicolon && IsFunctionRBrace) {
+    while (FormatTok->is(tok::semi)) {
+      FormatTok->Optional = true;
+      nextToken();
+    }
+  }
+
   HandleVerilogBlockLabel();
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   Line->Level = InitialLevel;
 
   if (FormatTok->is(tok::kw_noexcept)) {
     // A noexcept in a requires expression.
     nextToken();
   }
 
   if (FormatTok->is(tok::arrow)) {
     // Following the } or noexcept we can find a trailing return type arrow
     // as part of an implicit conversion constraint.
     nextToken();
     parseStructuralElement();
   }
 
-  if (MunchSemi && FormatTok->is(tok::semi))
+  // When this is a function block and there is an unnecessary semicolon
+  // afterwards then mark it as optional (so the RemoveSemi pass can get rid of
+  // it later).
+  if (MunchSemi && FormatTok->is(tok::semi)) {
     nextToken();
+  }
 
   if (PPStartHash == PPEndHash) {
     Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
     if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) {
       // Update the opening line to add the forward reference as well
       (*CurrentLines)[OpeningLineIndex].MatchingClosingBlockLineIndex =
           CurrentLines->size() - 1;
     }
   }
 
   return IfLBrace;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
   CHECK_PARSE_BOOL(InsertBraces);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
   CHECK_PARSE_BOOL(Cpp11BracedListStyle);
   CHECK_PARSE_BOOL(ReflowComments);
   CHECK_PARSE_BOOL(RemoveBracesLLVM);
+  CHECK_PARSE_BOOL(RemoveSemicolon);
   CHECK_PARSE_BOOL(SortUsingDeclarations);
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
   CHECK_PARSE_BOOL(SpacesInConditionalStatement);
   CHECK_PARSE_BOOL(SpaceInEmptyBlock);
   CHECK_PARSE_BOOL(SpaceInEmptyParentheses);
   CHECK_PARSE_BOOL(SpacesInContainerLiterals);
   CHECK_PARSE_BOOL(SpacesInCStyleCastParentheses);

 }
 
 TEST_F(FormatTest, FormatsVariableTemplates) {
   verifyFormat("inline bool var = is_integral_v<int> && is_signed_v<int>;");
   verifyFormat("template <typename T> "
                "inline bool var = is_integral_v<T> && is_signed_v<T>;");
 }
 
+TEST_F(FormatTest, RemoveSemicolon) {
+  FormatStyle Style = getLLVMStyle();
+  Style.RemoveSemicolon = true;
+
+  verifyFormat("int max(int a, int b) { return a > b ? a : b; }",
+               "int max(int a, int b) { return a > b ? a : b; };", Style);
+
+  verifyFormat("int max(int a, int b) { return a > b ? a : b; }",
+               "int max(int a, int b) { return a > b ? a : b; };;", Style);
+
+  verifyFormat("class Foo {\n"
+               "  int getSomething() const { return something; }\n"
+               "};",
+               "class Foo {\n"
+               "  int getSomething() const { return something; };\n"
+               "};",
+               Style);
+
+  verifyFormat("class Foo {\n"
+               "  int getSomething() const { return something; }\n"
+               "};",
+               "class Foo {\n"
+               "  int getSomething() const { return something; };;\n"
+               "};",
+               Style);
+
+  verifyFormat("for (;;) {\n"
+               "}",
+               Style);
+
+  // These tests are here to show a problem that may not be easily
+  // solved, our implementation to remove semicolons is only as good
+  // as our FunctionLBrace detection and this fails for empty braces
+  // because we can't distringuish this from a bracelist
+  // when we fix that..then these tests can be corrected to remove the ;
+  // and to add a space between the `)` and `{`
+  verifyFormat("void main(){};", "void main() {};", Style);
+  verifyFormat("void foo(){}; //\n"
+               ";\n"
+               "int bar;",
+               "void foo(){}; //\n"
+               "; int bar;",
+               Style);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang