[clang] [clang-tools-extra] [clang][dataflow] For bugprone-unchecked-optional-access report range (PR #131055)
Jan Voung via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 14 06:33:43 PDT 2025
https://github.com/jvoung updated https://github.com/llvm/llvm-project/pull/131055
>From b93c10f029fb33e9f7261cbb174d097be4137006 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Thu, 13 Mar 2025 02:26:41 +0000
Subject: [PATCH 1/3] [clang][dataflow] For bugprone-unchecked-optional-access
report range
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;
^~
---
.../bugprone/UncheckedOptionalAccessCheck.cpp | 17 ++++++++++-------
.../Models/UncheckedOptionalAccessModel.h | 11 +++++++++--
.../Models/UncheckedOptionalAccessModel.cpp | 14 +++++++-------
.../UncheckedOptionalAccessModelTest.cpp | 10 +++++-----
4 files changed, 31 insertions(+), 21 deletions(-)
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..c1d7f6c015558 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,8 @@ 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()};
+ auto Range = CharSourceRange::getTokenRange(ObjectExpr->getSourceRange());
+ return {UncheckedOptionalAccessDiagnostic{Range}};
}
auto buildDiagnoseMatchSwitch(
@@ -1143,8 +1142,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..60f1a662a512b 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1336,7 +1336,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),
@@ -1369,17 +1369,17 @@ class UncheckedOptionalAccessTest
}
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}, {});
}
}
>From 4cb096096caf60fcd5e35b19f2ade5e2dd861e0a Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Thu, 13 Mar 2025 02:42:35 +0000
Subject: [PATCH 2/3] Keep part of old FIXME
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 1 +
1 file changed, 1 insertion(+)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index c1d7f6c015558..164d2574132dd 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -1132,6 +1132,7 @@ diagnoseUnwrapCall(const Expr *ObjectExpr, const Environment &Env) {
}
// Record that this unwrap is *not* provably safe.
+ // FIXME: include the name of the optional (if applicable).
auto Range = CharSourceRange::getTokenRange(ObjectExpr->getSourceRange());
return {UncheckedOptionalAccessDiagnostic{Range}};
}
>From afe06967414e5ff9a002fd511f3ccd45f94ac75d Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Fri, 14 Mar 2025 13:32:46 +0000
Subject: [PATCH 3/3] Test for the range
---
.../UncheckedOptionalAccessModelTest.cpp | 48 ++++++++++++++++++-
1 file changed, 47 insertions(+), 1 deletion(-)
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 60f1a662a512b..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"),
@@ -1364,8 +1373,14 @@ 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;
@@ -1380,6 +1395,12 @@ class UncheckedOptionalAccessTest
TD.emitDiagnostic(FullSourceLoc(Diag.Range.getBegin(), SrcMgr),
DiagnosticsEngine::Error,
"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)
More information about the cfe-commits
mailing list