llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: None (vabridgers) <details> <summary>Changes</summary> Adds a new clang-tidy check that does a scope reduction analysis, supporting SEI DCL19-C, MISRA C++:2008 Rule 3-4-1, and MISRA +C:2012 Rule 8-9. --- Patch is 21.26 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/175429.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/misc/CMakeLists.txt (+1) - (modified) clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp (+2) - (added) clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp (+282) - (added) clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h (+28) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst (+50) - (added) clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp (+181) ``````````diff diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index e34b0cf687be3..c1c3dd656b811 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -37,6 +37,7 @@ add_clang_library(clangTidyMiscModule STATIC OverrideWithDifferentVisibilityCheck.cpp PredictableRandCheck.cpp RedundantExpressionCheck.cpp + ScopeReductionCheck.cpp StaticAssertCheck.cpp ThrowByValueCatchByReferenceCheck.cpp UnconventionalAssignOperatorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index f8550b30b9789..899c27033ba3b 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -26,6 +26,7 @@ #include "OverrideWithDifferentVisibilityCheck.h" #include "PredictableRandCheck.h" #include "RedundantExpressionCheck.h" +#include "ScopeReductionCheck.h" #include "StaticAssertCheck.h" #include "ThrowByValueCatchByReferenceCheck.h" #include "UnconventionalAssignOperatorCheck.h" @@ -75,6 +76,7 @@ class MiscModule : public ClangTidyModule { CheckFactories.registerCheck<PredictableRandCheck>("misc-predictable-rand"); CheckFactories.registerCheck<RedundantExpressionCheck>( "misc-redundant-expression"); + CheckFactories.registerCheck<ScopeReductionCheck>("misc-scope-reduction"); CheckFactories.registerCheck<StaticAssertCheck>("misc-static-assert"); CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>( "misc-throw-by-value-catch-by-reference"); diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp new file mode 100644 index 0000000000000..51fb0598fa765 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.cpp @@ -0,0 +1,282 @@ +//===----------------------------------------------------------------------===// +// +// 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 "../utils/ASTUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::misc { + +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) { + 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 (size_t i = 1; i < UseScopeChains.size(); ++i) { + const CompoundStmt *CommonScope = nullptr; + + // Find first scope that appears in both chains (common ancestor) + for (const auto *Scope1 : UseScopeChains[0]) { + for (const auto *Scope2 : UseScopeChains[i]) { + 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)) { + const ForStmt *CommonForLoop = nullptr; + bool AllUsesInSameForLoop = true; + + for (const auto *Use : Uses) { + const ForStmt *ContainingForLoop = nullptr; + const Stmt *Current = Use; + + // Walk up the AST to find a containing ForStmt + while (Current) { + auto ParentNodes = Parents.getParents(*Current); + if (ParentNodes.empty()) + break; + + if (const auto *FS = ParentNodes[0].get<ForStmt>()) { + ContainingForLoop = FS; + break; + } + + const Stmt *Parent = ParentNodes[0].get<Stmt>(); + if (!Parent) { + // Handle Decl parents like we do in the existing logic + if (const auto *DeclParent = ParentNodes[0].get<Decl>()) { + auto DeclParentNodes = Parents.getParents(*DeclParent); + if (!DeclParentNodes.empty()) + Parent = DeclParentNodes[0].get<Stmt>(); + } + if (!Parent) + break; + } + Current = Parent; + } + + if (!ContainingForLoop) { + AllUsesInSameForLoop = false; + break; + } + + if (!CommonForLoop) { + CommonForLoop = ContainingForLoop; + } else if (CommonForLoop != ContainingForLoop) { + AllUsesInSameForLoop = false; + break; + } + } + + if (AllUsesInSameForLoop && CommonForLoop) { + diag(Var->getLocation(), + "variable '%0' can be declared in for-loop initialization") + << Var->getName(); + return; + } + } +} + +} // namespace clang::tidy::misc diff --git a/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h new file mode 100644 index 0000000000000..690f58feb93f7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/ScopeReductionCheck.h @@ -0,0 +1,28 @@ +//===----------------------------------------------------------------------===// +// +// 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_MISC_SCOPEREDUCTIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SCOPEREDCUTIONCHECK_H + +#include "../../clang-tidy/utils/DeclRefExprUtils.h" +#include "../ClangTidyCheck.h" +#include "clang/AST/ASTContext.h" + +namespace clang::tidy::misc { + +class ScopeReductionCheck : public ClangTidyCheck { +public: + ScopeReductionCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace clang::tidy::misc + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SCOPEREDCUTIONCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 7747c5d0e96a7..2566b4f5ca730 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -294,6 +294,11 @@ New checks Finds virtual function overrides with different visibility than the function in the base class. +- New :doc:`misc-scope-reduction + <clang-tidy/checks/misc/scope-reduction>` check. + + Checks for opportunities to minimize scope of local variables. + - New :doc:`readability-inconsistent-ifelse-braces <clang-tidy/checks/readability/inconsistent-ifelse-braces>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 3dabf887dc2e1..248f237d9c318 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -280,6 +280,7 @@ Clang-Tidy Checks :doc:`misc-override-with-different-visibility <misc/override-with-different-visibility>`, :doc:`misc-predictable-rand <misc/predictable-rand>`, :doc:`misc-redundant-expression <misc/redundant-expression>`, "Yes" + :doc:`misc-scope-reduction <misc/scope-reduction>`, :doc:`misc-static-assert <misc/static-assert>`, "Yes" :doc:`misc-throw-by-value-catch-by-reference <misc/throw-by-value-catch-by-reference>`, :doc:`misc-unconventional-assign-operator <misc/unconventional-assign-operator>`, diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst new file mode 100644 index 0000000000000..11a85f406f5bc --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc/scope-reduction.rst @@ -0,0 +1,50 @@ +.. title:: clang-tidy - misc-scope-reduction + +misc-scope-reduction +=========================== + +Detects local variables in functions whose scopes can be minimized. This check +covers guidelines described by SEI DCL19-C, MISRA C++:2008 Rule 3-4-1, and MISRA +C:2012 Rule 8-9. + +Examples: + +.. code-block:: cpp + + void test_deep_nesting() { + int deep = 1; // 'deep' can be declared in a smaller scope + if (true) { + if (true) { + if (true) { + if (true) { + int result = deep * 4; + } + } + } + } + } + + void test_switch_multiple_cases(int value) { + int accumulator = 0; // 'accumulator' can be declared in a smaller scope + switch (value) { + case 1: + accumulator += 10; + break; + case 2: + accumulator += 20; + break; + } + } + + void test_for_loop_expressions() { + int i; // 'i' can be declared in the for-loop initialization + for (i = 0; i < 5; i++) { + // loop body + } + } + +References +---------- +This check corresponds to the CERT C Coding Standard rules +`DCL19-C. Minimize the scope of variables and functions +<https://wiki.sei.cmu.edu/confluence/spaces/c/pages/87152335/DCL19-C.+Minimize+the+scope+of+variables+and+functions>`_. diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp new file mode 100644 index 0000000000000..ba9e0e9d3c466 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/scope-reduction.cpp @@ -0,0 +1,181 @@ +// RUN: %check_clang_tidy %s misc-scope-reduction %t -- -- + +// Test case 1: Variable can be moved to smaller scope (if-block) +void test_if_scope() { + int x = 42; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'x' can be declared in a smaller scope + if (true) { + int y = x + 1; + } +} + +// Test case 2: Variable used across multiple scopes - should NOT warn +int test_multiple_scopes(int v) { + int y = 0; // Should NOT warn - used in if-block and return + if (v) { + y = 10; + } + return y; +} + +// Test case 3: Variable can be moved to nested if-block +void test_nested_if() { + int a = 5; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'a' can be declared in a smaller scope + if (true) { + if (true) { + int b = a * 2; + } + } +} + +// Test case 4: Variable used in same scope - should NOT warn +void test_same_scope() { + int x = 10; // Should NOT warn - used in same scope + int y = x + 5; +} + +// Test case 5: Variable can be moved to while loop body +void test_while_loop() { + int counter = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'counter' can be declared in a smaller scope + while (true) { + counter++; + if (counter > 10) break; + } +} + +// Test case 6: Variable used in multiple branches of same if-statement +void test_if_branches(bool condition) { + int value = 100; // Should NOT warn - used in both branches + if (condition) { + value *= 2; + } else { + value /= 2; + } +} + +// Test case 7: Variable can be moved to for-loop body +void test_for_loop_body() { + int temp = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'temp' can be declared in a smaller scope + for (int i = 0; i < 10; i++) { + temp = i * i; + } +} + +// Test case 8: Variable used in for-loop expressions - should NOT warn (current limitation) +void test_for_loop_expressions() { + int i; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'i' can be declared in for-loop initialization + for (i = 0; i < 5; i++) { + // loop body + } +} + +// Test case 9: Variable can be moved to switch case +void test_switch_case(int value) { + int result = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'result' can be declared in a smaller scope + switch (value) { + case 1: + result = 10; + break; + default: + break; + } +} + +// Test case 10: Variable used across multiple switch cases - should NOT warn +void test_switch_multiple_cases(int value) { + int accumulator = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'accumulator' can be declared in a smaller scope + switch (value) { + case 1: + accumulator += 10; + break; + case 2: + accumulator += 20; + break; + } +} + +// Test case 11: Variable with complex initialization can be moved +void test_complex_init() { + int complex = (5 + 3) * 2; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'complex' can be declared in a smaller scope + if (true) { + int doubled = complex * 2; + } +} + +// Test case 12: Multiple variables, some can be moved, some cannot +int test_mixed_variables(bool flag) { + int movable = 10; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'movable' can be declared in a smaller scope + int unmovable = 20; // Should NOT warn - used across scopes + + if (flag) { + int local = movable + 5; + unmovable += 1; + } + + return unmovable; +} + +// Test case 13: Variable in try-catch block +void test_try_catch() { + int error_code = 0; // CHECK-MESSAGES: :[[@LINE]]:7: warning: variable 'error_code' can be declared in a smaller scope + try { + error_code = 404; + } catch (...) { + // handle exception + } +} + +// Test case 14: Variable used in catch block and try block - should NOT warn +void test_try_catch_shared() { + int shared = 0; // Should NOT warn - used in both try and catch + tr... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/175429 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
