[Modules] Fix conservative assertion for import diagnostics

[bruno]

Summary

We currenltly assert when want to diagnose a missing import and the decl in question is already visible. It turns out that the decl in question might be visible because another decl from the same module actually made the module visible in a previous error diagnostic.

Make the assertion more flexible and avoid re-exporting the module if it's already visible.

[rsmith]

lib/Sema/SemaLookup.cpp

4971–4972 ↗

(On Diff #97729)

Decl could also now be visible due to its lexically-enclosing context having a visible merged definition. I think this assertion should either be removed or changed to something like

assert(!isVisible(Decl) || <ever created a module import for error recovery>);

[bruno]

Updated the patch after Richard's comments. Removed the assertion but bail out early if the module is already visible.

cfe/trunk/lib/Sema/SemaDecl.cpp

for (auto *DC = CurContext; DC; DC = DC->getLexicalParent()) {
         cast<Decl>(DC)->setHidden(false);
     }
   }
 }
 
 void Sema::createImplicitModuleImportForErrorRecovery(SourceLocation Loc,
                                                       Module *Mod) {
   // Bail if we're not allowed to implicitly import a module here.
-  if (isSFINAEContext() || !getLangOpts().ModulesErrorRecovery)
+  if (isSFINAEContext() || !getLangOpts().ModulesErrorRecovery ||
+      VisibleModules.isVisible(Mod))
     return;
 
   // Create the implicit import declaration.
   TranslationUnitDecl *TU = getASTContext().getTranslationUnitDecl();
   ImportDecl *ImportD = ImportDecl::CreateImplicit(getASTContext(), TU,
                                                    Loc, Mod, Loc);
   TU->addDecl(ImportD);
   Consumer.HandleImplicitImportDecl(ImportD);

cfe/trunk/lib/Sema/SemaLookup.cpp

if (ObjCProtocolDecl *PD = dyn_cast<ObjCProtocolDecl>(D))
     return PD->getDefinition();
   if (TemplateDecl *TD = dyn_cast<TemplateDecl>(D))
     return getDefinitionToImport(TD->getTemplatedDecl());
   return nullptr;
 }
 
 void Sema::diagnoseMissingImport(SourceLocation Loc, NamedDecl *Decl,
                                  MissingImportKind MIK, 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;
 
   Module *Owner = getOwningModule(Decl);
   assert(Owner && "definition of hidden declaration is not in a module");

cfe/trunk/test/Modules/Inputs/diagnose-missing-import/a.h

+#ifndef A_h
+#define A_h
+
+@class NSString;
+static NSString * const xyzRiskyCloseOpenParam = @"riskyCloseParam";
+static inline void XYZLogEvent(NSString* eventName, NSString* params);
+
+#endif

cfe/trunk/test/Modules/Inputs/diagnose-missing-import/module.modulemap

+module NCI {
+  explicit module A { header "a.h" }
+}

cfe/trunk/test/Modules/diagnose-missing-import.m

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/diagnose-missing-import \
+// RUN:   -Werror=implicit-function-declaration -fsyntax-only \
+// RUN:   -fimplicit-module-maps -verify %s
+@import NCI;
+
+void foo() {
+  XYZLogEvent(xyzRiskyCloseOpenParam, xyzRiskyCloseOpenParam); // expected-error {{implicit declaration of function 'XYZLogEvent'}} expected-error {{declaration of 'XYZLogEvent' must be imported}} expected-error {{declaration of 'xyzRiskyCloseOpenParam' must be imported from module 'NCI.A'}} expected-error {{declaration of 'xyzRiskyCloseOpenParam' must be imported from module 'NCI.A'}}
+}
+
+// expected-note@Inputs/diagnose-missing-import/a.h:5 {{previous declaration is here}}
+// expected-note@Inputs/diagnose-missing-import/a.h:5 {{previous declaration is here}}
+// expected-note@Inputs/diagnose-missing-import/a.h:6 {{previous declaration is here}}
+