From: Richard Smith Date: Wed, 5 Jul 2017 07:47:11 +0000 (+0000) Subject: [modules ts] Improve merging of module-private declarations. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=c5dd58546ce4d20cd71cc26cb790e7f91c8f908f;p=clang [modules ts] Improve merging of module-private declarations. These cases occur frequently for declarations in the global module (above the module-declaration) in a Modules TS module interface. When we merge a definition from another module into such a module-private definition, ensure that we transitively make everything lexically within that definition visible to that translation unit. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@307129 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/DeclBase.h b/include/clang/AST/DeclBase.h index 8658d886a9..041f0fd484 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 (getModuleOwnershipKind() == ModuleOwnershipKind::VisibleWhenImported) + if (isHidden()) setModuleOwnershipKind(ModuleOwnershipKind::Visible); } diff --git a/lib/AST/DeclBase.cpp b/lib/AST/DeclBase.cpp index 842ae9b6cc..cd2c83a02f 100644 --- a/lib/AST/DeclBase.cpp +++ b/lib/AST/DeclBase.cpp @@ -283,8 +283,10 @@ void Decl::setLexicalDeclContext(DeclContext *DC) { setLocalOwningModule(cast(DC)->getOwningModule()); } - assert((!hasOwningModule() || getOwningModule() || isModulePrivate()) && - "hidden declaration has no owning module"); + assert( + (getModuleOwnershipKind() != ModuleOwnershipKind::VisibleWhenImported || + getOwningModule()) && + "hidden declaration has no owning module"); } void Decl::setDeclContextsImpl(DeclContext *SemaDC, DeclContext *LexicalDC, diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp index 63b3f6a1cb..85596ed52e 100644 --- a/lib/Sema/SemaLookup.cpp +++ b/lib/Sema/SemaLookup.cpp @@ -1396,6 +1396,13 @@ bool Sema::hasVisibleMergedDefinition(NamedDecl *Def) { } bool Sema::hasMergedDefinitionInCurrentModule(NamedDecl *Def) { + // FIXME: When not in local visibility mode, we can't tell the difference + // between a declaration being visible because we merged a local copy of + // the same declaration into it, and it being visible because its owning + // module is visible. + if (Def->getModuleOwnershipKind() == Decl::ModuleOwnershipKind::Visible && + getLangOpts().ModulesLocalVisibility) + return true; for (Module *Merged : Context.getModulesWithMergedDefinition(Def)) if (Merged->getTopLevelModuleName() == getLangOpts().CurrentModule) return true; @@ -1509,25 +1516,33 @@ bool LookupResult::isVisibleSlow(Sema &SemaRef, NamedDecl *D) { // 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 (!D->isFromASTFile() || SemaRef.hasMergedDefinitionInCurrentModule(D)) + return true; + } else { + // 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.) + if (D->isModulePrivate() + ? DeclModule->getTopLevelModuleName() == + SemaRef.getLangOpts().CurrentModule || + SemaRef.hasMergedDefinitionInCurrentModule(D) + : SemaRef.isModuleVisible(DeclModule) || + SemaRef.hasVisibleMergedDefinition(D)) + return true; } - // 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.) - if (D->isModulePrivate() - ? DeclModule->getTopLevelModuleName() == - SemaRef.getLangOpts().CurrentModule || - SemaRef.hasMergedDefinitionInCurrentModule(D) - : SemaRef.isModuleVisible(DeclModule) || - SemaRef.hasVisibleMergedDefinition(D)) - return true; + // Determine whether a decl context is a file context for the purpose of + // visibility. This looks through some (export and linkage spec) transparent + // contexts, but not others (enums). + auto IsEffectivelyFileContext = [](const DeclContext *DC) { + return DC->isFileContext() || isa(DC) || + isa(DC); + }; - // If this declaration is not at namespace scope nor module-private, + // If this declaration is not at namespace scope // then it is visible if its lexical parent has a visible definition. DeclContext *DC = D->getLexicalDeclContext(); - if (!D->isModulePrivate() && DC && !DC->isFileContext() && - !isa(DC) && !isa(DC)) { + if (DC && !IsEffectivelyFileContext(DC)) { // For a parameter, check whether our current template declaration's // lexical context is visible, not whether there's some other visible // definition of it, because parameters aren't "within" the definition. @@ -1535,32 +1550,45 @@ bool LookupResult::isVisibleSlow(Sema &SemaRef, NamedDecl *D) { // In C++ we need to check for a visible definition due to ODR merging, // and in C we must not because each declaration of a function gets its own // set of declarations for tags in prototype scope. - if ((D->isTemplateParameter() || isa(D) - || (isa(DC) && !SemaRef.getLangOpts().CPlusPlus)) - ? isVisible(SemaRef, cast(DC)) - : SemaRef.hasVisibleDefinition(cast(DC))) { - if (SemaRef.CodeSynthesisContexts.empty() && - // FIXME: Do something better in this case. - !SemaRef.getLangOpts().ModulesLocalVisibility) { - // Cache the fact that this declaration is implicitly visible because - // its parent has a visible definition. - D->setVisibleDespiteOwningModule(); - } - return true; + bool VisibleWithinParent; + if (D->isTemplateParameter() || isa(D) || + (isa(DC) && !SemaRef.getLangOpts().CPlusPlus)) + VisibleWithinParent = isVisible(SemaRef, cast(DC)); + else if (D->isModulePrivate()) { + // A module-private declaration is only visible if an enclosing lexical + // parent was merged with another definition in the current module. + VisibleWithinParent = false; + do { + if (SemaRef.hasMergedDefinitionInCurrentModule(cast(DC))) { + VisibleWithinParent = true; + break; + } + DC = DC->getLexicalParent(); + } while (!IsEffectivelyFileContext(DC)); + } else { + VisibleWithinParent = SemaRef.hasVisibleDefinition(cast(DC)); } - return false; + + if (VisibleWithinParent && SemaRef.CodeSynthesisContexts.empty() && + // FIXME: Do something better in this case. + !SemaRef.getLangOpts().ModulesLocalVisibility) { + // Cache the fact that this declaration is implicitly visible because + // its parent has a visible definition. + D->setVisibleDespiteOwningModule(); + } + return VisibleWithinParent; } + // FIXME: All uses of DeclModule below this point should also check merged + // modules. + if (!DeclModule) + return false; + // Find the extra places where we need to look. llvm::DenseSet &LookupModules = SemaRef.getLookupModules(); if (LookupModules.empty()) return false; - if (!DeclModule) { - DeclModule = SemaRef.getOwningModule(D); - assert(DeclModule && "hidden decl not from a module"); - } - // If our lookup set contains the decl's module, it's visible. if (LookupModules.count(DeclModule)) return true; diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index 4d9ddd2ff5..abed258656 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -573,6 +573,8 @@ void ASTDeclReader::VisitDecl(Decl *D) { else Reader.HiddenNamesMap[Owner].push_back(D); } + } else if (ModulePrivate) { + D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate); } } diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp index c6129d326c..f7a49e4100 100644 --- a/lib/Serialization/ASTWriter.cpp +++ b/lib/Serialization/ASTWriter.cpp @@ -1462,7 +1462,7 @@ void ASTWriter::WriteControlBlock(Preprocessor &PP, ASTContext &Context, } // Module map file - if (WritingModule) { + if (WritingModule && WritingModule->Kind == Module::ModuleMapModule) { Record.clear(); auto &Map = PP.getHeaderSearchInfo().getModuleMap(); diff --git a/test/CXX/modules-ts/dcl.dcl/dcl.module/p5.cpp b/test/CXX/modules-ts/dcl.dcl/dcl.module/p5.cpp new file mode 100644 index 0000000000..734b89173d --- /dev/null +++ b/test/CXX/modules-ts/dcl.dcl/dcl.module/p5.cpp @@ -0,0 +1,33 @@ +// RUN: rm -f %t +// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %s -o %t -DINTERFACE +// RUN: %clang_cc1 -std=c++1z -fmodules-ts -fmodule-file=%t %s -verify -DIMPLEMENTATION +// RUN: %clang_cc1 -std=c++1z -fmodules-ts -fmodule-file=%t %s -verify -DEARLY_IMPLEMENTATION +// RUN: %clang_cc1 -std=c++1z -fmodules-ts -fmodule-file=%t %s -verify -DUSER + +// expected-no-diagnostics + +#ifdef USER +import Foo; +#endif + +#ifdef EARLY_IMPLEMENTATION +module Foo; +#endif + +template struct type_template { + typedef T type; + void f(type); +}; + +template void type_template::f(type) {} + +template class = type_template> +struct default_template_args {}; + +#ifdef INTERFACE +export module Foo; +#endif + +#ifdef IMPLEMENTATION +module Foo; +#endif