[clang-format] [C++20] [Module] clang-format couldn't recognize partitions

[MyDeveloperDay]

Summary

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

clang-format is butchering modules, this could easily become a barrier to entry for modules given clang-formats wide spread use.

Prevent the following from adding spaces around the : (cf was considering the ':' as an InheritanceColon)

import <ctime>;
import foo:bar;
import :bar;
export module foo:bar;
export import foo:bar;
export import :bar;
module foo:bar;

to

import<ctime>;
import foo : bar;
import : bar;
export module foo : bar;
export import foo : bar;
export import : bar;
module foo : bar;

[ChuanqiXu]

There could be more tests in case we want support c++20 module more. This shouldn't be a break issue for this patch since this aims to fix the space in partitions only.

export template<...>;
export struct {
  ...
};
export int func() {
   // ...
}
export using A = B;
template<...>
export using A = B<T>;
export {

};
export type_name var;

Feel free to ignore this if you want to support them in later patches.

Thanks!

clang/docs/ReleaseNotes.rst

231

clang/unittests/Format/FormatTest.cpp

22406

The module name could contain dot '.' in the middle. Although it seems like wouldn't be broken by clang-format, I think it is better to test it.

22411

It lacks a test for import "header";

[MyDeveloperDay]

Add some more tests

[• Quuxplusone]

clang/unittests/Format/FormatTest.cpp

22443

I'd like to see module :private;
I'd like to see import <foo/bar.h>;
I'd like to see import foo...bar;
I'd like to see import ..........;
Is module foo:private; a thing?
Is module public:while; a thing?

[MyDeveloperDay]

Nasty, but let me take a look

module: private;
import <foo / bar.h>;
import foo... bar;
import..........;
module foo: private;
module public : while;

they all feel all kinds of wrong!

[HazardyKnusperkeks]

The current goal seems to be achieved. I have no idea what are acceptable module names or so, I've not yet looked into them.

[• Quuxplusone]

clang/unittests/Format/FormatTest.cpp

22443

I don't intend to block this PR waiting on fixing these, but please at least add the "I'd like to see" test cases with TODO comments. That way, whoever tackles it will have the test cases already written.

As usual, I'm ambivalent about the "Is X a thing?" test cases — they arguably don't matter but arguably should be tested anyway to make sure clang-format doesn't crash on them.

[ChuanqiXu]

LGTM with @Quuxplusone's opinions addressed.

[MyDeveloperDay]

Add yet more unit tests and cover those.

Mark one test as TODO as I'm not sure if its allowed syntax

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

3226

You can remove kw_export as it must be followed by import or module, based on how TT_ModulePartitionColon is set on Line 908.

3230–3231

You can drop !Left.is(Keywords.kw_import) as import :bar is already covered by Line 3226. Also, I would remove kw_public and kw_private as they are not special WRT other keywords when followed by TT_ModulePartitionColon.

3235

Is module :public a thing in C++20? If not, I would remove kw_public.

3238

You can fold this line into Line 3223 above.

clang/lib/Format/UnwrappedLineParser.cpp

1253

Maybe move this entire block into a function?

1260

You can change this line to else if (FormatTok->is(tok::less)) { as FormatTok can't be null.

1262–1264

You can combine these three lines into one: while (FormatTok && FormatTok->isNot(tok::semi)) {

1267–1268

Don't you want to mark up to the matching >?

[• Quuxplusone]

clang/lib/Format/TokenAnnotator.cpp

3235

For the record, my understanding is that module :public is not a thing, and neither is module public:while or any other combination of keywords except for module :private. (I don't even think module foo:private is a thing; is it?)
So +1 to removing kw_public. However, if the codebase happens to already have a function for isIdentifierOrKeyword, I think this would be a perfect place to use it. Consider an autoformatter-as-you-type dealing with module foo:public[X]_methods or module :if[X]stream.

[ChuanqiXu]

clang/lib/Format/TokenAnnotator.cpp

3226

export could be followed by many other declarations. (Maybe the logic on line 908 is not right, I am not sure)

[MyDeveloperDay]

Address review comments

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1055–1077

Possible infinite loops if the import statement is the last line and not terminated by a ;?

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1055–1077

won't FormatTok become null at eof?

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1055–1077

ok I see this..

[MyDeveloperDay]

Fix infinite loop

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1055

A while (!eof()) or while (FormatTok->isNot(tok::eof) would be safer here just in case the last line is import followed by eof.

1062–1063

Possible infinite loop here as well. Maybe change it to while (!FormatTok->isOneOf(tok::greater, tok::semi, tok::eof) {?

[MyDeveloperDay]

Address review comments, guard against eof

clang/docs/ReleaseNotes.rst

- Option ``QualifierAligment`` has been added in order to auto-arrange the
   `const` `volatile` `static` `inline` `constexpr` `restrict`
   in variable and parameter declarations to be either ``Right`` aligned
   or ``Left`` aligned or ``Custom`` using ``QualifierOrder``.
 
 - Option ``QualifierOrder`` has been added to allow the order
   `const` `volatile` `static` `inline` `constexpr` `restrict`
   to be controlled relative to the `type`.
 
+- Improved Cpp20 Modules support.
+
 libclang
 --------
 
 - ...
 
 Static Analyzer
 ---------------
 

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   TYPE(JsAndAndEqual)                                                          \
   TYPE(LambdaArrow)                                                            \
   TYPE(LambdaLBrace)                                                           \
   TYPE(LambdaLSquare)                                                          \
   TYPE(LeadingJavaAnnotation)                                                  \
   TYPE(LineComment)                                                            \
   TYPE(MacroBlockBegin)                                                        \
   TYPE(MacroBlockEnd)                                                          \
+  TYPE(ModulePartitionColon)                                                   \
   TYPE(NamespaceMacro)                                                         \
   TYPE(NonNullAssertion)                                                       \
   TYPE(NullCoalescingEqual)                                                    \
   TYPE(NullCoalescingOperator)                                                 \
   TYPE(NullPropagatingOperator)                                                \
   TYPE(ObjCBlockLBrace)                                                        \
   TYPE(ObjCBlockLParen)                                                        \
   TYPE(ObjCDecl)                                                               \

clang/lib/Format/TokenAnnotator.cpp

case tok::colon:
           Tok->setType(TT_AttributeColon);
           break;
         }
         if (Contexts.back().ContextKind == tok::l_paren) {
           Tok->setType(TT_CSharpNamedArgumentColon);
           break;
         }
       }
-      if (Contexts.back().ColonIsDictLiteral ||
+      if (Line.First->isOneOf(Keywords.kw_module, Keywords.kw_import) ||
+          Line.First->startsSequence(tok::kw_export, Keywords.kw_module) ||
+          Line.First->startsSequence(tok::kw_export, Keywords.kw_import)) {
+        Tok->setType(TT_ModulePartitionColon);
+      } else if (Contexts.back().ColonIsDictLiteral ||
                  Style.Language == FormatStyle::LK_Proto ||
                  Style.Language == FormatStyle::LK_TextProto) {
         Tok->setType(TT_DictLiteral);
         if (Style.Language == FormatStyle::LK_TextProto) {
           if (FormatToken *Previous = Tok->getPreviousNonComment())
             Previous->setType(TT_SelectorName);
         }
       } else if (Contexts.back().ColonIsObjCMethodExpr ||
                  Line.startsWith(TT_ObjCMethodSpecifier)) {
         Tok->setType(TT_ObjCMethodExpr);

 }
 
 bool TokenAnnotator::spaceRequiredBefore(const AnnotatedLine &Line,
                                          const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
   auto HasExistingWhitespace = [&Right]() {
     return Right.WhitespaceRange.getBegin() != Right.WhitespaceRange.getEnd();
   };
+
   if (Right.Tok.getIdentifierInfo() && Left.Tok.getIdentifierInfo())
     return true; // Never ever merge two identifiers.
   if (Style.isCpp()) {
+    // Space between import <iostream>.
+    if (Left.is(Keywords.kw_import) && Right.is(tok::less))
+      return true;
+    // No space between module :.
+    if (Left.isOneOf(Keywords.kw_module, tok::kw_export, Keywords.kw_import) &&
+        Right.is(TT_ModulePartitionColon))
+      return true;
+    // No space between import foo:bar but keep a space between import :bar;
+    if (Left.isOneOf(tok::identifier, tok::kw_public, tok::kw_private) &&
+        !Left.is(Keywords.kw_import) && Right.is(TT_ModulePartitionColon))
+      return false;
+    // No space between :bar;
+    if (Left.is(TT_ModulePartitionColon) &&
+        Right.isOneOf(tok::identifier, tok::kw_public, tok::kw_private))
+      return false;
+    // import .....;
+    if (Left.is(Keywords.kw_import) && Right.is(tok::ellipsis))
+      return true;
+    if (Left.is(tok::ellipsis) && Right.is(tok::identifier) &&
+        Line.First->is(Keywords.kw_import))
+      return false;
+
     if (Left.is(tok::kw_operator))
       return Right.is(tok::coloncolon);
     if (Right.is(tok::l_brace) && Right.is(BK_BracedInit) &&
         !Left.opensScope() && Style.SpaceBeforeCpp11BracedList)
       return true;
   } else if (Style.Language == FormatStyle::LK_Proto ||
              Style.Language == FormatStyle::LK_TextProto) {
     if (Right.is(tok::period) &&

clang/lib/Format/UnwrappedLineParser.cpp

static bool isC78ParameterDecl(const FormatToken *Tok, const FormatToken *Next,
   if (!Tok || Tok->isNot(tok::identifier))
     return false;
 
   return Tok->Previous && Tok->Previous->isOneOf(tok::l_paren, tok::comma);
 }
 
 // readTokenWithJavaScriptASI reads the next token and terminates the current
 // line if JavaScript Automatic Semicolon Insertion must
 // happen between the current token and the next token.
 //
 // This method is conservative - it cannot cover all edge cases of JavaScript,
 // but only aims to correctly handle certain well known cases. It *must not*
 // return true in speculative cases.
 void UnwrappedLineParser::readTokenWithJavaScriptASI() {
   FormatToken *Previous = FormatTok;
   readToken();
   FormatToken *Next = FormatTok;
 
   bool IsOnSameLine =
       CommentsBeforeNextToken.empty()
           ? Next->NewlinesBefore == 0
           : CommentsBeforeNextToken.front()->NewlinesBefore == 0;
   if (IsOnSameLine)
     return;
 
   bool PreviousMustBeValue = mustBeJSIdentOrValue(Keywords, Previous);
   bool PreviousStartsTemplateExpr =
       Previous->is(TT_TemplateString) && Previous->TokenText.endswith("${");
   if (PreviousMustBeValue || Previous->is(tok::r_paren)) {
     // If the line contains an '@' sign, the previous token might be an
     // annotation, which can precede another identifier/value.
     bool HasAt = std::find_if(Line->Tokens.begin(), Line->Tokens.end(),
                               [](UnwrappedLineNode &LineNode) {
                                 return LineNode.Tok->is(tok::at);
                               }) != Line->Tokens.end();
     if (HasAt)
       return;
   }
   if (Next->is(tok::exclaim) && PreviousMustBeValue)
if (FormatTok->is(Keywords.kw_import)) {
         if (!FormatTok->is(tok::string_literal))
           return;
         nextToken();
         if (FormatTok->is(tok::semi))
           nextToken();
         addUnwrappedLine();
         return;
       }
+      if (Style.isCpp()) {
+        nextToken();
+        while (FormatTok) {
+          if (FormatTok->is(tok::colon)) {
+            FormatTok->setType(TT_ModulePartitionColon);
+          }
+          // Handle import <foo/bar.h> as we would an include statement
+          if (FormatTok && FormatTok->is(tok::less)) {
+            nextToken();
+            while (FormatTok) {
+              if (FormatTok->is(tok::semi))
+                break;
+              // Mark tokens up to the trailing line comments as implicit string
+              // literals.
+              if (FormatTok->isNot(tok::comment) &&
+                  !FormatTok->TokenText.startswith("//"))
+                FormatTok->setType(TT_ImplicitStringLiteral);
+              nextToken();
+            }
+          }
+          if (FormatTok->is(tok::semi)) {
+            nextToken();
+            break;
+          }
+          nextToken();
+        }
+        addUnwrappedLine();
+        return;
+      }
     }
     if (Style.isCpp() &&
         FormatTok->isOneOf(Keywords.kw_signals, Keywords.kw_qsignals,
                            Keywords.kw_slots, Keywords.kw_qslots)) {
       nextToken();
       if (FormatTok->is(tok::colon)) {
         nextToken();
         addUnwrappedLine();

clang/unittests/Format/FormatTest.cpp

constexpr StringRef Code =
       "check if everything is working.\n"
       "  const std::string SmallString = \"Hello World\";\n"
       "  // Small line comment\n"
       "  return String.size() > SmallString.size();\n"
       "}";
   EXPECT_EQ(Code, format(Code, Style));
 }
 
+TEST_F(FormatTest, Cpp20ModulesSupport) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Never;
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
+
+  verifyFormat("export import foo;", Style);
+  verifyFormat("export import foo:bar;", Style);
+  verifyFormat("export import foo.bar;", Style);
+  verifyFormat("export import foo.bar:baz;", Style);
+  verifyFormat("export import :bar;", Style);
+  verifyFormat("export module foo:bar;", Style);
+  verifyFormat("export module foo;", Style);
+  verifyFormat("export module foo.bar;", Style);
+  verifyFormat("export module foo.bar:baz;", Style);
+  verifyFormat("export import <string_view>;", Style);
+
+  verifyFormat("export type_name var;", Style);
+  verifyFormat("template <class T> export using A = B<T>;", Style);
+  verifyFormat("export using A = B;", Style);
+  verifyFormat("export int func() {\n"
+               "  foo();\n"
+               "}",
+               Style);
+  verifyFormat("export struct {\n"
+               "  int foo;\n"
+               "};",
+               Style);
+  verifyFormat("export {\n"
+               "  int foo;\n"
+               "};",
+               Style);
+  verifyFormat("export export char const *hello() { return \"hello\"; }");
+
+  verifyFormat("import bar;", Style);
+  verifyFormat("import foo.bar;", Style);
+  verifyFormat("import foo:bar;", Style);
+  verifyFormat("import :bar;", Style);
+  verifyFormat("import <ctime>;", Style);
+  verifyFormat("import \"header\";", Style);
+
+  verifyFormat("module foo;", Style);
+  verifyFormat("module foo:bar;", Style);
+  verifyFormat("module foo.bar;", Style);
+  verifyFormat("module;", Style);
+
+  verifyFormat("export namespace hi {\n"
+               "const char *sayhi();\n"
+               "}",
+               Style);
+
+  verifyFormat("module :private;", Style);
+  verifyFormat("import <foo/bar.h>;", Style);
+  verifyFormat("import foo...bar;", Style);
+  verifyFormat("import ..........;", Style);
+  verifyFormat("module foo:private;", Style);
+
+  // TODO validate this use cases? can we have keywords here?
+  // verifyFormat("module public:while;", Style);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang