From 7538b3604519b03d32221cdcc346cc1c181b50fb Mon Sep 17 00:00:00 2001 From: Wei Yi Tee Date: Mon, 19 Sep 2022 16:56:35 +0000 Subject: [PATCH] [clang][dataflow] Replace usage of deprecated functions with the optional check - Update `transfer` and `diagnose` to take `const CFGElement *` as input in `Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel`. - Update `clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp` accordingly. - Rename `runDataflowAnalysisOnCFG` to `runDataflowAnalysis` and remove the deprecated `runDataflowAnalysis` (this was only used by the now updated optional check). Reviewed By: gribozavr2, sgatev Differential Revision: https://reviews.llvm.org/D133930 --- .../bugprone/UncheckedOptionalAccessCheck.cpp | 7 +- .../Analysis/FlowSensitive/DataflowAnalysis.h | 40 +---------- .../Models/UncheckedOptionalAccessModel.h | 14 ++-- .../Models/UncheckedOptionalAccessModel.cpp | 84 ++++++++++++---------- .../UncheckedOptionalAccessModelTest.cpp | 12 +--- 5 files changed, 59 insertions(+), 98 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp index 5ee1a01..b2f6b9e 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp @@ -59,12 +59,11 @@ analyzeFunction(const FunctionDecl &FuncDecl, ASTContext &ASTCtx) { BlockToOutputState = dataflow::runDataflowAnalysis( *Context, Analysis, Env, [&ASTCtx, &Diagnoser, &Diagnostics]( - const CFGStmt &Stmt, + const CFGElement &Elt, const DataflowAnalysisState &State) mutable { - auto StmtDiagnostics = - Diagnoser.diagnose(ASTCtx, Stmt.getStmt(), State.Env); - llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics)); + auto EltDiagnostics = Diagnoser.diagnose(ASTCtx, &Elt, State.Env); + llvm::move(EltDiagnostics, std::back_inserter(Diagnostics)); }); if (!BlockToOutputState) return llvm::None; diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h index 4e084d57..098c13c 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h @@ -136,9 +136,6 @@ template struct DataflowAnalysisState { Environment Env; }; -// FIXME: Rename to `runDataflowAnalysis` after usages of the overload that -// applies to `CFGStmt` have been replaced. -// /// Performs dataflow analysis and returns a mapping from basic block IDs to /// dataflow analysis states that model the respective basic blocks. The /// returned vector, if any, will have the same size as the number of CFG @@ -149,7 +146,7 @@ template struct DataflowAnalysisState { template llvm::Expected>>> -runDataflowAnalysisOnCFG( +runDataflowAnalysis( const ControlFlowContext &CFCtx, AnalysisT &Analysis, const Environment &InitEnv, std::function -llvm::Expected>>> -runDataflowAnalysis( - const ControlFlowContext &CFCtx, AnalysisT &Analysis, - const Environment &InitEnv, - std::function &)> - PostVisitStmt = nullptr) { - std::function &)> - PostVisitCFG = nullptr; - if (PostVisitStmt) { - PostVisitCFG = - [&PostVisitStmt]( - const CFGElement &Element, - const DataflowAnalysisState &State) { - if (auto Stmt = Element.getAs()) { - PostVisitStmt(*Stmt, State); - } - }; - } - return runDataflowAnalysisOnCFG(CFCtx, Analysis, InitEnv, PostVisitCFG); -} - /// Abstract base class for dataflow "models": reusable analysis components that /// model a particular aspect of program semantics in the `Environment`. For /// example, a model may capture a type and its related functions. diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h index 25054de..66aabb5 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -15,10 +15,10 @@ #define CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H #include "clang/AST/ASTContext.h" -#include "clang/AST/Stmt.h" +#include "clang/Analysis/CFG.h" +#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h" #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h" #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" -#include "clang/Analysis/FlowSensitive/MatchSwitch.h" #include "clang/Analysis/FlowSensitive/NoopLattice.h" #include "clang/Basic/SourceLocation.h" #include @@ -45,14 +45,14 @@ class UncheckedOptionalAccessModel : public DataflowAnalysis { public: UncheckedOptionalAccessModel( - ASTContext &AstContext, UncheckedOptionalAccessModelOptions Options = {}); + ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options = {}); /// Returns a matcher for the optional classes covered by this model. static ast_matchers::DeclarationMatcher optionalClassDecl(); static NoopLattice initialElement() { return {}; } - void transfer(const Stmt *Stmt, NoopLattice &State, Environment &Env); + void transfer(const CFGElement *Elt, NoopLattice &L, Environment &Env); bool compareEquivalent(QualType Type, const Value &Val1, const Environment &Env1, const Value &Val2, @@ -63,7 +63,7 @@ public: Environment &MergedEnv) override; private: - MatchSwitch> TransferMatchSwitch; + CFGMatchSwitch> TransferMatchSwitch; }; class UncheckedOptionalAccessDiagnoser { @@ -71,11 +71,11 @@ public: UncheckedOptionalAccessDiagnoser( UncheckedOptionalAccessModelOptions Options = {}); - std::vector diagnose(ASTContext &Context, const Stmt *Stmt, + std::vector diagnose(ASTContext &Ctx, const CFGElement *Elt, const Environment &Env); private: - MatchSwitch> + CFGMatchSwitch> DiagnoseMatchSwitch; }; diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index eef3cc8..1ffd886 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -18,8 +18,9 @@ #include "clang/AST/ExprCXX.h" #include "clang/AST/Stmt.h" #include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Analysis/CFG.h" +#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h" #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" -#include "clang/Analysis/FlowSensitive/MatchSwitch.h" #include "clang/Analysis/FlowSensitive/NoopLattice.h" #include "clang/Analysis/FlowSensitive/Value.h" #include "clang/Basic/SourceLocation.h" @@ -559,41 +560,42 @@ auto buildTransferMatchSwitch( // lot of duplicated work (e.g. string comparisons), consider providing APIs // that avoid it through memoization. auto IgnorableOptional = ignorableOptional(Options); - return MatchSwitchBuilder() + return CFGMatchSwitchBuilder() // Attach a symbolic "has_value" state to optional values that we see for // the first time. - .CaseOf( + .CaseOfCFGStmt( expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()), initializeOptionalReference) // make_optional - .CaseOf(isMakeOptionalCall(), transferMakeOptionalCall) + .CaseOfCFGStmt(isMakeOptionalCall(), transferMakeOptionalCall) // optional::optional - .CaseOf( + .CaseOfCFGStmt( isOptionalInPlaceConstructor(), [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { assignOptionalValue(*E, State, State.Env.getBoolLiteralValue(true)); }) - .CaseOf( + .CaseOfCFGStmt( isOptionalNulloptConstructor(), [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { assignOptionalValue(*E, State, State.Env.getBoolLiteralValue(false)); }) - .CaseOf(isOptionalValueOrConversionConstructor(), - transferValueOrConversionConstructor) + .CaseOfCFGStmt(isOptionalValueOrConversionConstructor(), + transferValueOrConversionConstructor) // optional::operator= - .CaseOf(isOptionalValueOrConversionAssignment(), - transferValueOrConversionAssignment) - .CaseOf(isOptionalNulloptAssignment(), - transferNulloptAssignment) + .CaseOfCFGStmt( + isOptionalValueOrConversionAssignment(), + transferValueOrConversionAssignment) + .CaseOfCFGStmt(isOptionalNulloptAssignment(), + transferNulloptAssignment) // optional::value - .CaseOf( + .CaseOfCFGStmt( valueCall(IgnorableOptional), [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { @@ -601,22 +603,25 @@ auto buildTransferMatchSwitch( }) // optional::operator*, optional::operator-> - .CaseOf(valueOperatorCall(IgnorableOptional), - [](const CallExpr *E, const MatchFinder::MatchResult &, - LatticeTransferState &State) { - transferUnwrapCall(E, E->getArg(0), State); - }) + .CaseOfCFGStmt(valueOperatorCall(IgnorableOptional), + [](const CallExpr *E, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + transferUnwrapCall(E, E->getArg(0), State); + }) // optional::has_value - .CaseOf(isOptionalMemberCallWithName("has_value"), - transferOptionalHasValueCall) + .CaseOfCFGStmt( + isOptionalMemberCallWithName("has_value"), + transferOptionalHasValueCall) // optional::operator bool - .CaseOf(isOptionalMemberCallWithName("operator bool"), - transferOptionalHasValueCall) + .CaseOfCFGStmt( + isOptionalMemberCallWithName("operator bool"), + transferOptionalHasValueCall) // optional::emplace - .CaseOf( + .CaseOfCFGStmt( isOptionalMemberCallWithName("emplace"), [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { @@ -625,7 +630,7 @@ auto buildTransferMatchSwitch( }) // optional::reset - .CaseOf( + .CaseOfCFGStmt( isOptionalMemberCallWithName("reset"), [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { @@ -634,21 +639,22 @@ auto buildTransferMatchSwitch( }) // optional::swap - .CaseOf(isOptionalMemberCallWithName("swap"), - transferSwapCall) + .CaseOfCFGStmt(isOptionalMemberCallWithName("swap"), + transferSwapCall) // std::swap - .CaseOf(isStdSwapCall(), transferStdSwapCall) + .CaseOfCFGStmt(isStdSwapCall(), transferStdSwapCall) // opt.value_or("").empty() - .CaseOf(isValueOrStringEmptyCall(), transferValueOrStringEmptyCall) + .CaseOfCFGStmt(isValueOrStringEmptyCall(), + transferValueOrStringEmptyCall) // opt.value_or(X) != X - .CaseOf(isValueOrNotEqX(), transferValueOrNotEqX) + .CaseOfCFGStmt(isValueOrNotEqX(), transferValueOrNotEqX) // returns optional - .CaseOf(isCallReturningOptional(), - transferCallReturningOptional) + .CaseOfCFGStmt(isCallReturningOptional(), + transferCallReturningOptional) .Build(); } @@ -677,9 +683,9 @@ auto buildDiagnoseMatchSwitch( // lot of duplicated work (e.g. string comparisons), consider providing APIs // that avoid it through memoization. auto IgnorableOptional = ignorableOptional(Options); - return MatchSwitchBuilder>() + return CFGMatchSwitchBuilder>() // optional::value - .CaseOf( + .CaseOfCFGStmt( valueCall(IgnorableOptional), [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, const Environment &Env) { @@ -687,7 +693,7 @@ auto buildDiagnoseMatchSwitch( }) // optional::operator*, optional::operator-> - .CaseOf( + .CaseOfCFGStmt( valueOperatorCall(IgnorableOptional), [](const CallExpr *E, const MatchFinder::MatchResult &, const Environment &Env) { @@ -708,10 +714,10 @@ UncheckedOptionalAccessModel::UncheckedOptionalAccessModel( : DataflowAnalysis(Ctx), TransferMatchSwitch(buildTransferMatchSwitch(Options)) {} -void UncheckedOptionalAccessModel::transfer(const Stmt *S, NoopLattice &L, - Environment &Env) { +void UncheckedOptionalAccessModel::transfer(const CFGElement *Elt, + NoopLattice &L, Environment &Env) { LatticeTransferState State(L, Env); - TransferMatchSwitch(*S, getASTContext(), State); + TransferMatchSwitch(*Elt, getASTContext(), State); } bool UncheckedOptionalAccessModel::compareEquivalent(QualType Type, @@ -745,8 +751,8 @@ UncheckedOptionalAccessDiagnoser::UncheckedOptionalAccessDiagnoser( : DiagnoseMatchSwitch(buildDiagnoseMatchSwitch(Options)) {} std::vector UncheckedOptionalAccessDiagnoser::diagnose( - ASTContext &Context, const Stmt *Stmt, const Environment &Env) { - return DiagnoseMatchSwitch(*Stmt, Context, Env); + ASTContext &Ctx, const CFGElement *Elt, const Environment &Env) { + return DiagnoseMatchSwitch(*Elt, Ctx, Env); } } // namespace dataflow diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index c06cd3a..845209b 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -14,10 +14,8 @@ #include "clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h" #include "clang/Basic/SourceLocation.h" #include "clang/Tooling/Tooling.h" -#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLExtras.h" -#include "llvm/ADT/StringExtras.h" #include "llvm/Support/Error.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -1248,13 +1246,9 @@ private: Diagnoser = UncheckedOptionalAccessDiagnoser(Options)]( ASTContext &Ctx, const CFGElement &Elt, const TypeErasedDataflowAnalysisState &State) mutable { - auto Stmt = Elt.getAs(); - if (!Stmt) { - return; - } - auto StmtDiagnostics = - Diagnoser.diagnose(Ctx, Stmt->getStmt(), State.Env); - llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics)); + auto EltDiagnostics = + Diagnoser.diagnose(Ctx, &Elt, State.Env); + llvm::move(EltDiagnostics, std::back_inserter(Diagnostics)); }) .withASTBuildArgs( {"-fsyntax-only", "-std=c++17", "-Wno-undefined-inline"}) -- 2.7.4