[Modules] More descriptive diagnostics for misplaced import directive

[sepavloff]

Summary

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.

[rsmith]

This doesn't recover from the errors properly any more (leaving junk behind in the 'current module' stack), and somewhat breaks the original purpose of these checks, which is to detect and diagnose missing close braces. Here's what I suggest:

1. Keep the isEofOrEom calls as-is.
2. Diagnose the unexpected EOM tokens as you do now (but customize the diagnostics in the case of a module_end token).
3. Once you've diagnosed an EOM token, don't consume it; instead update the annotation so you know not to diagnose it again, and bail out of parsing the current construct.

Then, when we hit an EOM, we'll diagnose it once at the innermost level, bail out all the way to the top level, and then handle it properly.

include/clang/Sema/Sema.h

1781–1784

I don't think this is the right interface. The calls from the parser expect an error to be issued, and don't do the right thing if the module import is valid, so this should instead be called something more like diagnoseMisplacedModuleImport, and should either assert if the current context allows module imports or not perform the check.

lib/Parse/ParseDeclCXX.cpp

2857–2864

Please factor this out into a bool TryParseMisplacedModuleImport() function.

2858–2859

For module_end, we probably still want to give the "missing '}'" diagnostic, because that's almost certainly the problem if we reach the end of a module while still within a function or namespace. Maybe pass a flag into checkModuleImportContext to say whether you're entering or leaving the module?

2862–2863

This is not a good way to recover from the problem: for that, we should probably do what we used to do, and bail out to the top level.

[sepavloff]

Fixed module boundary treatment.

This version must fix problems in the treatment of annot_modulbegin and
annot_module_end. Error recover for the token annot_module_include found
in wrong context can be made as previously, just by ignoring it. To handle
token 'annot_module_end' parser bails out to upper level, where the module
end can be processed, it will complain on missing '}' etc. As for
'annot_module_start', now parser tries to recover by skipping all content
till matching 'annot_module_end'.

Also made changes according to reviewer's notes.

[rsmith]

include/clang/Basic/DiagnosticParseKinds.td

1018–1019

These diagnostics are phrased from the point of view of the implementation, and should instead be phrased from the point of view of the user.

The first diagnostic would be more useful if it said something like "missing '}' at end of module". That's pretty close to the diagnostic we already produce; maybe instead of changing the behavior of this case you could improve BalancedDelimiterTracker to use a custom diagnostic for the case where the missing closing delimiter is instead an end-of-module token?

The second diagnostic seems like an improvement, but from the user's perspective, this wasn't the start of a submodule, it was a module import / #include. Maybe just use diagnoseMisplacedModuleImport here?

lib/Parse/ParseDeclCXX.cpp

3070

This shouldn't be named try* if it has a precondition that the current token is an EoM token. (The try form, if there is one, should check for that token itself.)

lib/Parse/Parser.cpp

1999

This will result in you producing two errors "unexpected end of module" and "missing }" for each construct that is still open at the end of the module. I don't think that's what we want.

2003

This is liable to produce bad follow-on diagnostics; I don't think this is a reasonable way to recover. I can see a few feasible options here:

1. Emit a fatal error when this happens (suppressing further diagnostics)
2. Break out to the top level of parsing and resume from there (that is, assume that a modular header expects to be included at the top level and that the user didn't screw up their module map)
3. Enter the module and carry on parsing from here

My preference would be either (1) or (2). But either way, we should diagnose the still-open bracketing constructs, because the problem is likely to be a missing close brace at the end of an unrelated header file.

2018

Again, this will recover very poorly. In this case, our best bet seems to be to recover by importing the module.

[silvas]

lib/Parse/Parser.cpp

2003

I strongly prefer (1). In all cases I have recorded in my notes when modularizing, the error: expected '}' diagnostic indicated one of two things:

• that a header needed to be marked textual in the module map.
• that a #include had to be moved to the top of the file (this case was likely behaving unexpectedly in the non-modules case and "happened to work").

For the sake of our users, it is probably best to immediately fatal error and suggest an alternative (the suggestions can be a separate patch; my recommendations are below).

I believe a user is most likely to run into this diagnostic when developing an initial set of module maps, so our diagnostic should be tailored to that audience. These users may have little experience with modules when they encounter this diagnostic, so a notes suggesting a likely remedy helps them develop confidence by providing authoritative advice (and avoiding a round trip to the documentation).

My fine-grained recommendations from experience are:

• #include inside extern "C": always meant to be modular, always the fix is to move it out of the braced block.
• #include inside namespace: always meant to be modular, always the fix is to move it out of the braced block.
• #include inside class: always textual (e.g. bringing in methods like TableGen .inc files; sometimes similar ".inc" files are manually written and not autogenerated. I have observed e.g. "classfoo_methods.h" files; another instance of this is stamping out ".def" files)
• #include inside array initializer: always textual. Typically ".def" files
• #include inside function: always textual. Typically ".def" files (e.g. generate a switch)

".inl" files are theoretically a problem inside namespaces, but I have not observed this issue in practice. In my experience code taking the effort to have ".inl" files is quite clean and avoids such textual dependencies. The issues I have seen in practice with ".inl" files usually end up coming out through a different part of clang. Improving that is for a separate patch though.

[sepavloff]

Updated patch

Thanks to all for fruitful discussion!

The new version tries to addresses review notes. It differs from the previous version in:

• Method tryParseMisplacedModuleImport now depends only on current token and is used in loop condition. Probably a better name for it is something like skipMisplacedModuleImport.
• BalancedDelimiterTracker is updated so that it prints different message if module ended before closing bracket has been seen.
• If annot_module_begin is seen where it must not, recovery depends on context. Inside a namespace the module is imported as a recovery, a note suggesting to move import to top level is issued. Inside a class of a compound statement fatal error issued, there is no way to handle this case at upper level.

[silvas]

test/Modules/auto-module-import.m

89

I think what this means is marking the *header* as textual. So I would suggest the wording "consider marking the header as textual".

[sepavloff]

Ping.

Thanks,
--Serge

[silvas]

ping?

[sepavloff]

Updated diagnostics according to Sean's notes ('textual module' -> 'textual header')

[rsmith]

Sorry for the delay. I'm generally happy with the direction of this patch; I think this will substantially improve how we diagnose this set of problems.

include/clang/Basic/DiagnosticParseKinds.td

1018

I would drop the "the" in this error.

include/clang/Basic/DiagnosticSemaKinds.td

7725

We'll already have said something like:

error: import of module 'foo' appears within class X
note: class X begins here

There are two likely situations here: either the header was intended to be included textually, or the user forgot a } or similar and didn't expect for the import to be inside the class. Either way, this note is not useful (either they thought it *was* at the top level, or the note is not applicable).

How about dropping this note? The "begins here" note seems to have already covered the interesting case.

7726

Textual headers are not the common case, so "consider" seems too strong; how about weakening this to something like:

"if this header is intended to be included textually, mark it 'textual' in the module map"

lib/Parse/ParseDeclCXX.cpp

213–214

Move this try... call to the end of the && here and elsewhere; it's much more likely that we'll hit an r_brace.

lib/Parse/Parser.cpp

1994–1995

This is the opposite of how all the other try functions behave: they return true if they consumed the thing they were asked to consume.

1996–1998

Refactor this as follows:

Rename this to parseMisplacedModuleImport. Add an inline tryParseMisplacedModuleImport to Parser.h, which checks if the current token is an EOM token. If it is, it calls this function. If it is not, it returns immediately.

The purpose of having these try functions with thin wrappers in the .h file is so that we can inline the cheap "are we looking at the right kind of token" check (and allow it to be combined with other nearby checks on the token kind).

lib/Sema/SemaDecl.cpp

14270–14283

It looks like this will currently produce repeated diagnostics for an import within multiple namespaces:

module.map:

module Foo { module X { header "foo1.h" } module Y { header "foo2.h" } }

foo1.h:

namespace A {
  namespace B {
    #include "foo2.h"
  }
}

I believe this will diagnose:

1. import of "Foo.Y" within "B"
2. expected } at end of "B"
3. import of "Foo.Y" within "A"
4. expected } at end of "A"

I think we should treat this case as fatal too.

14281

I think we should only suggest changing the module map if our current module and the module of the included file are within the same top-level module. It would be unfortunate for us to suggest modifying, say, the libc module map if we see some end user's code including a C library header in the wrong context.

You can check M->getTopLevelModuleName() against S.getLangOpts().CurrentModule and S.getLangOpts().ImplementationOfModule to see whether it's a submodule of the current module.

[silvas]

include/clang/Basic/DiagnosticSemaKinds.td

7725

I think this makes sense. error: ... import ... appears within ... seems like sufficient information about the nature of the error.

lib/Sema/SemaDecl.cpp

14281

From my experience, your suggested heuristic is insufficient; we definitely need to cover the case that crosses top-level modules (or is from a .cpp file to a module it imports); actually, that is the only case that I have run into in practice.

In this patch, I think the right thing is to just not emit note_module_need_top_level nor note_module_need_textual. A separate patch can use a flag -Wintial-modules-migration (or whatever) to emit these notes a bit more aggressively (the flag would be off by default).

test/Modules/misplaced.cpp

2–13

This test does not cover the appears within class case.

[sepavloff]

Updated patch.

[sepavloff]

Thanks for helpful notes!

There are similar cases in Objective C. Should they be implemented in this patch or separate patch is OK?

include/clang/Basic/DiagnosticParseKinds.td

1018

Done.

include/clang/Basic/DiagnosticSemaKinds.td

7725

The message is removed.

7726

This message is also removed, as Sean proposed.

lib/Parse/ParseDeclCXX.cpp

213–214

Moved.

lib/Parse/Parser.cpp

1994–1995

Meaning of the return value is inverted.

1996–1998

Thank you for explanation. Fixed.

lib/Sema/SemaDecl.cpp

14270–14283

Yes, the messages were as you described. Now import inside a namespace is also a fatal error. Test misplaced-4.cpp checks this case.

14281

As Sean proposed, these are not emitted.

test/Modules/misplaced.cpp

2–13

This case was checked in malformed.cpp but for the sake of consistency separate test was added.

[rsmith]

include/clang/Parse/Parser.h

2559–2561

Use Tok.isOneOf here.

[rsmith]

A separate patch for the ObjC cases is fine.

include/clang/Basic/DiagnosticParseKinds.td

def warn_pragma_unroll_cuda_value_in_parens : Warning<
   InGroup<CudaCompat>;
 } // end of Parse Issue category.
 
 let CategoryName = "Modules Issue" in {
 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 the end of module">;
 }
 
 let CategoryName = "Generics Issue" in {
 
 def err_objc_expected_type_parameter : Error<
   "expected type parameter name">;
 
 def err_objc_parameterized_implementation : Error<
   "@implementation cannot have type parameters">;
 
 def err_objc_type_args_after_protocols : Error<
   "protocol qualifiers must precede type arguments">;
 }
 
 } // end of Parser diagnostics

include/clang/Basic/DiagnosticSemaKinds.td

def err_module_unimported_use_multiple : Error<
   "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">;
 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'">;
 def err_module_import_in_implementation : Error<
   "@import of module '%0' in implementation of '%1'; use #import">;
+def note_module_need_top_level : Note<"consider moving the import to top level">;
+def note_module_need_textual : Note<"consider marking the module as textual">;
 }
 
 let CategoryName = "Documentation Issue" in {
 def warn_not_a_doxygen_trailing_member_comment : Warning<
   "not a Doxygen trailing comment">, InGroup<Documentation>, DefaultIgnore;
 } // end of documentation issue category
 
 let CategoryName = "Instrumentation Issue" in {

include/clang/Parse/Parser.h

Decl *ParseExplicitInstantiation(unsigned Context,
                                    SourceLocation ExternLoc,
                                    SourceLocation TemplateLoc,
                                    SourceLocation &DeclEnd,
                                    AccessSpecifier AS = AS_none);
 
   //===--------------------------------------------------------------------===//
   // Modules
   DeclGroupPtrTy ParseModuleImport(SourceLocation AtLoc);
+  bool tryParseMisplacedModuleImport();
 
   //===--------------------------------------------------------------------===//
   // C++11/G++: Type Traits [Type-Traits.html in the GCC manual]
   ExprResult ParseTypeTrait();
   
   //===--------------------------------------------------------------------===//
   // Embarcadero: Arary and Expression Traits
   ExprResult ParseArrayTypeTrait();
   ExprResult ParseExpressionTrait();
 
   //===--------------------------------------------------------------------===//
   // Preprocessor code-completion pass-through

include/clang/Sema/Sema.h

public:
   /// #include or similar preprocessing directive.
   void ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod);
 
   /// \brief The parsed has entered a submodule.
   void ActOnModuleBegin(SourceLocation DirectiveLoc, Module *Mod);
   /// \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.
   ///
   /// This routine is typically used when an entity found by name lookup
   /// is actually hidden within a module that we know about but the user
   /// has forgotten to import.
   void createImplicitModuleImportForErrorRecovery(SourceLocation Loc,
                                                   Module *Mod);

lib/Parse/ParseDeclCXX.cpp

 /// ParseInnerNamespace - Parse the contents of a namespace.
 void Parser::ParseInnerNamespace(std::vector<SourceLocation> &IdentLoc,
                                  std::vector<IdentifierInfo *> &Ident,
                                  std::vector<SourceLocation> &NamespaceLoc,
                                  unsigned int index, SourceLocation &InlineLoc,
                                  ParsedAttributes &attrs,
                                  BalancedDelimiterTracker &Tracker) {
   if (index == Ident.size()) {
-    while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
+    while (tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
+           Tok.isNot(tok::eof)) {
       ParsedAttributesWithRange attrs(AttrFactory);
       MaybeParseCXX11Attributes(attrs);
       MaybeParseMicrosoftAttributes(attrs);
       ParseExternalDeclaration(attrs);
     }
 
     // The caller is what called check -- we are simply calling
     // the close for it.
if (Tok.is(tok::annot_pragma_ms_pointers_to_members)) {
     return DeclGroupPtrTy();
   }
 
   if (Tok.is(tok::annot_pragma_ms_pragma)) {
     HandlePragmaMSPragma();
     return DeclGroupPtrTy();
   }
 
   // If we see a namespace here, a close brace was missing somewhere.
   if (Tok.is(tok::kw_namespace)) {
     DiagnoseUnexpectedNamespace(cast<NamedDecl>(TagDecl));
     return DeclGroupPtrTy();
   }
 
   AccessSpecifier NewAS = getAccessSpecifierIfPresent();
   if (NewAS != AS_none) {
     // Current token is a C++ access specifier.
     AS = NewAS;
     SourceLocation ASLoc = Tok.getLocation();
     unsigned TokLength = Tok.getLength();
     ConsumeToken();
     AccessAttrs.clear();
     MaybeParseGNUAttributes(AccessAttrs);
 
void Parser::ParseCXXMemberSpecification(SourceLocation RecordLoc,
   if (TagType == DeclSpec::TST_class)
     CurAS = AS_private;
   else
     CurAS = AS_public;
   ParsedAttributesWithRange AccessAttrs(AttrFactory);
 
   if (TagDecl) {
     // While we still have something to read, read the member-declarations.
-    while (Tok.isNot(tok::r_brace) && !isEofOrEom())
+    while (tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
+           Tok.isNot(tok::eof)) {
       // 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);
   }
 
   // If attributes exist after class contents, parse them.
   ParsedAttributes attrs(AttrFactory);
   MaybeParseGNUAttributes(attrs);

lib/Parse/ParseStmt.cpp

DeclGroupPtrTy Res =
         Actions.FinalizeDeclaratorGroup(getCurScope(), DS, DeclsInGroup);
     StmtResult R = Actions.ActOnDeclStmt(Res, LabelLoc, Tok.getLocation());
 
     ExpectAndConsumeSemi(diag::err_expected_semi_declaration);
     if (R.isUsable())
       Stmts.push_back(R.get());
   }
 
-  while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
+  while (tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
+         Tok.isNot(tok::eof)) {
     if (Tok.is(tok::annot_pragma_unused)) {
       HandlePragmaUnused();
       continue;
     }
 
     StmtResult R;
     if (Tok.isNot(tok::kw___extension__)) {
       R = ParseStatementOrDeclaration(Stmts, false);

lib/Parse/Parser.cpp

Parser::DeclGroupPtrTy Parser::ParseModuleImport(SourceLocation AtLoc) {
   DeclResult Import = Actions.ActOnModuleImport(AtLoc, ImportLoc, Path);
   ExpectAndConsumeSemi(diag::err_module_expected_semi);
   if (Import.isInvalid())
     return DeclGroupPtrTy();
   
   return Actions.ConvertDeclToDeclGroup(Import.get());
 }
 
+/// \brief Try recover parser when module annotation appears where it must not
+/// be found.
+/// \returns true if the recover was successful and parsing may be continued, or
+/// false if parser must bail out to top level and handle the token there.
+bool Parser::tryParseMisplacedModuleImport() {
+  while (true) {
+    switch (Tok.getKind()) {
+    case tok::annot_module_end:
+      // Inform caller that recovery failed, the error must be handled at upper
+      // level.
+      return false;
+    case tok::annot_module_begin:
+      Actions.diagnoseMisplacedModuleImport(reinterpret_cast<Module *>(
+        Tok.getAnnotationValue()), Tok.getLocation());
+      return false;
+    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 true;
+    }
+  }
+  return true;
+}
+
 bool BalancedDelimiterTracker::diagnoseOverflow() {
   P.Diag(P.Tok, diag::err_bracket_depth_exceeded)
     << P.getLangOpts().BracketDepth;
   P.Diag(P.Tok, diag::note_bracket_depth);
   P.cutOffParsing();
   return true;
 }
 
if (getDepth() < MaxDepth)
     return false;
     
   return diagnoseOverflow();
 }
 
 bool BalancedDelimiterTracker::diagnoseMissingClose() {
   assert(!P.Tok.is(Close) && "Should have consumed closing delimiter");
 
+  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
   // token.
   if (P.Tok.isNot(tok::r_paren) && P.Tok.isNot(tok::r_brace) &&
       P.Tok.isNot(tok::r_square) &&
       P.SkipUntil(Close, FinalToken,
                   Parser::StopAtSemi | Parser::StopBeforeMatch) &&

lib/Sema/SemaDecl.cpp

static void checkModuleImportContext(Sema &S, Module *M,
                                      DeclContext *DC) {
   if (auto *LSD = dyn_cast<LinkageSpecDecl>(DC)) {
     switch (LSD->getLanguage()) {
     case LinkageSpecDecl::lang_c:
       if (!M->IsExternC) {
         S.Diag(ImportLoc, diag::err_module_import_in_extern_c)
           << M->getFullModuleName();
         S.Diag(LSD->getLocStart(), diag::note_module_import_in_extern_c);
+        S.Diag(ImportLoc, diag::note_module_need_top_level);
         return;
       }
       break;
     case LinkageSpecDecl::lang_cxx:
       break;
     }
     DC = LSD->getParent();
   }
 
   while (isa<LinkageSpecDecl>(DC))
     DC = DC->getParent();
   if (!isa<TranslationUnitDecl>(DC)) {
+    if (DC->isNamespace()) {
       S.Diag(ImportLoc, diag::err_module_import_not_at_top_level)
         << 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;
+      S.Diag(ImportLoc, diag::note_module_need_top_level);
+    } else {
+      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;
+      S.Diag(ImportLoc, diag::note_module_need_textual);
     }
   }
+}
+
+void Sema::diagnoseMisplacedModuleImport(Module *M, SourceLocation ImportLoc) {
+  return checkModuleImportContext(*this, M, ImportLoc, CurContext);
+}
 
 DeclResult Sema::ActOnModuleImport(SourceLocation AtLoc, 
                                    SourceLocation ImportLoc, 
                                    ModuleIdPath Path) {
   Module *Mod =
       getModuleLoader().loadModule(ImportLoc, Path, Module::AllVisible,
                                    /*IsIncludeDirective=*/false);
   if (!Mod)

test/Modules/auto-module-import.m

 // not actually part of the module.
 #include <Module/NotInModule.h> // expected-warning{{treating #include as an import of module 'Module.NotInModule'}} \
   // expected-warning{{missing submodule 'Module.NotInModule'}}
 
 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'}} \
+             expected-note {{consider marking the module as textual}}
+}

test/Modules/extern_c.cpp

 #define HEADER "c-header.h"
 #endif
 
 #include HEADER
 
 #if defined(EXTERN_C) && !defined(EXTERN_CXX) && defined(CXX_HEADER)
 // expected-error@-3 {{import of C++ module 'cxx_library' appears within extern "C" language linkage specification}}
 // expected-note@-17 {{extern "C" language linkage specification begins here}}
+// expected-note@-5 {{consider moving the import to top level}}
 #elif defined(NAMESPACE)
-// expected-error-re@-6 {{import of module '{{c_library.inner|cxx_library}}' appears within namespace 'M'}}
-// expected-note@-24 {{namespace 'M' begins here}}
+// expected-error-re@-7 {{import of module '{{c_library.inner|cxx_library}}' appears within namespace 'M'}}
+// expected-note@-25 {{namespace 'M' begins here}}
+// expected-note@-9 {{consider moving the import to top level}}
 #endif
 
 #ifdef EXTERN_CXX
 }
 #endif
 
 #ifdef EXTERN_C
 }

test/Modules/malformed.cpp

 // This test explicitly cd's to the test/Modules directory so that we can test
 // that filenames found via relative -I paths are printed correctly.
 //
 // RUN: rm -rf %t
 // RUN: cd %S
 // RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I Inputs/malformed -DHEADER="a1.h" %s 2>&1 | FileCheck %s --check-prefix=CHECK-A
 // RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I Inputs/malformed -DHEADER="b1.h" %s 2>&1 | FileCheck %s --check-prefix=CHECK-B
 // RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I Inputs/malformed -DHEADER="c.h" malformed.cpp 2>&1 | FileCheck %s --check-prefix=CHECK-C
 
 #define STR2(x) #x
 #define STR(x) STR2(x)
 #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 the 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.
 //
 // CHECK-C: In module 'c' imported from malformed.cpp:12:
 // CHECK-C: {{^}}Inputs/malformed/c.h:1:33: error: type 'int' cannot be used prior to '::'
 // CHECK-C: {{^}}malformed.cpp:[[@LINE-5]]:15: note: in instantiation of

test/Modules/misplaced.cpp

+// 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'}} \
+                    // expected-note{{consider moving the import to top level}}
+}
+
+void func1() {  // expected-note{{function 'func1' begins here}}
+#include "dummy.h"  // expected-error{{import of module 'dummy' appears within function 'func1'}} \
+                    // expected-note{{consider marking the module as textual}}
+}