================ @@ -0,0 +1,283 @@ +//===----------------------------------------------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +// This checker uses a 7-step algorithm to accomplish scope analysis of a +// variable and determine if it's in too large a scope. Note that the +// clang-tidy framework is aimed mainly at supporting text-manipulation, +// diagnostics, or common AST patterns. Scope reduction analysis is +// quite specialized, and there's not much support specifically for +// those steps. Perhaps someone else knows better and can help simplify +// this code in a more concrete way other than simply suggesting it can +// be simpler. +// +// The 7-step algorithm used by this checker for scope reduction analysis is: +// 1) Filter out variables declared in for-loop initializations +// - Those variables are already in optimal scope, and can be skipped +// 2) Collect variable uses +// - find all DeclRefExpr nodes that reference the variable +// 3) Build scope chains +// - for each use, find all compound statements that contain it (from +// innermost to outermost) +// 4) Find the innermost compound statement that contains all uses +// - This is the smallest scope where the variable could be declared +// 5) Find declaration scope +// - Locate the compound statement containing the variable declaration +// 6) Verify nesting +// - Ensure the usage scope is actually nested within the declaration scope +// 7) Alternate analysis - check for for-loop initialization opportunity +// - This is only run if compound stmt analysis didn't find smaller scope +// - Only check local variables, not parameters +// - Determine if all uses are within the same for-loop and suggest +// for-loop initialization +// +// The algo works by finding the smallest scope that could contain the variable +// declaration while still encompassing all it's uses. + +#include "ScopeReductionCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::misc { + +// TODO: Try using utils::decl_ref_expr::allDeclRefExprs here. +static void +collectVariableUses(const Stmt *S, const VarDecl *Var, + llvm::SmallVector<const DeclRefExpr *, 8> &Uses) { + if (!S) + return; + + if (const auto *DRE = dyn_cast<DeclRefExpr>(S)) { + if (DRE->getDecl() == Var) + Uses.push_back(DRE); + } + + for (const Stmt *Child : S->children()) + collectVariableUses(Child, Var, Uses); +} + +void ScopeReductionCheck::registerMatchers(MatchFinder *Finder) { + // TODO: Try adding unless(hasParent(declStmt(hasParent(forStmt( to matcher + // to simplify check code. + Finder->addMatcher(varDecl(hasLocalStorage()).bind("var"), this); +} + +void ScopeReductionCheck::check( + const ast_matchers::MatchFinder::MatchResult &Result) { + const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var"); + if (!Var) + return; + + // Step 1: Filter out variables declared in for-loop initializations + // These variables are already in their optimal scope and shouldn't be + // analyzed + auto &Parents = Result.Context->getParentMapContext(); + auto ParentNodes = Parents.getParents(DynTypedNode::create(*Var)); + + if (!ParentNodes.empty()) { + if (const auto *Parent = ParentNodes[0].get<Stmt>()) { + if (isa<DeclStmt>(Parent)) { + // Check if DeclStmt's parent is ForStmt + auto GrandParentNodes = Parents.getParents(*Parent); + if (!GrandParentNodes.empty()) { + if (const auto *GrandParent = GrandParentNodes[0].get<Stmt>()) { + if (isa<ForStmt>(GrandParent)) + return; // Skip for-loop declared variables + } + } + } + } + } + + // auto *Context = Result.Context; + auto *Function = dyn_cast<FunctionDecl>(Var->getDeclContext()); + if (!Function || !Function->hasBody()) + return; + + // Step 2: Collect all uses of this variable within the function + llvm::SmallVector<const DeclRefExpr *, 8> Uses; + collectVariableUses(Function->getBody(), Var, Uses); + + // No uses, return with no diagnostics + if (Uses.empty()) + return; + + // Step 3: For each variable use, find all compound statements that contain it + // This builds a "scope chain" from innermost to outermost for each use + const CompoundStmt *InnermostScope = nullptr; + + // For each use, find all compound statements that contain it + llvm::SmallVector<llvm::SmallVector<const CompoundStmt *, 4>, 8> + UseScopeChains; + + for (const auto *Use : Uses) { + llvm::SmallVector<const CompoundStmt *, 4> ScopeChain; + const Stmt *Current = Use; + + // Walk up the AST from this use to fins all containing compound stmts + while (Current) { + auto ParentNodes = Parents.getParents(*Current); + if (ParentNodes.empty()) + break; + + const Stmt *Parent = ParentNodes[0].get<Stmt>(); + if (!Parent) { + // Try to get Decl parent and continue from there + if (const auto *DeclParent = ParentNodes[0].get<Decl>()) { + auto DeclParentNodes = Parents.getParents(*DeclParent); + if (!DeclParentNodes.empty()) + Parent = DeclParentNodes[0].get<Stmt>(); + } + if (!Parent) + break; + } + + if (const auto *CS = dyn_cast<CompoundStmt>(Parent)) + ScopeChain.push_back(CS); + + Current = Parent; + } + + if (!ScopeChain.empty()) + UseScopeChains.push_back(ScopeChain); + } + + // Step 4: Find the innermost scope that contains all uses + // This is the smallest scope where var could be declared + if (!UseScopeChains.empty()) { + // Start with first use's innermost scope + InnermostScope = UseScopeChains[0][0]; + + // For each subsequent use, find common ancestor scope + for (const auto &ScopeChain : llvm::drop_begin(UseScopeChains)) { + const CompoundStmt *CommonScope = nullptr; + + // Find first scope that appears in both chains (common ancestor) + for (const auto *Scope1 : UseScopeChains[0]) { + for (const auto *Scope2 : ScopeChain) { + if (Scope1 == Scope2) { + CommonScope = Scope1; + break; + } + } + if (CommonScope) + break; + } + + if (CommonScope) + InnermostScope = CommonScope; + } + } + + // Step 5: Check if current var declaration is broader than necessary + if (InnermostScope) { + // Find the compound statement containing the variable declaration + const DynTypedNode Current = DynTypedNode::create(*Var); + const CompoundStmt *VarScope = nullptr; + + auto ParentNodes = Parents.getParents(Current); + while (!ParentNodes.empty()) { + const Stmt *Parent = ParentNodes[0].get<Stmt>(); + if (!Parent) + break; + + if (const auto *CS = dyn_cast<CompoundStmt>(Parent)) { + VarScope = CS; + break; + } + ParentNodes = Parents.getParents(*Parent); + } + + // Step 6: Verify that usage scope is nested within decl scope + if (VarScope && VarScope != InnermostScope) { + // Walk up from innermost usage to see if the decl scope is reached + const Stmt *CheckScope = InnermostScope; + bool IsNested = false; + + while (CheckScope) { + auto CheckParents = Parents.getParents(*CheckScope); + if (CheckParents.empty()) + break; + + const Stmt *CheckParent = CheckParents[0].get<Stmt>(); + if (CheckParent == VarScope) { + IsNested = true; + break; + } + CheckScope = CheckParent; + } + + // Only report if the usage scope is truly nested within the decl scope + if (IsNested) { + diag(Var->getLocation(), + "variable '%0' can be declared in a smaller scope") + << Var->getName(); + return; // early exit + } + } + } + + // Step 7: Alternative analysis - check for for-loop initialization + // opportunity This only runs if the compound statement analysis didn't find a + // smaller scope Only check local variables, not parameters + if (!isa<ParmVarDecl>(Var)) { ---------------- vabridgers wrote:
As implemented yes, the case came up when implementing scope reduction analysis for for-loops as an alternative to the main analysis. But I believe parameters should be excluded for all cases. Your thoughts? https://github.com/llvm/llvm-project/pull/175429 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
