[clang][module] Improve incomplete-umbrella warning

[zixuw]

Summary

• Change the warning message for -Wincomplete-umbrella to report the location of the umbrella header;

[zixuw]

Ping

[vsapsai]

Didn't get into details but overall GenModuleActionWrapper approach looks pretty complicated. Haven't tried to accomplish the same myself, so cannot say if such complexity is warranted or not. What happens with fixits in modules when you don't have GenModuleActionWrapper? How does it work with modules disabled? Also looks like there are no tests for incomplete umbrella warning when modules are disabled.

[zixuw]

In D82118#2154280, @vsapsai wrote:

Didn't get into details but overall GenModuleActionWrapper approach looks pretty complicated. Haven't tried to accomplish the same myself, so cannot say if such complexity is warranted or not.

Yes the approach is complicated. But one thing to note is that the GenModuleActionWrapper-related facilities are upstreamed from apple/llvm-project. I adopted this approach just because it was available. Haven't explored other ways, but yes there might be a simpler approach.

What happens with fixits in modules when you don't have GenModuleActionWrapper?

If the fixit action is not forwarded to the new compiler instance, it will be handled by the 'outer' compiler instance, which has a different source manager that does not see the module files, so the source location cannot be correctly interpreted. From my experiments, the fixit hint is trying to write into the <builtin> file buffer in this test case, when I don't have the action forwarded using GenModuleActionWrapper. In this case the fixit rewriter quietly fails because <builtin> cannot be rewritten, but since this is not caught I think there might be cases where the wrong file ends up getting updated.

How does it work with modules disabled? Also looks like there are no tests for incomplete umbrella warning when modules are disabled.

Haven't tested this, will give a try.

[vsapsai]

In D82118#2154310, @zixuw wrote:

In D82118#2154280, @vsapsai wrote:

Didn't get into details but overall GenModuleActionWrapper approach looks pretty complicated. Haven't tried to accomplish the same myself, so cannot say if such complexity is warranted or not.

Yes the approach is complicated. But one thing to note is that the GenModuleActionWrapper-related facilities are upstreamed from apple/llvm-project. I adopted this approach just because it was available. Haven't explored other ways, but yes there might be a simpler approach.

My concern about this approach is that it doesn't seem to be composable and propagating specific frontend actions to building modules looks ad-hoc and error-prone. Specifically, from the composition perspective what should we do when both indexing and fix-it are enabled? We can claim they should be mutually exclusive but don't know if it is applicable in general case. From the perspective of propagating other frontend actions to building modules I'm wondering if we need to have custom handling for other actions or not. Personally, I haven't checked that yet.

What happens with fixits in modules when you don't have GenModuleActionWrapper?

If the fixit action is not forwarded to the new compiler instance, it will be handled by the 'outer' compiler instance, which has a different source manager that does not see the module files, so the source location cannot be correctly interpreted. From my experiments, the fixit hint is trying to write into the <builtin> file buffer in this test case, when I don't have the action forwarded using GenModuleActionWrapper. In this case the fixit rewriter quietly fails because <builtin> cannot be rewritten, but since this is not caught I think there might be cases where the wrong file ends up getting updated.

That doesn't sound good. Though making a source manager support multiple compiler instances doesn't look like an easy task (and maybe not worthwhile).

[bruno]

Hi Zixu, thanks for working on improving this.

I agree with @vsapsai on the the GenModuleActionWrapper approach. Also, it seems to me that even though it would somehow improve the accuracy, it would be solving a more general problem, not really specific to this patch. What about CHECKs for the introduced fix-its? I don't see a #include or #import being matched in the tests.

clang/lib/Lex/PPLexerChange.cpp

301

Is it possible to incorporate the string selection as part of the diagnostic description in the .td file?

[zixuw]

Hey Bruno! Thanks for the review.

In D82118#2224423, @bruno wrote:

Hi Zixu, thanks for working on improving this.

I agree with @vsapsai on the the GenModuleActionWrapper approach. Also, it seems to me that even though it would somehow improve the accuracy, it would be solving a more general problem, not really specific to this patch.

Yes the wrapper is definitely problematic. I'm checking how the diagnostic itself is handled correctly and start from there when I have time. For now would it be better to separate this into multiple patches and get the diagnostic improvement in first?

clang/test/Modules/Inputs/incomplete-umbrella/normal-module/Umbrella.h

1–3 ↗

(On Diff #271810)

What about CHECKs for the introduced fix-its? I don't see a #include or #import being matched in the tests.

Is this the check you're looking for? It is checked in incomplete-umbrella-fixit.m:

RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t -Wno-everything -Wincomplete-umbrella -fixit %s && FileCheck %t/Umbrella.h --input-file %t/Umbrella.h --match-full-lines

[bruno]

Yes the wrapper is definitely problematic. I'm checking how the diagnostic itself is handled correctly and start from there when I have time. For now would it be better to separate this into multiple patches and get the diagnostic improvement in first?

Yep, that would be better!

clang/test/Modules/Inputs/incomplete-umbrella/normal-module/Umbrella.h

1–3 ↗

(On Diff #271810)

Oh, right. my bad! Can we get a test for #include too?

[zixuw]

Okay so I think I traced down to the root problem: the diagnostic message prints out fine because TextDiagnosticPrinter uses FullSourceLoc, which actually embeds an appropriate SourceManager within it:

TextDiag->emitDiagnostic(
    FullSourceLoc(Info.getLocation(), Info.getSourceManager()), Level,
    DiagMessageStream.str(), Info.getRanges(), Info.getFixItHints());

However, FixItHint is handled by a different path: a FixItHint only stores a trivial location (a SourceRange) which does not embed a SourceManager. And when a FixItRewriter constructs, it initializes a bunch of helper objects (Editor, Rewriter, Commit, etc.) using the current SourceManager. When trying to commit a fix-it hint, the helper objects use their (const ref to) SourceManager, which is used by the main compilation, to resolve the plain location in the FixItHint, which is generated in the new compiler instance for building a module. Therefore the location resolution is wrong.
To fix this, we can make FixItHint use FullSourceLoc instead, and also modify all relevant parties (FixItRewriter, EditedSource, Rewriter, Commit, ...) to use the embedded SourceManager instead of having a const ref to one initialized SourceManager.
My main concern is that I'm not very familiar with all the rewrite facilities, and changing EditedSource etc. might affect other users of the facilities.
What do you think? @arphaman @bruno @vsapsai

[zixuw]

Separate warning message improvement and fix-it hint.

[zixuw]

@arphaman @vsapsai @bruno
This diff has been sitting here for too long. I've separated out the fix-it hint part (working on implementing a 'FullSourceRange' to be used by FixItHint now) and this is just a concise change for the warning message now. Does it look good?

[vsapsai]

Noticed that the warning and the fix-it might not work well with pragmas suppressing diagnostic and with header guards. But it's not a regression and I don't think it is worth improving these use cases preemptively.

clang/lib/Lex/PPLexerChange.cpp

266–271

I was going to suggest to spell out the type explicitly instead of auto because I wasn't able to guess the type. But when I've checked, Module::Header didn't look particularly better. I'll leave it up to you to decide which one is more readable, for wider context there was a discussion RFC: Modernizing our use of auto.

269

Can we rename EndLoc to better convey its purpose, not how it was computed? I was thinking about something like SuggestedHeadersLoc or ExpectedHeadersLoc.

[zixuw]

Address review:

• Use explicit type name Module::Header instead of auto since it's a short typename and using auto does not improve readability;
• Change variable EndLoc to ExpectedHeadersLoc so it's self-explanatory.

[vsapsai]

Looks good to me. I would wait a few days before committing in case other reviewers have comments but not too long as you can address extra comments post-commit.

clang/lib/Lex/PPLexerChange.cpp

static void collectAllSubModulesWithUmbrellaHeader(
     const Module &Mod, SmallVectorImpl<const Module *> &SubMods) {
   if (Mod.getUmbrellaHeader())
     SubMods.push_back(&Mod);
   for (auto *M : Mod.submodules())
     collectAllSubModulesWithUmbrellaHeader(*M, SubMods);
 }
 
 void Preprocessor::diagnoseMissingHeaderInUmbrellaDir(const Module &Mod) {
-  assert(Mod.getUmbrellaHeader() && "Module must use umbrella header");
-  SourceLocation StartLoc =
-      SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID());
-  if (getDiagnostics().isIgnored(diag::warn_uncovered_module_header, StartLoc))
+  const Module::Header &UmbrellaHeader = Mod.getUmbrellaHeader();
+  assert(UmbrellaHeader.Entry && "Module must use umbrella header");
+  const FileID &File = SourceMgr.translateFile(UmbrellaHeader.Entry);
+  SourceLocation ExpectedHeadersLoc = SourceMgr.getLocForEndOfFile(File);
+  if (getDiagnostics().isIgnored(diag::warn_uncovered_module_header,
+                                 ExpectedHeadersLoc))
     return;
 
   ModuleMap &ModMap = getHeaderSearchInfo().getModuleMap();
   const DirectoryEntry *Dir = Mod.getUmbrellaDir().Entry;
   llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
   std::error_code EC;
   for (llvm::vfs::recursive_directory_iterator Entry(FS, Dir->getName(), EC),
        End;
        Entry != End && !EC; Entry.increment(EC)) {
     using llvm::StringSwitch;
 
     // Check whether this entry has an extension typically associated with
     // headers.
     if (!StringSwitch<bool>(llvm::sys::path::extension(Entry->path()))
              .Cases(".h", ".H", ".hh", ".hpp", true)
              .Default(false))
       continue;
 
     if (auto Header = getFileManager().getFile(Entry->path()))
       if (!getSourceManager().hasFileInfo(*Header)) {
         if (!ModMap.isHeaderInUnavailableModule(*Header)) {
           // Find the relative path that would access this header.
           SmallString<128> RelativePath;
           computeRelativePath(FileMgr, Dir, *Header, RelativePath);
-          Diag(StartLoc, diag::warn_uncovered_module_header)
+          Diag(ExpectedHeadersLoc, diag::warn_uncovered_module_header)
               << Mod.getFullModuleName() << RelativePath;
         }
       }
   }
 }
 
 /// HandleEndOfFile - This callback is invoked when the lexer hits the end of
 /// the current file.  This either returns the EOF token or pops a level off
 /// the include stack and keeps going.
 bool Preprocessor::HandleEndOfFile(Token &Result, bool isEndOfMacro) {
   assert(!CurTokenLexer &&
          "Ending a file when currently in a macro!");
 

clang/test/Modules/incomplete-umbrella.m

 // RUN: rm -rf %t
 // RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F%S/Inputs/incomplete-umbrella -fsyntax-only %s 2>&1 | FileCheck %s
 
 #import <Foo/Foo.h>
 #import <Foo/Bar.h>
 #import <Foo/Baz.h>
 @import Foo.Private;
 
-// CHECK: warning: umbrella header for module 'Foo' does not include header 'Bar.h'
-// CHECK: warning: umbrella header for module 'Foo.Private' does not include header 'Baz.h'
+// CHECK: While building module 'Foo' imported from {{.*[/\]}}incomplete-umbrella.m:4:
+// CHECK-NEXT: In file included from <module-includes>:1:
+// CHECK-NEXT: {{.*Foo[.]framework[/\]Headers[/\]}}FooPublic.h:2:1: warning: umbrella header for module 'Foo' does not include header 'Bar.h'
+// CHECK: While building module 'Foo' imported from {{.*[/\]}}incomplete-umbrella.m:4:
+// CHECK-NEXT: In file included from <module-includes>:2:
+// CHECK-NEXT: {{.*Foo[.]framework[/\]PrivateHeaders[/\]}}Foo.h:2:1: warning: umbrella header for module 'Foo.Private' does not include header 'Baz.h'
 int foo() {
   int a = BAR_PUBLIC;
   int b = BAZ_PRIVATE;
   return 0;
 }