[PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive

[Richard Smith via cfe-commits]

rsmith added a comment.

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.


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1016
@@ -1015,2 +1015,3 @@
   "expected ';' after module name">;
+def err_missing_before_module_end : Error<"expected %0 at the end of module">;
 }
----------------
I would drop the "the" in this error.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7774
@@ -7771,1 +7773,3 @@
   "@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 header as textual">;
----------------
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.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7775
@@ -7772,1 +7774,3 @@
+def note_module_need_top_level : Note<"consider moving the import to top level">;
+def note_module_need_textual : Note<"consider marking the header as textual">;
 }
----------------
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"

================
Comment at: lib/Parse/ParseDeclCXX.cpp:213
@@ -212,2 +212,3 @@
   if (index == Ident.size()) {
-    while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
+    while (tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
+           Tok.isNot(tok::eof)) {
----------------
Move this `try...` call to the end of the `&&` here and elsewhere; it's much more likely that we'll hit an `r_brace`.

================
Comment at: lib/Parse/Parser.cpp:2023-2024
@@ +2022,4 @@
+/// 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() {
----------------
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.

================
Comment at: lib/Parse/Parser.cpp:2025-2027
@@ +2024,5 @@
+/// 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:
----------------
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).

================
Comment at: lib/Sema/SemaDecl.cpp:14361-14366
@@ -14359,6 +14360,8 @@
   if (!isa<TranslationUnitDecl>(DC)) {
-    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;
+    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;
+      S.Diag(ImportLoc, diag::note_module_need_top_level);
+    } else {
----------------
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.

================
Comment at: lib/Sema/SemaDecl.cpp:14372
@@ +14371,3 @@
+             diag::note_module_import_not_at_top_level) << DC;
+      S.Diag(ImportLoc, diag::note_module_need_textual);
+    }
----------------
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.


http://reviews.llvm.org/D11844