From 16280f9d13d776349f9dacd3378e307d662f4769 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 5 Jul 2017 01:42:07 +0000 Subject: [PATCH] [modules ts] Declarations from a module interface unit are only visible outside the module if declared in an export block. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@307115 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/DeclBase.h | 2 +- include/clang/Sema/Sema.h | 2 ++ lib/AST/DeclBase.cpp | 2 +- lib/Parse/Parser.cpp | 2 ++ lib/Sema/Sema.cpp | 12 +++++++ lib/Sema/SemaDecl.cpp | 35 ++++++++++++------- lib/Sema/SemaLookup.cpp | 28 +++++++++++---- .../modules-ts/basic/basic.link/p2/module.cpp | 17 +++++++++ .../basic/basic.link/p2/module.cppm | 29 +++++++++++++++ .../modules-ts/basic/basic.link/p2/other.cpp | 16 +++++++++ .../dcl.module/dcl.module.import/p1.cpp | 4 +-- test/SemaCXX/modules-ts.cppm | 8 ++++- 12 files changed, 134 insertions(+), 23 deletions(-) create mode 100644 test/CXX/modules-ts/basic/basic.link/p2/module.cpp create mode 100644 test/CXX/modules-ts/basic/basic.link/p2/module.cppm create mode 100644 test/CXX/modules-ts/basic/basic.link/p2/other.cpp diff --git a/include/clang/AST/DeclBase.h b/include/clang/AST/DeclBase.h index 0f1f481ae4..8658d886a9 100644 --- a/include/clang/AST/DeclBase.h +++ b/include/clang/AST/DeclBase.h @@ -749,7 +749,7 @@ public: /// Set that this declaration is globally visible, even if it came from a /// module that is not visible. void setVisibleDespiteOwningModule() { - if (hasOwningModule()) + if (getModuleOwnershipKind() == ModuleOwnershipKind::VisibleWhenImported) setModuleOwnershipKind(ModuleOwnershipKind::Visible); } diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 1fc0b2e502..da961ab7e8 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -1266,6 +1266,7 @@ public: void emitAndClearUnusedLocalTypedefWarnings(); + void ActOnStartOfTranslationUnit(); void ActOnEndOfTranslationUnit(); void CheckDelegatingCtorCycles(); @@ -1541,6 +1542,7 @@ public: llvm::SmallVectorImpl *Modules); bool hasVisibleMergedDefinition(NamedDecl *Def); + bool hasMergedDefinitionInCurrentModule(NamedDecl *Def); /// Determine if \p D and \p Suggested have a structurally compatible /// layout as described in C11 6.2.7/1. diff --git a/lib/AST/DeclBase.cpp b/lib/AST/DeclBase.cpp index a0594a0203..842ae9b6cc 100644 --- a/lib/AST/DeclBase.cpp +++ b/lib/AST/DeclBase.cpp @@ -283,7 +283,7 @@ void Decl::setLexicalDeclContext(DeclContext *DC) { setLocalOwningModule(cast(DC)->getOwningModule()); } - assert((!hasOwningModule() || getOwningModule()) && + assert((!hasOwningModule() || getOwningModule() || isModulePrivate()) && "hidden declaration has no owning module"); } diff --git a/lib/Parse/Parser.cpp b/lib/Parse/Parser.cpp index af29b5e9c6..1ed7ef9663 100644 --- a/lib/Parse/Parser.cpp +++ b/lib/Parse/Parser.cpp @@ -526,6 +526,8 @@ void Parser::LateTemplateParserCleanupCallback(void *P) { } bool Parser::ParseFirstTopLevelDecl(DeclGroupPtrTy &Result) { + Actions.ActOnStartOfTranslationUnit(); + // C11 6.9p1 says translation units must have at least one top-level // declaration. C++ doesn't have this restriction. We also don't want to // complain if we have a precompiled header, although technically if the PCH diff --git a/lib/Sema/Sema.cpp b/lib/Sema/Sema.cpp index 34f5e26be8..dc9f977d41 100644 --- a/lib/Sema/Sema.cpp +++ b/lib/Sema/Sema.cpp @@ -705,6 +705,18 @@ void Sema::emitAndClearUnusedLocalTypedefWarnings() { UnusedLocalTypedefNameCandidates.clear(); } +/// This is called before the very first declaration in the translation unit +/// is parsed. Note that the ASTContext may have already injected some +/// declarations. +void Sema::ActOnStartOfTranslationUnit() { + if (getLangOpts().ModulesTS) { + // We start in the global module; all those declarations are implicitly + // module-private (though they do not have module linkage). + Context.getTranslationUnitDecl()->setModuleOwnershipKind( + Decl::ModuleOwnershipKind::ModulePrivate); + } +} + /// ActOnEndOfTranslationUnit - This is called at the very end of the /// translation unit when EOF is reached and all but the top-level scope is /// popped. diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index ab2661cd71..31b24f91c1 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -16054,8 +16054,6 @@ Sema::DeclGroupPtrTy Sema::ActOnModuleDecl(SourceLocation StartLoc, return nullptr; } - // FIXME: Create a ModuleDecl and return it. - // FIXME: Most of this work should be done by the preprocessor rather than // here, in order to support macro import. @@ -16069,6 +16067,8 @@ Sema::DeclGroupPtrTy Sema::ActOnModuleDecl(SourceLocation StartLoc, ModuleName += Piece.first->getName(); } + // FIXME: If we've already seen a module-declaration, report an error. + // If a module name was explicitly specified on the command line, it must be // correct. if (!getLangOpts().CurrentModule.empty() && @@ -16081,6 +16081,7 @@ Sema::DeclGroupPtrTy Sema::ActOnModuleDecl(SourceLocation StartLoc, const_cast(getLangOpts()).CurrentModule = ModuleName; auto &Map = PP.getHeaderSearchInfo().getModuleMap(); + Module *Mod; switch (MDK) { case ModuleDeclKind::Module: { @@ -16099,12 +16100,9 @@ Sema::DeclGroupPtrTy Sema::ActOnModuleDecl(SourceLocation StartLoc, } // Create a Module for the module that we're defining. - Module *Mod = Map.createModuleForInterfaceUnit(ModuleLoc, ModuleName); + Mod = Map.createModuleForInterfaceUnit(ModuleLoc, ModuleName); assert(Mod && "module creation should not fail"); - - // Enter the semantic scope of the module. - ActOnModuleBegin(ModuleLoc, Mod); - return nullptr; + break; } case ModuleDeclKind::Partition: @@ -16114,14 +16112,26 @@ Sema::DeclGroupPtrTy Sema::ActOnModuleDecl(SourceLocation StartLoc, case ModuleDeclKind::Implementation: std::pair ModuleNameLoc( PP.getIdentifierInfo(ModuleName), Path[0].second); - - DeclResult Import = ActOnModuleImport(ModuleLoc, ModuleLoc, ModuleNameLoc); - if (Import.isInvalid()) + Mod = getModuleLoader().loadModule(ModuleLoc, Path, Module::AllVisible, + /*IsIncludeDirective=*/false); + if (!Mod) return nullptr; - return ConvertDeclToDeclGroup(Import.get()); + break; } - llvm_unreachable("unexpected module decl kind"); + // Enter the semantic scope of the module. + ModuleScopes.push_back({}); + ModuleScopes.back().Module = Mod; + ModuleScopes.back().OuterVisibleModules = std::move(VisibleModules); + VisibleModules.setVisible(Mod, ModuleLoc); + + // From now on, we have an owning module for all declarations we see. + // However, those declarations are module-private unless explicitly + // exported. + Context.getTranslationUnitDecl()->setLocalOwningModule(Mod); + + // FIXME: Create a ModuleDecl. + return nullptr; } DeclResult Sema::ActOnModuleImport(SourceLocation StartLoc, @@ -16310,6 +16320,7 @@ Decl *Sema::ActOnStartExportDecl(Scope *S, SourceLocation ExportLoc, CurContext->addDecl(D); PushDeclContext(S, D); + D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::VisibleWhenImported); return D; } diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp index 2e7fb875a2..63b3f6a1cb 100644 --- a/lib/Sema/SemaLookup.cpp +++ b/lib/Sema/SemaLookup.cpp @@ -1395,6 +1395,13 @@ bool Sema::hasVisibleMergedDefinition(NamedDecl *Def) { return false; } +bool Sema::hasMergedDefinitionInCurrentModule(NamedDecl *Def) { + for (Module *Merged : Context.getModulesWithMergedDefinition(Def)) + if (Merged->getTopLevelModuleName() == getLangOpts().CurrentModule) + return true; + return false; +} + template static bool hasVisibleDefaultArgument(Sema &S, const ParmDecl *D, @@ -1495,16 +1502,25 @@ bool LookupResult::isVisibleSlow(Sema &SemaRef, NamedDecl *D) { assert(D->isHidden() && "should not call this: not in slow case"); Module *DeclModule = SemaRef.getOwningModule(D); - assert(DeclModule && "hidden decl not from a module"); + if (!DeclModule) { + // A module-private declaration with no owning module means this is in the + // global module in the C++ Modules TS. This is visible within the same + // translation unit only. + // FIXME: Don't assume that "same translation unit" means the same thing + // as "not from an AST file". + assert(D->isModulePrivate() && "hidden decl has no module"); + return !D->isFromASTFile(); + } // If the owning module is visible, and the decl is not module private, // then the decl is visible too. (Module private is ignored within the same // top-level module.) - // FIXME: Check the owning module for module-private declarations rather than - // assuming "same AST file" is the same thing as "same module". - if ((!D->isFromASTFile() || !D->isModulePrivate()) && - (SemaRef.isModuleVisible(DeclModule) || - SemaRef.hasVisibleMergedDefinition(D))) + if (D->isModulePrivate() + ? DeclModule->getTopLevelModuleName() == + SemaRef.getLangOpts().CurrentModule || + SemaRef.hasMergedDefinitionInCurrentModule(D) + : SemaRef.isModuleVisible(DeclModule) || + SemaRef.hasVisibleMergedDefinition(D)) return true; // If this declaration is not at namespace scope nor module-private, diff --git a/test/CXX/modules-ts/basic/basic.link/p2/module.cpp b/test/CXX/modules-ts/basic/basic.link/p2/module.cpp new file mode 100644 index 0000000000..3fc6044d8e --- /dev/null +++ b/test/CXX/modules-ts/basic/basic.link/p2/module.cpp @@ -0,0 +1,17 @@ +// RUN: %clang_cc1 -std=c++1z -fmodules-ts %S/module.cppm -emit-module-interface -o %t +// RUN: %clang_cc1 -std=c++1z -fmodules-ts -fmodule-file=%t %s -verify +// expected-no-diagnostics +module M; + +// FIXME: Use of internal linkage entities should be rejected. +void use_from_module_impl() { + external_linkage_fn(); + module_linkage_fn(); + internal_linkage_fn(); + (void)external_linkage_class{}; + (void)module_linkage_class{}; + (void)internal_linkage_class{}; + (void)external_linkage_var; + (void)module_linkage_var; + (void)internal_linkage_var; +} diff --git a/test/CXX/modules-ts/basic/basic.link/p2/module.cppm b/test/CXX/modules-ts/basic/basic.link/p2/module.cppm new file mode 100644 index 0000000000..bb261700db --- /dev/null +++ b/test/CXX/modules-ts/basic/basic.link/p2/module.cppm @@ -0,0 +1,29 @@ +// RUN: %clang_cc1 -std=c++1z -fmodules-ts %s -verify +// expected-no-diagnostics +export module M; + +export int external_linkage_var; +int module_linkage_var; +static int internal_linkage_var; + +export void external_linkage_fn() {} +void module_linkage_fn() {} +static void internal_linkage_fn() {} + +export struct external_linkage_class {}; +struct module_linkage_class {}; +namespace { + struct internal_linkage_class {}; +} + +void use() { + external_linkage_fn(); + module_linkage_fn(); + internal_linkage_fn(); + (void)external_linkage_class{}; + (void)module_linkage_class{}; + (void)internal_linkage_class{}; + (void)external_linkage_var; + (void)module_linkage_var; + (void)internal_linkage_var; +} diff --git a/test/CXX/modules-ts/basic/basic.link/p2/other.cpp b/test/CXX/modules-ts/basic/basic.link/p2/other.cpp new file mode 100644 index 0000000000..8370777e7e --- /dev/null +++ b/test/CXX/modules-ts/basic/basic.link/p2/other.cpp @@ -0,0 +1,16 @@ +// RUN: %clang_cc1 -std=c++1z -fmodules-ts %S/module.cppm -emit-module-interface -o %t +// RUN: %clang_cc1 -std=c++1z -fmodules-ts -fmodule-file=%t %s -verify +import M; + +void use_from_module_impl() { + external_linkage_fn(); + module_linkage_fn(); // expected-error {{undeclared identifier}} + internal_linkage_fn(); // expected-error {{undeclared identifier}} + (void)external_linkage_class{}; + (void)module_linkage_class{}; // expected-error {{undeclared identifier}} expected-error 0+{{}} + (void)internal_linkage_class{}; // expected-error {{undeclared identifier}} expected-error 0+{{}} + // expected-note@module.cppm:9 {{here}} + (void)external_linkage_var; + (void)module_linkage_var; // expected-error {{undeclared identifier}} + (void)internal_linkage_var; // expected-error {{undeclared identifier}} +} diff --git a/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp b/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp index aaf43d6584..aad31b4b46 100644 --- a/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp +++ b/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.import/p1.cpp @@ -1,7 +1,7 @@ // RUN: rm -rf %t // RUN: mkdir -p %t -// RUN: echo 'export module x; int a, b;' > %t/x.cppm -// RUN: echo 'export module x.y; int c;' > %t/x.y.cppm +// RUN: echo 'export module x; export int a, b;' > %t/x.cppm +// RUN: echo 'export module x.y; export int c;' > %t/x.y.cppm // // RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/x.cppm -o %t/x.pcm // RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=%t/x.pcm %t/x.y.cppm -o %t/x.y.pcm diff --git a/test/SemaCXX/modules-ts.cppm b/test/SemaCXX/modules-ts.cppm index 29122ec7da..2ea4958bff 100644 --- a/test/SemaCXX/modules-ts.cppm +++ b/test/SemaCXX/modules-ts.cppm @@ -13,7 +13,13 @@ export module foo; // expected-note@modules-ts.cppm:* {{loaded from}} #endif -static int m; // ok, internal linkage, so no redefinition error +static int m; +#if TEST == 2 // FIXME: 'm' has internal linkage, so there should be no error here +// expected-error@-2 {{redefinition of '}} +// expected-note@-3 {{unguarded header; consider using #ifdef guards or #pragma once}} +// FIXME: We should drop the "header from" in this diagnostic. +// expected-note-re@modules-ts.cppm:1 {{'{{.*}}modules-ts.cppm' included multiple times, additional include site in header from module 'foo'}} +#endif int n; #if TEST >= 2 // expected-error@-2 {{redefinition of '}} -- 2.40.0