llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Andre Sun (and2049) <details> <summary>Changes</summary> Allow `readability-container-contains` to fix explicit boolean comparisons spelled within a single macro argument when the parameter's uses preserve expression grouping or constitute the entire macro body. Keep diagnostics without fixes for unsupported macro uses, including ambiguous forwarding cases. Closes #<!-- -->226833 Assisted by: Claude Code --- Full diff: https://github.com/llvm/llvm-project/pull/232196.diff 4 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp (+113-14) - (modified) clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h (+5) - (modified) clang-tools-extra/docs/ReleaseNotes.md (+5) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp (+164) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp index 701fab500f646a..97c9c3c2457518 100644 --- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp @@ -7,14 +7,78 @@ //===----------------------------------------------------------------------===// #include "ContainerContainsCheck.h" +#include "../utils/ASTUtils.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" +#include "clang/Lex/Preprocessor.h" using namespace clang::ast_matchers; namespace clang::tidy::readability { +// Returns true if the macro body uses the parameter expanded at `ParamUseLoc` +// only parenthesized or as the entire body, so every expansion parses alike. +static bool isParameterUsedOnlyParenthesized(SourceLocation ParamUseLoc, + Preprocessor &PP, + const SourceManager &SM) { + const StringRef MacroName = + Lexer::getImmediateMacroName(ParamUseLoc, SM, PP.getLangOpts()); + if (MacroName.empty()) + return false; + const MacroInfo *MI = + PP.getMacroDefinitionAtLoc(PP.getIdentifierInfo(MacroName), + SM.getExpansionLoc(ParamUseLoc)) + .getMacroInfo(); + if (!MI) + return false; + + const ArrayRef<Token> Tokens = MI->tokens(); + const SourceLocation ParamDefLoc = SM.getSpellingLoc(ParamUseLoc); + const auto *ParamTok = llvm::find_if(Tokens, [ParamDefLoc](const Token &Tok) { + return Tok.getLocation() == ParamDefLoc; + }); + if (ParamTok == Tokens.end()) + return false; + if (Tokens.size() == 1) + return true; + + const IdentifierInfo *Param = ParamTok->getIdentifierInfo(); + for (size_t I = 0, N = Tokens.size(); I < N; ++I) { + if (Tokens[I].getIdentifierInfo() != Param) + continue; + if (I == 0 || I + 1 == N || Tokens[I - 1].isNot(tok::l_paren) || + Tokens[I + 1].isNot(tok::r_paren)) + return false; + // Parentheses that may complete a call or a macro invocation, including + // one reached through a parameter or another expansion, do not group. + if (I >= 2 && + Tokens[I - 2].isOneOf(tok::identifier, tok::r_paren, tok::r_square, + tok::r_brace, tok::greater, tok::greatergreater)) + return false; + } + return true; +} + +// Returns true if every macro argument that `Loc` is spelled through satisfies +// `isParameterUsedOnlyParenthesized`. +static bool isSpelledThroughParenthesizedMacroArguments( + SourceLocation Loc, Preprocessor &PP, const SourceManager &SM) { + for (; Loc.isMacroID(); Loc = SM.getImmediateMacroCallerLoc(Loc)) { + SourceLocation ParamUseLoc; + if (SM.isMacroArgExpansion(Loc, &ParamUseLoc) && + !isParameterUsedOnlyParenthesized(ParamUseLoc, PP, SM)) + return false; + } + return true; +} + +void ContainerContainsCheck::registerPPCallbacks( + const SourceManager &SM, Preprocessor *PP, + Preprocessor * /*ModuleExpanderPP*/) { + this->PP = PP; +} + void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) { const auto Literal0 = integerLiteral(equals(0)); const auto Literal1 = integerLiteral(equals(1)); @@ -93,37 +157,72 @@ void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) { const auto *Comparison = Negated ? NegativeComparison : PositiveComparison; const StringRef ContainsFunName = Result.Nodes.getNodeAs<CXXMethodDecl>("contains_fun")->getName(); - const Expr *SearchExpr = Call->getArg(0)->IgnoreParenImpCasts(); // Diagnose the issue. const auto Diag = diag(Call->getExprLoc(), "use '%0' to check for membership") << ContainsFunName; - // Don't fix it if it's in a macro invocation. Leave fixing it to the user. - const SourceLocation FuncCallLoc = Comparison->getEndLoc(); - if (!FuncCallLoc.isValid() || FuncCallLoc.isMacroID()) + const SourceLocation ComparisonEndLoc = Comparison->getEndLoc(); + if (ComparisonEndLoc.isInvalid()) return; + const SourceManager &SM = *Result.SourceManager; + const LangOptions &LangOpts = Result.Context->getLangOpts(); + + const bool HasPrefix = Comparison->getBeginLoc() != Call->getBeginLoc(); + CharSourceRange PrefixRange = CharSourceRange::getCharRange( + Comparison->getBeginLoc(), Call->getBeginLoc()); + CharSourceRange FunctionNameRange = + CharSourceRange::getTokenRange(Call->getExprLoc()); + CharSourceRange ArgumentsAndSuffixRange = CharSourceRange::getTokenRange( + Call->getArg(0)->getBeginLoc(), ComparisonEndLoc); + SourceLocation NegationLoc = Call->getBeginLoc(); + + if (ComparisonEndLoc.isMacroID()) { + if (!PP || isa<ImplicitCastExpr>(Comparison) || + !Comparison->getType()->isBooleanType() || + !utils::rangeIsEntirelyWithinMacroArgument(Comparison->getSourceRange(), + &SM) || + !isSpelledThroughParenthesizedMacroArguments(Comparison->getBeginLoc(), + *PP, SM) || + !isSpelledThroughParenthesizedMacroArguments(ComparisonEndLoc, *PP, SM)) + return; + + const CharSourceRange CallRange = Lexer::makeFileCharRange( + CharSourceRange::getTokenRange(Call->getSourceRange()), SM, LangOpts); + if (HasPrefix) + PrefixRange = Lexer::makeFileCharRange(PrefixRange, SM, LangOpts); + FunctionNameRange = + Lexer::makeFileCharRange(FunctionNameRange, SM, LangOpts); + ArgumentsAndSuffixRange = + Lexer::makeFileCharRange(ArgumentsAndSuffixRange, SM, LangOpts); + if (CallRange.isInvalid() || PrefixRange.isInvalid() || + FunctionNameRange.isInvalid() || ArgumentsAndSuffixRange.isInvalid()) + return; + NegationLoc = CallRange.getBegin(); + } + + bool Invalid = false; const StringRef SearchExprText = Lexer::getSourceText( - CharSourceRange::getTokenRange(SearchExpr->getSourceRange()), - *Result.SourceManager, Result.Context->getLangOpts()); + CharSourceRange::getTokenRange(Call->getArg(0)->getSourceRange()), SM, + LangOpts, &Invalid); + if (Invalid) + return; // Remove everything before the function call. - Diag << FixItHint::CreateRemoval(CharSourceRange::getCharRange( - Comparison->getBeginLoc(), Call->getBeginLoc())); + if (HasPrefix) + Diag << FixItHint::CreateRemoval(PrefixRange); // Rename the function to `contains`. - Diag << FixItHint::CreateReplacement(Call->getExprLoc(), ContainsFunName); + Diag << FixItHint::CreateReplacement(FunctionNameRange, ContainsFunName); // Replace arguments and everything after the function call. - Diag << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(Call->getArg(0)->getBeginLoc(), - Comparison->getEndLoc()), - (SearchExprText + ")").str()); + Diag << FixItHint::CreateReplacement(ArgumentsAndSuffixRange, + (SearchExprText + ")").str()); // Add negation if necessary. if (Negated) - Diag << FixItHint::CreateInsertion(Call->getBeginLoc(), "!"); + Diag << FixItHint::CreateInsertion(NegationLoc, "!"); } } // namespace clang::tidy::readability diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h index 8d043910fc2bac..5ecba315aec074 100644 --- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h +++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h @@ -23,6 +23,8 @@ class ContainerContainsCheck : public ClangTidyCheck { public: ContainerContainsCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context) {} + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; void registerMatchers(ast_matchers::MatchFinder *Finder) final; void check(const ast_matchers::MatchFinder::MatchResult &Result) final; bool isLanguageVersionSupported(const LangOptions &LO) const final { @@ -31,6 +33,9 @@ class ContainerContainsCheck : public ClangTidyCheck { std::optional<TraversalKind> getCheckTraversalKind() const override { return TK_IgnoreUnlessSpelledInSource; } + +private: + Preprocessor *PP = nullptr; }; } // namespace clang::tidy::readability diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 69f3ecee49926b..2372f433d417a2 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -330,6 +330,11 @@ infrastructure are described first, followed by tool-specific sections. avoid offering fix-its for string literals originating from macro expansions. +- Improved {doc}`readability-container-contains + <clang-tidy/checks/readability/container-contains>` check by offering fix-its + for explicit comparisons spelled entirely within a macro argument that the + macro body only uses parenthesized. + - Improved {doc}`readability-convert-member-functions-to-static <clang-tidy/checks/readability/convert-member-functions-to-static>` check by fixing a crash when checking a const-qualified method declared with the diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp index 0cecd936027a53..0dda8843b3b40f 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp @@ -213,6 +213,170 @@ int testMacroExpansion(std::unordered_set<int> &MySet) { return 0; } +// Explicit comparisons spelled entirely within a single macro argument. +bool testMacroArgument(std::set<int> &MySet, int Key) { +#define ONCE(E) ((E)) + // CHECK-FIXES: #define ONCE(E) ((E)) + bool A = ONCE(MySet.find(Key) != MySet.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool A = ONCE(MySet.contains(Key)); + bool B = ONCE(MySet.find(Key) == MySet.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool B = ONCE(!MySet.contains(Key)); + bool C = ONCE(MySet.count(Key) != 0); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool C = ONCE(MySet.contains(Key)); + bool D = ONCE(0 == MySet.count(Key)); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool D = ONCE(!MySet.contains(Key)); +#undef ONCE + +#define REPEAT(E) ((E) && (E) && (E)) + // CHECK-FIXES: #define REPEAT(E) ((E) && (E) && (E)) + bool E = REPEAT(MySet.find(Key) != MySet.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool E = REPEAT(MySet.contains(Key)); + bool F = REPEAT(MySet.find(Key) == MySet.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool F = REPEAT(!MySet.contains(Key)); +#undef REPEAT + +#define ID(E) E + bool L = ID(MySet.find(Key) != MySet.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool L = ID(MySet.contains(Key)); +#undef ID + + // No fix when a use of the parameter is not parenthesized. +#define TWICE(E) E && E + // CHECK-FIXES: #define TWICE(E) E && E + bool G = TWICE(MySet.find(Key) == MySet.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool G = TWICE(MySet.find(Key) == MySet.end()); +#undef TWICE + + // No fix for an implicitly converted `count` inside a macro argument. +#define MIXED(E) (E && E == 2) + // CHECK-FIXES: #define MIXED(E) (E && E == 2) + bool H = MIXED(MySet.count(Key)); + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool H = MIXED(MySet.count(Key)); +#undef MIXED + + // Operands spelled through other macros. +#define MY_SET MySet +#define ONCE(E) ((E)) + bool I = ONCE(MY_SET.find(Key) != MY_SET.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool I = ONCE(MY_SET.contains(Key)); +#undef MY_SET +#define KEY Key + bool K = ONCE(MySet.find(KEY) != MySet.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool K = ONCE(MySet.contains(KEY)); +#undef KEY +#define METHOD find + bool M = ONCE(MySet.METHOD(Key) != MySet.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool M = ONCE(MySet.contains(Key)); +#undef METHOD +#define PAREN_KEY (Key) + bool N = ONCE(MySet.find(PAREN_KEY) != MySet.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool N = ONCE(MySet.contains(PAREN_KEY)); +#undef PAREN_KEY + bool O = ONCE(MySet.find((Key)) != MySet.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool O = ONCE(MySet.contains((Key))); + + // No fix when the comparison mixes argument and macro body tokens. +#define IS_FOUND(E) (E != MySet.end()) + // CHECK-FIXES: #define IS_FOUND(E) (E != MySet.end()) + bool J = IS_FOUND(MySet.find(Key)); + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: bool J = IS_FOUND(MySet.find(Key)); +#undef IS_FOUND +#undef ONCE + return A && B && C && D && E && F && G && H && I && J && K && L && M && N && O; +} + +struct ConstexprContainer { + constexpr const int *find(int X) const { return X == 1 ? &Value : nullptr; } + constexpr const int *end() const { return nullptr; } + constexpr unsigned count(int X) const { return X == 1; } + constexpr bool contains(int X) const { return X == 1; } + int Value = 1; +}; + +constexpr ConstexprContainer CC; +#define REPEAT(E) ((E) && (E) && (E)) +#define TWICE(E) E && E +#define MIXED(E) (E && E == 2) +#define MIX_COMPARISON(E) ((E) && (E + 1)) +static_assert(REPEAT(CC.find(1) != CC.end()), ""); +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use 'contains' to check for membership [readability-container-contains] +// CHECK-FIXES: static_assert(REPEAT(CC.contains(1)), ""); +static_assert(TWICE(CC.find(2) == CC.end()), ""); +// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use 'contains' to check for membership [readability-container-contains] +// CHECK-FIXES: static_assert(TWICE(CC.find(2) == CC.end()), ""); +static_assert(!MIXED(CC.count(1)), ""); +// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use 'contains' to check for membership [readability-container-contains] +// CHECK-FIXES: static_assert(!MIXED(CC.count(1)), ""); +static_assert(!MIX_COMPARISON(CC.count(1) != 0), ""); +// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: use 'contains' to check for membership [readability-container-contains] +// CHECK-FIXES: static_assert(!MIX_COMPARISON(CC.count(1) != 0), ""); +#undef REPEAT +#undef TWICE +#undef MIXED +#undef MIX_COMPARISON + +// No fix when a use may be the argument of a call or macro invocation. +#define BAD(E) (E + 1) +#define ID(X) X +#define FORWARD(E) ((E) && BAD(E)) +static_assert(!FORWARD(CC.count(1) != 0), ""); +// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use 'contains' to check for membership [readability-container-contains] +// CHECK-FIXES: static_assert(!FORWARD(CC.count(1) != 0), ""); +#define VIA_PARAMETER(F, E) ((E) && F(E)) +static_assert(!VIA_PARAMETER(BAD, CC.count(1) != 0), ""); +// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: use 'contains' to check for membership [readability-container-contains] +// CHECK-FIXES: static_assert(!VIA_PARAMETER(BAD, CC.count(1) != 0), ""); +#define VIA_EXPANSION(E) ((E) && ID(BAD)(E)) +static_assert(!VIA_EXPANSION(CC.count(1) != 0), ""); +// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: use 'contains' to check for membership [readability-container-contains] +// CHECK-FIXES: static_assert(!VIA_EXPANSION(CC.count(1) != 0), ""); +constexpr bool identity(bool B) { return B; } +#define CALL(E) ((E) && identity(E)) +static_assert(CALL(CC.count(1) != 0), ""); +// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains] +// CHECK-FIXES: static_assert(CALL(CC.count(1) != 0), ""); +#undef BAD +#undef ID +#undef FORWARD +#undef VIA_PARAMETER +#undef VIA_EXPANSION +#undef CALL + +struct NonBoolIterator { + int operator!=(NonBoolIterator) const; +}; + +struct NonBoolComparisonContainer { + NonBoolIterator find(int) const; + NonBoolIterator end() const; + bool contains(int) const; +}; + +// No fix for a non-`bool` comparison inside a macro argument. +int testMacroArgumentNonBoolComparison(NonBoolComparisonContainer &C, int Key) { +#define ONCE(E) ((E)) + int R = ONCE(C.find(Key) != C.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: use 'contains' to check for membership [readability-container-contains] + // CHECK-FIXES: int R = ONCE(C.find(Key) != C.end()); +#undef ONCE + return R; +} + // The following map has the same interface as `std::map`. template <class Key, class T> struct CustomMap { `````````` </details> https://github.com/llvm/llvm-project/pull/232196 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
