[C++20] [Module] Fix bug47116 and implement [module.interface]/p6

[ChuanqiXu]

Summary

This fixes https://bugs.llvm.org/show_bug.cgi?id=47116.

According to https://eel.is/c++draft/module.interface#2, it is meaningless to export an entity which is not in namespace scope.
The reason why the compiler crashes is that the compiler missed ExportDecl when the compiler traverse the subclass of DeclContext. So here is the crash.

Also, this patch implements https://eel.is/c++draft/module.interface#6 in Sema::CheckRedeclaration* functions.

Test Plan: check-all

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7784–7785

I think this diagnostic text is more clear based on the standards text you cited. This would also come with a note diagnostic to point to the previous declaration.

clang/lib/Sema/SemaDecl.cpp

5748–5750

I don't believe this is sufficient to cover [module.interface]p6. I tried out the example from the paragraph in the standard and we still silently accept it.

[ChuanqiXu]

clang/lib/Sema/SemaDecl.cpp

5748–5750

Yes, this patch is not intended to cover [module.interface]p6. The intention is to fix the crash since I feel it would take a longer time to support [module.interface]p6. And crash is not good any way. I plan to support [module.interface]p6 in successive patches. Do you happy with this?
BTW, I have a plan to support clang's c++20 module to a workable state. And I am working on the reachable definition in https://eel.is/c++draft/module.reach#3 and https://eel.is/c++draft/module.global.frag#3.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7784–7785

Given that this is not intended for p6 specifically, I think my suggestion is incorrect. But I am also not certain your diagnostic is either, but it's really hard to tell given where our current support is for modules. All of the other compilers suggest that an unqualified id is expected to be found, and I tend to agree -- there's no declaration there *to* export, just the type specifier for a declaration. This makes me think the issue is elsewhere and perhaps we shouldn't even be getting into diagnoseQualifiedDeclaration().

clang/lib/Sema/SemaDecl.cpp

5748–5750

Thank you for the clarification! Fixing the crash is definitely a step in the right direction.

[ChuanqiXu]

Add more tests.

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7784–7785

I think we need to touch diagnoseQualifiedDeclaration() after all. Since there is a potential risk of crashing in it. If we didn't fix it in diagnoseQualifiedDeclaration() and find other place to fix the issue, I think it may crash potentially one day in the process of developing or we might ignore some paths to diagnoseQualifiedDeclaration()`. It would be a disaster.
And you said "there's no declaration there *to* export". And I noticed that there is error/warning in Sema::ParsedFreeStandingDeclSpec which would emit this kind of error/warning. But as the title describes, it only works for free standing declaration, which is not the same with the issue in bug47715. And diagnoseQualifiedDeclaration() would handle the qualified redeclaration. So it looks a good place to me.
BTW, I found the current patch could handle [module.interface]/p6 partially for qualified redeclaration surprisingly. See the newly added test case for example.
Finally, given that we need to touch diagnoseQualifiedDeclaration() to fix the crash, I think the patch should be good and we could try to cover [module.interface]/p6 in successive patches. Do you think so?

clang/lib/Sema/SemaDecl.cpp

5748–5750

BYW, I added a new issue for the example in [module.interface]p6 here: https://bugs.llvm.org/show_bug.cgi?id=52395.

5748–5750

s/BYW/BTW

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7784–7785

I think we need to touch diagnoseQualifiedDeclaration() after all. Since there is a potential risk of crashing in it.

My point is that we may have wanted to reject this code before ever needing to call diagnoseQualifiedDeclaration() in the first place. aka, it might be just as valid to assert we never see an ExportDecl here because the caller should have already handled that case.

And you said "there's no declaration there *to* export". And I noticed that there is error/warning in Sema::ParsedFreeStandingDeclSpec which would emit this kind of error/warning. But as the title describes, it only works for free standing declaration, which is not the same with the issue in bug47715.

The declaration is invalid in C++20 because it does not declare anything (this is the same example with the export keywords removed): https://godbolt.org/z/8zc9q7fno

Clang fails the first one because we don't yet implement P0634R3, but the presence of the export keyword should not change diagnostic behavior here.

[urnathan]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7784–7785

Agreed, 'export' is only applicable to namespace-scope declarations. We should reject it applying to non-namepace-scope entities.

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7784–7785

Got it. It is important to make the code semantics consistent with the spec.

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7784–7785

@aaron.ballman @urnathan I have sent a draft to reject the codes before diagnoseQualifiedDeclaration(). The draft is here: https://reviews.llvm.org/D113239. The draft is not completed to review the details. The key idea is to insert checks for ExportDecl on the fly to construct the AST. Due to the complexity of semantics analysis , we might need to insert the checks many times. It may not be too terrible since I found many Check* functions in Sema. So may be this is what we used to do.

And the draft could reject the example in https://eel.is/c++draft/module.interface#6. It looks like not so hard as I imaged.

BTW, I have tried to let the compiler to emit "declaration does not declare anything" when it sees "export template <typename T> X<T>::Y". But I failed. It is not easy for me. And I found that the compiler wouldn't emit "declaration does not declare anything" when the declaration is in a DeclContext. Here is an example: https://godbolt.org/z/3j63PnTc7. So the compiler used to handle this case in diagnoseQualifiedDeclaration(). Personally, it looks not bad to handle export template<> X::Y in diagnoseQualifiedDeclaration() at least from the names. And I am wondering if we could export a non-namespace-scope name without using qualifiers? If no, it looks better for me to handle this in diagnoseQualifiedDeclaration(). BTW, the following example is not what I meant:

struct X { struct Y {}; };
export using Z = typename X::Y;

Finally, it looks like we need to touch diagnoseQualifiedDeclaration() after all. Otherwise, it would crash for that example.

[ChuanqiXu]

Updated diagnostic messages. It looks good to me to reject for exporting non-namespace-scope names in diagnoseQualifiedDeclaration().

[ChuanqiXu]

@aaron.ballman @urnathan ping~

[urnathan]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7784–7785

I think the wording is awkward. 'here' carries the implication there is a somewhere where it would be ok, but there isn't.
'cannot export %0 as it is not at namespace scope'

clang/test/CXX/module/module.interface/p6.cpp

47

um, is that really invalid? it looks to me that one's reexporting an already exported namespace entity. It's perfectly valid to write::
namespace A{ export struct X;}
namespace A { export struct X;}
so I don't see why 'export struct A::X;' is invalid. (I realize this is not a change you;ve made).

also applies to foo.

48

Can we note (a) as the original declaration was not exported, and note where it is?
'cannot export redeclaration %0 as it is not exported'
'note: declared here'

also applies to bar

[ChuanqiXu]

• Address comments.
• Implement [module.interface]/p6.

In the process of addressing @urnathan 's comment, I found that it is better to handle [module.interface]/p6 in Sema::CheckRedeclaration* functions. And I add more tests about p6 now. Now it looks much better now.

[ChuanqiXu]

clang/test/CXX/module/module.interface/p2-2.cpp

1 ↗

(On Diff #386728)

I move them here since this is described in p2 instead of p6. I don't merge this into p2.cpp since that I feel it is not harmonious to merge this test with the test in p2 now.

[ChuanqiXu]

Remove dead codes.

[ChuanqiXu]

Fix a mismatch in test

[ChuanqiXu]

@aaron.ballman @urnathan gentle ping~

[ChuanqiXu]

Rebased

[ChuanqiXu]

@rsmith @aaron.ballman @urnathan gentle ping~

[aaron.ballman]

clang/include/clang/AST/DeclBase.h

616 ↗

(On Diff #393030)

I think it would be good to add some comments to document the function (as done with surrounding code).

clang/include/clang/Basic/DiagnosticSemaKinds.td

7784–7785

7786–7787

Both of these tweak the formatting somewhat, but the important thing is dropping the trailing punctuation from the diagnostic text.

clang/lib/AST/DeclBase.cpp

999 ↗

(On Diff #393030)

clang/lib/Sema/SemaDecl.cpp

1622

1643–1645

5750

[ChuanqiXu]

Address comments

[ChuanqiXu]

clang/lib/Sema/SemaDecl.cpp

1622

I didn't address this since it belongs to C++20 instead of C++2b.

[ChuanqiXu]

@aaron.ballman @urnathan gentle ping~

[ChuanqiXu]

@aaron.ballman @urnathan gentle ping~

[ChuanqiXu]

@aaron.ballman @urnathan gentle ping~

[urnathan]

LGTM, thanks for working on this

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_typecheck_deleted_function : Error<
   "conversion function %diff{from $ to $|between types}0,1 "
   "invokes a deleted function">;
 
 def err_expected_class_or_namespace : Error<"%0 is not a class"
   "%select{ or namespace|, namespace, or enumeration}1">;
 def err_invalid_declarator_scope : Error<"cannot define or redeclare %0 here "
   "because namespace %1 does not enclose namespace %2">;
+def err_export_non_namespace_scope_name : Error<"cannot export %0 here since "
+  "only the entities in namespace scope could be exported.">;
+def err_export_qualified_redeclaration : Error<"cannot export redeclaration "
+  "%0 here.">;
 def err_invalid_declarator_global_scope : Error<
   "definition or redeclaration of %0 cannot name the global scope">;
 def err_invalid_declarator_in_function : Error<
   "definition or redeclaration of %0 not allowed inside a function">;
 def err_invalid_declarator_in_block : Error<
   "definition or redeclaration of %0 not allowed inside a block">;
 def err_not_tag_in_scope : Error<
   "no %select{struct|interface|union|class|enum}0 named %1 in %2">;

clang/lib/Sema/SemaDecl.cpp

if (NewIsModuleInterface || OldIsModuleInterface) {
     Diag(Old->getLocation(), diag::note_previous_declaration);
     New->setInvalidDecl();
     return true;
   }
 
   return false;
 }
 
 static bool isUsingDecl(NamedDecl *D) {
   return isa<UsingShadowDecl>(D) ||
          isa<UnresolvedUsingTypenameDecl>(D) ||
          isa<UnresolvedUsingValueDecl>(D);
 }
 
 /// Removes using shadow declarations from the lookup results.
 static void RemoveUsingDecls(LookupResult &R) {
   LookupResult::Filter F = R.makeFilter();
   while (F.hasNext())
     if (isUsingDecl(F.next()))
       F.erase();
 
   F.done();
 }
 
 /// Check for this common pattern:
 /// @code
 /// class S {
 ///   S(const S&); // DO NOT IMPLEMENT
 ///   void operator=(const S&); // DO NOT IMPLEMENT
 /// };
 /// @endcode
 static bool IsDisallowedCopyOrAssign(const CXXMethodDecl *D) {
   // FIXME: Should check for private access too but access is set after we get
   // the decl here.
   if (D->doesThisDeclarationHaveABody())
     return false;
 
   if (const CXXConstructorDecl *CD = dyn_cast<CXXConstructorDecl>(D))
     return CD->isCopyConstructor();
   return D->isCopyAssignmentOperator();
else if (isa<TranslationUnitDecl>(DC))
       Diag(Loc, diag::err_invalid_declarator_global_scope)
         << Name << SS.getRange();
     else if (isa<FunctionDecl>(Cur))
       Diag(Loc, diag::err_invalid_declarator_in_function)
         << Name << SS.getRange();
     else if (isa<BlockDecl>(Cur))
       Diag(Loc, diag::err_invalid_declarator_in_block)
         << Name << SS.getRange();
+    else if (isa<ExportDecl>(Cur)) {
+      if (!isa<NamespaceDecl>(DC))
+        Diag(Loc, diag::err_export_non_namespace_scope_name) << Name;
       else
+        Diag(Loc, diag::err_export_qualified_redeclaration) << Name;
+    } else
       Diag(Loc, diag::err_invalid_declarator_scope)
       << Name << cast<NamedDecl>(Cur) << cast<NamedDecl>(DC) << SS.getRange();
 
     return true;
   }
 
   if (Cur->isRecord()) {
     // Cannot qualify members within a class.

clang/test/CXX/module/module.interface/p6.cpp

+// RUN: %clang_cc1 -std=c++20 %s -verify
+
+export module X;
+
+export template <typename T>
+struct X {
+  struct iterator {
+    T node;
+  };
+  void foo() {}
+  template <typename U>
+  U bar();
+};
+
+export template <typename T> X<T>::iterator;                      // expected-error {{cannot export 'iterator' here since only the entities in namespace scope could be exported.}}
+export template <typename T> void X<T>::foo();                    // expected-error {{cannot export 'foo' here since only the entities in namespace scope could be exported.}}
+export template <typename T> template <typename U> U X<T>::bar(); // expected-error {{cannot export 'bar' here since only the entities in namespace scope could be exported.}}
+
+export struct Y {
+  struct iterator {
+    int node;
+  };
+  void foo() {}
+  template <typename U>
+  U bar();
+};
+
+export struct Y::iterator;               // expected-error {{cannot export 'iterator' here since only the entities in namespace scope could be exported.}}
+export void Y::foo();                    // expected-error {{cannot export 'foo' here since only the entities in namespace scope could be exported.}}
+export template <typename U> U Y::bar(); // expected-error {{cannot export 'bar' here since only the entities in namespace scope could be exported.}}
+
+export {
+  template <typename T> X<T>::iterator; // expected-error {{cannot export 'iterator' here since only the entities in namespace scope could be exported.}}
+  struct Y::iterator;                   // expected-error {{cannot export 'iterator' here since only the entities in namespace scope could be exported.}}
+}
+
+namespace A {
+export struct X;
+struct Y;
+export void foo();
+void bar();
+} // namespace A
+
+// [module.interface]/p6
+// A redeclaration of an entity X is implicitly exported if X was introduced by an exported declaration
+void A::foo();
+export struct A::X;   // expected-error {{cannot export redeclaration 'X' here.}}
+export struct A::Y;   // expected-error {{cannot export redeclaration 'Y' here.}}
+export void A::foo(); // expected-error {{cannot export redeclaration 'foo' here.}}
+// The compiler couldn't export A::bar() here since A::bar() is declared above without exported.
+// See [module.interface]/p6 for details.
+export void A::bar(); // expected-error {{cannot export redeclaration 'bar' here.}}