[clang][deps] Properly capture the global module and '\n' for all module directives

[naveen-seth]

Previously, the newline after a module directive was not properly captured (or printed).

According to P1857R3, each directive must, after skipping horizontal whitespace, be at the start of a logical line.
Because the newline after module directives was missing, this invalidated the following line.

This also fixes or removes tests that were in violation of P1857R3.
This extends to Objective-C directives, which should also follow P1857R3.

This also ensures that the global module fragment module; is captured by the dependency directives scanner.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Naveen Seth Hanig (naveen-seth)

Changes

With this change, the dependency directive scanner now properly identifies "module;" as a directive, as per P1857R3.
Previously, the global module fragment was not recognized by the scanner.

---

Full diff: https://github.com/llvm/llvm-project/pull/148685.diff

2 Files Affected:

• (modified) clang/lib/Lex/DependencyDirectivesScanner.cpp (+7)
• (modified) clang/unittests/Lex/DependencyDirectivesScannerTest.cpp (+5-4)

diff --git a/clang/lib/Lex/DependencyDirectivesScanner.cpp b/clang/lib/Lex/DependencyDirectivesScanner.cpp
index d894c265a07a2..8822e760274d0 100644
--- a/clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ b/clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -728,6 +728,13 @@ bool Scanner::lexModule(const char *&First, const char *const End) {
       return false;
     break;
   }
+  case ';': {
+    // Handle the global module fragment `module;`.
+    if (Id == "module" && !Export)
+      break;
+    skipLine(First, End);
+    return false;
+  }
   case '<':
   case '"':
     break;
diff --git a/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp b/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
index d2ef27155df94..92f6f401ec6b7 100644
--- a/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ b/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -1122,16 +1122,17 @@ ort \
     )";
   ASSERT_FALSE(
       minimizeSourceToDependencyDirectives(Source, Out, Tokens, Directives));
-  EXPECT_STREQ("#include \"textual-header.h\"\nexport module m;"
+  EXPECT_STREQ("module;#include \"textual-header.h\"\nexport module m;"
                "exp\\\nort import:l[[rename]];"
                "import<<=3;import a b d e d e f e;"
                "import foo[[no_unique_address]];import foo();"
                "import f(:sefse);import f(->a=3);"
                "<TokBeforeEOF>\n",
                Out.data());
-  ASSERT_EQ(Directives.size(), 11u);
-  EXPECT_EQ(Directives[0].Kind, pp_include);
-  EXPECT_EQ(Directives[1].Kind, cxx_export_module_decl);
+  ASSERT_EQ(Directives.size(), 12u);
+  EXPECT_EQ(Directives[0].Kind, cxx_module_decl);
+  EXPECT_EQ(Directives[1].Kind, pp_include);
+  EXPECT_EQ(Directives[2].Kind, cxx_export_module_decl);
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, ObjCMethodArgs) {

[Bigcheese]

This needs a \n after the module; so that #include starts on a new line.

[Bigcheese]

Looks good to me, but I'd also like @akyrtzi to take a look if they have time.

[akyrtzi]

Why is this check removed?

[naveen-seth]

From P1857R3:

The entire import or module directive (including the closing ;) must be on a single logical line and for module must not come from an #include.

While this paper addresses C++, I've consulted with @Bigcheese on whether Objective-C modules should follow the same rules.
In the same spirit, a line-splice was added to this test:

llvm-project/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp

Lines 645 to 647 in 586b438

	ASSERT_FALSE(minimizeSourceToDependencyDirectives(
	"@import /*x*/ A /*x*/ . /*x*/ B /*x*/ \\n /*x*/ ; /*x*/", Out));
	EXPECT_STREQ("@import A.B\\n;\n", Out.data());

This patch currently fails CI due to some clang-scan-deps test failures caused by these changes. I'll fix those issues now. Apologies for missing this earlier.

[akyrtzi]

Could you keep the check but update what the result of the scanner for this input should be?