[PATCH] D32828: [Modules] Fix conservative assertion for import diagnostics

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.

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.


https://reviews.llvm.org/D32828

Files:
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaLookup.cpp
  test/Modules/Inputs/diagnose-missing-import/a.h
  test/Modules/Inputs/diagnose-missing-import/module.modulemap
  test/Modules/diagnose-missing-import.m


Index: test/Modules/diagnose-missing-import.m
===================================================================
--- /dev/null
+++ test/Modules/diagnose-missing-import.m
@@ -0,0 +1,14 @@
+// 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
+ at 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 at Inputs/diagnose-missing-import/a.h:5 {{previous declaration is here}}
+// expected-note at Inputs/diagnose-missing-import/a.h:5 {{previous declaration is here}}
+// expected-note at Inputs/diagnose-missing-import/a.h:6 {{previous declaration is here}}
+
Index: test/Modules/Inputs/diagnose-missing-import/module.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/diagnose-missing-import/module.modulemap
@@ -0,0 +1,3 @@
+module NCI {
+  explicit module A { header "a.h" }
+}
Index: test/Modules/Inputs/diagnose-missing-import/a.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/diagnose-missing-import/a.h
@@ -0,0 +1,8 @@
+#ifndef A_h
+#define A_h
+
+ at class NSString;
+static NSString * const xyzRiskyCloseOpenParam = @"riskyCloseParam";
+static inline void XYZLogEvent(NSString* eventName, NSString* params);
+
+#endif
Index: lib/Sema/SemaLookup.cpp
===================================================================
--- lib/Sema/SemaLookup.cpp
+++ lib/Sema/SemaLookup.cpp
@@ -4960,16 +4960,16 @@
 
 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");
+  assert((!isVisible(Decl) || VisibleModules.isVisible(Owner)) &&
+         "missing import for non-hidden decl?");
 
   llvm::SmallVector<Module*, 8> OwningModules;
   OwningModules.push_back(Owner);
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -15925,7 +15925,8 @@
 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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D32828.97729.patch
Type: text/x-patch
Size: 3296 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170503/b6e1a699/attachment.bin>