[C++20] [Modules] Improve the import-and-include pattern

The import and include problem is a long-standing issue with the use of
C++20 modules. This patch tried to improve this by skipping parsing
class and functions if their declaration is already defined in modules.

The scale of the patch itself is small as the patch reuses previous
optimization. Maybe we can skip parsing other declarations in the
future. But the patch itself should be good.

6 files changed, 117 insertions, 26 deletions

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index da90708..2b835be 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -15328,6 +15328,16 @@ public:
     NamedDecl *Hidden;
     return hasVisibleDefinition(const_cast<NamedDecl *>(D), &Hidden);
   }
+  /// Determine if \p D has a definition which allows we redefine it in current
+  /// TU. \p Suggested is the definition that should be made visible to expose
+  /// the definition.
+  bool isRedefinitionAllowedFor(NamedDecl *D, NamedDecl **Suggested,
+                                bool &Visible);
+  bool isRedefinitionAllowedFor(const NamedDecl *D, bool &Visible) {
+    NamedDecl *Hidden;
+    return isRedefinitionAllowedFor(const_cast<NamedDecl *>(D), &Hidden,
+                                    Visible);
+  }
 
   /// Determine if \p D has a reachable definition. If not, suggest a
   /// declaration that should be made reachable to expose the definition.
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 8ddbaf3..d419f66 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -15837,17 +15837,18 @@ Sema::CheckForFunctionRedefinition(FunctionDecl *FD,
   if (TypoCorrectedFunctionDefinitions.count(Definition))
     return;
 
-  // If we don't have a visible definition of the function, and it's inline or
-  // a template, skip the new definition.
-  if (SkipBody && !hasVisibleDefinition(Definition) &&
+  bool DefinitionVisible = false;
+  if (SkipBody && isRedefinitionAllowedFor(Definition, DefinitionVisible) &&
       (Definition->getFormalLinkage() == Linkage::Internal ||
        Definition->isInlined() || Definition->getDescribedFunctionTemplate() ||
        Definition->getNumTemplateParameterLists())) {
     SkipBody->ShouldSkip = true;
     SkipBody->Previous = const_cast<FunctionDecl*>(Definition);
-    if (auto *TD = Definition->getDescribedFunctionTemplate())
-      makeMergedDefinitionVisible(TD);
-    makeMergedDefinitionVisible(const_cast<FunctionDecl*>(Definition));
+    if (!DefinitionVisible) {
+      if (auto *TD = Definition->getDescribedFunctionTemplate())
+        makeMergedDefinitionVisible(TD);
+      makeMergedDefinitionVisible(const_cast<FunctionDecl *>(Definition));
+    }
     return;
   }
 
@@ -18196,8 +18197,10 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc,
                 // However, ensure the decl passes the structural compatibility
                 // check in C11 6.2.7/1 (or 6.1.2.6/1 in C89).
                 NamedDecl *Hidden = nullptr;
-                if (SkipBody && (!hasVisibleDefinition(Def, &Hidden) ||
-                                 getLangOpts().C23)) {
+                bool HiddenDefVisible = false;
+                if (SkipBody &&
+                    (isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible) ||
+                     getLangOpts().C23)) {
                   // There is a definition of this tag, but it is not visible.
                   // We explicitly make use of C++'s one definition rule here,
                   // and assume that this definition is identical to the hidden
@@ -18213,13 +18216,15 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc,
 
                     ProcessDeclAttributeList(S, SkipBody->New, Attrs);
                     return Def;
-                  } else {
-                    SkipBody->ShouldSkip = true;
-                    SkipBody->Previous = Def;
-                    makeMergedDefinitionVisible(Hidden);
-                    // Carry on and handle it like a normal definition. We'll
-                    // skip starting the definition later.
                   }
+
+                  SkipBody->ShouldSkip = true;
+                  SkipBody->Previous = Def;
+                  if (!HiddenDefVisible && Hidden)
+                    makeMergedDefinitionVisible(Hidden);
+                  // Carry on and handle it like a normal definition. We'll
+                  // skip starting the definition later.
+
                 } else if (!IsExplicitSpecializationAfterInstantiation) {
                   // A redeclaration in function prototype scope in C isn't
                   // visible elsewhere, so merely issue a warning.
@@ -20842,3 +20847,13 @@ bool Sema::shouldIgnoreInHostDeviceCheck(FunctionDecl *Callee) {
   return LangOpts.CUDA && !LangOpts.CUDAIsDevice &&
          CUDA().IdentifyTarget(Callee) == CUDAFunctionTarget::Global;
 }
+
+bool Sema::isRedefinitionAllowedFor(NamedDecl *D, NamedDecl **Suggested,
+                                    bool &Visible) {
+  Visible = hasVisibleDefinition(D, Suggested);
+  // The redefinition of D in the **current** TU is allowed if D is invisible or
+  // D is defined in the global module of other module units. We didn't check if
+  // it is in global module as, we'll check the redefinition in named module
+  // later with better diagnostic message.
+  return D->isInAnotherModuleUnit() || !Visible;
+}
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 72d98ca..0edc850 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -2078,14 +2078,19 @@ DeclResult Sema::CheckClassTemplate(
         // If we have a prior definition that is not visible, treat this as
         // simply making that previous definition visible.
         NamedDecl *Hidden = nullptr;
-        if (SkipBody && !hasVisibleDefinition(Def, &Hidden)) {
+        bool HiddenDefVisible = false;
+        if (SkipBody &&
+            isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible)) {
           SkipBody->ShouldSkip = true;
           SkipBody->Previous = Def;
-          auto *Tmpl = cast<CXXRecordDecl>(Hidden)->getDescribedClassTemplate();
-          assert(Tmpl && "original definition of a class template is not a "
-                         "class template?");
-          makeMergedDefinitionVisible(Hidden);
-          makeMergedDefinitionVisible(Tmpl);
+          if (!HiddenDefVisible && Hidden) {
+            auto *Tmpl =
+                cast<CXXRecordDecl>(Hidden)->getDescribedClassTemplate();
+            assert(Tmpl && "original definition of a class template is not a "
+                           "class template?");
+            makeMergedDefinitionVisible(Hidden);
+            makeMergedDefinitionVisible(Tmpl);
+          }
         } else {
           Diag(NameLoc, diag::err_redefinition) << Name;
           Diag(Def->getLocation(), diag::note_previous_definition);
@@ -8956,10 +8961,13 @@ DeclResult Sema::ActOnClassTemplateSpecialization(
   if (TUK == TagUseKind::Definition) {
     RecordDecl *Def = Specialization->getDefinition();
     NamedDecl *Hidden = nullptr;
-    if (Def && SkipBody && !hasVisibleDefinition(Def, &Hidden)) {
+    bool HiddenDefVisible = false;
+    if (Def && SkipBody &&
+        isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible)) {
       SkipBody->ShouldSkip = true;
       SkipBody->Previous = Def;
-      makeMergedDefinitionVisible(Hidden);
+      if (!HiddenDefVisible && Hidden)
+        makeMergedDefinitionVisible(Hidden);
     } else if (Def) {
       SourceRange Range(TemplateNameLoc, RAngleLoc);
       Diag(TemplateNameLoc, diag::err_redefinition) << Specialization << Range;
diff --git a/clang/test/CXX/drs/cwg279.cpp b/clang/test/CXX/drs/cwg279.cpp
index 3c63486..d10ceb2 100644
--- a/clang/test/CXX/drs/cwg279.cpp
+++ b/clang/test/CXX/drs/cwg279.cpp
@@ -46,8 +46,8 @@ extern S2 *q2;
 
 // FIXME: This is well-formed, because [basic.def.odr]/15 is satisfied.
 struct S3 {};
-// since-cxx20-error@-1 {{redefinition of 'S3'}}
-//   since-cxx20-note@cwg279_A.cppm:23 {{previous definition is here}}
+// since-cxx20-error@-1 {{declaration of 'S3' in the global module follows declaration in module cwg279_A}}
+//   since-cxx20-note@cwg279_A.cppm:23 {{previous declaration is here}}
 extern S3 *q3;
 // since-cxx20-error@-1 {{declaration of 'q3' in the global module follows declaration in module cwg279_A}}
 //   since-cxx20-note@cwg279_A.cppm:24 {{previous declaration is here}}
diff --git a/clang/test/Modules/redundant-template-default-arg2.cpp b/clang/test/Modules/redundant-template-default-arg2.cpp
index ae1f0c7..6e22d82 100644
--- a/clang/test/Modules/redundant-template-default-arg2.cpp
+++ b/clang/test/Modules/redundant-template-default-arg2.cpp
@@ -33,8 +33,8 @@ int v2; // expected-error {{declaration of 'v2' in the global module follows dec
         // expected-note@foo.cppm:6 {{previous declaration is here}}
 
 template <typename T>
-class my_array {}; // expected-error {{redefinition of 'my_array'}}
-                   // expected-note@foo.cppm:9 {{previous definition is here}}
+class my_array {}; // expected-error {{declaration of 'my_array' in the global module follows declaration in module foo}}
+                   // expected-note@foo.cppm:9 {{previous declaration is here}}
 
 template <template <typename> typename C = my_array>
 int v3; // expected-error {{declaration of 'v3' in the global module follows declaration in module foo}}
diff --git a/clang/test/Modules/skip-body-2.cppm b/clang/test/Modules/skip-body-2.cppm
new file mode 100644
index 0000000..781f4fe
--- /dev/null
+++ b/clang/test/Modules/skip-body-2.cppm
@@ -0,0 +1,58 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-reduced-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/a.cpp -fmodule-file=a=%t/a.pcm -ast-dump | FileCheck %s
+
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/a.cpp -fmodule-file=a=%t/a.pcm -ast-dump | FileCheck %s
+
+//--- a.h
+namespace a {
+class A {
+public:
+    int aaaa;
+
+    int get() {
+        return aaaa;
+    }
+};
+
+
+template <class T>
+class B {
+public:
+    B(T t): t(t) {}
+    T t;
+};
+
+using BI = B<int>;
+
+inline int get(A a, BI b) {
+    return a.get() + b.t;
+}
+
+}
+
+//--- a.cppm
+export module a;
+
+export extern "C++" {
+#include "a.h"
+}
+
+//--- a.cpp
+import a;
+#include "a.h"
+
+int test() {
+    a::A aa;
+    a::BI bb(43);
+    return get(aa, bb);
+}
+
+// CHECK-NOT: DefinitionData
+// CHECK: FunctionDecl {{.*}} get 'int (A, BI)' {{.*}}
+// CHECK-NOT: CompoundStmt
+// CHECK: FunctionDecl {{.*}} test {{.*}}