[clang] Draft: Implement P1703R1

[tbaeder]

Summary

As in: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1703r1.html

Posting this mostly to get some feedback on the general path taken.

From reading the paper, I understand that the preprocessor should gain a "import-keyword" token, but that is already taken by the GNU #import extension.

The implementation here starts converting newlines to eod tokens when seeing a export or import token. However, that means ignoring the eod tokens when parsing an export block, an exported declaration or a non-header-unit module import.

Let me know what you think.

[urnathan]

I can;t really tell what you're doing. C++20 lexes import (&module) declarations as preprocessor directives. the parser must also recognize only such declarations that came through from lexing such directives. It is unrelated to #import. GCC's implementation drops the preprocessor into directive mode if it sees 'module' or 'import' (optionally preceeded by 'export') at the start of a line[1]. Those keywords are converted to unspellable internal tokens that the parser recognizes to parse module and import declarations. An end-of-pragma token is injected at the end-of-line, and again the parser expects that after the terminating ';'. (it's leveraging the pragma machinery)

[1] it also peeks the next non-white-space token for '<', '", or <begin-identifier-char>, as specified in the std.

occurrences of 'module' and 'import' that are not lexed that way go through as regular identifier tokens, not keywords.

[urnathan]

oh, also in 'import "bob";', "bob" needs to be lexed as a #include name, not a string. (and likewise 'import <bob>;') Hence treating these lines in the preprocessor in directive-processing-mode was the way GCC went.

clang/test/Modules/P1703R1.cpp

49–50

This confuses me. That's a stray 'export' token, followed by a well-formed import-directive

52

that second 'import' is not a token, it's an identifier. This is an ill-formed import-directive with unexpected tokens afte the first ';' You seem to be dropping into lex-import-mode not at the start of a line?

72

what is meant by 'normal'? many of these look ill-formed to me.

87

yup :) gcc tries to tell the user about the single logical-line requirement:

   inform (token->location, "perhaps insert a line break, or other"
	      " disambiguation, to prevent this being considered a"
	      " module control-line");

the expectation here is that this was pre c++20 code that fell into this lexing trap. Specifically things like:

template<typename T> class module {...};

module<mytype> x;

[urnathan]

Ah, I'd had a thinko about what 1703 was. that's superseded by <strike>p1757</strike>wg21.link/p1857. That's what is needed. (your patch makes more sense in the 1703 context)

[tbaeder]

In D122885#3426070, @urnathan wrote:

Ah, I'd had a thinko about what 1703 was. that's superseded by <strike>p1757</strike>wg21.link/p1857. That's what is needed. (your patch makes more sense in the 1703 context)

Ha, okay. Yes, that makes more sense. Both papers are linked in https://clang.llvm.org/cxx_status.html. Sorry for the confusion, I'll have a look at 1857R3 instead.

[ChuanqiXu]

BTW, I think it is helpful to add @iains's patches as parent versions to avoid conflicting.

[tbaeder]

What patches are you talking about exactly?

[ChuanqiXu]

In D122885#3431618, @tbaeder wrote:

What patches are you talking about exactly?

I mean D121588 D121589 D121590 D121591 D122119

[iains]

FWIW probably these 5 patches are pretty independent of changes to the preprocessor...
... the first four of those are extremely unlikely to clash - since they are concerned with the driver.

[urnathan]

Either update this when you're ready, or abandon and create a new one for 1857.

clang/include/clang/Basic/DiagnosticLexKinds.td

 def warn_module_conflict : Warning<
   "module '%0' conflicts with already-imported module '%1': %2">,
   InGroup<ModuleConflict>;
 
 // C++20 modules
 def err_header_import_semi_in_macro : Error<
   "semicolon terminating header import declaration cannot be produced "
   "by a macro">;
+def err_header_import_in_macro : Error<
+  "import keyword cannot be produced by a macro">;
+def err_header_export_in_macro : Error<
+  "export keyword cannot be produced by a macro">;
 def err_header_import_not_header_unit : Error<
   "header file %0 (aka '%1') cannot be imported because "
   "it is not known to be a header unit">;
 
 def warn_header_guard : Warning<
   "%0 is used as a header guard here, followed by #define of a different macro">,
   InGroup<DiagGroup<"header-guard">>;
 def note_header_guard : Note<

clang/include/clang/Basic/DiagnosticParseKinds.td

 def err_unsupported_module_partition : Error<
   "module partitions are only supported for C++20 onwards">;
 def err_import_not_allowed_here : Error<
   "imports must immediately follow the module declaration">;
 def err_partition_import_outside_module : Error<
   "module partition imports must be within a module purview">;
 def err_import_in_wrong_fragment : Error<
   "module%select{| partition}0 imports cannot be in the %select{global|private}1 module fragment">;
+def err_import_keyword_not_at_start_of_line : Error<
+  "import keyword must be at start of line">;
+def err_extra_tokens_after_module_import : Error<
+  "extra tokens after module import">;
+
 
 def err_export_empty : Error<"export declaration cannot be empty">;
 }
 
 let CategoryName = "Generics Issue" in {
 
 def err_objc_expected_type_parameter : Error<
   "expected type parameter name">;

clang/lib/Lex/Preprocessor.cpp

if (Result.is(tok::code_completion) && Result.getIdentifierInfo()) {
     // identifiers and completion tokens.
     Result.setIdentifierInfo(nullptr);
   }
 
   // Update ImportSeqState to track our position within a C++20 import-seq
   // if this token is being produced as a result of phase 4 of translation.
   if (getLangOpts().CPlusPlusModules && LexLevel == 1 &&
       !Result.getFlag(Token::IsReinjected)) {
+    if (CurPPLexer)
+      CurPPLexer->ParsingPreprocessorDirective = false;
+
     switch (Result.getKind()) {
     case tok::l_paren: case tok::l_square: case tok::l_brace:
       ImportSeqState.handleOpenBracket();
       break;
     case tok::r_paren: case tok::r_square:
       ImportSeqState.handleCloseBracket();
       break;
     case tok::r_brace:
       ImportSeqState.handleCloseBrace();
       break;
     case tok::semi:
       ImportSeqState.handleSemi();
       break;
     case tok::header_name:
     case tok::annot_header_unit:
       ImportSeqState.handleHeaderName();
       break;
     case tok::kw_export:
+      if (getLangOpts().CPlusPlusModules && Result.getLocation().isMacroID())
+        Diag(Result, diag::err_header_export_in_macro);
+      if (CurPPLexer && getLangOpts().CPlusPlusModules)
+        CurPPLexer->ParsingPreprocessorDirective = true;
       ImportSeqState.handleExport();
       break;
     case tok::identifier:
       if (Result.getIdentifierInfo()->isModulesImport()) {
+        if (getLangOpts().CPlusPlusModules && Result.getLocation().isMacroID())
+          Diag(Result, diag::err_header_import_in_macro);
+
         ImportSeqState.handleImport();
         if (ImportSeqState.afterImportSeq()) {
           ModuleImportLoc = Result.getLocation();
           ModuleImportPath.clear();
           ModuleImportExpectsIdentifier = true;
           CurLexerKind = CLK_LexAfterModuleImport;
         }
         break;
       }
       LLVM_FALLTHROUGH;
     default:
+      if (CurPPLexer && getLangOpts().CPlusPlusModules)
+        CurPPLexer->ParsingPreprocessorDirective = false;
       ImportSeqState.handleMisc();
       break;
     }
   }
 
   LastTokenWasAt = Result.is(tok::at);
   --LexLevel;
 

 ///
 ///     module-name-qualifier
 ///           module-name-qualifier[opt] identifier .
 ///
 /// We respond to a pp-import by importing macros from the named module.
 bool Preprocessor::LexAfterModuleImport(Token &Result) {
   // Figure out what kind of lexer we actually have.
   recomputeCurLexerKind();
+  // C++20 modules require the import statement to be on a single line
+  bool ConvertNewlinesToEod = getLangOpts().CPlusPlusModules;
 
+  if (ConvertNewlinesToEod && CurPPLexer)
+    CurPPLexer->ParsingPreprocessorDirective = true;
   // Lex the next token. The header-name lexing rules are used at the start of
   // a pp-import.
   //
   // For now, we only support header-name imports in C++20 mode.
   // FIXME: Should we allow this in all language modes that support an import
   // declaration as an extension?
   if (ModuleImportPath.empty() && getLangOpts().CPlusPlusModules) {
     if (LexHeaderName(Result))
if (Result.is(tok::header_name)) {
     // Enter the header-name token into the token stream; a Lex action cannot
     // both return a token and cache tokens (doing so would corrupt the token
     // cache if the call to Lex comes from CachingLex / PeekAhead).
     Suffix.push_back(Result);
 
     // Consume the pp-import-suffix and expand any macros in it now. We'll add
     // it back into the token stream later.
     CollectPpImportSuffix(Suffix);
+
+    if (ConvertNewlinesToEod && CurPPLexer)
+      CurPPLexer->ParsingPreprocessorDirective = false;
+
     if (Suffix.back().isNot(tok::semi)) {
       // This is not a pp-import after all.
       EnterTokens(Suffix);
       return false;
     }
 
     // C++2a [cpp.module]p1:
     //   The ';' preprocessing-token terminating a pp-import shall not have
case ImportAction::Failure:
       EnterTokens(Suffix);
       return true;
     }
 
     EnterTokens(Suffix);
     return false;
   }
 
+  if (ConvertNewlinesToEod && CurPPLexer)
+    CurPPLexer->ParsingPreprocessorDirective = false;
+
   // The token sequence
   //
   //   import identifier (. identifier)*
   //
   // indicates a module import directive. We already saw the 'import'
   // contextual keyword, so now we're looking for the identifiers.
   if (ModuleImportExpectsIdentifier && Result.getKind() == tok::identifier) {
     // We expected to see an identifier here, and we did; continue handling

clang/lib/Parse/ParseDeclCXX.cpp

 ///       export-declaration:
 ///         'export' declaration
 ///         'export' '{' declaration-seq[opt] '}'
 ///
 Decl *Parser::ParseExportDeclaration() {
   assert(Tok.is(tok::kw_export));
   SourceLocation ExportLoc = ConsumeToken();
 
+  TryConsumeToken(tok::eod);
+
   ParseScope ExportScope(this, Scope::DeclScope);
   Decl *ExportDecl = Actions.ActOnStartExportDecl(
       getCurScope(), ExportLoc,
       Tok.is(tok::l_brace) ? Tok.getLocation() : SourceLocation());
 
   if (Tok.isNot(tok::l_brace)) {
     // FIXME: Factor out a ParseExternalDeclarationWithAttrs.
     ParsedAttributes Attrs(AttrFactory);

clang/lib/Parse/Parser.cpp

bool Parser::ParseTopLevelDecl(DeclGroupPtrTy &Result,
 
   Result = nullptr;
   switch (Tok.getKind()) {
   case tok::annot_pragma_unused:
     HandlePragmaUnused();
     return false;
 
   case tok::kw_export:
-    switch (NextToken().getKind()) {
-    case tok::kw_module:
+    if (NextToken().is(tok::kw_module))
       goto module_decl;
-
+    else if (NextToken().is(tok::identifier) ||
+             (NextToken().is(tok::eod) &&
+              GetLookAheadToken(2).is(tok::identifier))) {
       // Note: no need to handle kw_import here. We only form kw_import under
-    // the Modules TS, and in that case 'export import' is parsed as an
-    // export-declaration containing an import-declaration.
+      // the Modules TS, and in that case 'export imort' is parsed as an
+      // export-declaration containing an import-declaration
 
       // Recognize context-sensitive C++20 'export module' and 'export import'
       // declarations.
-    case tok::identifier: {
-      IdentifierInfo *II = NextToken().getIdentifierInfo();
+      IdentifierInfo *II;
+      int ColonColonToken;
+      if (NextToken().is(tok::eod)) {
+        II = GetLookAheadToken(2).getIdentifierInfo();
+        ColonColonToken = 3;
+      } else {
+        II = NextToken().getIdentifierInfo();
+        ColonColonToken = 2;
+      }
+
       if ((II == Ident_module || II == Ident_import) &&
-          GetLookAheadToken(2).isNot(tok::coloncolon)) {
+          GetLookAheadToken(ColonColonToken).isNot(tok::coloncolon)) {
         if (II == Ident_module)
           goto module_decl;
         else
           goto import_decl;
       }
-      break;
-    }
-
-    default:
-      break;
     }
     break;
 
   case tok::kw_module:
   module_decl:
     Result = ParseModuleDecl(ImportState);
     return false;
 

 Parser::DeclGroupPtrTy
 Parser::ParseModuleDecl(Sema::ModuleImportState &ImportState) {
   SourceLocation StartLoc = Tok.getLocation();
 
   Sema::ModuleDeclKind MDK = TryConsumeToken(tok::kw_export)
                                  ? Sema::ModuleDeclKind::Interface
                                  : Sema::ModuleDeclKind::Implementation;
 
+  TryConsumeToken(tok::eod);
+
   assert(
       (Tok.is(tok::kw_module) ||
        (Tok.is(tok::identifier) && Tok.getIdentifierInfo() == Ident_module)) &&
       "not a module declaration");
   SourceLocation ModuleLoc = ConsumeToken();
 
   // Attributes appear after the module name, not before.
   // FIXME: Suggest moving the attributes later with a fixit.

 ///                   attribute-specifier-seq[opt] ';'
 ///           'export'[opt] 'import' module-partition
 ///                   attribute-specifier-seq[opt] ';'
 ///           'export'[opt] 'import' header-name
 ///                   attribute-specifier-seq[opt] ';'
 Decl *Parser::ParseModuleImport(SourceLocation AtLoc,
                                 Sema::ModuleImportState &ImportState) {
   SourceLocation StartLoc = AtLoc.isInvalid() ? Tok.getLocation() : AtLoc;
+  bool ImportTokAtStartOfLine = Tok.isAtStartOfLine();
 
   SourceLocation ExportLoc;
   TryConsumeToken(tok::kw_export, ExportLoc);
+  TryConsumeToken(tok::eod);
 
   assert((AtLoc.isInvalid() ? Tok.isOneOf(tok::kw_import, tok::identifier)
                             : Tok.isObjCAtKeyword(tok::objc_import)) &&
          "Improper start to module import");
   bool IsObjCAtImport = Tok.isObjCAtKeyword(tok::objc_import);
   SourceLocation ImportLoc = ConsumeToken();
 
   // For C++20 modules, we can have "name" or ":Partition name" as valid input.
if (!getLangOpts().CPlusPlusModules)
       Diag(ColonLoc, diag::err_unsupported_module_partition)
           << SourceRange(ColonLoc, Path.back().second);
     // Recover by leaving partition empty.
     else if (ParseModuleName(ColonLoc, Path, /*IsImport*/ true))
       return nullptr;
     else
       IsPartition = true;
   } else {
+    TryConsumeToken(tok::eod);
     if (ParseModuleName(ImportLoc, Path, /*IsImport*/ true))
       return nullptr;
   }
 
+  // Diagnose C++20 import keyword not being at the start of the line
+  // now, since this should only be emitted for header unit imports.
+  if (getLangOpts().CPlusPlusModules && HeaderUnit && !ImportTokAtStartOfLine)
+    Diag(StartLoc, diag::err_import_keyword_not_at_start_of_line);
+
   ParsedAttributes Attrs(AttrFactory);
   MaybeParseCXX11Attributes(Attrs);
   // We don't support any module import attributes yet.
   ProhibitCXX11Attributes(Attrs, diag::err_attribute_not_import_attr);
 
   if (PP.hadModuleLoaderFatalFailure()) {
     // With a fatal failure in the module loader, we abort parsing.
     cutOffParsing();
Decl *Parser::ParseModuleImport(SourceLocation AtLoc,
   DeclResult Import;
   if (HeaderUnit)
     Import =
         Actions.ActOnModuleImport(StartLoc, ExportLoc, ImportLoc, HeaderUnit);
   else if (!Path.empty())
     Import = Actions.ActOnModuleImport(StartLoc, ExportLoc, ImportLoc, Path,
                                        IsPartition);
   ExpectAndConsumeSemi(diag::err_module_expected_semi);
+
+  if (getLangOpts().CPlusPlusModules && HeaderUnit &&
+      !Tok.isOneOf(tok::eof, tok::eod) && !Tok.isAtStartOfLine())
+    Diag(Tok, diag::err_extra_tokens_after_module_import);
+
   if (Import.isInvalid())
     return nullptr;
 
   // Using '@import' in framework headers requires modules to be enabled so that
   // the header is parseable. Emit a warning to make the user aware.
   if (IsObjCAtImport && AtLoc.isValid()) {
     auto &SrcMgr = PP.getSourceManager();
     auto *FE = SrcMgr.getFileEntryForID(SrcMgr.getFileID(AtLoc));

clang/test/CXX/cpp/cpp.module/p1.cpp

 // RUN: %clang_cc1 -std=c++2a -emit-header-module -fmodule-name=attrs -x c++-header %S/Inputs/empty.h %S/Inputs/attrs.h -o %t.pcm
 // RUN: %clang_cc1 -std=c++2a %s -fmodule-file=%t.pcm -E -verify -I%S/Inputs | FileCheck %s
 
 #define SEMI ;
 // expected-error@+1 {{semicolon terminating header import declaration cannot be produced by a macro}}
 import "empty.h" SEMI // CHECK: import attrs.{{.*}};
 
-#define IMPORT import "empty.h"
-IMPORT; // CHECK: import attrs.{{.*}};
+#define IMPORT "empty.h"
+import IMPORT; // CHECK: import attrs.{{.*}};
 
-#define IMPORT_ANGLED import <empty.h>
-IMPORT_ANGLED; // CHECK: import attrs.{{.*}};
+#define IMPORT_ANGLED <empty.h>
+import IMPORT_ANGLED; // CHECK: import attrs.{{.*}};
 
 // Ensure that macros only become visible at the semicolon.
 // CHECK: import attrs.{{.*}} ATTRS ;
 import "attrs.h" ATTRS ;
 // CHECK: {{\[\[}} ]] int n;
 ATTRS int n;

clang/test/CXX/lex/lex.pptoken/p3-2a.cpp

 
 _Pragma("clang no_such_pragma (");
 // CHECK: import <foo  bar>;
 import <foo  bar>;
 
 #define HEADER <foo  bar>
 // CHECK: import <foo bar>;
 import HEADER;
-
-// CHECK: import <foo bar>;
-import <
-foo
-  bar
->;
-
-// CHECK: import{{$}}
-// CHECK: {{^}}<foo bar>;
-import
-<
-foo
-  bar
->;
-
-// CHECK: import{{$}}
-// CHECK: {{^}}<foo  bar>;
-import
-<foo  bar>;
-
-#define IMPORT import <foo  bar>
-// CHECK: import <foo bar>;
-IMPORT;

clang/test/Modules/P1703R1.cpp

+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: mkdir -p %t/system
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit \
+// RUN: -xc++-header header.h -o header.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -isystem system \
+// RUN: -xc++-system-header system-header.h -o system/system-header.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface dummy.cpp -o dummy.pcm
+
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=header.pcm \
+// RUN: -fmodule-file=system/system-header.pcm \
+// RUN: -fmodule-file=dummy.pcm \
+// RUN: -emit-module-interface -isystem system \
+// RUN: main.cpp -verify
+
+//--- header.h
+int foo(int);
+
+//--- system/system-header.h
+int foo2(int);
+
+//--- dummy.cpp
+export module dummy;
+
+//--- main.cpp
+export module main;
+#define IMPORT import
+#define EXPORT export
+#define HEADER "header.h"
+#define SYSTEM_HEADER <system-header.h>
+#define SEMI ;
+
+
+/// Fine.
+import "header.h";
+export import "header.h";
+
+
+import
+"header.h";// expected-error {{expected a module name after 'import'}}
+export import
+"header.h"; // expected-error {{expected a module name after 'import'}}
+export
+import "header.h"; // expected-error {{expected a module name after 'import'}}
+
+import "header.h"; import "header.h"; // expected-error {{import keyword must be at start of line}}
+                                      // expected-error@-1 {{extra tokens after module import}}
+
+import "header.h" SEMI // expected-error {{semicolon terminating header import declaration cannot be produced by a macro}}
+IMPORT "header.h"; // expected-error {{import keyword cannot be produced by a macro}}
+EXPORT import "header.h"; // expected-error {{export keyword cannot be produced by a macro}}
+import HEADER; /// Fine.
+
+
+import <system-header.h>;
+export import <system-header.h>;
+
+import
+<system-header.h>; // expected-error {{expected a module name after 'import'}}
+IMPORT <system-header.h>;// expected-error {{import keyword cannot be produced by a macro}}
+EXPORT import <system-header.h>;// expected-error {{export keyword cannot be produced by a macro}}
+import SYSTEM_HEADER;
+
+
+
+/// Normal module imports are unaffected
+import dummy;
+import
+dummy;
+export
+import
+dummy;
+export import
+dummy;
+export
+import dummy;
+import dummy; import dummy;
+
+
+
+// TODO: The diagnostics here could be much better.
+export
+import "header.h"; // expected-error {{expected a module name after 'import'}}
+
+export import
+"header.h" // expected-error {{expected a module name after 'import'}}
+
+
+export void foo() {}
+export void
+baz(){}
+export
+void bar(){}
+
+export {
+  void a(){}
+}