diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -20,6 +20,7 @@ TypePromotionInMathFnCheck.cpp UnnecessaryCopyInitialization.cpp UnnecessaryValueParamCheck.cpp + UnusedNoSideEffectCheck.cpp LINK_LIBS clangTidy diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -24,6 +24,7 @@ #include "TypePromotionInMathFnCheck.h" #include "UnnecessaryCopyInitialization.h" #include "UnnecessaryValueParamCheck.h" +#include "UnusedNoSideEffectCheck.h" namespace clang { namespace tidy { @@ -61,6 +62,8 @@ "performance-unnecessary-copy-initialization"); CheckFactories.registerCheck( "performance-unnecessary-value-param"); + CheckFactories.registerCheck( + "performance-unused-no-side-effect"); } }; diff --git a/clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h b/clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h @@ -0,0 +1,79 @@ +//===--- UnusedNoSideEffectCheck.h - clang-tidy -----------------*- C++ -*-===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNUSEDNOSIDEEFFECTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNUSEDNOSIDEEFFECTCHECK_H + +#include "../ClangTidyCheck.h" +#include "clang/AST/Decl.h" +#include "clang/AST/Expr.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/Type.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/StringSet.h" + +#include + +namespace clang { +namespace tidy { +namespace performance { + +/// Finds variables that are not used and do not have user-visible side effects. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/performance-unused-no-side-effect.html +class UnusedNoSideEffectCheck : public ClangTidyCheck { +public: + UnusedNoSideEffectCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + + struct NoSideEffectsInfo { + std::string Type; + llvm::SmallVector ExcludedMethods; + }; + +private: + class Visitor; + + bool hasNoSideEffects(ASTContext &AstContext, QualType Type, + const CXXMethodDecl *Method = nullptr); + + bool hasNoSideEffectsImpl(ASTContext &AstContext, QualType Type, + const CXXMethodDecl *Method, bool AllowReferences, + llvm::DenseSet &Seen); + + // Configuration for the types that are considered side-effect-free by the + // check. + + // Types that inherit directly from these types are considered side-effect + // free. + llvm::SmallVector TypesByBase; + + // The following non-template types are considered side-effect free. + llvm::SmallVector RawTypes; + + // Instantiations of these template types with side-effect free types are + // considered side-effect-free. + llvm::SmallVector TemplateTypes; + + // List of smart pointer types, which are template types that have special + // rules wrt side-effect-free-ness. + llvm::SmallVector SmartPointerTypes; + + // Functions that are known not to read data from some of their arguments, + // represented as a map from function name to set of not-read-from arguments. + llvm::StringMap> UnusedFunctionArguments; +}; + +} // namespace performance +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNUSEDNOSIDEEFFECTCHECK_H diff --git a/clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp @@ -0,0 +1,686 @@ +//===--- UnusedNoSideEffectCheck.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 "UnusedNoSideEffectCheck.h" + +#include "clang/AST/ASTContext.h" +#include "clang/AST/Attrs.inc" +#include "clang/AST/Decl.h" +#include "clang/AST/Expr.h" +#include "clang/AST/ExprCXX.h" +#include "clang/AST/Stmt.h" +#include "clang/AST/Type.h" +#include "clang/AST/TypeLoc.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/ASTMatchers/ASTMatchersInternal.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Basic/Specifiers.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringMap.h" +#include "llvm/Support/JSON.h" +#include "llvm/Support/raw_ostream.h" + +#include +#include +#include +#include + +namespace clang { +namespace tidy { +namespace performance { + +// TODO(veluca): The FixIts don't really fix everything and can break code. +static constexpr bool EmitFixits = false; + +using namespace ::clang::ast_matchers; // NOLINT: Too many names. + +class UnusedNoSideEffectCheck::Visitor + : public RecursiveASTVisitor { +public: + explicit Visitor(UnusedNoSideEffectCheck *Check) : Check(Check) {} + + bool shouldVisitImplicitCode() { return true; } + bool shouldVisitTemplateInstantiations() const { return true; } + + bool dataTraverseStmtPre(Stmt *S) { + if (CurrentExprOrDeclStmt != nullptr) { + return true; + } + if (isa(S)) { + CurrentExprOrDeclStmt = S; + } + return true; + } + + bool dataTraverseStmtPost(Stmt *S) { + if (CurrentExprOrDeclStmt == S) { + CurrentExprOrDeclStmt = nullptr; + } + return true; + } + + bool VisitStmt(Stmt *S) { + // Mark all children as observable by default. + for (const auto *Child : S->children()) { + markObservable(Child); + } + return true; + } + + bool VisitCompoundStmt(CompoundStmt *Compound) { + // Compound statements (i.e. blocks) do not produce a value, nor have side + // effects by themselves. + for (const auto *Child : Compound->children()) { + markMaybeNotObservable(Child); + } + return true; + } + + bool VisitBinaryOperator(BinaryOperator *Op) { + if (isObservable(Op)) { + return true; + } + // If this is not an assignment operation, its arguments are only observed + // iff the result of the operation is observed. If this is an assignment, + // then its RHS is always observed. + if (!Op->isAssignmentOp()) { + markMaybeNotObservable(Op->getRHS()); + } + markMaybeNotObservable(Op->getLHS()); + return true; + } + + bool VisitUnaryOperator(UnaryOperator *Op) { + if (isObservable(Op) || Op->getOpcode() == UnaryOperatorKind::UO_Coawait || + Op->getOpcode() == UnaryOperatorKind::UO_Deref) { + return true; + } + // Unary operators don't have side effects, except co_await and + // dereferencing. + markMaybeNotObservable(Op->getSubExpr()); + return true; + } + + bool VisitCallExpr(CallExpr *Call) { + if (isObservable(Call)) { + return true; + } + const auto *Callee = Call->getDirectCallee(); + if (!Callee) { + return true; + } + CalleesForStmts[CurrentExprOrDeclStmt].insert(Callee); + const auto *CXXMethod = dyn_cast(Callee); + if (!CXXMethod || CXXMethod->isStatic()) { + std::string QualName = Callee->getQualifiedNameAsString(); + const auto Iter = Check->UnusedFunctionArguments.find(QualName); + if (Iter != Check->UnusedFunctionArguments.end()) { + for (size_t i = 0; i < Call->getNumArgs(); i++) { + if (Iter->second.contains(i) || Iter->second.empty()) { + markMaybeNotObservable(Call->getArg(i)); + } + } + } + return true; + } + if (!Check->hasNoSideEffects(CXXMethod->getASTContext(), + CXXMethod->getThisObjectType(), CXXMethod)) { + return true; + } + // Here we know that the function is a method or operator call on a struct + // or class, and that there are no side effects on `this`. + if (const auto *Callee = dyn_cast(Call->getCallee())) { + markMaybeNotObservable(Callee); + } + if (const auto *CXXMethodCall = dyn_cast(Call)) { + markMaybeNotObservable(CXXMethodCall->getImplicitObjectArgument()); + return true; + } + // Otherwise, this is a call with operator syntax, and "this" is the first + // argument. + assert(isa(Call)); + markMaybeNotObservable(Call->getArg(0)); + return true; + } + + bool VisitMemberExpr(MemberExpr *MemberExpr) { + if (isObservable(MemberExpr)) { + return true; + } + // Member access does not have side effects. + markMaybeNotObservable(MemberExpr->getBase()); + return true; + } + + bool VisitDeclRefExpr(DeclRefExpr *DeclRef) { + const auto *Decl = dyn_cast(DeclRef->getDecl()); + if (!Decl) { + return true; + } + StmtsForVars[Decl].insert(CurrentExprOrDeclStmt); + // Also consider used all variables that are used in a non-ODR context, i.e. + // array sizes, variables that appear in decltype expressions, or variables + // that appear as template parameters. + if (!isObservable(DeclRef) && !DeclRef->isNonOdrUse()) { + return true; + } + UsedDecls.insert(Decl); + return true; + } + + bool VisitExprWithCleanups(ExprWithCleanups *ExprWithCleanups) { + if (isObservable(ExprWithCleanups)) { + return true; + } + markMaybeNotObservable(ExprWithCleanups->getSubExpr()); + return true; + } + + bool VisitImplicitCastExpr(ImplicitCastExpr *ImplicitCast) { + if (isObservable(ImplicitCast) || + ImplicitCast->getCastKind() == CastKind::CK_LValueToRValue) { + return true; + } + // Implicit casts don't have side effects, except lvalue-to-rvalue. + markMaybeNotObservable(ImplicitCast->getSubExpr()); + return true; + } + bool VisitParenExpr(ParenExpr *Paren) { + if (isObservable(Paren)) { + return true; + } + markMaybeNotObservable(Paren->getSubExpr()); + return true; + } + bool VisitDeclStmt(DeclStmt *Decl) { + // We do not support producing fixits for variables in multiple + // declarations. + if (Decl->isSingleDecl() && isa(Decl->getSingleDecl())) { + StmtsForVars[dyn_cast(Decl->getSingleDecl())].insert(Decl); + } + return true; + } + + // Utility methods to track observability of statements. By default, all + // statements are observable, and statements are marked as not observable when + // they are part of non-observable statements of a specific kind; for example, + // direct members of a CompoundStmt are not observable. + void markObservable(const Stmt *Stmt) { ObservableStmts.insert(Stmt); } + void markMaybeNotObservable(const Stmt *Stmt) { ObservableStmts.erase(Stmt); } + bool isObservable(const Stmt *Stmt) const { + return ObservableStmts.contains(Stmt); + } + + UnusedNoSideEffectCheck *Check; + llvm::DenseSet ObservableStmts; + llvm::DenseSet UsedDecls; + // Statements in which a given variable is used/declared. Used for fixits. + llvm::DenseMap> StmtsForVars; + llvm::DenseMap> + CalleesForStmts; + Stmt *CurrentExprOrDeclStmt = nullptr; +}; + +void UnusedNoSideEffectCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(functionDecl(hasBody(compoundStmt())).bind("func"), this); +} + +void UnusedNoSideEffectCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs("func"); + + Visitor Vis(this); + if (const auto *CXXConstructor = dyn_cast(MatchedDecl)) { + for (const auto *Init : CXXConstructor->inits()) { + Vis.markObservable(Init->getInit()); + } + } + Vis.TraverseFunctionDecl(const_cast(MatchedDecl)); + + for (const auto &KV : Vis.StmtsForVars) { + const auto *VarDecl = KV.first; + if (Vis.UsedDecls.contains(VarDecl)) + continue; + + // Common ways to mark variables as unused intentionally. + if (VarDecl->getName().empty()) + continue; + if (VarDecl->getName().startswith("_")) + continue; + if (VarDecl->getName().startswith("unused_")) + continue; + if (VarDecl->hasAttr()) + continue; + + // Unused function parameters are very common. + // TODO(veluca): consider also handling unused `ParmVarDecl`s of non-cheap + // types. + if (isa(VarDecl)) { + continue; + } + + // Skip variables of types that are not known to be side effect free. + if (!hasNoSideEffects(VarDecl->getASTContext(), VarDecl->getType())) { + continue; + } + + // Handle smart pointer constructors. + if (const auto *RecordDecl = VarDecl->getType()->getAsRecordDecl()) { + std::string Name = RecordDecl->getQualifiedNameAsString().c_str(); + bool IsSmartPointer = false; + for (const auto &TypeInfo : SmartPointerTypes) { + if (Name == TypeInfo.Type) { + IsSmartPointer = true; + } + } + + // TODO(veluca): consider also handling custom deleters. + + // Detect some constructors. + if (IsSmartPointer && VarDecl->hasInit()) { + const Expr *Init = VarDecl->getInit(); + if (isa(Init)) { + Init = dyn_cast(Init)->getSubExpr(); + } + const auto *CXXConstruct = dyn_cast(Init); + if (!CXXConstruct || CXXConstruct->getNumArgs() > 1) { + // We don't understand this constructor: conservatively do nothing. + continue; + } + // Bail out on constructors that take a pointer that is not + // immediately produced by a new expression. + if (CXXConstruct->getNumArgs() == 1 && + CXXConstruct->getArg(0)->getType()->isPointerType() && + !isa(CXXConstruct->getArg(0))) { + continue; + } + } + } + + // Avoid reporting captured variables that are not used in the body of a + // lambda, or global/static variables. + if (!MatchedDecl->getSourceRange().fullyContains( + VarDecl->getSourceRange())) { + continue; + } + + llvm::DenseSet Callees; + { + auto Diag = diag(VarDecl->getLocation(), "variable %0 is never read") + << VarDecl; + if (EmitFixits) { + for (const auto *StmtToDelete : KV.second) { + // TODO(veluca): semicolons. + Diag << FixItHint::CreateRemoval(StmtToDelete->getSourceRange()); + for (const auto *FnDecl : Vis.CalleesForStmts[StmtToDelete]) { + Callees.insert(FnDecl); + } + } + } + } + } +} + +bool fromJSON( + const llvm::json::Value &E, + llvm::SmallVector &Out, + llvm::json::Path P) { + if (const auto *O = E.getAsObject()) { + Out.clear(); + for (const auto &KV : *O) { + UnusedNoSideEffectCheck::NoSideEffectsInfo Entry; + Entry.Type = KV.first.str(); + if (const auto *A = KV.second.getAsArray()) { + for (const auto &Method : *A) { + llvm::Optional M = Method.getAsString(); + if (M) { + Entry.ExcludedMethods.push_back(M.getValue().str()); + } else { + P.field(KV.first).report("expected array of strings"); + } + } + } else { + P.field(KV.first).report("expected array"); + return false; + } + Out.push_back(std::move(Entry)); + } + return true; + } + P.report("expected object"); + return false; +} + +UnusedNoSideEffectCheck::UnusedNoSideEffectCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) { + // Operator= is disabled by default because often used to force deallocations + constexpr const char *kDefaultSmartPointerTypes = + R"({"std::unique_ptr":["swap","operator=","reset"],"std::shared_ptr":["swap","operator=", "reset"]})"; + + // std::string & related classes are handled by std::basic_string & friends. + constexpr const char *kDefaultRawTypes = R"( + { + "absl::AlphaNum": [], + "absl::Cord": ["swap", "Swap", "CopyTo", "AppendTo", "CopyToArray", + "DumpTreeStructure", "Dump", "operator="], + "absl::Duration": ["swap"], + "absl::Time": ["swap"], + "absl::TimeZone": ["swap"], + "absl::int128": [], + "absl::string_view": ["swap", "copy"], + "absl::uint128": [] + } +)"; + constexpr const char *kDefaultTypesByBase = + R"({"proto2::Message":["Swap","SerializeToString"]})"; + + constexpr const char *kDefaultTemplateTypes = R"r( + { + "absl::FunctionRef": ["operator()", "operator=", "swap"], + "absl::Hash": [], + "absl::StatusOr": ["swap"], + "absl::btree_set": ["swap"], + "absl::btree_map": ["swap"], + "absl::btree_multiset": ["swap"], + "absl::btree_multimap": ["swap"], + "absl::container_internal::hash_default_hash": [], + "absl::container_internal::hash_default_eq": [], + "absl::flat_hash_set": ["swap"], + "absl::flat_hash_map": ["swap"], + "absl::hash_internal::Hash": [], + "absl::node_hash_set": ["swap"], + "absl::node_hash_map": ["swap"], + "std::array": [], + "std::allocator": ["deallocate"], + "std::basic_string": ["swap"], + "std::basic_string_view": ["swap"], + "std::char_traits": [], + "std::deque": ["swap"], + "std::function": ["operator()", "operator=", "swap"], + "std::list": ["swap"], + "std::map": ["swap"], + "std::multiset": ["swap"], + "std::multimap": ["swap"], + "std::optional": ["swap"], + "std::pair": [], + "std::priority_queue": ["swap"], + "std::queue": ["swap"], + "std::set": ["swap"], + "std::tuple": [], + "std::unordered_set": ["swap"], + "std::unordered_map": ["swap"], + "std::unordered_multiset": ["swap"], + "std::unordered_multimap": ["swap"], + "std::variant": ["swap"], + "std::vector": ["swap"] + } +)r"; + + static constexpr const char *kDefaultFunctionAllowList = R"( + { + "absl::CordAppend": [0], + "absl::CordCat": [], + "absl::CordContains": [], + "absl::CordSplit": [], + "absl::CordJoin": [], + "absl::StringPrintf": [], + "absl::StrAppend": [0], + "absl::StrCat": [], + "absl::StrContains": [], + "absl::StrSplit": [], + "absl::StrJoin": [], + "absl::Substitute": [], + "absl::SubstituteAndAppend": [0] + } +)"; + + auto FunctionAllowListOrError = + llvm::json::parse>>( + Options.get("FunctionAllowList", kDefaultFunctionAllowList), + "FunctionAllowList"); + if (FunctionAllowListOrError) { + for (const auto &kv : FunctionAllowListOrError.get()) { + UnusedFunctionArguments[kv.first].clear(); + for (size_t v : kv.second) { + UnusedFunctionArguments[kv.first].insert(v); + } + } + } + + if (auto TypesByBaseOrError = + llvm::json::parse>( + Options.get("TypesByBase", kDefaultTypesByBase), "TypesByBase")) { + TypesByBase = TypesByBaseOrError.get(); + } + + if (auto RawTypesOrError = + llvm::json::parse>( + Options.get("RawTypes", kDefaultRawTypes), "RawTypes")) { + RawTypes = RawTypesOrError.get(); + } + + if (auto SmartPointerTypesOrError = + llvm::json::parse>( + Options.get("SmartPointerTypes", kDefaultSmartPointerTypes), + "SmartPointerTypes")) { + SmartPointerTypes = SmartPointerTypesOrError.get(); + } + + if (auto TemplateTypesOrError = + llvm::json::parse>( + Options.get("TemplateTypes", kDefaultTemplateTypes), + "TemplateTypes")) { + TemplateTypes = TemplateTypesOrError.get(); + } +} + +void UnusedNoSideEffectCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + std::string Output; + llvm::raw_string_ostream OS(Output); + { + llvm::json::OStream J(OS); + J.object([&] { + for (const auto &kv : UnusedFunctionArguments) { + J.attributeArray(kv.first(), [&] { + for (const auto &v : kv.second) { + J.value(v); + } + }); + } + }); + Options.store(Opts, "FunctionAllowList", Output); + } + + auto SerializeTypeInfo = + [&](const llvm::SmallVector &Types, const char *Name) { + llvm::json::OStream J(OS); + Output.clear(); + J.object([&] { + for (const auto &Info : Types) { + J.attributeArray(Info.Type, [&] { + for (const auto &Excl : Info.ExcludedMethods) { + J.value(Excl); + } + }); + } + }); + Options.store(Opts, Name, Output); + }; + SerializeTypeInfo(TypesByBase, "TypesByBase"); + SerializeTypeInfo(RawTypes, "RawTypes"); + SerializeTypeInfo(SmartPointerTypes, "SmartPointerTypes"); + SerializeTypeInfo(TemplateTypes, "TemplateTypes"); +} + +bool UnusedNoSideEffectCheck::hasNoSideEffects(ASTContext &AstContext, + QualType Type, + const CXXMethodDecl *Method) { + llvm::DenseSet Seen; + return hasNoSideEffectsImpl(AstContext, Type, Method, + /*AllowReferences=*/false, Seen); +} + +bool UnusedNoSideEffectCheck::hasNoSideEffectsImpl( + ASTContext &AstContext, QualType Type, const CXXMethodDecl *Method, + bool AllowReferences, llvm::DenseSet &Seen) { + if (Type.isNull()) { + return true; + } + Type = Type.getCanonicalType(); + // With recursive types (i.e. lists or trees), recursively visiting member + // variables would lead to infinite loops. OTOH, such members cannot make a + // type have side effects unless other members do so already, so we can just + // return true here. + if (Seen.count(Type)) { + return true; + } + Seen.insert(Type); + if (Type->isBuiltinType()) { + return true; + } + if (Type->isFunctionPointerType() || Type->isFunctionType()) { + return true; + } + if (Type->isReferenceType()) { + return AllowReferences; + } + if (Type->isPointerType()) { + return true; + } + + // Check known types. + const auto *Record = Type->getAs(); + if (Record) { + const auto *RecordDecl = Record->getDecl(); + const llvm::SmallVector *ExcludedList = nullptr; + + { + std::string RecordDeclName = RecordDecl->getQualifiedNameAsString(); + for (const auto &TypeInfo : RawTypes) { + if (RecordDeclName == TypeInfo.Type) { + ExcludedList = &TypeInfo.ExcludedMethods; + } + } + } + + const auto *CXXRecord = dyn_cast(RecordDecl); + + if (CXXRecord && CXXRecord->hasDefinition()) { + CXXRecord = CXXRecord->getDefinition(); + for (const auto &Base : CXXRecord->bases()) { + const auto *BaseType = Base.getType()->getAs(); + if (!BaseType) + continue; + std::string BaseTypeName = + BaseType->getDecl()->getQualifiedNameAsString(); + for (const auto &TypeInfo : TypesByBase) { + if (BaseTypeName == TypeInfo.Type) { + ExcludedList = &TypeInfo.ExcludedMethods; + } + } + } + } + + const auto *CXXTemplateSpecialization = + dyn_cast(RecordDecl); + if (CXXTemplateSpecialization) { + for (const auto TmplArg : + CXXTemplateSpecialization->getTemplateArgs().asArray()) { + if (TmplArg.getKind() == TemplateArgument::ArgKind::Type && + !hasNoSideEffectsImpl(AstContext, TmplArg.getAsType(), + /*Method=*/nullptr, AllowReferences, Seen)) { + return false; + } + if (TmplArg.getKind() == TemplateArgument::ArgKind::Pack) { + for (auto TArg : TmplArg.getPackAsArray()) { + if (TArg.getKind() == TemplateArgument::ArgKind::Type && + !hasNoSideEffectsImpl(AstContext, TArg.getAsType(), + /*Method=*/nullptr, AllowReferences, + Seen)) { + return false; + } + } + } + } + const auto TemplateName = + CXXTemplateSpecialization->getSpecializedTemplate() + ->getTemplatedDecl() + ->getQualifiedNameAsString(); + for (const auto &TypeInfo : RawTypes) { + if (TemplateName == TypeInfo.Type) { + ExcludedList = &TypeInfo.ExcludedMethods; + } + } + for (const auto &TypeInfo : TemplateTypes) { + if (TemplateName == TypeInfo.Type) { + ExcludedList = &TypeInfo.ExcludedMethods; + } + } + for (const auto &TypeInfo : SmartPointerTypes) { + if (TemplateName == TypeInfo.Type) { + ExcludedList = &TypeInfo.ExcludedMethods; + } + } + } + + // Unknown type. + if (ExcludedList == nullptr) { + // Always return false if we are calling a method. + if (Method != nullptr) { + return false; + } + // If the type is POD we return true. + if (Type.isPODType(AstContext)) { + return true; + } + // If we don't know enough about this type, conservatively return false. + if (!CXXRecord || !CXXRecord->hasDefinition()) { + return false; + } + // Otherwise, return true iff all struct members and bases are + // side-effect-free themselves, and there is no user-defined destructor. + if (CXXRecord->hasUserDeclaredDestructor()) { + return false; + } + for (const auto &Base : CXXRecord->bases()) { + if (!hasNoSideEffectsImpl(AstContext, Base.getType(), + /*Method=*/nullptr, AllowReferences, Seen)) + return false; + } + for (const auto *Field : CXXRecord->fields()) { + if (!hasNoSideEffectsImpl(AstContext, Field->getType(), + /*Method=*/nullptr, + AllowReferences || + Field->getAccess() != + AccessSpecifier::AS_public, + Seen)) + return false; + } + return true; + } + + if (Method == nullptr) { + return true; + } + + std::string MethodName = Method->getNameAsString(); + for (const auto &Excl : *ExcludedList) { + if (Excl == MethodName) { + return false; + } + } + return true; + } + + return false; +} + +} // namespace performance +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -124,6 +124,11 @@ Replaces groups of adjacent macros with an unscoped anonymous enum. +- New :doc:`performance-unused-no-side-effect + ` check. + + Finds variables that are not used and do not have user-visible side effects. + - New :doc:`portability-std-allocator-const ` check. Report use of ``std::vector`` (and similar containers of const diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -288,6 +288,7 @@ `performance-type-promotion-in-math-fn `_, "Yes" `performance-unnecessary-copy-initialization `_, "Yes" `performance-unnecessary-value-param `_, "Yes" + `performance-unused-no-side-effect `_, "Yes" `portability-restrict-system-includes `_, "Yes" `portability-simd-intrinsics `_, `portability-std-allocator-const `_, diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst @@ -0,0 +1,199 @@ +.. title:: clang-tidy - performance-unused-no-side-effect + +performance-unused-no-side-effect +================================= + +Finds variables that are not used and do not have user-visible side effects. + +This check is similar to the -Wunused-variable and -Wunused-but-set-variable +compiler warnings; however, it integrates (configurable) knowledge about +library types and functions to be able to extend these checks to more types of +variables that do not produce user-visible side effects but i.e. perform +allocations; not using such variables is almost always a programming error. + +A variable is defined to be side-effect-free and unused if: + +- It has a side-effect-free type (defined below). +- It is not passed to a function that reads its content and whose result is used. + +A type is side-effect-free if it is any of the following: + +- A type with a base class specified in `TypesByBase`. +- A type listed in `RawTypes`. +- A template type listed in `TemplateTypes` whose template arguments are + themselves side-effect-free types. +- A smart pointer (listed in `SmartPointerTypes`) whose pointee is of + side-effect-free type. +- A primitive type. +- A pointer. +- A struct/class with no user-defined destructor and members of trivial type. + +A function is considered to not read an argument unless the return value is +used in the following cases: + +- It is a method of the explicitly listed types and the argument is ``this``, + except: + + - The exceptions associated with the type (eg. ``std::vector::swap``). + - A constructor of a smart pointer type that takes a non-newly-allocated + pointer as argument. + +- A function listed in `FunctionAllowList`, if the argument is in index ``i`` and + bit ``i`` is set in the corresponding allow list entry. + +Examples of snippets that trigger the check with the default configuration: + +.. code-block:: c++ + + void f() { + std::vector vec; + vec.push_back(1); + } + + +Examples that do *not* trigger the check: + +.. code-block:: c++ + + void f() { + std::vector vec; + vec.push_back(1); + std::cout << vec[0]; + } + + void g(std::vector& outp) { + std::vector vec; + vec.push_back(1); + vec.swap(outp); + } + + + +Configuration +------------- +The types that are considered unused can be specified in the check +configuration. As more than just a simple list of types is used for +configuration, the configuration options are JSON. + +`TypesByBase` +^^^^^^^^^^^^^ +Types in this group will cause any type that derives from them to be considered +side effect free. All methods on such types will also be considered side effect +free, except for the exceptions explicitly listed as the value of the mapping. + +The default value for this configuration option is + +.. code-block:: json + + {"proto2::Message": ["Swap", "SerializeToString"]} + +`RawTypes` +^^^^^^^^^^ +Similar to `TypesByBase`, except it only applies to the types themselves. + +The default value for this configuration option is + +.. code-block:: json + + { + "absl::AlphaNum": [], + "absl::Cord": ["swap", "Swap", "CopyTo", "AppendTo", "CopyToArray", + "DumpTreeStructure", "Dump", "operator="], + "absl::Duration": ["swap"], + "absl::Time": ["swap"], + "absl::TimeZone": ["swap"], + "absl::int128": [], + "absl::string_view": ["swap", "copy"], + "absl::uint128": [] + } + +`TemplateTypes` +^^^^^^^^^^^^^^^ +Similar to `TypesByBase`, except it only applies to specializations of the +specified template with side-effect-free types. + +The default value for this configuration option is + +.. code-block:: json + + { + "absl::FunctionRef": ["operator()", "operator=", "swap"], + "absl::Hash": [], + "absl::StatusOr": ["swap"], + "absl::btree_set": ["swap"], + "absl::btree_map": ["swap"], + "absl::btree_multiset": ["swap"], + "absl::btree_multimap": ["swap"], + "absl::container_internal::hash_default_hash": [], + "absl::container_internal::hash_default_eq": [], + "absl::flat_hash_set": ["swap"], + "absl::flat_hash_map": ["swap"], + "absl::hash_internal::Hash": [], + "absl::node_hash_set": ["swap"], + "absl::node_hash_map": ["swap"], + "std::array": [], + "std::allocator": ["deallocate"], + "std::basic_string": ["swap"], + "std::basic_string_view": ["swap"], + "std::char_traits": [], + "std::deque": ["swap"], + "std::function": ["operator()", "operator=", "swap"], + "std::list": ["swap"], + "std::map": ["swap"], + "std::multiset": ["swap"], + "std::multimap": ["swap"], + "std::optional": ["swap"], + "std::pair": [], + "std::priority_queue": ["swap"], + "std::queue": ["swap"], + "std::set": ["swap"], + "std::tuple": [], + "std::unordered_set": ["swap"], + "std::unordered_map": ["swap"], + "std::unordered_multiset": ["swap"], + "std::unordered_multimap": ["swap"], + "std::variant": ["swap"], + "std::vector": ["swap"] + } + +`SmartPointerTypes` +^^^^^^^^^^^^^^^^^^^ +Similar to `TemplateTypes`, except constructors are handled in a special way: +only constructors that take a newly allocated raw pointer are considered +side-effect free. + +The default value for this configuration option is + +.. code-block:: json + + { + "std::unique_ptr": ["swap", "operator=", "reset"], + "std::shared_ptr": ["swap", "operator=", "reset"] + } + +`FunctionAllowList` +^^^^^^^^^^^^^^^^^^^ +List of functions that are considered not to read some of their arguments +unless their return value is read. Arguments are identified by index (starting +from 0): the value `i` being present in the array implies that the `i`-th +argument is unused. If the array is empty, all arguments are unused. + +The default value for this configuration option is + +.. code-block:: json + + { + "absl::CordAppend": [0], + "absl::CordCat": [], + "absl::CordContains": [], + "absl::CordSplit": [], + "absl::CordJoin": [], + "absl::StringPrintf": [], + "absl::StrAppend": [0], + "absl::StrCat": [], + "absl::StrContains": [], + "absl::StrSplit": [], + "absl::StrJoin": [], + "absl::Substitute": [], + "absl::SubstituteAndAppend": [0] + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp @@ -0,0 +1,144 @@ +// RUN: %check_clang_tidy %s performance-unused-no-side-effect %t -- \ +// RUN: -config="{CheckOptions: [{key: \"performance-unused-no-side-effect.TypesByBase\", value: '{\"Marker\":[\"swap\"]}'}]}" + +namespace std { + +template +struct char_traits {}; + +template +struct allocator {}; + +template < + class CharT, + class Traits = std::char_traits, + class Allocator = std::allocator> +class basic_string { +public: + basic_string &operator+=(const char *); + ~basic_string(); +}; + +using string = basic_string; + +class thread { +public: + ~thread(); +}; + +template > +class vector { +public: + void push_back(T); +}; + +template +class unique_ptr { +public: + unique_ptr(T *); +}; + +} // namespace std + +struct Marker { +}; + +struct SideEffectFree : Marker { + SideEffectFree(); + ~SideEffectFree(); + void swap(SideEffectFree *); +}; + +struct POD { + int A; +}; + +template +struct CustomPair { + T t; + U u; +}; + +void f(const std::string &); + +void unusedString() { + std::string SomeString; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect] +} + +void unusedStringWithMethodCall() { + std::string SomeString; + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: variable 'SomeString' is never read [performance-unused-no-side-effect] + SomeString += "hi"; +} + +void unusedStdThread() { + std::thread T; +} + +void usedString() { + std::string SomeString; + SomeString += "hi"; + f(SomeString); +} + +void vectorString() { + std::vector VecString; + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: variable 'VecString' is never read [performance-unused-no-side-effect] + VecString.push_back(std::string{}); +} + +void vectorThread() { + std::vector VecThread; + VecThread.push_back(std::thread{}); +} + +void withMarker() { + SideEffectFree M; + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: variable 'M' is never read [performance-unused-no-side-effect] +} + +void withMarkerAndExcludedMethod(SideEffectFree *S) { + SideEffectFree M; + M.swap(S); +} + +void withPOD() { + POD P; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'P' is never read [performance-unused-no-side-effect] + P.A = 1; +} + +int withPODReturn() { + POD P; + P.A = 1; + return P.A; +} + +void withLoops() { + std::vector VecLoops; + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: variable 'VecLoops' is never read [performance-unused-no-side-effect] + for (int i = 0; i < 15; i++) { + VecLoops.push_back(1); + } + int j = 0; + while (j < 1) { + VecLoops.push_back(2); + j++; + } + (VecLoops).push_back(3); +} + +void withSmartPointer(int *Arg) { + std::unique_ptr Unused(new int); + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: variable 'Unused' is never read [performance-unused-no-side-effect] + + std::unique_ptr Used(Arg); +} + +void withCustomPair() { + CustomPair> Unused{}; + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: variable 'Unused' is never read [performance-unused-no-side-effect] + + CustomPair Used; +}