Changeset View
Standalone View
clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp
- This file was added.
| //===--- UnintendedADLCheck.cpp - clang-tidy ------------------------------===// | |||||
| // | |||||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | |||||
| // See https://llvm.org/LICENSE.txt for license information. | |||||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | |||||
| // | |||||
| //===----------------------------------------------------------------------===// | |||||
| #include "UnintendedADLCheck.h" | |||||
| #include "../utils/ASTUtils.h" | |||||
| #include "../utils/OptionsUtils.h" | |||||
| #include "clang/AST/ASTContext.h" | |||||
| #include "clang/ASTMatchers/ASTMatchFinder.h" | |||||
| #include <algorithm> | |||||
| using namespace clang::ast_matchers; | |||||
| namespace clang { | |||||
| namespace tidy { | |||||
| namespace bugprone { | |||||
| namespace { | |||||
| AST_MATCHER_P(CallExpr, ignoredOperator, bool, IgnoreOverloadedOperators) { | |||||
| return IgnoreOverloadedOperators && isa<CXXOperatorCallExpr>(Node); | |||||
| } | |||||
| AST_MATCHER(UnresolvedLookupExpr, requiresADL) { return Node.requiresADL(); } | |||||
logan-5: Oops: this should maybe be `OS.flush(); return N;` so that N is NRVO'd or else implicit-moved. | |||||
| AST_MATCHER_P(UnresolvedLookupExpr, isSpelledAsAnyOf, std::vector<StringRef>, | |||||
| Names) { | |||||
| return std::any_of(Names.begin(), Names.end(), | |||||
| [LookupName = Node.getName().getAsString()]( | |||||
| StringRef Name) { return Name == LookupName; }); | |||||
| } | |||||
| } // namespace | |||||
| UnintendedADLCheck::UnintendedADLCheck(StringRef Name, | |||||
| ClangTidyContext *Context) | |||||
| : ClangTidyCheck(Name, Context), | |||||
| IgnoreOverloadedOperators(Options.get("IgnoreOverloadedOperators", 1)), | |||||
| Whitelist(utils::options::parseStringList(Options.get( | |||||
| "Whitelist", "swap;make_error_code;make_error_condition;data;begin;" | |||||
do you mean std::swap? If you it should be fully qualified. That means, the list should be extended to at least all standard-library facilities that basically required ADL to work. And then we need data on different code bases (e.g. LLVM is a good start) how much noise gets generated. JonasToth: do you mean `std::swap`? If you it should be fully qualified.
Doesn't `std::error_code` rely on… | |||||
I distinctly don't mean std::swap -- I want to whitelist any unqualified function call spelled simply swap. Overloaded operators are the poster child for ADL's usefulness, so that's why this check has a special default-on IgnoreOverloadedOperators option. That whitelists a whole ton of legitimate stuff including std::cout << x and friends. I don't see a ton of discussion online about error_code/make_error_code and ADL being very much intertwined. I'm not particularly familiar with those constructs myself though, and I could just be out of the loop. I do see a fair number of unqualified calls to make_error_code within LLVM, though most of those resolve to llvm::make_error_code, the documentation for which says it exists because std::make_error_code can't be reliably/portably used with ADL. That makes me think make_error_code would belong in LLVM's project-specific whitelist configuration, not the check's default. Speaking of which, I did run this check over LLVM while developing and found it not particularly noisy as written. That is, it generated a fair number of warnings, but only on constructs that, when examined closely, were a little suspicious or non-obvious. logan-5: I distinctly //don't// mean `std::swap` -- I want to whitelist any unqualified function call… | |||||
I don't have a solid understanding of the error_code world as well. All I know is, that you specialize some templates with your own types in order to use the generic error_code-world. But overloaded operators being ignored by default is good and that point is gone :) JonasToth: I don't have a solid understanding of the `error_code` world as well. All I know is, that you… | |||||
Yes, make_error_code is used via ADL. --> https://www.boost.org/doc/libs/1_72_0/libs/outcome/doc/html/motivation/plug_error_code.html JonasToth: Yes, `make_error_code` is used via ADL. --> https://www.boost. | |||||
Not Done ReplyInline Actions+1, both make_error_code and make_error_condition should be on the whitelist. (I am the author of [P0824 "Summary of SG14 discussion of <system_error>"](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0824r1.html#best-arthur). I also confirm that libc++ <system_error> does call them unqualified on purpose.) I would like to see some discussion and/or TODO-comments about the other standard designated customization points: data, begin, end, rbegin, rend, crbegin, crend, size, ssize, and empty. This might deserve input from the libc++ implementors. Quuxplusone: +1, both `make_error_code` and `make_error_condition` should be on the whitelist. (I am the… | |||||
Added make_error_condition to the whitelist. My inclination would be to just add all those standard customization points to the default whitelist. Users can easily supply a smaller whitelist if they want. logan-5: Added `make_error_condition` to the whitelist.
My inclination would be to just add all those… | |||||
| "end;rbegin;rend;crbegin;crend;size;ssize;empty"))) {} | |||||
| void UnintendedADLCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { | |||||
| Options.store(Opts, "IgnoreOverloadedOperators", IgnoreOverloadedOperators); | |||||
| Options.store(Opts, "Whitelist", | |||||
| utils::options::serializeStringList(Whitelist)); | |||||
| } | |||||
As long as you're asserting anyway, I think you should assert that the last two characters are ::. Quuxplusone: As long as you're asserting anyway, I think you should assert that the last two characters… | |||||
| void UnintendedADLCheck::registerMatchers(MatchFinder *Finder) { | |||||
| const std::vector<StringRef> WhitelistRefs(Whitelist.begin(), | |||||
| Whitelist.end()); | |||||
| Finder->addMatcher( | |||||
| callExpr(usesADL(), unless(ignoredOperator(IgnoreOverloadedOperators)), | |||||
| callee(functionDecl(unless(hasAnyName(WhitelistRefs))) | |||||
| .bind("ADLcallee"))) | |||||
| .bind("ADLcall"), | |||||
| this); | |||||
| Finder->addMatcher( | |||||
| callExpr(unless(ignoredOperator(IgnoreOverloadedOperators)), | |||||
| has(unresolvedLookupExpr(requiresADL(), | |||||
| unless(isSpelledAsAnyOf(WhitelistRefs))) | |||||
| .bind("templateADLexpr"))) | |||||
| .bind("templateADLcall"), | |||||
| this); | |||||
| } | |||||
| static std::string getArgumentTypesAsString(const CallExpr *const Call) { | |||||
| auto Begin = Call->arg_begin(); | |||||
| const auto End = Call->arg_end(); | |||||
| assert(Begin != End); | |||||
| std::string Result; | |||||
| llvm::raw_string_ostream OS(Result); | |||||
| OS << "'" << (*Begin)->getType().getAsString() << "'"; | |||||
| for (++Begin; Begin != End; ++Begin) | |||||
| OS << ", '" << (*Begin)->getType().getAsString() << "'"; | |||||
| return Result; | |||||
| } | |||||
| void UnintendedADLCheck::check(const MatchFinder::MatchResult &Result) { | |||||
please add a message to the assertion to describe why you are asserting this condition, to better understand the cause of the error, e.g. assert(Call && "Matcher should only match for two distinct cases"); or something like this. JonasToth: please add a message to the assertion to describe why you are asserting this condition, to… | |||||
| if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("ADLcall")) { | |||||
| // Ignore matches in macros. This avoids large numbers of false positives in | |||||
| // e.g. assert(). Same below. | |||||
| if (Call->getBeginLoc().isMacroID()) | |||||
| return; | |||||
Can't you just bind directly to the unresolvedExpr? JonasToth: Can't you just bind directly to the `unresolvedExpr`? | |||||
I need the "templateADLCall" node for the diagnostic caret, and the "templateADLexpr" for the name/spelling of the call. I might totally be misunderstanding what you're suggesting here. logan-5: I need the `"templateADLCall"` node for the diagnostic caret, and the `"templateADLexpr"` for… | |||||
Ah, i overlooked that bit. Then The Lookup should be asserted as well, to ensure the matchers works in all circumstances. (future refactorings included) JonasToth: Ah, i overlooked that bit. Then The `Lookup` should be `assert`ed as well, to ensure the… | |||||
| const auto *Callee = Result.Nodes.getNodeAs<FunctionDecl>("ADLcallee"); | |||||
| diag(Call->getBeginLoc(), "expression calls '%0' through ADL") | |||||
| << Callee->getQualifiedNameAsString(); | |||||
| if (!match(isInTemplateInstantiation(), *Call, *Result.Context).empty()) | |||||
| diag(Call->getBeginLoc(), "with argument type%s0 %1", DiagnosticIDs::Note) | |||||
| << Call->getNumArgs() << getArgumentTypesAsString(Call); | |||||
| } else if (const auto *Call = | |||||
| Result.Nodes.getNodeAs<CallExpr>("templateADLcall")) { | |||||
| // Ignore matches in macros. | |||||
| if (Call->getBeginLoc().isMacroID()) | |||||
| return; | |||||
| const auto *Lookup = | |||||
| Result.Nodes.getNodeAs<UnresolvedLookupExpr>("templateADLexpr"); | |||||
| assert(Lookup && | |||||
| "'templateADLcall' matcher must bind associated unresolved lookup"); | |||||
| diag(Call->getBeginLoc(), | |||||
| "unqualified call to '%0' may be resolved through ADL") | |||||
| << Lookup->getName().getAsString(); | |||||
| } | |||||
| } | |||||
| } // namespace bugprone | |||||
| } // namespace tidy | |||||
| } // namespace clang | |||||
Never mind me if this becomes too ugly, but, I would have expected bool OnlyDependsOnFundamentalArraySizes(const Expr *) to be recursive, and I would have expected the llvm::all_of-over-a-parameter-list to happen in the caller. To properly handle corner cases like template<class T> void test() {
char a[sizeof(T)][sizeof(T)]; foo(a);
}Experimentally, it appears that Clang thinks that the type of char b[sizeof(T)]; ... (b+1) ... is dependent. (It's definitely char*, but it comes from applying + to an expression with dependent type.) I have no idea what the paper standard says about it. Quuxplusone: Never mind me if this becomes too ugly, but, I would have expected `bool… | |||||
I can't imagine it super matters for raw_string_ostream, but I think pedantically this needs an OS.flush() before the return. logan-5: I can't imagine it super matters for raw_string_ostream, but I think pedantically this needs an… | |||||
Oops: this should maybe be OS.flush(); return N; so that N is NRVO'd or else implicit-moved. return OS.str(); does a copy.