[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.

Author: ChuanqiXu9

clang/include/clang/Sema/Sema.h

@@ -15328,6 +15328,16 @@ class Sema final : public SemaBase {
     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.

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;
+}

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;

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}}

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
         // [email protected]:6 {{previous declaration is here}}
 
 template <typename T>
-class my_array {}; // expected-error {{redefinition of 'my_array'}}
-                   // [email protected]:9 {{previous definition is here}}
+class my_array {}; // expected-error {{declaration of 'my_array' in the global module follows declaration in module foo}}
+                   // [email protected]: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}}

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 {{.*}}