[C++20][Modules] Update handling of implicit inlines [P1779R3]

[iains]

Summary

This provides updates to
[class.mfct]:
Pre C++20 [class.mfct]p2:

A member function may be defined (8.4) in its class definition, in
which case it is an inline member function (7.1.2)

Post C++20 [class.mfct]p1:

If a member function is attached to the global module and is defined
in its class definition, it is inline.

and
[class.friend]:
Pre-C++20 [class.friend]p5

A function can be defined in a friend declaration of a
class . . . . Such a function is implicitly inline.

Post C++20 [class.friend]p7

Such a function is implicitly an inline function if it is attached
to the global module.

We add the output of implicit-inline to the TextNodeDumper, and amend
a couple of existing tests to account for this, plus add tests for the
cases covered above.

[iains]

the other provisions of P1779 are handled elsewhere:

1. private module fragment (which is also under discussion in core) and
2. implicit inline status for constexpr/consteval which has been implemented elsewhere (and is not modules-dependent).

[ChuanqiXu]

clang/lib/AST/TextNodeDumper.cpp

1723–1725

Is this necessary to this revision?

clang/lib/Sema/SemaDecl.cpp

9434

If I remember correctly, when we run -std=c++2b, getLangOpts().CPlusPlus20 would be true too. So we could check getLangOpts().CPlusPlus20 only here.

9435–9437

nit: this is not required.

9724

ditto

[iains]

clang/lib/AST/TextNodeDumper.cpp

1723–1725

yes, to test it

clang/lib/Sema/SemaDecl.cpp

9434

will double-check.

9435–9437

well, the logic is required ;) .. you have suggested a different way to write..

9724

likewise.

[iains]

rebased, factored code to address comments.

[iains]

clang/lib/Sema/SemaDecl.cpp

9435–9437

actually, I think you meant !NewFD->getOwningModule() || NewFD->getOwningModule()->isGlobalModule() but I've pulled the test out so that it's only done once.

[ChuanqiXu]

It looks like the tests lack the cast that the methods are attached to the global module fragment.

clang/lib/Sema/SemaDecl.cpp

9435–9437

Yeah, thanks!

clang/test/AST/ast-dump-constant-expr.cpp

94

Maybe it is good to add the newline although it is not your fault.

[iains]

In D129045#3627856, @ChuanqiXu wrote:

It looks like the tests lack the cast that the methods are attached to the global module fragment.

(maybe I misunderstand what you are saying here)

bool ImplicitInlineCXX20 = !getLangOpts().CPlusPlus20 ||
                               !NewFD->getOwningModule() ||
                               NewFD->getOwningModule()->isGlobalModule();

We are in the global module if there is no owning module, or if the owning module is the GMF.
We always set this to true before C++20, matching the unconditional true default argument it the setImplicitlyInline() method.

[ChuanqiXu]

In D129045#3627878, @iains wrote:

In D129045#3627856, @ChuanqiXu wrote:

It looks like the tests lack the cast that the methods are attached to the global module fragment.

(maybe I misunderstand what you are saying here)

bool ImplicitInlineCXX20 = !getLangOpts().CPlusPlus20 ||
                               !NewFD->getOwningModule() ||
                               NewFD->getOwningModule()->isGlobalModule();

We are in the global module if there is no owning module, or if the owning module is the GMF.
We always set this to true before C++20, matching the unconditional true default argument it the setImplicitlyInline() method.

I mean the tests instead of the implementation. It looks like there is no GMF in the tests.

[iains]

updated testcases to fix a missing new line and add included header in a GMF.

[iains]

In D129045#3627901, @ChuanqiXu wrote:

In D129045#3627878, @iains wrote:

In D129045#3627856, @ChuanqiXu wrote:

It looks like the tests lack the cast that the methods are attached to the global module fragment.

(maybe I misunderstand what you are saying here)

I mean the tests instead of the implementation. It looks like there is no GMF in the tests.

added.

[ChuanqiXu]

LGTM.

[iains]

reopening to post the patch I plan to re-land.

[iains]

rebased, fixed interaction with clang modules.

clang/lib/AST/TextNodeDumper.cpp

if (MD->size_overridden_methods() != 0) {
           OS << ", ";
           dumpOverride(Override);
         }
         OS << " ]";
       });
     }
   }
 
+  if (!D->isInlineSpecified() && D->isInlined()) {
+    OS << " implicit-inline";
+  }
   // Since NumParams comes from the FunctionProtoType of the FunctionDecl and
   // the Params are set later, it is possible for a dump during debugging to
   // encounter a FunctionDecl that has been created but hasn't been assigned
   // ParmVarDecls yet.
   if (!D->param_empty() && !D->param_begin())
     OS << " <<<NULL params x " << D->getNumParams() << ">>>";
 }
 

clang/lib/Sema/SemaDecl.cpp

Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
   // C++ scope specifier, or is the object of a friend declaration, the lexical
   // context will be different from the semantic context.
   NewFD->setLexicalDeclContext(CurContext);
 
   if (IsLocalExternDecl)
     NewFD->setLocalExternDecl();
 
   if (getLangOpts().CPlusPlus) {
+    // The rules for implicit inlines changed in C++20 for methods and friends
+    // with an in-class definition (when such a definition is not attached to
+    // the global module).  User-specified 'inline' overrides this (set when
+    // the function decl is created above).
+    // FIXME: We need a better way to separate C++ standard and clang modules.
+    bool ImplicitInlineCXX20 = !getLangOpts().CPlusPlusModules ||
+                               !NewFD->getOwningModule() ||
+                               NewFD->getOwningModule()->isGlobalModule() ||
+                               NewFD->getOwningModule()->isModuleMapModule();
     bool isInline = D.getDeclSpec().isInlineSpecified();
     bool isVirtual = D.getDeclSpec().isVirtualSpecified();
     bool hasExplicit = D.getDeclSpec().hasExplicitSpecifier();
     isFriend = D.getDeclSpec().isFriendSpecified();
     if (isFriend && !isInline && D.isFunctionDefinition()) {
-      // C++ [class.friend]p5
+      // Pre-C++20 [class.friend]p5
       //   A function can be defined in a friend declaration of a
       //   class . . . . Such a function is implicitly inline.
-      NewFD->setImplicitlyInline();
+      // Post C++20 [class.friend]p7
+      //   Such a function is implicitly an inline function if it is attached
+      //   to the global module.
+      NewFD->setImplicitlyInline(ImplicitInlineCXX20);
     }
 
     // If this is a method defined in an __interface, and is not a constructor
     // or an overloaded operator, then set the pure flag (isVirtual will already
     // return true).
     if (const CXXRecordDecl *Parent =
           dyn_cast<CXXRecordDecl>(NewFD->getDeclContext())) {
       if (Parent->isInterface() && cast<CXXMethodDecl>(NewFD)->isUserProvided())
         NewFD->setPure(true);
 
       // C++ [class.union]p2
case FunctionDefinitionKind::Defaulted:
       break;
 
     case FunctionDefinitionKind::Deleted:
       NewFD->setDeletedAsWritten();
       break;
     }
 
     if (isa<CXXMethodDecl>(NewFD) && DC == CurContext &&
-        D.isFunctionDefinition()) {
-      // C++ [class.mfct]p2:
+        D.isFunctionDefinition() && !isInline) {
+      // Pre C++20 [class.mfct]p2:
       //   A member function may be defined (8.4) in its class definition, in
       //   which case it is an inline member function (7.1.2)
-      NewFD->setImplicitlyInline();
+      // Post C++20 [class.mfct]p1:
+      //   If a member function is attached to the global module and is defined
+      //   in its class definition, it is inline.
+      NewFD->setImplicitlyInline(ImplicitInlineCXX20);
     }
 
     if (SC == SC_Static && isa<CXXMethodDecl>(NewFD) &&
         !CurContext->isRecord()) {
       // C++ [class.static]p1:
       //   A data or function member of a class may be declared static
       //   in a class definition, in which case it is a static member of
       //   the class.

clang/test/AST/ast-dump-constant-expr.cpp

void test() {
     //(void) test_SU();
   }
 
   consteval void consteval_method() {}
 };
 
 // CHECK:Dumping Test:
 // CHECK-NEXT:CXXRecordDecl {{.*}} <{{.*}}ast-dump-constant-expr.cpp:43:1, line:57:1> line:43:8 struct Test definition
-// CHECK:|-CXXMethodDecl {{.*}} <line:44:3, line:54:3> line:44:8 test 'void ()'
+// CHECK:|-CXXMethodDecl {{.*}} <line:44:3, line:54:3> line:44:8 test 'void ()' implicit-inline
 // CHECK-NEXT:| `-CompoundStmt {{.*}} <col:15, line:54:3>
 // CHECK-NEXT:|   |-CStyleCastExpr {{.*}} <line:45:5, col:20> 'void' <ToVoid>
 // CHECK-NEXT:|   | `-ConstantExpr {{.*}} <col:11, col:20> 'int'
 // CHECK-NEXT:|   |   |-value: Int 42
 // CHECK-NEXT:|   |   `-CallExpr {{.*}} <col:11, col:20> 'int'
 // CHECK-NEXT:|   |     `-ImplicitCastExpr {{.*}} <col:11> 'int (*)()' <FunctionToPointerDecay>
 // CHECK-NEXT:|   |       `-DeclRefExpr {{.*}} <col:11> 'int ()' lvalue Function {{.*}} 'test_Int' 'int ()'
 // CHECK-NEXT:|   |-CStyleCastExpr {{.*}} <line:46:5, col:22> 'void' <ToVoid>

 // CHECK-NEXT:|   |     `-ImplicitCastExpr {{.*}} <col:11> '_Complex float (*)()' <FunctionToPointerDecay>
 // CHECK-NEXT:|   |       `-DeclRefExpr {{.*}} <col:11> '_Complex float ()' lvalue Function {{.*}} 'test_ComplexFloat' '_Complex float ()'
 // CHECK-NEXT:|   `-CStyleCastExpr {{.*}} <line:49:5, col:23> 'void' <ToVoid>
 // CHECK-NEXT:|     `-ConstantExpr {{.*}} <col:11, col:23> '__int128'
 // CHECK-NEXT:|       |-value: Int 18446744073709551616
 // CHECK-NEXT:|       `-CallExpr {{.*}} <col:11, col:23> '__int128'
 // CHECK-NEXT:|         `-ImplicitCastExpr {{.*}} <col:11> '__int128 (*)()' <FunctionToPointerDecay>
 // CHECK-NEXT:|           `-DeclRefExpr {{.*}} <col:11> '__int128 ()' lvalue Function {{.*}} 'test_Int128' '__int128 ()'
-// CHECK-NEXT:`-CXXMethodDecl {{.*}} <line:56:3, col:38> col:18 consteval consteval_method 'void ()'
+// CHECK-NEXT:`-CXXMethodDecl {{.*}} <line:56:3, col:38> col:18 consteval consteval_method 'void ()' implicit-inline
 

clang/test/AST/ast-dump-lambda.cpp

 // CHECK-NEXT:    |   |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
 // CHECK-NEXT:    |   | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
 // CHECK-NEXT:    |   | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT:    |   | |-MoveConstructor exists simple trivial needs_implicit
 // CHECK-NEXT:    |   | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT:    |   | |-MoveAssignment exists simple trivial needs_implicit
 // CHECK-NEXT:    |   | `-Destructor simple irrelevant trivial needs_implicit
 // CHECK-NEXT:    |   |-CXXRecordDecl {{.*}} <col:3, col:10> col:10{{( imported)?}} implicit struct V
-// CHECK-NEXT:    |   `-CXXMethodDecl {{.*}} <line:17:5, line:20:5> line:17:10{{( imported)?}} f 'void ()'
+// CHECK-NEXT:    |   `-CXXMethodDecl {{.*}} <line:17:5, line:20:5> line:17:10{{( imported)?}} f 'void ()' implicit-inline
 // CHECK-NEXT:    |     `-CompoundStmt {{.*}} <col:14, line:20:5>
 // CHECK-NEXT:    |       |-LambdaExpr {{.*}} <line:18:7, col:15> '(lambda at {{.*}}ast-dump-lambda.cpp:18:7)'
 // CHECK-NEXT:    |       | |-CXXRecordDecl {{.*}} <col:7> col:7{{( imported)?}} implicit{{( <undeserialized declarations>)?}} class definition
 // CHECK-NEXT:    |       | | |-DefinitionData lambda standard_layout trivially_copyable can_const_default_init
 // CHECK-NEXT:    |       | | | |-DefaultConstructor
 // CHECK-NEXT:    |       | | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT:    |       | | | |-MoveConstructor exists simple trivial needs_implicit
 // CHECK-NEXT:    |       | | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param

clang/test/CXX/class/class.friend/p7-cxx20.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 no-modules.cpp -fsyntax-only -ast-dump | \
+// RUN: FileCheck --match-full-lines --check-prefix=CHECK-NM %s
+// RUN: %clang_cc1 -std=c++20 -xc++-user-header header-unit.h -ast-dump | \
+// RUN: FileCheck --match-full-lines --check-prefix=CHECK-HU %s
+// RUN: %clang_cc1 -std=c++20 module.cpp -ast-dump | \
+// RUN: FileCheck --match-full-lines --check-prefix=CHECK-MOD %s
+
+//--- no-modules.cpp
+
+class X {
+  friend void x(){};
+};
+
+// CHECK-NM: `-CXXRecordDecl {{.*}} <no-modules.cpp:2:1, line:4:1> line:2:7 class X definition
+// CHECK-NM:   |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 implicit class X
+// CHECK-NM-NEXT: `-FriendDecl {{.*}} <line:3:3, col:19> col:15
+// CHECK-NM-NEXT: `-FunctionDecl {{.*}} parent {{.*}} <col:3, col:19> col:15 x 'void ()' implicit-inline
+
+//--- header-unit.h
+
+class Y {
+  friend void y(){};
+};
+
+// CHECK-HU: `-CXXRecordDecl {{.*}} <./header-unit.h:2:1, line:4:1> line:2:7 class Y definition
+// CHECK-HU: |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 implicit class Y
+// CHECK-HU-NEXT: `-FriendDecl {{.*}} <line:3:3, col:19> col:15
+// CHECK-HU-NEXT: `-FunctionDecl {{.*}} parent {{.*}} <col:3, col:19> col:15 y 'void ()' implicit-inline
+
+// A textually-included header
+//--- header.h
+
+class A {
+  friend void a(){};
+};
+
+//--- module.cpp
+module;
+#include "header.h"
+
+export module M;
+
+class Z {
+  friend void z(){};
+};
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <./header.h:2:1, line:4:1> line:2:7 in M.<global> hidden class A definition
+// CHECK-MOD: | |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M.<global> hidden implicit class A
+// CHECK-MOD-NEXT: | `-FriendDecl {{.*}} <line:3:3, col:19> col:15 in M.<global>
+// CHECK-MOD-NEXT: |   `-FunctionDecl {{.*}} parent {{.*}} <col:3, col:19> col:15 in M.<global> hidden a 'void ()' implicit-inline
+
+// CHECK-MOD: `-CXXRecordDecl {{.*}} <module.cpp:6:1, line:8:1> line:6:7 in M hidden class Z{{( ReachableWhenImported)?}} definition
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Z{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: `-FriendDecl {{.*}} <line:7:3, col:19> col:15 in M{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: `-FunctionDecl {{.*}} parent {{.*}} <col:3, col:19> col:15 in M hidden z 'void ()'{{( ReachableWhenImported)?}}

clang/test/CXX/class/class.mfct/p1-cxx20.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 no-modules.cpp -fsyntax-only -ast-dump | \
+// RUN: FileCheck --match-full-lines --check-prefix=CHECK-NM %s
+// RUN: %clang_cc1 -std=c++20 -xc++-user-header header-unit.h -ast-dump | \
+// RUN: FileCheck --match-full-lines --check-prefix=CHECK-HU %s
+// RUN: %clang_cc1 -std=c++20 module.cpp -ast-dump | \
+// RUN: FileCheck --match-full-lines --check-prefix=CHECK-MOD %s
+
+//--- no-modules.cpp
+
+class X {
+  void x(){};
+};
+
+// CHECK-NM: `-CXXRecordDecl {{.*}} <no-modules.cpp:2:1, line:4:1> line:2:7 class X definition
+// CHECK-NM:   |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 implicit class X
+// CHECK-NM-NEXT: `-CXXMethodDecl {{.*}} <line:3:3, col:12> col:8 x 'void ()' implicit-inline
+
+// A header unit header
+//--- header-unit.h
+
+class Y {
+  void y(){};
+};
+
+// CHECK-HU: `-CXXRecordDecl {{.*}} <./header-unit.h:2:1, line:4:1> line:2:7 class Y definition
+// CHECK-HU: |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 implicit class Y
+// CHECK-HU-NEXT: `-CXXMethodDecl {{.*}} <line:3:3, col:12> col:8 y 'void ()' implicit-inline
+
+// A textually-included header
+//--- header.h
+
+class A {
+  void a(){};
+};
+
+//--- module.cpp
+module;
+#include "header.h"
+
+export module M;
+
+class Z {
+  void z(){};
+};
+
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <./header.h:2:1, line:4:1> line:2:7 in M.<global> hidden class A definition
+// CHECK-MOD: | |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M.<global> hidden implicit class A
+// CHECK-MOD-NEXT: | `-CXXMethodDecl {{.*}} <line:3:3, col:12> col:8 in M.<global> hidden a 'void ()' implicit-inline
+
+// CHECK-MOD: `-CXXRecordDecl {{.*}} <module.cpp:6:1, line:8:1> line:6:7 in M hidden class Z{{( ReachableWhenImported)?}} definition
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Z{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: `-CXXMethodDecl {{.*}} <line:7:3, col:12> col:8 in M hidden z 'void ()'{{( ReachableWhenImported)?}}