[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Graydon Hoare via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL290219: [modules] Handle modules with nonstandard names in 
module.private.modulemaps (authored by graydon).

Changed prior to commit:
  https://reviews.llvm.org/D27852?vs=82161=82177#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27852

Files:
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
  cfe/trunk/include/clang/Lex/HeaderSearch.h
  cfe/trunk/lib/Lex/HeaderSearch.cpp
  cfe/trunk/lib/Lex/ModuleMap.cpp
  
cfe/trunk/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
  
cfe/trunk/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
  
cfe/trunk/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
  
cfe/trunk/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
  cfe/trunk/test/Modules/implicit-private-with-different-name.m

Index: cfe/trunk/lib/Lex/ModuleMap.cpp
===
--- cfe/trunk/lib/Lex/ModuleMap.cpp
+++ cfe/trunk/lib/Lex/ModuleMap.cpp
@@ -1490,6 +1490,42 @@
 ActiveModule->NoUndeclaredIncludes = true;
   ActiveModule->Directory = Directory;
 
+  if (!ActiveModule->Parent) {
+StringRef MapFileName(ModuleMapFile->getName());
+if (MapFileName.endswith("module.private.modulemap") ||
+MapFileName.endswith("module_private.map")) {
+  // Adding a top-level module from a private modulemap is likely a
+  // user error; we check to see if there's another top-level module
+  // defined in the non-private map in the same dir, and if so emit a
+  // warning.
+  for (auto E = Map.module_begin(); E != Map.module_end(); ++E) {
+auto const *M = E->getValue();
+if (!M->Parent &&
+M->Directory == ActiveModule->Directory &&
+M->Name != ActiveModule->Name) {
+  Diags.Report(ActiveModule->DefinitionLoc,
+   diag::warn_mmap_mismatched_top_level_private)
+<< ActiveModule->Name << M->Name;
+  // The pattern we're defending against here is typically due to
+  // a module named FooPrivate which is supposed to be a submodule
+  // called Foo.Private. Emit a fixit in that case.
+  auto D =
+Diags.Report(ActiveModule->DefinitionLoc,
+ diag::note_mmap_rename_top_level_private_as_submodule);
+  D << ActiveModule->Name << M->Name;
+  StringRef Bad(ActiveModule->Name);
+  if (Bad.consume_back("Private")) {
+SmallString<128> Fixed = Bad;
+Fixed.append(".Private");
+D << FixItHint::CreateReplacement(ActiveModule->DefinitionLoc,
+  Fixed);
+  }
+  break;
+}
+  }
+}
+  }
+
   bool Done = false;
   do {
 switch (Tok.Kind) {
Index: cfe/trunk/lib/Lex/HeaderSearch.cpp
===
--- cfe/trunk/lib/Lex/HeaderSearch.cpp
+++ cfe/trunk/lib/Lex/HeaderSearch.cpp
@@ -194,16 +194,36 @@
   Module *Module = ModMap.findModule(ModuleName);
   if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps)
 return Module;
-  
+
+  StringRef SearchName = ModuleName;
+  Module = lookupModule(ModuleName, SearchName);
+
+  // The facility for "private modules" -- adjacent, optional module maps named
+  // module.private.modulemap that are supposed to define private submodules --
+  // is sometimes misused by frameworks that name their associated private
+  // module FooPrivate, rather than as a submodule named Foo.Private as
+  // intended. Here we compensate for such cases by looking in directories named
+  // Foo.framework, when we previously looked and failed to find a
+  // FooPrivate.framework.
+  if (!Module && SearchName.consume_back("Private"))
+Module = lookupModule(ModuleName, SearchName);
+  return Module;
+}
+
+Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName) {
+  Module *Module = nullptr;
+
   // Look through the various header search paths to load any available module
   // maps, searching for a module map that describes this module.
   for (unsigned Idx = 0, N = SearchDirs.size(); Idx != N; ++Idx) {
 if (SearchDirs[Idx].isFramework()) {
-  // Search for or infer a module map for a framework.
+  // Search for or infer a module map for a framework. Here we use
+  // SearchName rather than ModuleName, to permit finding private modules
+  // named FooPrivate in buggy frameworks named Foo.
   SmallString<128> FrameworkDirName;
   FrameworkDirName += SearchDirs[Idx].getFrameworkDir()->getName();
-  llvm::sys::path::append(FrameworkDirName, ModuleName + ".framework");
-  if (const DirectoryEntry *FrameworkDir 
+  

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno accepted this revision.
bruno added a comment.

Right, I missed it, LGTM


https://reviews.llvm.org/D27852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Graydon Hoare via Phabricator via cfe-commits]

graydon added inline comments.



Comment at: test/Modules/implicit-private-with-different-name.m:13
+// 
expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{top-level
 module 'APrivate' in private module map, expected a submodule of 'A'}}
+// 
expected-note@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{make
 'APrivate' a submodule of 'A' to ensure it can be found by name}}
+// CHECK: 
fix-it:"{{.*}}/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap":{1:18-1:26}:"A.Private"

bruno wrote:
> I think we can enhance the usability a bit here, how about:
> 
> warning: top-level module 'APrivate' in private module map, expected a 
> submodule of 'A'
> note: make 'APrivate' a submodule of 'A' to ensure it can be found by name
>   framework module APrivate {
>^
> A.Private
> 
> 
I'm not sure what you're requesting; as far as I can see that is exactly the 
diagnostic I'm emitting (along with the fixit)


https://reviews.llvm.org/D27852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a reviewer: bruno.
bruno added inline comments.



Comment at: test/Modules/implicit-private-with-different-name.m:13
+// 
expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{top-level
 module 'APrivate' in private module map, expected a submodule of 'A'}}
+// 
expected-note@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{make
 'APrivate' a submodule of 'A' to ensure it can be found by name}}
+// CHECK: 
fix-it:"{{.*}}/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap":{1:18-1:26}:"A.Private"

I think we can enhance the usability a bit here, how about:

warning: top-level module 'APrivate' in private module map, expected a 
submodule of 'A'
note: make 'APrivate' a submodule of 'A' to ensure it can be found by name
  framework module APrivate {
   ^
A.Private




https://reviews.llvm.org/D27852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Graydon Hoare via Phabricator via cfe-commits]

graydon updated this revision to Diff 82161.
graydon added a comment.

Updates to address review comments


https://reviews.llvm.org/D27852

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticLexKinds.td
  include/clang/Lex/HeaderSearch.h
  lib/Lex/HeaderSearch.cpp
  lib/Lex/ModuleMap.cpp
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
  test/Modules/implicit-private-with-different-name.m

Index: test/Modules/implicit-private-with-different-name.m
===
--- /dev/null
+++ test/Modules/implicit-private-with-different-name.m
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t
+
+// Build PCH using A, with adjacent private module APrivate, which winds up being implicitly referenced
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -emit-pch -o %t-A.pch %s
+
+// Use the PCH with no explicit way to resolve PrivateA, still pick it up through MODULE_DIRECTORY reference in PCH control block
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only
+
+// Check the fixit
+// RUN: %clang_cc1  -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{top-level module 'APrivate' in private module map, expected a submodule of 'A'}}
+// expected-note@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{make 'APrivate' a submodule of 'A' to ensure it can be found by name}}
+// CHECK: fix-it:"{{.*}}/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap":{1:18-1:26}:"A.Private"
+
+#ifndef HEADER
+#define HEADER
+#import "A/aprivate.h"
+const int *y = 
+#endif
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
@@ -0,0 +1,4 @@
+framework module APrivate {
+  header "aprivate.h"
+  export *
+}
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module A {
+  header "a.h"
+  export *
+}
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
@@ -0,0 +1 @@
+extern int APRIVATE;
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
@@ -0,0 +1 @@
+extern int APUBLIC;
Index: lib/Lex/ModuleMap.cpp
===
--- lib/Lex/ModuleMap.cpp
+++ lib/Lex/ModuleMap.cpp
@@ -1490,6 +1490,42 @@
 ActiveModule->NoUndeclaredIncludes = true;
   ActiveModule->Directory = Directory;
 
+  if (!ActiveModule->Parent) {
+StringRef MapFileName(ModuleMapFile->getName());
+if (MapFileName.endswith("module.private.modulemap") ||
+MapFileName.endswith("module_private.map")) {
+  // Adding a top-level module from a private modulemap is likely a
+  // user error; we check to see if there's another top-level module
+  // defined in the non-private map in the same dir, and if so emit a
+  // warning.
+  for (auto E = Map.module_begin(); E != Map.module_end(); ++E) {
+auto const *M = E->getValue();
+if (!M->Parent &&
+M->Directory == ActiveModule->Directory &&
+M->Name != ActiveModule->Name) {
+  Diags.Report(ActiveModule->DefinitionLoc,
+   diag::warn_mmap_mismatched_top_level_private)
+<< ActiveModule->Name << M->Name;
+  // The pattern we're defending against here is typically due to
+  // a module named FooPrivate which is supposed to be a submodule
+ 

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Graydon Hoare via Phabricator via cfe-commits]

graydon marked 5 inline comments as done.
graydon added a comment.

Addressed review comments


https://reviews.llvm.org/D27852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi,

Thanks for working on this.




Comment at: include/clang/Basic/DiagnosticLexKinds.td:647
+  "top-level module '%0' in private module map, expected a submodule of '%1'">,
+  InGroup>;
 

It would be nice if we could also emit a FixIt to tell the user how should the 
modulemap be fixed.



Comment at: lib/Lex/HeaderSearch.cpp:208
+  // FooPrivate.framework.
+  if (!Module && SearchName.consume_back("Private")) {
+Module = lookupModule(ModuleName, SearchName);

Remove the curly braces here



Comment at: lib/Lex/HeaderSearch.cpp:211
+  }
+
+  return Module;

Remove this newline



Comment at: lib/Lex/HeaderSearch.cpp:216
+Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName) 
{
+
+  Module *Module = nullptr;

Remove this newline



Comment at: test/Modules/implicit-private-with-different-name.m:9
+
+// 
expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{expected
 a submodule}}
+

Can you check for the entire warning message?


https://reviews.llvm.org/D27852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Manman Ren via Phabricator via cfe-commits]

manmanren accepted this revision.
manmanren added a comment.
This revision is now accepted and ready to land.

Thanks,
Manman


https://reviews.llvm.org/D27852



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Graydon Hoare via Phabricator via cfe-commits]

graydon created this revision.
graydon added reviewers: manmanren, doug.gregor.
graydon added a subscriber: cfe-commits.

The module system supports accompanying a primary module (say Foo) with
an auxiliary "private" module (defined in an adjacent module.private.modulemap
file) that augments the primary module when associated private headers are
available. The feature is intended to be used to augment the primary
module with a submodule (say Foo.Private), however some users in the wild
are choosing to augment the primary module with an additional top-level module
with a "similar" name (in all cases so far: FooPrivate).

This "works" when a user of the module initially imports a private header,
such as '#import "Foo/something_private.h"' since the Foo import winds up
importing FooPrivate in passing. But if the import is subsequently recorded
in a PCH file, reloading the PCH will fail to validate because of a cross-check
that attempts to find the module.modulemap (or module.private.modulemap) using
HeaderSearch algorithm, applied to the "FooPrivate" name. Since it's stored in
Foo.framework/Modules, not FooPrivate.framework/Modules, the check fails and
the PCH is rejected.

This patch adds a compensatory workaround in the HeaderSearch algorithm
when searching (and failing to find) a module of the form FooPrivate: the
name used to derive filesystem paths is decoupled from the module name
being searched for, and if the initial search fails and the module is
named "FooPrivate", the filesystem search name is altered to remove the
"Private" suffix, and the algorithm is run a second time (still looking for
a module named FooPrivate, but looking in directories derived from Foo).

Accompanying this change is a new warning that triggers when a user loads
a module.private.modulemap that defines a top-level module with a different
name from the top-level module defined in its adjacent module.modulemap.


https://reviews.llvm.org/D27852

Files:
  include/clang/Basic/DiagnosticLexKinds.td
  include/clang/Lex/HeaderSearch.h
  lib/Lex/HeaderSearch.cpp
  lib/Lex/ModuleMap.cpp
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
  test/Modules/implicit-private-with-different-name.m

Index: test/Modules/implicit-private-with-different-name.m
===
--- /dev/null
+++ test/Modules/implicit-private-with-different-name.m
@@ -0,0 +1,15 @@
+// RUN: rm -rf %t
+
+// Build PCH using A, with adjacent private module APrivate, which winds up being implicitly referenced
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -emit-pch -o %t-A.pch %s
+
+// Use the PCH with no explicit way to resolve PrivateA, still pick it up through MODULE_DIRECTORY reference in PCH control block
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only
+
+// expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{expected a submodule}}
+
+#ifndef HEADER
+#define HEADER
+#import "A/aprivate.h"
+const int *y = 
+#endif
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
@@ -0,0 +1,4 @@
+framework module APrivate {
+  header "aprivate.h"
+  export *
+}
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module A {
+  header "a.h"
+  export *
+}
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
@@ -0,0 +1 @@
+extern int APRIVATE;
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
@@ -0,0 +1 @@
+extern int APUBLIC;
Index: lib/Lex/ModuleMap.cpp
===
---