[Modules] More descriptive diagnostics for misplaced import directive

If an import directive was put into wrong context, the error message was obscure,
complaining on misbalanced braces. To get more descriptive messages, annotation
tokens related to modules are processed where they must not be seen.

Differential Revision: http://reviews.llvm.org/D11844

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@248085 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/DiagnosticParseKinds.td b/include/clang/Basic/DiagnosticParseKinds.td
index 3f86681303871a55140c2879b6ac5e0d114fc152..291eca2d717e107d71ab81193568b21e3863bd75 100644 (file)
--- a/include/clang/Basic/DiagnosticParseKinds.td
+++ b/include/clang/Basic/DiagnosticParseKinds.td
@@ -1013,6 +1013,7 @@ def err_module_expected_ident : Error<
   "expected a module name after module import">;
 def err_module_expected_semi : Error<
   "expected ';' after module name">;
+def err_missing_before_module_end : Error<"expected %0 at end of module">;
 }
 
 let CategoryName = "Generics Issue" in {

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 7d22327961b0e42602bd648c068cbf869661f9e2..3432b6c268714999f6d9c41a1051aa61488015b8 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7776,6 +7776,8 @@ def note_module_import_in_extern_c : Note<
   "extern \"C\" language linkage specification begins here">;
 def err_module_import_not_at_top_level : Error<
   "import of module '%0' appears within %1">;
+def err_module_import_not_at_top_level_fatal : Error<
+  "import of module '%0' appears within %1">, DefaultFatal;
 def note_module_import_not_at_top_level : Note<"%0 begins here">;
 def err_module_self_import : Error<
   "import of module '%0' appears within same top-level module '%1'">;

diff --git a/include/clang/Parse/Parser.h b/include/clang/Parse/Parser.h
index 272b504d1e05281db659ca12359aa7266e853871..54bac154894b0be4a70bcf7340d20717353a4ab3 100644 (file)
--- a/include/clang/Parse/Parser.h
+++ b/include/clang/Parse/Parser.h
@@ -2555,6 +2555,14 @@ private:
   //===--------------------------------------------------------------------===//
   // Modules
   DeclGroupPtrTy ParseModuleImport(SourceLocation AtLoc);
+  bool parseMisplacedModuleImport();
+  bool tryParseMisplacedModuleImport() {
+    tok::TokenKind Kind = Tok.getKind();
+    if (Kind == tok::annot_module_begin || Kind == tok::annot_module_end ||
+        Kind == tok::annot_module_include)
+      return parseMisplacedModuleImport();
+    return false;
+  }
 
   //===--------------------------------------------------------------------===//
   // C++11/G++: Type Traits [Type-Traits.html in the GCC manual]

diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index a7b74172a7e76b3c8e0e31081c35f189bccb6dd7..b55db2e8cb6c32103e33fdbee1f4e1ea261aac47 100644 (file)
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -1799,6 +1799,10 @@ public:
   /// \brief The parser has left a submodule.
   void ActOnModuleEnd(SourceLocation DirectiveLoc, Module *Mod);
 
+  /// \brief Check if module import may be found in the current context,
+  /// emit error if not.
+  void diagnoseMisplacedModuleImport(Module *M, SourceLocation ImportLoc);
+
   /// \brief Create an implicit import of the given module at the given
   /// source location, for error recovery, if possible.
   ///

diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
index 80d25426f66de01fc24f10d477d41d38862cec83..f5983c0295eb87d71a635cfb0f11a8507eac8344 100644 (file)
--- a/lib/Parse/ParseDecl.cpp
+++ b/lib/Parse/ParseDecl.cpp
@@ -3589,7 +3589,8 @@ void Parser::ParseStructUnionBody(SourceLocation RecordLoc,
   SmallVector<Decl *, 32> FieldDecls;
 
   // While we still have something to read, read the declarations in the struct.
-  while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
+  while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof) &&
+         !tryParseMisplacedModuleImport()) {
     // Each iteration of this loop reads one struct-declaration.
 
     // Check for extraneous top-level semicolon.

diff --git a/lib/Parse/ParseDeclCXX.cpp b/lib/Parse/ParseDeclCXX.cpp
index 1a0d262279f2c4db178b14eafc9488d2e029193f..5e59d5dba284b8a8cecb1dd98b7638da8c1c06bb 100644 (file)
--- a/lib/Parse/ParseDeclCXX.cpp
+++ b/lib/Parse/ParseDeclCXX.cpp
@@ -210,7 +210,8 @@ void Parser::ParseInnerNamespace(std::vector<SourceLocation> &IdentLoc,
                                  ParsedAttributes &attrs,
                                  BalancedDelimiterTracker &Tracker) {
   if (index == Ident.size()) {
-    while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
+    while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof) &&
+           !tryParseMisplacedModuleImport()) {
       ParsedAttributesWithRange attrs(AttrFactory);
       MaybeParseCXX11Attributes(attrs);
       MaybeParseMicrosoftAttributes(attrs);
@@ -3063,11 +3064,12 @@ void Parser::ParseCXXMemberSpecification(SourceLocation RecordLoc,
 
   if (TagDecl) {
     // While we still have something to read, read the member-declarations.
-    while (Tok.isNot(tok::r_brace) && !isEofOrEom())
+    while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof) &&
+           !tryParseMisplacedModuleImport()) {
       // Each iteration of this loop reads one member-declaration.
       ParseCXXClassMemberDeclarationWithPragmas(
           CurAS, AccessAttrs, static_cast<DeclSpec::TST>(TagType), TagDecl);
-
+    }
     T.consumeClose();
   } else {
     SkipUntil(tok::r_brace);

diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp
index b658cef234ec60c11ec477894a04ae7e8057f13c..312c5e6f511760db0b4203179db8644bf3e42fa2 100644 (file)
--- a/lib/Parse/ParseStmt.cpp
+++ b/lib/Parse/ParseStmt.cpp
@@ -944,7 +944,8 @@ StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
       Stmts.push_back(R.get());
   }
 
-  while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
+  while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof) &&
+         !tryParseMisplacedModuleImport()) {
     if (Tok.is(tok::annot_pragma_unused)) {
       HandlePragmaUnused();
       continue;

diff --git a/lib/Parse/Parser.cpp b/lib/Parse/Parser.cpp
index 35e853f64fc450898f2489ca41c73784d6a1f670..5b0704658a251cee624bcd78348686444b649562 100644 (file)
--- a/lib/Parse/Parser.cpp
+++ b/lib/Parse/Parser.cpp
@@ -2018,6 +2018,37 @@ Parser::DeclGroupPtrTy Parser::ParseModuleImport(SourceLocation AtLoc) {
   return Actions.ConvertDeclToDeclGroup(Import.get());
 }
 
+/// \brief Try recover parser when module annotation appears where it must not
+/// be found.
+/// \returns false if the recover was successful and parsing may be continued, or
+/// true if parser must bail out to top level and handle the token there.
+bool Parser::parseMisplacedModuleImport() {
+  while (true) {
+    switch (Tok.getKind()) {
+    case tok::annot_module_end:
+      // Inform caller that recovery failed, the error must be handled at upper
+      // level.
+      return true;
+    case tok::annot_module_begin:
+      Actions.diagnoseMisplacedModuleImport(reinterpret_cast<Module *>(
+        Tok.getAnnotationValue()), Tok.getLocation());
+      return true;
+    case tok::annot_module_include:
+      // Module import found where it should not be, for instance, inside a
+      // namespace. Recover by importing the module.
+      Actions.ActOnModuleInclude(Tok.getLocation(),
+                                 reinterpret_cast<Module *>(
+                                 Tok.getAnnotationValue()));
+      ConsumeToken();
+      // If there is another module import, process it.
+      continue;
+    default:
+      return false;
+    }
+  }
+  return false;
+}
+
 bool BalancedDelimiterTracker::diagnoseOverflow() {
   P.Diag(P.Tok, diag::err_bracket_depth_exceeded)
     << P.getLangOpts().BracketDepth;
@@ -2045,7 +2076,10 @@ bool BalancedDelimiterTracker::expectAndConsume(unsigned DiagID,
 bool BalancedDelimiterTracker::diagnoseMissingClose() {
   assert(!P.Tok.is(Close) && "Should have consumed closing delimiter");
 
-  P.Diag(P.Tok, diag::err_expected) << Close;
+  if (P.Tok.is(tok::annot_module_end))
+    P.Diag(P.Tok, diag::err_missing_before_module_end) << Close;
+  else
+    P.Diag(P.Tok, diag::err_expected) << Close;
   P.Diag(LOpen, diag::note_matching) << Kind;
 
   // If we're not already at some kind of closing bracket, skip to our closing

diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index 701d8c729020e138d998c20c00bd51165694e115..3101fda933cbce539d8a5d71849e25297e66b0cb 100644 (file)
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -14368,14 +14368,17 @@ static void checkModuleImportContext(Sema &S, Module *M,
   while (isa<LinkageSpecDecl>(DC))
     DC = DC->getParent();
   if (!isa<TranslationUnitDecl>(DC)) {
-    S.Diag(ImportLoc, diag::err_module_import_not_at_top_level)
-      << M->getFullModuleName() << DC;
+    S.Diag(ImportLoc, diag::err_module_import_not_at_top_level_fatal)
+        << M->getFullModuleName() << DC;
     S.Diag(cast<Decl>(DC)->getLocStart(),
-           diag::note_module_import_not_at_top_level)
-      << DC;
+           diag::note_module_import_not_at_top_level) << DC;
   }
 }
 
+void Sema::diagnoseMisplacedModuleImport(Module *M, SourceLocation ImportLoc) {
+  return checkModuleImportContext(*this, M, ImportLoc, CurContext);
+}
+
 DeclResult Sema::ActOnModuleImport(SourceLocation AtLoc, 
                                    SourceLocation ImportLoc, 
                                    ModuleIdPath Path) {

diff --git a/test/Modules/Inputs/misplaced/misplaced-a.h b/test/Modules/Inputs/misplaced/misplaced-a.h
new file mode 100644 (file)
index 0000000..f50e5ce
--- /dev/null
+++ b/test/Modules/Inputs/misplaced/misplaced-a.h
@@ -0,0 +1,5 @@
+namespace A {
+  namespace B {  // expected-note{{namespace 'A::B' begins here}}
+    #include "misplaced-b.h"  // expected-error{{import of module 'Misplaced.Sub_B' appears within namespace 'A::B'}}
+  }
+}

diff --git a/test/Modules/Inputs/misplaced/misplaced-b.h b/test/Modules/Inputs/misplaced/misplaced-b.h
new file mode 100644 (file)
index 0000000..68dd955
--- /dev/null
+++ b/test/Modules/Inputs/misplaced/misplaced-b.h
@@ -0,0 +1 @@
+int a;
\ No newline at end of file

diff --git a/test/Modules/Inputs/misplaced/misplaced.modulemap b/test/Modules/Inputs/misplaced/misplaced.modulemap
new file mode 100644 (file)
index 0000000..50aa7a4
--- /dev/null
+++ b/test/Modules/Inputs/misplaced/misplaced.modulemap
@@ -0,0 +1,8 @@
+module Misplaced {
+  module Sub_A {
+    header "misplaced-a.h"
+  }
+  module Sub_B {
+    header "misplaced-b.h"
+  }
+}

diff --git a/test/Modules/auto-module-import.m b/test/Modules/auto-module-import.m
index de2c355a4247bb3932f26e019604e3444634d009..76441fcfedcf30d7284eebcd459bdcca55fa4912 100644 (file)
--- a/test/Modules/auto-module-import.m
+++ b/test/Modules/auto-module-import.m
@@ -83,6 +83,7 @@ int getNotInModule() {
   return not_in_module;
 }
 
-void includeNotAtTopLevel() { // expected-note {{to match this '{'}}
-  #include <NoUmbrella/A.h> // expected-warning {{treating #include as an import}} expected-error {{expected '}'}}
-} // expected-error {{extraneous closing brace}}
+void includeNotAtTopLevel() { // expected-note {{function 'includeNotAtTopLevel' begins here}}
+  #include <NoUmbrella/A.h> // expected-warning {{treating #include as an import}} \
+                              expected-error {{import of module 'NoUmbrella.A' appears within function 'includeNotAtTopLevel'}}
+}

diff --git a/test/Modules/extern_c.cpp b/test/Modules/extern_c.cpp
index f505c11f7c9d2e0cbe4a409e9f5e5e202282b087..c9b4b8435fd4db4eafcf0da2ab0ce655e8bcf7b9 100644 (file)
--- a/test/Modules/extern_c.cpp
+++ b/test/Modules/extern_c.cpp
@@ -68,7 +68,7 @@ namespace N {
   extern "C" {
 #endif
     int f;
-#if !defined(CXX_HEADER)
+#if !defined(CXX_HEADER) && !defined(NAMESPACE)
     // expected-error@-2 {{redefinition of 'f' as different kind of symbol}}
     // expected-note@c-header.h:1 {{previous}}
 #endif
@@ -78,4 +78,6 @@ namespace N {
 }
 #endif
 
+#if !defined(NAMESPACE)
 suppress_expected_no_diagnostics_error error_here; // expected-error {{}}
+#endif

diff --git a/test/Modules/malformed.cpp b/test/Modules/malformed.cpp
index 7d5bcc8c9d6607686e67df5311b97e2929672767..040361c9eeb8ed4e1645b282ec1b7307a715855b 100644 (file)
--- a/test/Modules/malformed.cpp
+++ b/test/Modules/malformed.cpp
@@ -12,20 +12,14 @@
 #include STR(HEADER)
 
 // CHECK-A: While building module 'malformed_a'
-// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: expected '}'
+// CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} error: expected '}' at end of module
 // CHECK-A: {{^}}Inputs/malformed/a1.h:1:{{.*}} note: to match this '{'
 //
 // CHECK-A: While building module 'malformed_a'
 // CHECK-A: {{^}}Inputs/malformed/a2.h:1:{{.*}} error: extraneous closing brace
 
 // CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: expected '}'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:1:{{.*}} note: to match this '{'
-// CHECK-B: {{^}}Inputs/malformed/b1.h:3:{{.*}} error: extraneous closing brace ('}')
-//
-// CHECK-B: While building module 'malformed_b'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} error: redefinition of 'g'
-// CHECK-B: {{^}}Inputs/malformed/b2.h:1:{{.*}} note: previous definition is here
+// CHECK-B: {{^}}Inputs/malformed/b1.h:2:{{.*}} error: import of module 'malformed_b.b2' appears within 'S'
 
 void test() { f<int>(); }
 // Test that we use relative paths to name files within an imported module.

diff --git a/test/Modules/misplaced-1.cpp b/test/Modules/misplaced-1.cpp
new file mode 100644 (file)
index 0000000..d67ad19
--- /dev/null
+++ b/test/Modules/misplaced-1.cpp
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+namespace N1 {  // expected-note{{namespace 'N1' begins here}}
+#include "dummy.h"  // expected-error{{import of module 'dummy' appears within namespace 'N1'}}
+}

diff --git a/test/Modules/misplaced-2.cpp b/test/Modules/misplaced-2.cpp
new file mode 100644 (file)
index 0000000..da46099
--- /dev/null
+++ b/test/Modules/misplaced-2.cpp
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+void func1() {  // expected-note{{function 'func1' begins here}}
+#include "dummy.h"  // expected-error{{import of module 'dummy' appears within function 'func1'}}
+}

diff --git a/test/Modules/misplaced-3.cpp b/test/Modules/misplaced-3.cpp
new file mode 100644 (file)
index 0000000..6d18e13
--- /dev/null
+++ b/test/Modules/misplaced-3.cpp
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+class C1 {  // expected-note{{'C1' begins here}}
+#include "dummy.h"  // expected-error{{import of module 'dummy' appears within 'C1'}}
+}

diff --git a/test/Modules/misplaced-4.cpp b/test/Modules/misplaced-4.cpp
new file mode 100644 (file)
index 0000000..8f795d5
--- /dev/null
+++ b/test/Modules/misplaced-4.cpp
@@ -0,0 +1,2 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -emit-module -fmodule-name=Misplaced -fmodules-cache-path=%t -x c++ -I %S/Inputs %S/Inputs/misplaced/misplaced.modulemap -verify

diff --git a/test/Modules/misplaced-5.c b/test/Modules/misplaced-5.c
new file mode 100644 (file)
index 0000000..ae4b3f5
--- /dev/null
+++ b/test/Modules/misplaced-5.c
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
+
+struct S1 {  // expected-note{{'struct S1' begins here}}
+#include "dummy.h"  // expected-error{{import of module 'dummy' appears within 'struct S1'}}
+}