Author: Qiongsi Wu Date: 2026-04-08T14:22:07-07:00 New Revision: 69e0367e8221b8002b5d438fb70ff3daf36257fc
URL: https://github.com/llvm/llvm-project/commit/69e0367e8221b8002b5d438fb70ff3daf36257fc DIFF: https://github.com/llvm/llvm-project/commit/69e0367e8221b8002b5d438fb70ff3daf36257fc.diff LOG: [clang][Modules] Diagnosing Module Redefinition Across ModuleMaps (#190085) This PR enhances the module redefinition diagnostic to cover a very specific case. 1. A module, say `B`, is discovered first during header search. In other words, it is declared in a modulemap that shows up first on the search paths. 2. `B` is declared again in a different modulemap, which shows up in a later search path, and the compiler discovers `B` again when it is searching for a different named module. See the two tests added for examples of this specific scenario. Under such a scenario, the compiler now reports the module redefined error. Note that we are not diagnosing duplicating module definitions globally, because that requires looking through all search paths and loading all module maps, which is too expensive. Assisted-by: claude-opus-4.6 Added: clang/test/ClangScanDeps/modules-byname-dup-module-decl-diag.c clang/test/ClangScanDeps/modules-mmap-redef.c clang/test/Modules/implicit-module-redefinition-same-file.c clang/test/Modules/implicit-module-redefinition.c Modified: clang/lib/Lex/ModuleMap.cpp clang/lib/Tooling/DependencyScanningTool.cpp Removed: ################################################################################ diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp index 6c991430cb08b..c9c90d41ee55f 100644 --- a/clang/lib/Lex/ModuleMap.cpp +++ b/clang/lib/Lex/ModuleMap.cpp @@ -1756,9 +1756,20 @@ void ModuleMapLoader::handleModuleDecl(const modulemap::ModuleDecl &MD) { if (Module *Existing = Map.lookupModuleQualified(ModuleName, ActiveModule)) { // We might see a (re)definition of a module that we already have a // definition for in four cases: - // - If we loaded one definition from an AST file and we've just found a - // corresponding definition in a module map file, or + // - If the Existing module was loaded from an AST file and we've found its + // original source module map, or bool LoadedFromASTFile = Existing->IsFromModuleFile; + if (LoadedFromASTFile) { + OptionalFileEntryRef ExistingModMapFile = + Map.getContainingModuleMapFile(Existing); + OptionalFileEntryRef CurrentModMapFile = + SourceMgr.getFileEntryRefForID(ModuleMapFID); + if (ExistingModMapFile && CurrentModMapFile && + *ExistingModMapFile == *CurrentModMapFile) + LoadedFromASTFile = true; + else + LoadedFromASTFile = false; + } // - If we previously inferred this module from diff erent module map file. bool Inferred = Existing->IsInferred; // - If we're building a framework that vends a module map, we might've diff --git a/clang/lib/Tooling/DependencyScanningTool.cpp b/clang/lib/Tooling/DependencyScanningTool.cpp index d1f8805c6b24b..1d4ab579f5827 100644 --- a/clang/lib/Tooling/DependencyScanningTool.cpp +++ b/clang/lib/Tooling/DependencyScanningTool.cpp @@ -607,6 +607,9 @@ bool CompilerInstanceWithContext::computeDependencies( if (!ModResult) return false; + if (CI.getDiagnostics().hasErrorOccurred()) + return false; + MDC->applyDiscoveredDependencies(ModuleInvocation); if (!Controller.finalize(CI, ModuleInvocation)) diff --git a/clang/test/ClangScanDeps/modules-byname-dup-module-decl-diag.c b/clang/test/ClangScanDeps/modules-byname-dup-module-decl-diag.c new file mode 100644 index 0000000000000..5800fcb7a45e6 --- /dev/null +++ b/clang/test/ClangScanDeps/modules-byname-dup-module-decl-diag.c @@ -0,0 +1,65 @@ +// Test duplicating module decls discovered during by-name module scanning with +// a shared compiler instance. +// This tests covers the case where the current modulemap we are loading +// contains a module decl that satisfies two conditions: +// 1. The compiler has seen the module decl during previous lookups. +// 2. The previous decl comes from a diff erent modulemap. +// In this case, an error is produced with no dependency information returned. +// Specifically, we have the following setup: +// - "A" is a framework module whose modulemap also declares an empty "B". +// - A separate include path has its own B/module.modulemap that declares "B" +// with a header depending on module "Dep" +// We scan B first, and during A's scan, the compiler should report an error. + +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json + +// RUN: not clang-scan-deps -compilation-database %t/cdb.json -format \ +// RUN: experimental-full -module-names=B,A 2>&1 | \ +// RUN: sed 's:\\\\\?:/:g' | FileCheck -DPREFIX=%/t %s + +// CHECK: A.framework/Modules/module.modulemap:7:8: error: redefinition of module 'B' +// CHECK: include/B/module.modulemap:1:8: note: previously defined here + +//--- frameworks/A.framework/Modules/module.modulemap +framework module A [system] { + umbrella header "A.h" + export * + module * { export * } +} + +module B { + export * +} + +//--- frameworks/A.framework/Headers/A.h +// Framework A umbrella header +void a_func(void); + +//--- include/B/module.modulemap +module B { + header "B.h" + export * +} + +//--- include/B/B.h +#include "Dep.h" +void b_func(void); + +//--- include/Dep/module.modulemap +module Dep { + header "Dep.h" + export * +} + +//--- include/Dep/Dep.h +// Module Dep header +void dep_func(void); + +//--- cdb.json.template +[{ + "file": "", + "directory": "DIR", + "command": "clang -fmodules -fmodules-cache-path=DIR/cache -I DIR/include/B -F DIR/frameworks -I DIR/include/Dep -x c" +}] diff --git a/clang/test/ClangScanDeps/modules-mmap-redef.c b/clang/test/ClangScanDeps/modules-mmap-redef.c new file mode 100644 index 0000000000000..f21ba8dd417c9 --- /dev/null +++ b/clang/test/ClangScanDeps/modules-mmap-redef.c @@ -0,0 +1,39 @@ +// Test duplicating module definitions in the same modulemap. + +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json + +// RUN: not clang-scan-deps -compilation-database %t/cdb.json -format \ +// RUN: experimental-full -module-names=A 2>&1 | sed 's:\\\\\?:/:g' | \ +// RUN: FileCheck -DPREFIX=%/t %s + +// CHECK: include/A/module.modulemap:9:8: error: redefinition of module 'A' +// CHECK-NEXT: include/A/module.modulemap:1:8: note: previously defined here + +//--- include/A/module.modulemap +module A { + header "A.h" +} + +module A1 { + header "A1.h" +} + +module A{ + header "A.h" +} + +//--- include/A/A.h + +//--- include/A/A1.h + +//--- include/A/A2.h + + +//--- cdb.json.template +[{ + "file": "", + "directory": "DIR", + "command": "clang -fmodules -fmodules-cache-path=DIR/cache -I DIR/include/A -x c" +}] diff --git a/clang/test/Modules/implicit-module-redefinition-same-file.c b/clang/test/Modules/implicit-module-redefinition-same-file.c new file mode 100644 index 0000000000000..b229507c681ac --- /dev/null +++ b/clang/test/Modules/implicit-module-redefinition-same-file.c @@ -0,0 +1,36 @@ +// Test that implicit module builds diagnose redefinition of a module when the +// same modulemap file contains duplicate module declarations. + +// RUN: rm -rf %t +// RUN: split-file %s %t + +// RUN: not %clang_cc1 -x objective-c -fmodules -fimplicit-module-maps \ +// RUN: -I %t/include \ +// RUN: -fmodules-cache-path=%t/cache \ +// RUN: %t/test.m 2>&1 | FileCheck %s + +// CHECK: module.modulemap:9:8: error: redefinition of module 'A' +// CHECK: module.modulemap:1:8: note: previously defined here +// CHECK: fatal error: could not build module 'A' + +//--- include/module.modulemap +module A { + header "A.h" +} + +module A1 { + header "A1.h" +} + +module A { + header "A.h" +} + +//--- include/A.h +// empty + +//--- include/A1.h +// empty + +//--- test.m +@import A; diff --git a/clang/test/Modules/implicit-module-redefinition.c b/clang/test/Modules/implicit-module-redefinition.c new file mode 100644 index 0000000000000..d54e79d309943 --- /dev/null +++ b/clang/test/Modules/implicit-module-redefinition.c @@ -0,0 +1,56 @@ +// Test that implicit module builds diagnose redefinition of a module when two +// diff erent modulemaps define the same module name. +// +// Module "b" is defined in both first/module.modulemap and +// second/module.modulemap. + +// RUN: rm -rf %t +// RUN: split-file %s %t + +// We detect the error when we read a's modulemap in first +// and discovers another b, which we have seen earlier in +// second/module.modulemap. +// RUN: not %clang_cc1 -x objective-c -fmodules -fimplicit-module-maps \ +// RUN: -I %t/second -I %t/first \ +// RUN: -fmodules-cache-path=%t/cache \ +// RUN: %t/sourcefile.c 2>&1 | FileCheck %s + +// CHECK: first{{[/\\]}}module.modulemap:5:8: error: redefinition of module 'b' +// CHECK: second{{[/\\]}}module.modulemap:1:8: note: previously defined here + +// On the other hand, we do NOT detect error if we load a's modulemap first. +// Checking duplicating module decls in general is too expensive, since +// it requires loading all the modulemaps. The command below should succeed. +// RUN: %clang_cc1 -x objective-c -fmodules -fimplicit-module-maps \ +// RUN: -I %t/first -I %t/second \ +// RUN: -fmodules-cache-path=%t/cache \ +// RUN: %t/sourcefile.c + +//--- first/module.modulemap +module a { + header "a.h" +} + +module b { + export * +} + +//--- first/a.h +// empty + +//--- second/module.modulemap +module b { + header "b.h" +} + +//--- second/b.h +// empty + + +//--- sourcefile.c +@import b; +@import a; + +int main() { + return 0; +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
