[clang] 0d21436 - [C++20] [Modules] Warn if we found #include <filename> in module purview (#69555)

[via cfe-commits]

Author: Chuanqi Xu
Date: 2023-11-02T16:40:20+08:00
New Revision: 0d2143611425081bb9db5bb6ee57aaddfd1eda53

URL: https://github.com/llvm/llvm-project/commit/0d2143611425081bb9db5bb6ee57aaddfd1eda53
DIFF: https://github.com/llvm/llvm-project/commit/0d2143611425081bb9db5bb6ee57aaddfd1eda53.diff

LOG: [C++20] [Modules] Warn if we found #include <filename> in module purview (#69555)

Close https://github.com/llvm/llvm-project/issues/68615.

It is generally wrong to include <filename> in the module purview.
Although there are cases to include files in the module purview,
generally these use cases should include files by quotes instead of by
angles. Here we think the files got included by angles are the system
headers.

This is consistency with MSVC too:
https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warnings-by-compiler-version?view=msvc-170#warnings-introduced-in-visual-studio-2022-version-170-compiler-version-1930

Added: 
    clang/test/Preprocessor/include-in-module-purview.cppm

Modified: 
    clang/include/clang/Basic/DiagnosticLexKinds.td
    clang/lib/Lex/PPDirectives.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Basic/DiagnosticLexKinds.td b/clang/include/clang/Basic/DiagnosticLexKinds.td
index 940cca67368492f..564ca48cc32ac55 100644
--- a/clang/include/clang/Basic/DiagnosticLexKinds.td
+++ b/clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -922,6 +922,11 @@ def err_header_import_semi_in_macro : Error<
 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_pp_include_angled_in_module_purview : Warning<
+  "'#include <filename>' attaches the declarations to the named module '%0'"
+  ", which is not usually intended; consider moving that directive before "
+  "the module declaration">,
+  InGroup<DiagGroup<"include-angled-in-module-purview">>;
 
 def warn_header_guard : Warning<
   "%0 is used as a header guard here, followed by #define of a 
diff erent macro">,

diff  --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 6fd515a5f0c756b..d97a103833c2fa6 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -2537,6 +2537,10 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport(
     return {ImportAction::None};
   }
 
+  if (isAngled && isInNamedModule())
+    Diag(FilenameTok, diag::warn_pp_include_angled_in_module_purview)
+        << getNamedModuleName();
+
   // Look up the file, create a File ID for it.
   SourceLocation IncludePos = FilenameTok.getLocation();
   // If the filename string was the result of macro expansions, set the include

diff  --git a/clang/test/Preprocessor/include-in-module-purview.cppm b/clang/test/Preprocessor/include-in-module-purview.cppm
new file mode 100644
index 000000000000000..0a080112b43277c
--- /dev/null
+++ b/clang/test/Preprocessor/include-in-module-purview.cppm
@@ -0,0 +1,60 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -E -P -I%t -o %t/tmp 2>&1 | FileCheck %t/a.cppm
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -E -P -I%t -o - 2>&1 \
+// RUN:     -Wno-include-angled-in-module-purview | FileCheck %t/a.cppm --check-prefix=CHECK-NO-WARN
+
+//--- a.h
+// left empty
+
+//--- b.h
+#include <stddef.h>
+// The headers not get included shouldn't be affected.
+#ifdef WHATEVER
+#include <stdint.h>
+#endif
+
+//--- a.cppm
+module;
+#include <stddef.h>
+#include <a.h>
+#include <b.h>
+#include "a.h"
+#include "b.h"
+export module a;
+
+#include <stddef.h>
+#include <a.h>
+#include <b.h>
+#include "a.h"
+#include "b.h"
+
+// CHECK: a.cppm:9:10: warning: '#include <filename>' attaches the declarations to the named module 'a'
+// CHECK: a.cppm:10:10: warning: '#include <filename>' attaches the declarations to the named module 'a'
+// CHECK: a.cppm:11:10: warning: '#include <filename>' attaches the declarations to the named module 'a'
+// CHECK: In file included from {{.*}}/a.cppm:11
+// CHECK-NEXT: b.h:1:10: warning: '#include <filename>' attaches the declarations to the named module 'a'
+// CHECK: In file included from {{.*}}/a.cppm:13
+// CHECK-NEXT: b.h:1:10: warning: '#include <filename>' attaches the declarations to the named module 'a'
+
+module :private;
+#include <stddef.h>
+#include <a.h>
+#include <b.h>
+#include "a.h"
+#include "b.h"
+
+// CHECK: a.cppm:24:10: warning: '#include <filename>' attaches the declarations to the named module 'a'
+// CHECK: a.cppm:25:10: warning: '#include <filename>' attaches the declarations to the named module 'a'
+// CHECK: a.cppm:26:10: warning: '#include <filename>' attaches the declarations to the named module 'a'
+// CHECK: In file included from {{.*}}/a.cppm:26
+// CHECK-NEXT: b.h:1:10: warning: '#include <filename>' attaches the declarations to the named module 'a'
+// CHECK: In file included from {{.*}}/a.cppm:28
+// CHECK-NEXT: b.h:1:10: warning: '#include <filename>' attaches the declarations to the named module 'a'
+
+// We should have catched all warnings.
+// CHECK: 10 warnings generated.
+
+// CHECK-NO-WARN-NOT: warning