llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: StoeckOverflow <details> <summary>Changes</summary> This PR adds diagnostics for exact `Where.Parameters` selectors on top of the existing parsing, serialization, and Sema matching support. It diagnoses duplicate exact selectors during API notes conversion, including duplicate `Where.Parameters: []`, while still allowing broad name-only entries and same-name entries with different selectors. It also adds reader/Sema support to warn under `-Wapinotes` when an exact selector in API notes does not match any visible overload. The reader can now enumerate stored exact selectors for global functions and C++ methods, and Sema compares those against the selector candidates derived from the visible overload set. The diagnostic path follows the same matching policy as Sema, including the desugared alias fallback, so valid matched selectors do not produce false warnings. Tests cover duplicate selector errors, unmatched selector warnings for globals and C++ methods, broad-plus-exact coexistence, exact notes not silently applying on mismatch, and no false warnings for matched or alias-fallback selectors. Reviewers: @<!-- -->Xazax-hun @<!-- -->j-hui @<!-- -->egorzhdan --- Patch is 26.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/209408.diff 10 Files Affected: - (modified) clang/include/clang/APINotes/APINotesReader.h (+12) - (modified) clang/lib/APINotes/APINotesReader.cpp (+79) - (modified) clang/lib/APINotes/APINotesYAMLCompiler.cpp (+69-2) - (modified) clang/lib/Sema/SemaAPINotes.cpp (+155) - (added) clang/test/APINotes/Inputs/Headers/WhereParametersDiagnostics.apinotes (+50) - (added) clang/test/APINotes/Inputs/Headers/WhereParametersDiagnostics.h (+18) - (modified) clang/test/APINotes/Inputs/Headers/module.modulemap (+5) - (added) clang/test/APINotes/Inputs/WhereParametersDuplicateSelectorDiag/APINotes.apinotes (+66) - (added) clang/test/APINotes/Inputs/WhereParametersDuplicateSelectorDiag/WhereParametersDiagnostics.h (+16) - (added) clang/test/APINotes/where-parameters-diagnostics.cpp (+41) ``````````diff diff --git a/clang/include/clang/APINotes/APINotesReader.h b/clang/include/clang/APINotes/APINotesReader.h index 761745e20b61a..640be8ad5acb3 100644 --- a/clang/include/clang/APINotes/APINotesReader.h +++ b/clang/include/clang/APINotes/APINotesReader.h @@ -17,6 +17,7 @@ #include "clang/APINotes/Types.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/Support/Error.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/VersionTuple.h" @@ -169,6 +170,11 @@ class APINotesReader { lookupCXXMethod(ContextID CtxID, llvm::StringRef Name, llvm::ArrayRef<std::string> Parameters); + /// Collect exact parameter selectors stored for the given C++ method. + void collectCXXMethodParameterSelectors( + ContextID CtxID, llvm::StringRef Name, + llvm::SmallVectorImpl<llvm::SmallVector<std::string, 4>> &Selectors); + /// Look for information regarding the given global variable. /// /// \param Name The name of the global variable. @@ -195,6 +201,12 @@ class APINotesReader { llvm::ArrayRef<std::string> Parameters, std::optional<Context> Ctx = std::nullopt); + /// Collect exact parameter selectors stored for the given global function. + void collectGlobalFunctionParameterSelectors( + llvm::StringRef Name, + llvm::SmallVectorImpl<llvm::SmallVector<std::string, 4>> &Selectors, + std::optional<Context> Ctx = std::nullopt); + /// Look for information regarding the given enumerator. /// /// \param Name The name of the enumerator. diff --git a/clang/lib/APINotes/APINotesReader.cpp b/clang/lib/APINotes/APINotesReader.cpp index 888844fbb6cd9..944beccb9d2df 100644 --- a/clang/lib/APINotes/APINotesReader.cpp +++ b/clang/lib/APINotes/APINotesReader.cpp @@ -19,6 +19,8 @@ #include "llvm/Bitstream/BitstreamReader.h" #include "llvm/Support/DJB.h" #include "llvm/Support/OnDiskHashTable.h" +#include <string> +#include <utility> namespace clang { namespace api_notes { @@ -833,6 +835,16 @@ class APINotesReader::Implementation { /// optional if the string is unknown. std::optional<IdentifierID> getIdentifier(llvm::StringRef Str); + /// Retrieve the identifier string for the given ID, or an empty optional if + /// the ID is unknown. + std::optional<llvm::StringRef> getIdentifierString(IdentifierID ID); + + /// Collect exact parameter selectors stored in the given function-like table. + template <typename TableT> + void collectFunctionParameterSelectors( + TableT *Table, uint32_t ParentContextID, llvm::StringRef Name, + llvm::SmallVectorImpl<llvm::SmallVector<std::string, 4>> &Selectors); + /// Retrieve the selector ID for the given selector, or an empty /// optional if the string is unknown. std::optional<SelectorID> getSelector(ObjCSelectorRef Selector); @@ -893,6 +905,56 @@ APINotesReader::Implementation::getIdentifier(llvm::StringRef Str) { return *Known; } +std::optional<llvm::StringRef> +APINotesReader::Implementation::getIdentifierString(IdentifierID ID) { + if (!IdentifierTable) + return std::nullopt; + + if (ID == IdentifierID(0)) + return llvm::StringRef(); + + for (llvm::StringRef Identifier : IdentifierTable->keys()) { + auto KnownID = IdentifierTable->find(Identifier); + if (KnownID != IdentifierTable->end() && *KnownID == ID) + return Identifier; + } + + return std::nullopt; +} + +template <typename TableT> +void APINotesReader::Implementation::collectFunctionParameterSelectors( + TableT *Table, uint32_t ParentContextID, llvm::StringRef Name, + llvm::SmallVectorImpl<llvm::SmallVector<std::string, 4>> &Selectors) { + if (!Table) + return; + + std::optional<IdentifierID> NameID = getIdentifier(Name); + if (!NameID) + return; + + for (auto I = Table->key_begin(), E = Table->key_end(); I != E; ++I) { + FunctionTableKey Key = I.getInternalKey(); + if (Key.parentContextID != ParentContextID || + Key.nameID != static_cast<unsigned>(*NameID) || !Key.parameterTypeIDs) + continue; + + llvm::SmallVector<std::string, 4> ParameterSelector; + ParameterSelector.reserve(Key.parameterTypeIDs->size()); + bool Failed = false; + for (IdentifierID TypeID : *Key.parameterTypeIDs) { + std::optional<llvm::StringRef> TypeName = getIdentifierString(TypeID); + if (!TypeName) { + Failed = true; + break; + } + ParameterSelector.push_back(TypeName->str()); + } + if (!Failed) + Selectors.push_back(std::move(ParameterSelector)); + } +} + std::optional<FunctionTableKey> APINotesReader::Implementation::getFunctionKey(uint32_t ParentContextID, llvm::StringRef Name) { @@ -2351,6 +2413,13 @@ auto APINotesReader::lookupCXXMethod(ContextID CtxID, llvm::StringRef Name, return lookupCXXMethodImpl(CtxID, Name, Parameters); } +void APINotesReader::collectCXXMethodParameterSelectors( + ContextID CtxID, llvm::StringRef Name, + llvm::SmallVectorImpl<llvm::SmallVector<std::string, 4>> &Selectors) { + Implementation->collectFunctionParameterSelectors( + Implementation->CXXMethodTable.get(), CtxID.Value, Name, Selectors); +} + auto APINotesReader::lookupCXXMethodImpl(ContextID CtxID, llvm::StringRef Name) -> VersionedInfo<CXXMethodInfo> { if (!Implementation->CXXMethodTable) @@ -2418,6 +2487,16 @@ auto APINotesReader::lookupGlobalFunction( return lookupGlobalFunctionImpl(Name, Parameters, Ctx); } +void APINotesReader::collectGlobalFunctionParameterSelectors( + llvm::StringRef Name, + llvm::SmallVectorImpl<llvm::SmallVector<std::string, 4>> &Selectors, + std::optional<Context> Ctx) { + uint32_t ParentContextID = Ctx ? Ctx->id.Value : static_cast<uint32_t>(-1); + Implementation->collectFunctionParameterSelectors( + Implementation->GlobalFunctionTable.get(), ParentContextID, Name, + Selectors); +} + auto APINotesReader::lookupGlobalFunctionImpl(llvm::StringRef Name, std::optional<Context> Ctx) -> VersionedInfo<GlobalFunctionInfo> { diff --git a/clang/lib/APINotes/APINotesYAMLCompiler.cpp b/clang/lib/APINotes/APINotesYAMLCompiler.cpp index 1c3a59873db25..058f9da338661 100644 --- a/clang/lib/APINotes/APINotesYAMLCompiler.cpp +++ b/clang/lib/APINotes/APINotesYAMLCompiler.cpp @@ -18,6 +18,7 @@ #include "clang/APINotes/Types.h" #include "clang/Basic/LLVM.h" #include "clang/Basic/Specifiers.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringSet.h" #include "llvm/Support/SourceMgr.h" #include "llvm/Support/VersionTuple.h" @@ -788,6 +789,47 @@ bool clang::api_notes::parseAndDumpAPINotes(StringRef YI, namespace { using namespace api_notes; +struct KnownFunctionSelector { + llvm::StringRef Name; + llvm::ArrayRef<llvm::StringRef> Parameters; +}; + +static bool equalParameterSelectors(llvm::ArrayRef<llvm::StringRef> LHS, + llvm::ArrayRef<llvm::StringRef> RHS) { + if (LHS.size() != RHS.size()) + return false; + + for (unsigned I = 0, E = LHS.size(); I != E; ++I) + if (LHS[I] != RHS[I]) + return false; + + return true; +} + +static bool +hasFunctionSelector(llvm::ArrayRef<KnownFunctionSelector> KnownSelectors, + llvm::StringRef Name, + llvm::ArrayRef<llvm::StringRef> Parameters) { + for (const KnownFunctionSelector &Known : KnownSelectors) + if (Known.Name == Name && + equalParameterSelectors(Known.Parameters, Parameters)) + return true; + + return false; +} + +static std::string +formatWhereParameters(llvm::ArrayRef<llvm::StringRef> Parameters) { + std::string Result = "["; + for (unsigned I = 0, E = Parameters.size(); I != E; ++I) { + if (I) + Result += ", "; + Result += Parameters[I]; + } + Result += "]"; + return Result; +} + class YAMLConverter { const Module &M; APINotesWriter Writer; @@ -1162,11 +1204,24 @@ class YAMLConverter { Writer.addField(TagCtxID, Field.Name, FI, SwiftVersion); } + llvm::SmallVector<KnownFunctionSelector, 4> KnownMethodSelectors; for (const auto &CXXMethod : T.Methods) { auto WhereParameters = getWhereParameters(CXXMethod); if (!WhereParameters.first) continue; + if (WhereParameters.second) { + if (hasFunctionSelector(KnownMethodSelectors, CXXMethod.Name, + *WhereParameters.second)) { + emitError(llvm::Twine("duplicate definition of C++ method '") + + CXXMethod.Name + "' with Where.Parameters " + + formatWhereParameters(*WhereParameters.second)); + continue; + } + KnownMethodSelectors.push_back( + {CXXMethod.Name, *WhereParameters.second}); + } + CXXMethodInfo MI; convertFunction(CXXMethod, MI); if (WhereParameters.second) @@ -1243,13 +1298,25 @@ class YAMLConverter { // Write all global functions. llvm::StringSet<> KnownNameOnlyFunctions; + llvm::SmallVector<KnownFunctionSelector, 4> KnownFunctionSelectors; for (const auto &Function : TLItems.Functions) { auto WhereParameters = getWhereParameters(Function); if (!WhereParameters.first) continue; - // Check for duplicate name-only global functions. Selector-aware - // duplicate diagnostics are handled by a later overload-matching PR. + if (WhereParameters.second) { + if (hasFunctionSelector(KnownFunctionSelectors, Function.Name, + *WhereParameters.second)) { + emitError(llvm::Twine("duplicate definition of global function '") + + Function.Name + "' with Where.Parameters " + + formatWhereParameters(*WhereParameters.second)); + continue; + } + KnownFunctionSelectors.push_back( + {Function.Name, *WhereParameters.second}); + } + + // Check for duplicate name-only global functions. if (!WhereParameters.second && !KnownNameOnlyFunctions.insert(Function.Name).second) { emitError(llvm::Twine("multiple definitions of global function '") + diff --git a/clang/lib/Sema/SemaAPINotes.cpp b/clang/lib/Sema/SemaAPINotes.cpp index 1ab1eac4a5434..e9e90c93755d3 100644 --- a/clang/lib/Sema/SemaAPINotes.cpp +++ b/clang/lib/Sema/SemaAPINotes.cpp @@ -1017,6 +1017,50 @@ struct APINotesParameterSelectorCandidates { std::optional<APINotesParameterSelector> Desugared; }; +struct APINotesParameterSelectorSet { + SmallVector<APINotesParameterSelector, 4> Selectors; + + void add(const APINotesParameterSelector &Selector) { + Selectors.push_back(Selector); + } + + void add(ArrayRef<std::string> Parameters) { + APINotesParameterSelector Selector; + Selector.Parameters.append(Parameters.begin(), Parameters.end()); + add(Selector); + } + + void add(const APINotesParameterSelectorCandidates &Candidates) { + add(Candidates.Source); + if (Candidates.Desugared) + add(*Candidates.Desugared); + } + + bool contains(ArrayRef<std::string> Parameters) const { + for (const APINotesParameterSelector &Selector : Selectors) { + if (Selector.Parameters.size() != Parameters.size()) + continue; + + bool Matches = true; + for (unsigned I = 0, E = Parameters.size(); I != E; ++I) { + if (Selector.Parameters[I] == Parameters[I]) + continue; + Matches = false; + break; + } + if (Matches) + return true; + } + + return false; + } + + bool empty() const { return Selectors.empty(); } + + auto begin() const { return Selectors.begin(); } + auto end() const { return Selectors.end(); } +}; + static PrintingPolicy getAPINotesParameterSelectorPrintingPolicy(const ASTContext &Context) { PrintingPolicy Policy(Context.getLangOpts()); @@ -1072,6 +1116,84 @@ getAPINotesParameterSelectorCandidates(const Sema &S, const FunctionDecl *FD) { return Candidates; } +static APINotesParameterSelectorSet +makeParameterSelectorSet(ArrayRef<SmallVector<std::string, 4>> Selectors) { + APINotesParameterSelectorSet Set; + for (const SmallVector<std::string, 4> &Selector : Selectors) + Set.add(Selector); + return Set; +} + +static std::string +formatParameterSelectorForDiagnostic(ArrayRef<std::string> Parameters) { + std::string Result = "["; + for (unsigned I = 0, E = Parameters.size(); I != E; ++I) { + if (I) + Result += ", "; + Result += Parameters[I]; + } + Result += "]"; + return Result; +} + +static void collectOverloadParameterSelectors(const Sema &S, + const FunctionDecl *FD, + APINotesParameterSelectorSet &Set, + bool &IsRepresentative) { + IsRepresentative = false; + bool FoundRepresentative = false; + + auto AddCandidate = [&](const FunctionDecl *Candidate) { + if (!FoundRepresentative) { + IsRepresentative = + Candidate->getCanonicalDecl() == FD->getCanonicalDecl(); + FoundRepresentative = true; + } + + if (auto Candidates = getAPINotesParameterSelectorCandidates(S, Candidate)) + Set.add(*Candidates); + }; + + for (NamedDecl *ND : FD->getDeclContext()->lookup(FD->getDeclName())) { + if (auto *Candidate = dyn_cast<FunctionDecl>(ND)) + AddCandidate(Candidate); + } + + if (FoundRepresentative) + return; + + for (Decl *D : FD->getDeclContext()->decls()) { + auto *ND = dyn_cast<NamedDecl>(D); + if (!ND || ND->getDeclName() != FD->getDeclName()) + continue; + + if (auto *Candidate = dyn_cast<FunctionDecl>(ND)) + AddCandidate(Candidate); + } + + if (!FoundRepresentative) + AddCandidate(FD); +} + +static void diagnoseUnmatchedParameterSelectors( + Sema &S, SourceLocation Loc, StringRef Name, + const APINotesParameterSelectorSet &APINotesSelectors, + const APINotesParameterSelectorSet &DeclarationSelectors) { + if (DeclarationSelectors.empty()) + return; + + for (const APINotesParameterSelector &APINotesSelector : APINotesSelectors) { + if (DeclarationSelectors.contains(APINotesSelector.Parameters)) + continue; + + S.Diag(Loc, diag::warn_apinotes_message) + << (llvm::Twine("API notes entry for '") + Name + + "' has unmatched Where.Parameters " + + formatParameterSelectorForDiagnostic(APINotesSelector.Parameters)) + .str(); + } +} + // Apply the first exact selector entry found. This preserves source-spelling // precedence over the desugared fallback and avoids applying multiple exact // entries for the same declaration. @@ -1131,6 +1253,12 @@ void Sema::ProcessAPINotes(Decl *D) { if (FD->getDeclName().isIdentifier()) { auto ParameterSelectorCandidates = getAPINotesParameterSelectorCandidates(*this, FD); + + APINotesParameterSelectorSet DeclarationSelectors; + bool DiagnoseUnmatchedSelectors = false; + collectOverloadParameterSelectors(*this, FD, DeclarationSelectors, + DiagnoseUnmatchedSelectors); + for (auto Reader : Readers) { auto Info = Reader->lookupGlobalFunction(FD->getName(), APINotesContext); @@ -1143,6 +1271,17 @@ void Sema::ProcessAPINotes(Decl *D) { return Reader->lookupGlobalFunction(FD->getName(), Parameters, APINotesContext); }); + + if (DiagnoseUnmatchedSelectors && !DeclarationSelectors.empty()) { + SmallVector<SmallVector<std::string, 4>, 4> RawAPINotesSelectors; + Reader->collectGlobalFunctionParameterSelectors( + FD->getName(), RawAPINotesSelectors, APINotesContext); + APINotesParameterSelectorSet APINotesSelectors = + makeParameterSelectorSet(RawAPINotesSelectors); + diagnoseUnmatchedParameterSelectors( + *this, FD->getLocation(), FD->getName(), APINotesSelectors, + DeclarationSelectors); + } } } @@ -1348,6 +1487,22 @@ void Sema::ProcessAPINotes(Decl *D) { return Reader->lookupCXXMethod(Context->id, MethodName, Parameters); }); + + APINotesParameterSelectorSet DeclarationSelectors; + bool DiagnoseUnmatchedSelectors = false; + collectOverloadParameterSelectors(*this, CXXMethod, + DeclarationSelectors, + DiagnoseUnmatchedSelectors); + if (DiagnoseUnmatchedSelectors && !DeclarationSelectors.empty()) { + SmallVector<SmallVector<std::string, 4>, 4> RawAPINotesSelectors; + Reader->collectCXXMethodParameterSelectors( + Context->id, MethodName, RawAPINotesSelectors); + APINotesParameterSelectorSet APINotesSelectors = + makeParameterSelectorSet(RawAPINotesSelectors); + diagnoseUnmatchedParameterSelectors( + *this, CXXMethod->getLocation(), MethodName, + APINotesSelectors, DeclarationSelectors); + } } } } diff --git a/clang/test/APINotes/Inputs/Headers/WhereParametersDiagnostics.apinotes b/clang/test/APINotes/Inputs/Headers/WhereParametersDiagnostics.apinotes new file mode 100644 index 0000000000000..2cf62c2b5e6be --- /dev/null +++ b/clang/test/APINotes/Inputs/Headers/WhereParametersDiagnostics.apinotes @@ -0,0 +1,50 @@ +--- +Name: WhereParametersDiagnostics +Functions: +- Name: unmatchedGlobal + Where: + Parameters: + - int + SwiftName: shouldNotApplyGlobal(_:) +- Name: diagnosticBroadGlobal + SwiftPrivate: true +- Name: diagnosticBroadGlobal + Where: + Parameters: + - int + SwiftName: shouldNotApplyBroadGlobal(_:) +- Name: diagnosticMatchedGlobal + Where: + Parameters: + - int + SwiftName: diagnosticMatchedGlobal(_:) +- Name: diagnosticAliasMatchedGlobal + Where: + Parameters: + - int + SwiftName: diagnosticAliasMatchedGlobal(_:) +Tags: +- Name: DiagnosticWidget + Methods: + - Name: unmatchedMethod + Where: + Parameters: + - int + SwiftName: shouldNotApplyMethod(_:) + - Name: diagnosticBroadMethod + SwiftPrivate: true + - Name: diagnosticBroadMethod + Where: + Parameters: + - int + SwiftName: shouldNotApplyBroadMethod(_:) + - Name: diagnosticMatchedMethod + Where: + Parameters: + - int + SwiftName: diagnosticMatchedMethod(_:) + - Name: diagnosticAliasMatchedMethod + Where: + Parameters: + - int + SwiftName: diagnosticAliasMatchedMethod(_:) diff --git a/clang/test/APINotes/Inputs/Headers/WhereParametersDiagnostics.h b/clang/test/APINotes/Inputs/Headers/WhereParametersDiagnostics.h new file mode 100644 index 0000000000000..c39ccd57541aa --- /dev/null +++ b/clang/test/APINotes/Inputs/Headers/WhereParametersDiagnostics.h @@ -0,0 +1,18 @@ +#ifndef WHERE_PARAMETERS_DIAGNOSTICS_H +#define WHERE_PARAMETERS_DIAGNOSTICS_H + +using DiagnosticAliasInt = int; + +void unmatchedGlobal(float); +void diagnosticBroadGlobal(float); +void diagnosticMatchedGlobal(int); +void diagnosticAliasMatchedGlobal(DiagnosticAliasInt); + +struct DiagnosticWidget { + void unmatchedMethod(float); + void diagnosticBroadMethod(float); + void diagnosticMatchedMethod(int); + void diagnosticAliasMatchedMethod(DiagnosticAliasInt); +}; + +#endif // WHERE_PARAMETERS_DIAGNOSTICS_H diff --git a/clang/test/APINotes/Inputs/Headers/module.modulemap b/clang/test/APINotes/Inputs/Headers/module.modulemap index 592d482ea7a57..d00b96c55b839 100644 --- a/clang/test/APINotes/Inputs/Headers/module.modulemap +++ b/clang/test/APINotes/Inputs/Headers/module.modulemap @@ -75,3 +75,8 @@ module WhereParametersSema { header "WhereParametersSema.h" export * } + +module WhereParametersDiagnostics { + header "WhereParametersDiagnostics.h" + export * +} diff --git a/clang/test/APINotes/Inputs/WhereParametersDuplicateSelectorDiag/APINotes.apinotes b/clang/test/APINotes/Inputs/WhereParametersDuplicateSelectorDiag/APINotes.apinotes new file mode 100644 index 0000000000000..d99ea9c0f4ce9 --- /dev/null +++ b/clang/test/APINotes/Inputs/Where... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/209408 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
