llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clang
Author: Jan Voung (jvoung)
<details>
<summary>Changes</summary>
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;
^~
```
---
Full diff: https://github.com/llvm/llvm-project/pull/131055.diff
4 Files Affected:
- (modified)
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp (+10-7)
- (modified)
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
(+9-2)
- (modified)
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+8-7)
- (modified)
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
(+5-5)
``````````diff
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}, {});
}
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/131055
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
