Fri Jan 11 17:29:50 PST 2013

Author: dgregor
Date: Fri Jan 11 19:29:50 2013
New Revision: 172290

URL: http://llvm.org/viewvc/llvm-project?rev=172290&view=rev
Log:
Provide Decl::getOwningModule(), which determines the (sub)module in
which a particular declaration resides. Use this information to
customize the "definition of 'blah' must be imported from another
module" diagnostic with the module the user actually has to
import. Additionally, recover by importing that module, so we don't
complain about other names in that module.

Still TODO: coming up with decent Fix-Its for these cases, and expand
this recovery approach for other name lookup failures.


Modified:
    cfe/trunk/include/clang/AST/DeclBase.h
    cfe/trunk/include/clang/AST/ExternalASTSource.h
    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
    cfe/trunk/include/clang/Frontend/ASTUnit.h
    cfe/trunk/include/clang/Frontend/CompilerInstance.h
    cfe/trunk/include/clang/Lex/ModuleLoader.h
    cfe/trunk/include/clang/Sema/Lookup.h
    cfe/trunk/include/clang/Sema/Sema.h
    cfe/trunk/include/clang/Serialization/ASTReader.h
    cfe/trunk/lib/AST/DeclBase.cpp
    cfe/trunk/lib/Frontend/CompilerInstance.cpp
    cfe/trunk/lib/Sema/SemaDecl.cpp
    cfe/trunk/lib/Sema/SemaType.cpp
    cfe/trunk/lib/Serialization/ASTReader.cpp
    cfe/trunk/test/Modules/decldef.mm

Modified: cfe/trunk/include/clang/AST/DeclBase.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclBase.h?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================

--- cfe/trunk/include/clang/AST/DeclBase.h (original)
+++ cfe/trunk/include/clang/AST/DeclBase.h Fri Jan 11 19:29:50 2013
@@ -32,6 +32,7 @@
 class EnumDecl;
 class FunctionDecl;
 class LinkageSpecDecl;
+class Module;
 class NamedDecl;
 class NamespaceDecl;
 class ObjCCategoryDecl;
@@ -593,7 +594,18 @@
     
     return 0;
   }
-  
+
+private:
+  Module *getOwningModuleSlow() const;
+
+public:
+  Module *getOwningModule() const {
+    if (!isFromASTFile())
+      return 0;
+
+    return getOwningModuleSlow();
+  }
+
   unsigned getIdentifierNamespace() const {
     return IdentifierNamespace;
   }

Modified: cfe/trunk/include/clang/AST/ExternalASTSource.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ExternalASTSource.h?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/ExternalASTSource.h (original)
+++ cfe/trunk/include/clang/AST/ExternalASTSource.h Fri Jan 11 19:29:50 2013
@@ -25,6 +25,7 @@
 class DeclarationName;
 class ExternalSemaSource; // layering violation required for downcasting
 class FieldDecl;
+class Module;
 class NamedDecl;
 class RecordDecl;
 class Selector;
@@ -131,9 +132,12 @@
   /// \brief Ensures that the table of all visible declarations inside this
   /// context is up to date.
   ///
-  /// The default implementation of this functino is a no-op.
+  /// The default implementation of this function is a no-op.
   virtual void completeVisibleDeclsMap(const DeclContext *DC);
 
+  /// \brief Retrieve the module that corresponds to the given module ID.
+  virtual Module *getModule(unsigned ID) { return 0; }
+
   /// \brief Finds all declarations lexically contained within the given
   /// DeclContext, after applying an optional filter predicate.
   ///

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Jan 11 19:29:50 2013
@@ -6082,7 +6082,7 @@
   "local %select{struct|interface|union|class|enum}0 cannot be declared "
   "__module_private__">;
 def err_module_private_definition : Error<
-  "definition of %0 must be imported before it is required">;
+  "definition of %0 must be imported from module '%1' before it is required">;
 }
 
 let CategoryName = "Documentation Issue" in {

Modified: cfe/trunk/include/clang/Frontend/ASTUnit.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/ASTUnit.h?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/include/clang/Frontend/ASTUnit.h (original)
+++ cfe/trunk/include/clang/Frontend/ASTUnit.h Fri Jan 11 19:29:50 2013
@@ -837,6 +837,10 @@
     // ASTUnit doesn't know how to load modules (not that this matters).
     return ModuleLoadResult();
   }
+
+  virtual void makeModuleVisible(Module *Mod,
+                                 Module::NameVisibilityKind Visibility) { }
+
 };
 
 } // namespace clang

Modified: cfe/trunk/include/clang/Frontend/CompilerInstance.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CompilerInstance.h?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/include/clang/Frontend/CompilerInstance.h (original)
+++ cfe/trunk/include/clang/Frontend/CompilerInstance.h Fri Jan 11 19:29:50 2013
@@ -649,6 +649,10 @@
                                       ModuleIdPath Path,
                                       Module::NameVisibilityKind Visibility,
                                       bool IsInclusionDirective);
+
+  virtual void makeModuleVisible(Module *Mod,
+                                 Module::NameVisibilityKind Visibility);
+
 };
 
 } // end namespace clang

Modified: cfe/trunk/include/clang/Lex/ModuleLoader.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/ModuleLoader.h?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/include/clang/Lex/ModuleLoader.h (original)
+++ cfe/trunk/include/clang/Lex/ModuleLoader.h Fri Jan 11 19:29:50 2013
@@ -80,6 +80,10 @@
                                       ModuleIdPath Path,
                                       Module::NameVisibilityKind Visibility,
                                       bool IsInclusionDirective) = 0;
+
+  /// \brief Make the given module visible.
+  virtual void makeModuleVisible(Module *Mod,
+                                 Module::NameVisibilityKind Visibility) = 0;
 };
   
 }

Modified: cfe/trunk/include/clang/Sema/Lookup.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Lookup.h?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/include/clang/Sema/Lookup.h (original)
+++ cfe/trunk/include/clang/Sema/Lookup.h Fri Jan 11 19:29:50 2013
@@ -138,7 +138,8 @@
       IDNS(0),
       Redecl(Redecl != Sema::NotForRedeclaration),
       HideTags(true),
-      Diagnose(Redecl == Sema::NotForRedeclaration)
+      Diagnose(Redecl == Sema::NotForRedeclaration),
+      AllowHidden(Redecl == Sema::ForRedeclaration)
   {
     configure();
   }
@@ -158,7 +159,8 @@
       IDNS(0),
       Redecl(Redecl != Sema::NotForRedeclaration),
       HideTags(true),
-      Diagnose(Redecl == Sema::NotForRedeclaration)
+      Diagnose(Redecl == Sema::NotForRedeclaration),
+      AllowHidden(Redecl == Sema::ForRedeclaration)
   {
     configure();
   }
@@ -176,7 +178,8 @@
       IDNS(Other.IDNS),
       Redecl(Other.Redecl),
       HideTags(Other.HideTags),
-      Diagnose(false)
+      Diagnose(false),
+      AllowHidden(Other.AllowHidden)
   {}
 
   ~LookupResult() {
@@ -214,10 +217,16 @@
     return Redecl;
   }
 
+  /// \brief Specify whether hidden declarations are visible, e.g.,
+  /// for recovery reasons.
+  void setAllowHidden(bool AH) {
+    AllowHidden = AH;
+  }
+
   /// \brief Determine whether this lookup is permitted to see hidden
   /// declarations, such as those in modules that have not yet been imported.
   bool isHiddenDeclarationVisible() const {
-    return Redecl || LookupKind == Sema::LookupTagName;
+    return AllowHidden || LookupKind == Sema::LookupTagName;
   }
   
   /// Sets whether tag declarations should be hidden by non-tag
@@ -483,6 +492,7 @@
   /// \brief Change this lookup's redeclaration kind.
   void setRedeclarationKind(Sema::RedeclarationKind RK) {
     Redecl = RK;
+    AllowHidden = (RK == Sema::ForRedeclaration);
     configure();
   }
 
@@ -644,6 +654,9 @@
   bool HideTags;
 
   bool Diagnose;
+
+  /// \brief True if we should allow hidden declarations to be 'visible'.
+  bool AllowHidden;
 };
 
   /// \brief Consumes visible declarations found when searching for

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Fri Jan 11 19:29:50 2013
@@ -275,12 +275,6 @@
   /// This is only necessary for issuing pretty diagnostics.
   ExtVectorDeclsType ExtVectorDecls;
 
-  /// \brief The set of types for which we have already complained about the
-  /// definitions being hidden.
-  ///
-  /// This set is used to suppress redundant diagnostics.
-  llvm::SmallPtrSet<NamedDecl *, 4> HiddenDefinitions;
-
   /// FieldCollector - Collects CXXFieldDecls during parsing of C++ classes.
   OwningPtr<CXXFieldCollector> FieldCollector;
 
@@ -1456,6 +1450,14 @@
   DeclResult ActOnModuleImport(SourceLocation AtLoc, SourceLocation ImportLoc,
                                ModuleIdPath Path);
 
+  /// \brief Create an implicit import of the given module at the given
+  /// source location.
+  ///
+  /// This routine is typically used for error recovery, when the entity found
+  /// by name lookup is actually hidden within a module that we know about but
+  /// the user has forgotten to import.
+  void createImplicitModuleImport(SourceLocation Loc, Module *Mod);
+
   /// \brief Retrieve a suitable printing policy.
   PrintingPolicy getPrintingPolicy() const {
     return getPrintingPolicy(Context, PP);

Modified: cfe/trunk/include/clang/Serialization/ASTReader.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/include/clang/Serialization/ASTReader.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTReader.h Fri Jan 11 19:29:50 2013
@@ -1558,7 +1558,12 @@
   /// \brief Retrieve the submodule that corresponds to a global submodule ID.
   ///
   Module *getSubmodule(serialization::SubmoduleID GlobalID);
-  
+
+  /// \brief Retrieve the module that corresponds to the given module ID.
+  ///
+  /// Note: overrides method in ExternalASTSource
+  virtual Module *getModule(unsigned ID);
+
   /// \brief Retrieve a selector from the given module with its local ID
   /// number.
   Selector getLocalSelector(ModuleFile &M, unsigned LocalID);

Modified: cfe/trunk/lib/AST/DeclBase.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclBase.cpp?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/lib/AST/DeclBase.cpp (original)
+++ cfe/trunk/lib/AST/DeclBase.cpp Fri Jan 11 19:29:50 2013
@@ -59,6 +59,11 @@
   return Result;
 }
 
+Module *Decl::getOwningModuleSlow() const {
+  assert(isFromASTFile() && "Not from AST file?");
+  return getASTContext().getExternalSource()->getModule(getOwningModuleID());
+}
+
 const char *Decl::getDeclKindName() const {
   switch (DeclKind) {
   default: llvm_unreachable("Declaration not in DeclNodes.inc!");

Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Fri Jan 11 19:29:50 2013
@@ -1201,3 +1201,9 @@
   LastModuleImportResult = ModuleLoadResult(Module, false);
   return LastModuleImportResult;
 }
+
+void CompilerInstance::makeModuleVisible(Module *Mod,
+                                         Module::NameVisibilityKind Visibility){
+  ModuleManager->makeModuleVisible(Mod, Visibility);
+}
+

Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Jan 11 19:29:50 2013
@@ -11173,6 +11173,18 @@
   return Import;
 }
 
+void Sema::createImplicitModuleImport(SourceLocation Loc, Module *Mod) {
+  // Create the implicit import declaration.
+  TranslationUnitDecl *TU = getASTContext().getTranslationUnitDecl();
+  ImportDecl *ImportD = ImportDecl::CreateImplicit(getASTContext(), TU,
+                                                   Loc, Mod, Loc);
+  TU->addDecl(ImportD);
+  Consumer.HandleImplicitImportDecl(ImportD);
+
+  // Make the module visible.
+  PP.getModuleLoader().makeModuleVisible(Mod, Module::AllVisible);
+}
+
 void Sema::ActOnPragmaRedefineExtname(IdentifierInfo* Name,
                                       IdentifierInfo* AliasName,
                                       SourceLocation PragmaLoc,

Modified: cfe/trunk/lib/Sema/SemaType.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaType.cpp?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaType.cpp (original)
+++ cfe/trunk/lib/Sema/SemaType.cpp Fri Jan 11 19:29:50 2013
@@ -4392,9 +4392,14 @@
       // repeating the diagnostic.
       // FIXME: Add a Fix-It that imports the corresponding module or includes
       // the header.
-      if (isSFINAEContext() || HiddenDefinitions.insert(Def)) {
-        Diag(Loc, diag::err_module_private_definition) << T;
-        Diag(Def->getLocation(), diag::note_previous_definition);
+      Module *Owner = Def->getOwningModule();
+      Diag(Loc, diag::err_module_private_definition)
+        << T << Owner->getFullModuleName();
+      Diag(Def->getLocation(), diag::note_previous_definition);
+
+      if (!isSFINAEContext()) {
+        // Recover by implicitly importing this module.
+        createImplicitModuleImport(Loc, Owner);
       }
     }
 

Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReader.cpp Fri Jan 11 19:29:50 2013
@@ -6200,7 +6200,11 @@
   
   return SubmodulesLoaded[GlobalID - NUM_PREDEF_SUBMODULE_IDS];
 }
-                               
+
+Module *ASTReader::getModule(unsigned ID) {
+  return getSubmodule(ID);
+}
+
 Selector ASTReader::getLocalSelector(ModuleFile &M, unsigned LocalID) {
   return DecodeSelector(getGlobalSelectorID(M, LocalID));
 }

Modified: cfe/trunk/test/Modules/decldef.mm
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/decldef.mm?rev=172290&r1=172289&r2=172290&view=diff
==============================================================================
--- cfe/trunk/test/Modules/decldef.mm (original)
+++ cfe/trunk/test/Modules/decldef.mm Fri Jan 11 19:29:50 2013
@@ -2,13 +2,7 @@
 // RUN: %clang_cc1 -fmodules -I %S/Inputs -fmodule-cache-path %t %s -verify
 
 
-// in other file: expected-note{{previous definition is here}}
-
-
-
-
-
-// in other file: expected-note{{previous definition is here}}
+// In other file: expected-note {{previous definition is here}}
 
 @import decldef;
 A *a1; // expected-error{{unknown type name 'A'}}
@@ -19,10 +13,9 @@
 B *b;
 
 void testA(A *a) {
-  a->ivar = 17; // expected-error{{definition of 'A' must be imported before it is required}}
+  a->ivar = 17; // expected-error{{definition of 'A' must be imported from module 'decldef.Def' before it is required}}
 }
 
 void testB() {
-  B b; // expected-error{{definition of 'B' must be imported before it is required}}
-  B b2; // Note: the reundant error was silenced.
+  B b; // Note: redundant error silenced
 }



