[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

264

clang/unittests/Format/FormatTest.cpp

22601

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.

22606

It lacks a test for import "header";

[MyDeveloperDay]

Add some more tests

[• Quuxplusone]

clang/unittests/Format/FormatTest.cpp

22638

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

22638

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

3265

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

3269–3270

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.

3274

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

3277

You can fold this line into Line 3223 above.

clang/lib/Format/UnwrappedLineParser.cpp

1344

Maybe move this entire block into a function?

1351

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

1353–1355

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

1358–1359

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

[• Quuxplusone]

clang/lib/Format/TokenAnnotator.cpp

3274

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

3265

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

1119–1141

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

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1119–1141

won't FormatTok become null at eof?

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1119–1141

ok I see this..

[MyDeveloperDay]

Fix infinite loop

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1119

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

1126–1127

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`.
 
 - Add a ``Custom`` style to ``SpaceBeforeParens``, to better configure the
   space before parentheses. The custom options can be set using
   ``SpaceBeforeParensOptions``.
 
+- 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.
 
   // Leave a space between * and /* to avoid C4138 `comment end` found outside
   // of comment.
   if (Left.is(tok::star) && Right.is(tok::comment))
     return true;
 
   if (Style.isCpp()) {
+    // Space between import <iostream>.
+    // or import .....;
+    if (Left.is(Keywords.kw_import) && Right.isOneOf(tok::less, tok::ellipsis))
+      return true;
+    // No space between module :.
+    if (Left.isOneOf(Keywords.kw_module, Keywords.kw_import) &&
+        Right.is(TT_ModulePartitionColon))
+      return true;
+    // No space between import foo:bar but keep a space between import :bar;
+    if (Left.is(tok::identifier) && Right.is(TT_ModulePartitionColon))
+      return false;
+    // No space between :bar;
+    if (Left.is(TT_ModulePartitionColon) &&
+        Right.isOneOf(tok::identifier, tok::kw_private))
+      return false;
+    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.h

private:
   void parseIfThenElse();
   void parseTryCatch();
   void parseForOrWhileLoop();
   void parseDoWhile();
   void parseLabel(bool LeftAlignLabel = false);
   void parseCaseLabel();
   void parseSwitch();
   void parseNamespace();
+  void parseModuleImport();
   void parseNew();
   void parseAccessSpecifier();
   bool parseEnum();
   bool parseStructLike();
   void parseConcept();
   void parseRequires();
   void parseRequiresExpression(unsigned int OriginalLevel);
   void parseConstraintExpression(unsigned int OriginalLevel);

clang/lib/Format/UnwrappedLineParser.cpp

static bool isC78ParameterDecl(const FormatToken *Tok, const FormatToken *Next,
 
   Tok = Tok->Previous;
   if (!Tok || Tok->isNot(tok::identifier))
     return false;
 
   return Tok->Previous && Tok->Previous->isOneOf(tok::l_paren, tok::comma);
 }
 
+void UnwrappedLineParser::parseModuleImport() {
+  nextToken();
+  while (!eof()) {
+    if (FormatTok->is(tok::colon)) {
+      FormatTok->setType(TT_ModulePartitionColon);
+    }
+    // Handle import <foo/bar.h> as we would an include statement.
+    else if (FormatTok->is(tok::less)) {
+      nextToken();
+      while (!FormatTok->isOneOf(tok::semi, tok::greater, tok::eof)) {
+        // 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;
+}
+
 // 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() {
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()) {
+        parseModuleImport();
+        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();
         return;
       }
     }
     if (Style.isCpp() && FormatTok->is(TT_StatementMacro)) {
       parseStatementMacro();
       return;
     }
     if (Style.isCpp() && FormatTok->is(TT_NamespaceMacro)) {
       parseNamespace();
       return;
     }
     // In all other cases, parse the declaration.

clang/unittests/Format/FormatTest.cpp

 }
 
 TEST_F(FormatTest, FormatDecayCopy) {
   // error cases from unit tests
   verifyFormat("foo(auto())");
   verifyFormat("foo(auto{})");
   verifyFormat("foo(auto({}))");
   verifyFormat("foo(auto{{}})");
 
   verifyFormat("foo(auto(1))");
   verifyFormat("foo(auto{1})");
   verifyFormat("foo(new auto(1))");
   verifyFormat("foo(new auto{1})");
   verifyFormat("decltype(auto(1)) x;");
   verifyFormat("decltype(auto{1}) x;");
   verifyFormat("auto(x);");
   verifyFormat("auto{x};");
   verifyFormat("new auto{x};");
   verifyFormat("auto{x} = y;");
   verifyFormat("auto(x) = y;"); // actually a declaration, but this is clearly
                                 // the user's own fault
   verifyFormat("integral auto(x) = y;"); // actually a declaration, but this is
                                          // clearly the user's own fault
   verifyFormat("auto(*p)() = f;");       // actually a declaration; TODO FIXME
 }
 
+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);
+  verifyFormat("import a", Style);
+  verifyFormat("module a", Style);
+  verifyFormat("export import a", Style);
+  verifyFormat("export module a", Style);
+
+  verifyFormat("import", Style);
+  verifyFormat("module", Style);
+  verifyFormat("export", Style);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang