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

[Sean Silva via cfe-commits]

silvas requested changes to this revision.
silvas added a reviewer: silvas.
This revision now requires changes to proceed.

================
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">;
----------------
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.

================
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);
+    }
----------------
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).

================
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}}
+}
----------------
This test does not cover the `appears within class` case.


http://reviews.llvm.org/D11844