[clang][dataflow] For bugprone-unchecked-optional-access report range #131055
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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;
^~
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Jan Voung (jvoung) ChangesReport the range in diagnostics, in addition to the location instead of just: As a followup we should probably also report the location/range Full diff: https://github.com/llvm/llvm-project/pull/131055.diff 4 Files Affected:
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..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}, {});
}
}
|
ymand
reviewed
Mar 13, 2025
There was a problem hiding this comment.
Should we modify any of the tests to check for the range?
There was a problem hiding this comment.
Ah yes! Added an expectation annotation for ranges and a test case.
ymand
approved these changes
Mar 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Report the range in diagnostics, in addition to the location
in case the range helps disambiguate a little in chained
->expressions.instead of just:
As a followup we should probably also report the location/range
of an
->if that operator is used. Like: