Author: Jan Voung Date: 2025-03-17T16:04:15-04:00 New Revision: 6f659b0060d615435ceec53de407a8084656bc98
URL: https://github.com/llvm/llvm-project/commit/6f659b0060d615435ceec53de407a8084656bc98 DIFF: https://github.com/llvm/llvm-project/commit/6f659b0060d615435ceec53de407a8084656bc98.diff LOG: [clang][dataflow] For bugprone-unchecked-optional-access report range (#131055) Report the range in diagnostics, in addition to the location in case the range helps disambiguate a little in chained `->` expressions. ``` b->a->f->x = 1; ^~~~~~~ ``` instead of just: ``` b->a->f->x = 1; ^ ``` As a followup we should probably also report the location/range of an `->` if that operator is used. Like: ``` b->a->f->x = 1; ^~ ``` Added: Modified: clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp index 0a0e212f345ed..0b51d5677929c 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp @@ -19,6 +19,7 @@ namespace clang::tidy::bugprone { using ast_matchers::MatchFinder; using dataflow::UncheckedOptionalAccessDiagnoser; +using dataflow::UncheckedOptionalAccessDiagnostic; using dataflow::UncheckedOptionalAccessModel; static constexpr llvm::StringLiteral FuncID("fun"); @@ -52,14 +53,16 @@ void UncheckedOptionalAccessCheck::check( UncheckedOptionalAccessDiagnoser Diagnoser(ModelOptions); // FIXME: Allow user to set the (defaulted) SAT iterations max for // `diagnoseFunction` with config options. - if (llvm::Expected<llvm::SmallVector<SourceLocation>> Locs = - dataflow::diagnoseFunction<UncheckedOptionalAccessModel, - SourceLocation>(*FuncDecl, *Result.Context, - Diagnoser)) - for (const SourceLocation &Loc : *Locs) - diag(Loc, "unchecked access to optional value"); + if (llvm::Expected<llvm::SmallVector<UncheckedOptionalAccessDiagnostic>> + Diags = dataflow::diagnoseFunction<UncheckedOptionalAccessModel, + UncheckedOptionalAccessDiagnostic>( + *FuncDecl, *Result.Context, Diagnoser)) + for (const UncheckedOptionalAccessDiagnostic &Diag : *Diags) { + diag(Diag.Range.getBegin(), "unchecked access to optional value") + << Diag.Range; + } else - llvm::consumeError(Locs.takeError()); + llvm::consumeError(Diags.takeError()); } } // namespace clang::tidy::bugprone diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h index fb11c2e230e32..696c9f4a6cf5c 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -20,6 +20,7 @@ #include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.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 "llvm/ADT/SmallVector.h" @@ -71,12 +72,17 @@ class UncheckedOptionalAccessModel TransferMatchSwitch; }; +/// Diagnostic information for an unchecked optional access. +struct UncheckedOptionalAccessDiagnostic { + CharSourceRange Range; +}; + class UncheckedOptionalAccessDiagnoser { public: UncheckedOptionalAccessDiagnoser( UncheckedOptionalAccessModelOptions Options = {}); - llvm::SmallVector<SourceLocation> + llvm::SmallVector<UncheckedOptionalAccessDiagnostic> operator()(const CFGElement &Elt, ASTContext &Ctx, const TransferStateForDiagnostics<UncheckedOptionalAccessLattice> &State) { @@ -84,7 +90,8 @@ class UncheckedOptionalAccessDiagnoser { } private: - CFGMatchSwitch<const Environment, llvm::SmallVector<SourceLocation>> + CFGMatchSwitch<const Environment, + llvm::SmallVector<UncheckedOptionalAccessDiagnostic>> DiagnoseMatchSwitch; }; diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index c28424fac8fef..164d2574132dd 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -1120,8 +1120,8 @@ auto buildTransferMatchSwitch() { .Build(); } -llvm::SmallVector<SourceLocation> diagnoseUnwrapCall(const Expr *ObjectExpr, - const Environment &Env) { +llvm::SmallVector<UncheckedOptionalAccessDiagnostic> +diagnoseUnwrapCall(const Expr *ObjectExpr, const Environment &Env) { if (auto *OptionalLoc = cast_or_null<RecordStorageLocation>( getLocBehindPossiblePointer(*ObjectExpr, Env))) { auto *Prop = Env.getValue(locForHasValue(*OptionalLoc)); @@ -1132,9 +1132,9 @@ llvm::SmallVector<SourceLocation> diagnoseUnwrapCall(const Expr *ObjectExpr, } // Record that this unwrap is *not* provably safe. - // FIXME: include either the name of the optional (if applicable) or a source - // range of the access for easier interpretation of the result. - return {ObjectExpr->getBeginLoc()}; + // FIXME: include the name of the optional (if applicable). + auto Range = CharSourceRange::getTokenRange(ObjectExpr->getSourceRange()); + return {UncheckedOptionalAccessDiagnostic{Range}}; } auto buildDiagnoseMatchSwitch( @@ -1143,8 +1143,9 @@ auto buildDiagnoseMatchSwitch( // lot of duplicated work (e.g. string comparisons), consider providing APIs // that avoid it through memoization. auto IgnorableOptional = ignorableOptional(Options); - return CFGMatchSwitchBuilder<const Environment, - llvm::SmallVector<SourceLocation>>() + return CFGMatchSwitchBuilder< + const Environment, + llvm::SmallVector<UncheckedOptionalAccessDiagnostic>>() // optional::value .CaseOfCFGStmt<CXXMemberCallExpr>( valueCall(IgnorableOptional), diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 46fad6b655c4d..6f69ccbd36552 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -14,6 +14,7 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Frontend/TextDiagnostic.h" #include "clang/Tooling/Tooling.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLExtras.h" #include "llvm/Support/Error.h" @@ -1282,6 +1283,14 @@ static raw_ostream &operator<<(raw_ostream &OS, class UncheckedOptionalAccessTest : public ::testing::TestWithParam<OptionalTypeIdentifier> { protected: + // Check that after running the analysis on SourceCode, it produces the + // expected diagnostics according to [[unsafe]] annotations. + // - No annotations => no diagnostics. + // - Given "// [[unsafe]]" annotations on a line, we expect a diagnostic on + // that line. + // - Given "// [[unsafe:range_text]]" annotations on a line, we expect a + // diagnostic on that line, and we expect the diagnostic Range (printed as + // a string) to match the "range_text". void ExpectDiagnosticsFor(std::string SourceCode, bool IgnoreSmartPointerDereference = true) { ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target"), @@ -1336,7 +1345,7 @@ class UncheckedOptionalAccessTest T Make(); )"); UncheckedOptionalAccessModelOptions Options{IgnoreSmartPointerDereference}; - std::vector<SourceLocation> Diagnostics; + std::vector<UncheckedOptionalAccessDiagnostic> Diagnostics; llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>( AnalysisInputs<UncheckedOptionalAccessModel>( SourceCode, std::move(FuncMatcher), @@ -1364,22 +1373,34 @@ class UncheckedOptionalAccessTest &Annotations, const AnalysisOutputs &AO) { llvm::DenseSet<unsigned> AnnotationLines; - for (const auto &[Line, _] : Annotations) { + llvm::DenseMap<unsigned, std::string> AnnotationRangesInLines; + for (const auto &[Line, AnnotationWithMaybeRange] : Annotations) { AnnotationLines.insert(Line); + auto it = AnnotationWithMaybeRange.find(':'); + if (it != std::string::npos) { + AnnotationRangesInLines[Line] = + AnnotationWithMaybeRange.substr(it + 1); + } } auto &SrcMgr = AO.ASTCtx.getSourceManager(); llvm::DenseSet<unsigned> DiagnosticLines; - for (SourceLocation &Loc : Diagnostics) { - unsigned Line = SrcMgr.getPresumedLineNumber(Loc); + for (const UncheckedOptionalAccessDiagnostic &Diag : Diagnostics) { + unsigned Line = SrcMgr.getPresumedLineNumber(Diag.Range.getBegin()); DiagnosticLines.insert(Line); if (!AnnotationLines.contains(Line)) { IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts( new DiagnosticOptions()); TextDiagnostic TD(llvm::errs(), AO.ASTCtx.getLangOpts(), DiagOpts.get()); - TD.emitDiagnostic(FullSourceLoc(Loc, SrcMgr), + TD.emitDiagnostic(FullSourceLoc(Diag.Range.getBegin(), SrcMgr), DiagnosticsEngine::Error, - "unexpected diagnostic", {}, {}); + "unexpected diagnostic", {Diag.Range}, {}); + } else { + auto it = AnnotationRangesInLines.find(Line); + if (it != AnnotationRangesInLines.end()) { + EXPECT_EQ(Diag.Range.getAsRange().printToString(SrcMgr), + it->second); + } } } @@ -4085,6 +4106,31 @@ TEST_P(UncheckedOptionalAccessTest, ConstPointerRefAccessor) { /*IgnoreSmartPointerDereference=*/false); } +TEST_P(UncheckedOptionalAccessTest, DiagnosticsHaveRanges) { + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + struct A { + $ns::$optional<int> fi; + }; + struct B { + $ns::$optional<A> fa; + }; + + void target($ns::$optional<B> opt) { + opt.value(); // [[unsafe:<input.cc:12:7>]] + if (opt) { + opt // [[unsafe:<input.cc:14:9, line:16:13>]] + -> + fa.value(); + if (opt->fa) { + opt->fa->fi.value(); // [[unsafe:<input.cc:18:11, col:20>]] + } + } + } + )cc"); +} + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
