From a6dd8f13d65a5eec292974d1fdd03965c3fffbe3 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Mon, 15 Jun 2015 20:15:48 +0000 Subject: [PATCH] [modules] Better support for redefinitions of an entity from the same module. Support this across module save/reload and extend the 'missing import' diagnostics with a list of providing modules. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@239750 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticSemaKinds.td | 8 ++- include/clang/Sema/Sema.h | 5 ++ lib/Sema/SemaLookup.cpp | 63 ++++++++++++++----- lib/Sema/SemaType.cpp | 16 +---- lib/Serialization/ASTWriter.cpp | 3 - .../Inputs/submodules-merge-defs/defs.h | 2 +- .../submodules-merge-defs/module.modulemap | 1 + .../Inputs/submodules-merge-defs/use-defs-2.h | 1 + test/Modules/module-private.cpp | 2 +- test/Modules/submodules-merge-defs.cpp | 19 +++--- 10 files changed, 73 insertions(+), 47 deletions(-) create mode 100644 test/Modules/Inputs/submodules-merge-defs/use-defs-2.h diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index d318a20928..71cba0183b 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -7637,9 +7637,11 @@ def err_module_private_local_class : Error< "local %select{struct|interface|union|class|enum}0 cannot be declared " "__module_private__">; def err_module_private_declaration : Error< - "declaration of %0 must be imported from module '%1' before it is required">; -def err_module_private_definition : Error< - "definition of %0 must be imported from module '%1' before it is required">; + "%select{declaration|definition}0 of %1 must be imported from " + "module '%2' before it is required">; +def err_module_private_declaration_multiple : Error< + "%select{declaration|definition}0 of %1 must be imported from " + "one of the following modules before it is required:%2">; def err_module_import_in_extern_c : Error< "import of C++ module '%0' appears within extern \"C\" language linkage " "specification">; diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 685a1ab243..691f21d433 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -1730,6 +1730,11 @@ public: void createImplicitModuleImportForErrorRecovery(SourceLocation Loc, Module *Mod); + /// \brief Diagnose that the specified declaration needs to be visible but + /// isn't, and suggest a module import that would resolve the problem. + void diagnoseMissingImport(SourceLocation Loc, NamedDecl *Decl, + bool NeedDefinition, bool Recover = true); + /// \brief Retrieve a suitable printing policy. PrintingPolicy getPrintingPolicy() const { return getPrintingPolicy(Context, PP); diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp index 202f027f6f..b5ef3a42d1 100644 --- a/lib/Sema/SemaLookup.cpp +++ b/lib/Sema/SemaLookup.cpp @@ -4661,6 +4661,50 @@ static NamedDecl *getDefinitionToImport(NamedDecl *D) { return nullptr; } +void Sema::diagnoseMissingImport(SourceLocation Loc, NamedDecl *Decl, + bool NeedDefinition, bool Recover) { + assert(!isVisible(Decl) && "missing import for non-hidden decl?"); + + // Suggest importing a module providing the definition of this entity, if + // possible. + NamedDecl *Def = getDefinitionToImport(Decl); + if (!Def) + Def = Decl; + + // FIXME: Add a Fix-It that imports the corresponding module or includes + // the header. + Module *Owner = getOwningModule(Decl); + assert(Owner && "definition of hidden declaration is not in a module"); + + auto Merged = Context.getModulesWithMergedDefinition(Decl); + if (!Merged.empty()) { + std::string ModuleList; + ModuleList += "\n "; + ModuleList += Owner->getFullModuleName(); + unsigned N = 0; + for (Module *M : Merged) { + ModuleList += "\n "; + if (++N == 5 && Merged.size() != N) { + ModuleList += "[...]"; + break; + } + ModuleList += M->getFullModuleName(); + } + + Diag(Loc, diag::err_module_private_declaration_multiple) + << NeedDefinition << Decl << ModuleList; + } else { + Diag(Loc, diag::err_module_private_declaration) + << NeedDefinition << Decl << Owner->getFullModuleName(); + } + Diag(Decl->getLocation(), NeedDefinition ? diag::note_previous_definition + : diag::note_previous_declaration); + + // Try to recover by implicitly importing this module. + if (Recover) + createImplicitModuleImportForErrorRecovery(Loc, Owner); +} + /// \brief Diagnose a successfully-corrected typo. Separated from the correction /// itself to allow external validation of the result, etc. /// @@ -4687,23 +4731,8 @@ void Sema::diagnoseTypo(const TypoCorrection &Correction, NamedDecl *Decl = Correction.getCorrectionDecl(); assert(Decl && "import required but no declaration to import"); - // Suggest importing a module providing the definition of this entity, if - // possible. - NamedDecl *Def = getDefinitionToImport(Decl); - if (!Def) - Def = Decl; - Module *Owner = getOwningModule(Def); - assert(Owner && "definition of hidden declaration is not in a module"); - - Diag(Correction.getCorrectionRange().getBegin(), - diag::err_module_private_declaration) - << Def << Owner->getFullModuleName(); - Diag(Def->getLocation(), diag::note_previous_declaration); - - // Recover by implicitly importing this module. - if (ErrorRecovery) - createImplicitModuleImportForErrorRecovery( - Correction.getCorrectionRange().getBegin(), Owner); + diagnoseMissingImport(Correction.getCorrectionRange().getBegin(), Decl, + /*NeedDefinition*/ false, ErrorRecovery); return; } diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp index 5734f3b2ed..d3787ec44b 100644 --- a/lib/Sema/SemaType.cpp +++ b/lib/Sema/SemaType.cpp @@ -5239,20 +5239,8 @@ bool Sema::RequireCompleteTypeImpl(SourceLocation Loc, QualType T, // If we know about the definition but it is not visible, complain. NamedDecl *SuggestedDef = nullptr; if (!Diagnoser.Suppressed && Def && - !hasVisibleDefinition(Def, &SuggestedDef)) { - // Suppress this error outside of a SFINAE context if we've already - // emitted the error once for this type. There's no usefulness in - // repeating the diagnostic. - // FIXME: Add a Fix-It that imports the corresponding module or includes - // the header. - Module *Owner = getOwningModule(SuggestedDef); - Diag(Loc, diag::err_module_private_definition) - << T << Owner->getFullModuleName(); - Diag(SuggestedDef->getLocation(), diag::note_previous_definition); - - // Try to recover by implicitly importing this module. - createImplicitModuleImportForErrorRecovery(Loc, Owner); - } + !hasVisibleDefinition(Def, &SuggestedDef)) + diagnoseMissingImport(Loc, SuggestedDef, /*NeedDefinition*/true); // We lock in the inheritance model once somebody has asked us to ensure // that a pointer-to-member type is complete. diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp index 7e1bd07f1e..5bb0bec4f5 100644 --- a/lib/Serialization/ASTWriter.cpp +++ b/lib/Serialization/ASTWriter.cpp @@ -5764,8 +5764,5 @@ void ASTWriter::DeclarationMarkedOpenMPThreadPrivate(const Decl *D) { void ASTWriter::RedefinedHiddenDefinition(const NamedDecl *D, Module *M) { assert(!WritingAST && "Already writing the AST!"); assert(D->isHidden() && "expected a hidden declaration"); - if (!D->isFromASTFile()) - return; - DeclUpdates[D].push_back(DeclUpdate(UPD_DECL_EXPORTED, M)); } diff --git a/test/Modules/Inputs/submodules-merge-defs/defs.h b/test/Modules/Inputs/submodules-merge-defs/defs.h index ad3711f073..b98cbc3a76 100644 --- a/test/Modules/Inputs/submodules-merge-defs/defs.h +++ b/test/Modules/Inputs/submodules-merge-defs/defs.h @@ -31,7 +31,7 @@ template struct F { template int F::f() { return 0; } template template int F::g() { return 0; } template int F::n = 0; -template<> template int F::g() { return 0; } +//template<> template int F::g() { return 0; } // FIXME: Re-enable this once we support merging member specializations. template<> struct F { int h(); }; inline int F::h() { return 0; } template struct F { int i(); }; diff --git a/test/Modules/Inputs/submodules-merge-defs/module.modulemap b/test/Modules/Inputs/submodules-merge-defs/module.modulemap index f8ae60fe44..3b5493e2b8 100644 --- a/test/Modules/Inputs/submodules-merge-defs/module.modulemap +++ b/test/Modules/Inputs/submodules-merge-defs/module.modulemap @@ -2,6 +2,7 @@ module "stuff" { textual header "defs.h" module "empty" { header "empty.h" } module "use" { header "use-defs.h" } + module "use-2" { requires use_defs_twice header "use-defs-2.h" } } module "redef" { diff --git a/test/Modules/Inputs/submodules-merge-defs/use-defs-2.h b/test/Modules/Inputs/submodules-merge-defs/use-defs-2.h new file mode 100644 index 0000000000..31c69c6a44 --- /dev/null +++ b/test/Modules/Inputs/submodules-merge-defs/use-defs-2.h @@ -0,0 +1 @@ +#include "defs.h" diff --git a/test/Modules/module-private.cpp b/test/Modules/module-private.cpp index 9213a0f20c..478d36dd4d 100644 --- a/test/Modules/module-private.cpp +++ b/test/Modules/module-private.cpp @@ -14,7 +14,7 @@ void test() { int test_broken() { HiddenStruct hidden; // \ // expected-error{{must use 'struct' tag to refer to type 'HiddenStruct' in this scope}} \ - // expected-error{{definition of 'struct HiddenStruct' must be imported}} + // expected-error{{definition of 'HiddenStruct' must be imported}} // expected-note@Inputs/module_private_left.h:3 {{previous definition is here}} Integer i; // expected-error{{unknown type name 'Integer'}} diff --git a/test/Modules/submodules-merge-defs.cpp b/test/Modules/submodules-merge-defs.cpp index e942335151..52b12ef850 100644 --- a/test/Modules/submodules-merge-defs.cpp +++ b/test/Modules/submodules-merge-defs.cpp @@ -4,6 +4,7 @@ // RUN: %clang_cc1 -x c++ -std=c++11 -fmodules-cache-path=%t -fmodules -I %S/Inputs/submodules-merge-defs %s -verify -fno-modules-error-recovery -fmodules-local-submodule-visibility -DTEXTUAL // RUN: %clang_cc1 -x c++ -std=c++11 -fmodules-cache-path=%t -fmodules -I %S/Inputs/submodules-merge-defs %s -verify -fno-modules-error-recovery -fmodules-local-submodule-visibility // RUN: %clang_cc1 -x c++ -std=c++11 -fmodules-cache-path=%t -fmodule-maps -I %S/Inputs/submodules-merge-defs %s -verify -fno-modules-error-recovery -fmodules-local-submodule-visibility -DTEXTUAL -DEARLY_INDIRECT_INCLUDE +// RUN: %clang_cc1 -x c++ -std=c++11 -fmodules-cache-path=%t -fmodules -I %S/Inputs/submodules-merge-defs %s -verify -fno-modules-error-recovery -fmodules-local-submodule-visibility -fmodule-feature use_defs_twice -DIMPORT_USE_2 // Trigger import of definitions, but don't make them visible. #include "empty.h" @@ -11,7 +12,14 @@ #include "indirect.h" #endif -A pre_a; // expected-error {{must be imported}} expected-error {{must use 'struct'}} +A pre_a; // expected-error {{must use 'struct'}} +#ifdef IMPORT_USE_2 +// expected-error-re@-2 {{must be imported from one of {{.*}}stuff.use{{.*}}stuff.use-2}} +#elif EARLY_INDIRECT_INCLUDE +// expected-error@-4 {{must be imported from module 'merged-defs'}} +#else +// expected-error@-6 {{must be imported from module 'stuff.use'}} +#endif // expected-note@defs.h:1 +{{here}} // expected-note@defs.h:2 +{{here}} int pre_use_a = use_a(pre_a); // expected-error {{'A' must be imported}} expected-error {{'use_a' must be imported}} @@ -46,6 +54,8 @@ J<> pre_j; // expected-error {{must be imported}} expected-error {{too few}} // Make definitions from second module visible. #ifdef TEXTUAL #include "import-and-redefine.h" +#elif defined IMPORT_USE_2 +#include "use-defs-2.h" #else #include "merged-defs.h" #endif @@ -61,13 +71,6 @@ int post_use_dx = use_dx(post_dx); int post_e = E(0); int post_ff = F().f(); int post_fg = F().g(); -#ifdef EARLY_INDIRECT_INCLUDE -// FIXME: Properly track the owning module for a member specialization. -// expected-error@defs.h:34 {{redefinition}} -// expected-note@defs.h:34 {{previous definition}} -// expected-error@-5 {{no matching member function}} -// expected-note@defs.h:34 {{substitution failure}} -#endif J<> post_j; template class K> struct J; J<> post_j2; -- 2.40.0