-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang] Support header shadowing diagnostics in Clang header search #162491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[clang] Support header shadowing diagnostics in Clang header search #162491
Conversation
@llvm/pr-subscribers-clang Author: Jinjie Huang (Jinjie-Huang) ChangesWhen including a header file, multiple files with the same name may exist across different search paths. Therefore, this patch tries to provide a diagnostic message without changing the outcome of the header selection. It does this by performing an additional search for duplicate filenames across all search paths (both MSVC rules and standard paths). This informs the user about a potential "header shadowing" issue and clarifies which header path was actually used. Full diff: https://github.com/llvm/llvm-project/pull/162491.diff 6 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 0c994e0b5ca4d..b08adc59c9a8a 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -789,6 +789,8 @@ def ShadowFieldInConstructor : DiagGroup<"shadow-field-in-constructor",
def ShadowIvar : DiagGroup<"shadow-ivar">;
def ShadowUncapturedLocal : DiagGroup<"shadow-uncaptured-local">;
+def HeaderShadowing : DiagGroup<"header-shadowing">;
+
// -Wshadow-all is a catch-all for all shadowing. -Wshadow is just the
// shadowing that we think is unsafe.
def Shadow : DiagGroup<"shadow", [ShadowFieldInConstructorModified,
diff --git a/clang/include/clang/Basic/DiagnosticLexKinds.td b/clang/include/clang/Basic/DiagnosticLexKinds.td
index c7fe6e1db6d1f..8057e0e95e187 100644
--- a/clang/include/clang/Basic/DiagnosticLexKinds.td
+++ b/clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -951,6 +951,10 @@ def warn_quoted_include_in_framework_header : Warning<
def warn_framework_include_private_from_public : Warning<
"public framework header includes private framework header '%0'"
>, InGroup<FrameworkIncludePrivateFromPublic>;
+def warn_header_shadowed
+ : Warning<"multiple candidates for header '%0' found; "
+ "using the one from '%1', shadowed by '%2'">,
+ InGroup<HeaderShadowing>, DefaultIgnore;
def warn_deprecated_module_dot_map : Warning<
"'%0' as a module map name is deprecated, rename it to %select{module.modulemap|module.private.modulemap}1%select{| in the 'Modules' directory of the framework}2">,
InGroup<DeprecatedModuleDotMap>;
diff --git a/clang/include/clang/Lex/DirectoryLookup.h b/clang/include/clang/Lex/DirectoryLookup.h
index bb703dfad2b28..45c7360385a73 100644
--- a/clang/include/clang/Lex/DirectoryLookup.h
+++ b/clang/include/clang/Lex/DirectoryLookup.h
@@ -181,7 +181,7 @@ class DirectoryLookup {
ModuleMap::KnownHeader *SuggestedModule,
bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound,
bool &IsInHeaderMap, SmallVectorImpl<char> &MappedName,
- bool OpenFile = true) const;
+ bool NeedSuggest, bool OpenFile = true) const;
private:
OptionalFileEntryRef DoFrameworkLookup(
diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h
index 850aea41c4c3b..83f6e11cd6ddc 100644
--- a/clang/include/clang/Lex/HeaderSearch.h
+++ b/clang/include/clang/Lex/HeaderSearch.h
@@ -809,12 +809,11 @@ class HeaderSearch {
/// Look up the file with the specified name and determine its owning
/// module.
- OptionalFileEntryRef
- getFileAndSuggestModule(StringRef FileName, SourceLocation IncludeLoc,
- const DirectoryEntry *Dir, bool IsSystemHeaderDir,
- Module *RequestingModule,
- ModuleMap::KnownHeader *SuggestedModule,
- bool OpenFile = true, bool CacheFailures = true);
+ OptionalFileEntryRef getFileAndSuggestModule(
+ StringRef FileName, SourceLocation IncludeLoc, const DirectoryEntry *Dir,
+ bool IsSystemHeaderDir, Module *RequestingModule,
+ ModuleMap::KnownHeader *SuggestedModule, bool NeedSuggest,
+ bool OpenFile = true, bool CacheFailures = true);
/// Cache the result of a successful lookup at the given include location
/// using the search path at \c HitIt.
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 238c5e2f2d9a5..fe11be5555977 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -444,8 +444,8 @@ StringRef DirectoryLookup::getName() const {
OptionalFileEntryRef HeaderSearch::getFileAndSuggestModule(
StringRef FileName, SourceLocation IncludeLoc, const DirectoryEntry *Dir,
bool IsSystemHeaderDir, Module *RequestingModule,
- ModuleMap::KnownHeader *SuggestedModule, bool OpenFile /*=true*/,
- bool CacheFailures /*=true*/) {
+ ModuleMap::KnownHeader *SuggestedModule, bool NeedSuggest,
+ bool OpenFile /*=true*/, bool CacheFailures /*=true*/) {
// If we have a module map that might map this header, load it and
// check whether we'll have a suggestion for a module.
auto File = getFileMgr().getFileRef(FileName, OpenFile, CacheFailures);
@@ -462,6 +462,9 @@ OptionalFileEntryRef HeaderSearch::getFileAndSuggestModule(
return std::nullopt;
}
+ if (!NeedSuggest)
+ return *File;
+
// If there is a module that corresponds to this header, suggest it.
if (!findUsableModuleForHeader(
*File, Dir ? Dir : File->getFileEntry().getDir(), RequestingModule,
@@ -479,7 +482,7 @@ OptionalFileEntryRef DirectoryLookup::LookupFile(
Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule,
bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound,
bool &IsInHeaderMap, SmallVectorImpl<char> &MappedName,
- bool OpenFile) const {
+ bool NeedSuggest, bool OpenFile) const {
InUserSpecifiedSystemFramework = false;
IsInHeaderMap = false;
MappedName.clear();
@@ -501,7 +504,7 @@ OptionalFileEntryRef DirectoryLookup::LookupFile(
return HS.getFileAndSuggestModule(
TmpDir, IncludeLoc, getDir(), isSystemHeaderDirectory(),
- RequestingModule, SuggestedModule, OpenFile);
+ RequestingModule, SuggestedModule, NeedSuggest, OpenFile);
}
if (isFramework())
@@ -881,6 +884,37 @@ diagnoseFrameworkInclude(DiagnosticsEngine &Diags, SourceLocation IncludeLoc,
<< IncludeFilename;
}
+
+/// Return true if a shadow has been detected and the caller should
+/// stop and return the first-found file and module, false otherwise.
+static bool checkAndStoreCandidate(
+ ModuleMap::KnownHeader *SuggestedModule,
+ OptionalFileEntryRef CandidateFile, StringRef CandidateDir,
+ DiagnosticsEngine &Diags, StringRef Filename, SourceLocation IncludeLoc,
+ ModuleMap::KnownHeader &FirstModule, OptionalFileEntryRef &FirstHeader,
+ SmallString<1024> &FirstDir) {
+ if (!FirstHeader) {
+ // Found the first candidate
+ FirstHeader = CandidateFile;
+ FirstDir = CandidateDir;
+ if (SuggestedModule)
+ FirstModule = *SuggestedModule;
+ return false;
+ }
+
+ if (FirstDir != CandidateDir) {
+ // Found a second candidate from a different directory
+ Diags.Report(IncludeLoc, diag::warn_header_shadowed)
+ << Filename << FirstDir << CandidateDir;
+ if (SuggestedModule)
+ *SuggestedModule = FirstModule;
+ return true;
+ }
+
+ // Found a candidate from the same directory as the first one
+ return false;
+}
+
/// LookupFile - Given a "foo" or \<foo> reference, look up the indicated file,
/// return null on failure. isAngled indicates whether the file reference is
/// for system \#include's or not (i.e. using <> instead of ""). Includers, if
@@ -923,13 +957,18 @@ OptionalFileEntryRef HeaderSearch::LookupFile(
// Otherwise, just return the file.
return getFileAndSuggestModule(Filename, IncludeLoc, nullptr,
/*IsSystemHeaderDir*/ false,
- RequestingModule, SuggestedModule, OpenFile,
- CacheFailures);
+ RequestingModule, SuggestedModule, true,
+ OpenFile, CacheFailures);
}
// This is the header that MSVC's header search would have found.
+ bool First = true;
+ bool NeedSuggest = true;
ModuleMap::KnownHeader MSSuggestedModule;
OptionalFileEntryRef MSFE;
+ ModuleMap::KnownHeader FirstModule;
+ OptionalFileEntryRef FirstHeader;
+ SmallString<1024> FirstDir;
// Check to see if the file is in the #includer's directory. This cannot be
// based on CurDir, because each includer could be a #include of a
@@ -938,7 +977,6 @@ OptionalFileEntryRef HeaderSearch::LookupFile(
// headers.
if (!Includers.empty() && !isAngled) {
SmallString<1024> TmpDir;
- bool First = true;
for (const auto &IncluderAndDir : Includers) {
OptionalFileEntryRef Includer = IncluderAndDir.first;
@@ -962,10 +1000,15 @@ OptionalFileEntryRef HeaderSearch::LookupFile(
}();
if (OptionalFileEntryRef FE = getFileAndSuggestModule(
TmpDir, IncludeLoc, IncluderAndDir.second, IncluderIsSystemHeader,
- RequestingModule, SuggestedModule)) {
+ RequestingModule, SuggestedModule, NeedSuggest)) {
if (!Includer) {
assert(First && "only first includer can have no file");
- return FE;
+ checkAndStoreCandidate(SuggestedModule, FE,
+ IncluderAndDir.second.getName(), Diags,
+ Filename, IncludeLoc, FirstModule,
+ FirstHeader, FirstDir);
+ NeedSuggest = false;
+ break;
}
// Leave CurDir unset.
@@ -994,21 +1037,30 @@ OptionalFileEntryRef HeaderSearch::LookupFile(
diagnoseFrameworkInclude(Diags, IncludeLoc,
IncluderAndDir.second.getName(), Filename,
*FE);
- return FE;
+ checkAndStoreCandidate(SuggestedModule, FE,
+ IncluderAndDir.second.getName(), Diags, Filename, IncludeLoc,
+ FirstModule, FirstHeader, FirstDir);
+ NeedSuggest = false;
+ break;
}
// Otherwise, we found the path via MSVC header search rules. If
// -Wmsvc-include is enabled, we have to keep searching to see if we
// would've found this header in -I or -isystem directories.
- if (Diags.isIgnored(diag::ext_pp_include_search_ms, IncludeLoc)) {
- return FE;
- } else {
- MSFE = FE;
- if (SuggestedModule) {
- MSSuggestedModule = *SuggestedModule;
- *SuggestedModule = ModuleMap::KnownHeader();
- }
- break;
+ if (checkAndStoreCandidate(SuggestedModule, FE,
+ IncluderAndDir.second.getName(), Diags, Filename, IncludeLoc,
+ FirstModule, FirstHeader, FirstDir)) {
+ // Found mutiple candidates via MSVC rules
+ if (Diags.isIgnored(diag::ext_pp_include_search_ms, IncludeLoc))
+ return FirstHeader;
+ else
+ NeedSuggest = false;
+ break;
+ }
+ MSFE = FE;
+ if (SuggestedModule) {
+ MSSuggestedModule = *SuggestedModule;
+ *SuggestedModule = ModuleMap::KnownHeader();
}
}
First = false;
@@ -1069,6 +1121,7 @@ OptionalFileEntryRef HeaderSearch::LookupFile(
}
SmallString<64> MappedName;
+ First = true;
// Check each directory in sequence to see if it contains this file.
for (; It != search_dir_end(); ++It) {
@@ -1078,7 +1131,7 @@ OptionalFileEntryRef HeaderSearch::LookupFile(
OptionalFileEntryRef File = It->LookupFile(
Filename, *this, IncludeLoc, SearchPath, RelativePath, RequestingModule,
SuggestedModule, InUserSpecifiedSystemFramework, IsFrameworkFoundInDir,
- IsInHeaderMap, MappedName, OpenFile);
+ IsInHeaderMap, MappedName, NeedSuggest, OpenFile);
if (!MappedName.empty()) {
assert(IsInHeaderMap && "MappedName should come from a header map");
CacheLookup.MappedName =
@@ -1097,53 +1150,60 @@ OptionalFileEntryRef HeaderSearch::LookupFile(
if (!File)
continue;
- CurDir = It;
-
- IncludeNames[*File] = Filename;
-
- // This file is a system header or C++ unfriendly if the dir is.
- HeaderFileInfo &HFI = getFileInfo(*File);
- HFI.DirInfo = CurDir->getDirCharacteristic();
-
- // If the directory characteristic is User but this framework was
- // user-specified to be treated as a system framework, promote the
- // characteristic.
- if (HFI.DirInfo == SrcMgr::C_User && InUserSpecifiedSystemFramework)
- HFI.DirInfo = SrcMgr::C_System;
-
- // If the filename matches a known system header prefix, override
- // whether the file is a system header.
- for (unsigned j = SystemHeaderPrefixes.size(); j; --j) {
- if (Filename.starts_with(SystemHeaderPrefixes[j - 1].first)) {
- HFI.DirInfo = SystemHeaderPrefixes[j-1].second ? SrcMgr::C_System
- : SrcMgr::C_User;
- break;
+ if (First) {
+ First = false;
+ NeedSuggest = false;
+ CurDir = It;
+ IncludeNames[*File] = Filename;
+
+ // This file is a system header or C++ unfriendly if the dir is.
+ HeaderFileInfo &HFI = getFileInfo(*File);
+ HFI.DirInfo = CurDir->getDirCharacteristic();
+
+ // If the directory characteristic is User but this framework was
+ // user-specified to be treated as a system framework, promote the
+ // characteristic.
+ if (HFI.DirInfo == SrcMgr::C_User && InUserSpecifiedSystemFramework)
+ HFI.DirInfo = SrcMgr::C_System;
+
+ // If the filename matches a known system header prefix, override
+ // whether the file is a system header.
+ for (unsigned j = SystemHeaderPrefixes.size(); j; --j) {
+ if (Filename.starts_with(SystemHeaderPrefixes[j - 1].first)) {
+ HFI.DirInfo = SystemHeaderPrefixes[j - 1].second ? SrcMgr::C_System
+ : SrcMgr::C_User;
+ break;
+ }
}
- }
- if (checkMSVCHeaderSearch(Diags, MSFE, &File->getFileEntry(), IncludeLoc)) {
- if (SuggestedModule)
+ if (checkMSVCHeaderSearch(Diags, MSFE, &File->getFileEntry(), IncludeLoc) &&
+ SuggestedModule)
*SuggestedModule = MSSuggestedModule;
- return MSFE;
- }
- bool FoundByHeaderMap = !IsMapped ? false : *IsMapped;
- if (!Includers.empty())
- diagnoseFrameworkInclude(Diags, IncludeLoc,
- Includers.front().second.getName(), Filename,
- *File, isAngled, FoundByHeaderMap);
+ bool FoundByHeaderMap = !IsMapped ? false : *IsMapped;
+ if (!Includers.empty())
+ diagnoseFrameworkInclude(Diags, IncludeLoc,
+ Includers.front().second.getName(), Filename,
+ *File, isAngled, FoundByHeaderMap);
- // Remember this location for the next lookup we do.
- cacheLookupSuccess(CacheLookup, It, IncludeLoc);
- return File;
+ // Remember this location for the next lookup we do.
+ cacheLookupSuccess(CacheLookup, It, IncludeLoc);
+ }
+
+ if (checkAndStoreCandidate(SuggestedModule, File, It->getName(), Diags,
+ Filename, IncludeLoc, FirstModule, FirstHeader, FirstDir))
+ return FirstHeader;
}
- if (checkMSVCHeaderSearch(Diags, MSFE, nullptr, IncludeLoc)) {
+ if (First && checkMSVCHeaderSearch(Diags, MSFE, nullptr, IncludeLoc)) {
if (SuggestedModule)
*SuggestedModule = MSSuggestedModule;
return MSFE;
}
+ if (FirstHeader)
+ return FirstHeader;
+
// Otherwise, didn't find it. Remember we didn't find this.
CacheLookup.HitIt = search_dir_end();
return std::nullopt;
diff --git a/clang/test/Preprocessor/header-shadowing.c b/clang/test/Preprocessor/header-shadowing.c
new file mode 100644
index 0000000000000..08b3c3e3f8db7
--- /dev/null
+++ b/clang/test/Preprocessor/header-shadowing.c
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -Wheader-shadowing -Eonly %t/main.c -I %t/include 2>&1 | FileCheck %s --check-prefix=SHADOWING
+// SHADOWING: {{.*}} warning: multiple candidates for header 'header.h' found;
+
+//--- main.c
+#include "header.h"
+
+//--- include/header.h
+#pragma once
+
+//--- header.h
+#pragma once
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
92f3422
to
2663a53
Compare
2663a53
to
5e51efd
Compare
The current implementation directly reuses the search logic from HeaderSearch::LookupFile() and avoids side effects caused by performing additional searches. Alternatively, we could recalculate the default search paths after locating the corresponding header file and then perform the extra search. This could make the implementation more consistent and better isolate functionalities. Please let me know if the alternative seems preferable, I’m happy to adjust the implementation. |
When including a header file, multiple files with the same name may exist across different search paths, like:
|-- main.cpp
|-- header.h
|-- include
| └── header.h
The compiler usually picks the first match it finds (typically following MSVC rules for current/include-chain paths first, then regular -I paths), which may not be the user’s intended header.
This silent behavior can lead to subtle runtime API mismatches or increase the cost of resolving errors such as “error: use of undeclared identifier”, especially in large projects.
Therefore, this patch tries to provide a diagnostic message without changing the current header selection. It does this by performing an additional search for duplicate filenames across all search paths (both MSVC rules and standard paths). This informs the user about a potential "header shadowing" issue and clarifies which header path was actually used.
Since header searching is much cheaper than file loading, the added overhead should be within an acceptable range.