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

[Serge Pavlov via cfe-commits]

sepavloff added a comment.

Thanks for helpful notes!

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


================
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">;
 }
----------------
rsmith wrote:
> I would drop the "the" in this error.
Done.

================
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">;
----------------
silvas wrote:
> rsmith wrote:
> > 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.
> I think this makes sense. `error: ... import ... appears within ...` seems like sufficient information about the nature of the error.
The message is removed.

================
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">;
 }
----------------
rsmith wrote:
> 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"
This message is also removed, as Sean proposed.

================
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)) {
----------------
rsmith wrote:
> Move this `try...` call to the end of the `&&` here and elsewhere; it's much more likely that we'll hit an `r_brace`.
Moved.

================
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() {
----------------
rsmith wrote:
> 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.
Meaning of the return value is inverted.

================
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:
----------------
rsmith wrote:
> 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).
Thank you for explanation. Fixed.

================
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 {
----------------
rsmith wrote:
> 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.
Yes, the messages were as you described. Now import inside a namespace is also a fatal error. Test `misplaced-4.cpp` checks this case.

================
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);
+    }
----------------
silvas wrote:
> rsmith wrote:
> > 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.
> 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).
As Sean proposed, these are not emitted.

================
Comment at: test/Modules/misplaced.cpp:1-12
@@ +1,12 @@
+// 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 header as textual}}
+}
----------------
silvas wrote:
> This test does not cover the `appears within class` case.
This case was checked in `malformed.cpp` but for the sake of consistency separate test was added.


http://reviews.llvm.org/D11844