[clang-tidy] add abseil-unchecked-statusor-access #171188
Open
+1,526
−0
Conversation
This file contains hidden or 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
Created using spr 1.3.7
Member
|
@llvm/pr-subscribers-clang-tools-extra Author: Florian Mayer (fmayer) ChangesFull diff: https://github.com/llvm/llvm-project/pull/171188.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
index 8971530bab9b2..0ecfb25f31515 100644
--- a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
@@ -27,6 +27,7 @@
#include "StringFindStrContainsCheck.h"
#include "TimeComparisonCheck.h"
#include "TimeSubtractionCheck.h"
+#include "UncheckedStatusOrAccessCheck.h"
#include "UpgradeDurationConversionsCheck.h"
namespace clang::tidy {
@@ -69,6 +70,8 @@ class AbseilModule : public ClangTidyModule {
"abseil-time-subtraction");
CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(
"abseil-upgrade-duration-conversions");
+ CheckFactories.registerCheck<UncheckedStatusOrAccessCheck>(
+ "abseil-unchecked-statusor-access");
}
};
diff --git a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
index ca7cc6782f1e6..0c02ffc7306d1 100644
--- a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
@@ -25,6 +25,7 @@ add_clang_library(clangTidyAbseilModule STATIC
TimeComparisonCheck.cpp
TimeSubtractionCheck.cpp
UpgradeDurationConversionsCheck.cpp
+ UncheckedStatusOrAccessCheck.cpp
LINK_LIBS
clangTidy
diff --git a/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.cpp b/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.cpp
new file mode 100644
index 0000000000000..d8842cb2b55a2
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.cpp
@@ -0,0 +1,63 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UncheckedStatusOrAccessCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
+#include "clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Error.h"
+
+namespace clang::tidy::abseil {
+using ast_matchers::MatchFinder;
+using dataflow::statusor_model::UncheckedStatusOrAccessModel;
+using dataflow::statusor_model::UncheckedStatusOrAccessDiagnoser;
+
+static constexpr llvm::StringLiteral FuncID("fun");
+
+void UncheckedStatusOrAccessCheck::registerMatchers(MatchFinder *Finder) {
+ using namespace ast_matchers;
+ if (!getLangOpts().CPlusPlus) return;
+
+ auto has_statusor_call_descendant =
+ hasDescendant(callExpr(callee(cxxMethodDecl(ofClass(hasAnyName(
+ "absl::StatusOr", "absl::internal_statusor::OperatorBase"))))));
+ Finder->addMatcher(functionDecl(unless(isExpansionInSystemHeader()),
+ hasBody(has_statusor_call_descendant))
+ .bind(FuncID),
+ this);
+ Finder->addMatcher(
+ cxxConstructorDecl(hasAnyConstructorInitializer(
+ withInitializer(has_statusor_call_descendant)))
+ .bind(FuncID),
+ this);
+}
+void UncheckedStatusOrAccessCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ return;
+
+ const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>(FuncID);
+ if (FuncDecl->isTemplated())
+ return;
+
+ UncheckedStatusOrAccessDiagnoser Diagnoser;
+ if (llvm::Expected<llvm::SmallVector<SourceLocation>> Locs =
+ dataflow::diagnoseFunction<
+ UncheckedStatusOrAccessModel,
+ SourceLocation>(*FuncDecl, *Result.Context, Diagnoser))
+ for (const SourceLocation& Loc : *Locs)
+ diag(Loc, "unchecked access to 'absl::StatusOr' value");
+ else
+ llvm::consumeError(Locs.takeError());
+}
+
+} // namespace clang::tidy::abseil
diff --git a/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.h b/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.h
new file mode 100644
index 0000000000000..97ecd1befd6ee
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.h
@@ -0,0 +1,25 @@
+#ifndef DEVTOOLS_CYMBAL_CLANG_TIDY_RUNTIME_UPSTREAM_FRAMEWORK_UNCHECKED_STATUSOR_ACCESS_H_
+#define DEVTOOLS_CYMBAL_CLANG_TIDY_RUNTIME_UPSTREAM_FRAMEWORK_UNCHECKED_STATUSOR_ACCESS_H_
+
+#include <optional>
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang::tidy::abseil {
+
+// Warns when the code is unwrapping an absl::StatusOr<T> object without
+// assuring that it contains a value.
+//
+// For details on the dataflow analysis implemented in this check see:
+// http://google3/devtools/cymbal/nullability/statusor
+class UncheckedStatusOrAccessCheck : public ClangTidyCheck {
+ public:
+ using ClangTidyCheck::ClangTidyCheck;
+ void registerMatchers(ast_matchers::MatchFinder* Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult& Result) override;
+};
+
+} // namespace clang::abseil
+
+#endif // DEVTOOLS_CYMBAL_CLANG_TIDY_RUNTIME_UPSTREAM_FRAMEWORK_UNCHECKED_STATUSOR_ACCESS_H_
|
Member
|
@llvm/pr-subscribers-clang-tidy Author: Florian Mayer (fmayer) ChangesFull diff: https://github.com/llvm/llvm-project/pull/171188.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
index 8971530bab9b2..0ecfb25f31515 100644
--- a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
@@ -27,6 +27,7 @@
#include "StringFindStrContainsCheck.h"
#include "TimeComparisonCheck.h"
#include "TimeSubtractionCheck.h"
+#include "UncheckedStatusOrAccessCheck.h"
#include "UpgradeDurationConversionsCheck.h"
namespace clang::tidy {
@@ -69,6 +70,8 @@ class AbseilModule : public ClangTidyModule {
"abseil-time-subtraction");
CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(
"abseil-upgrade-duration-conversions");
+ CheckFactories.registerCheck<UncheckedStatusOrAccessCheck>(
+ "abseil-unchecked-statusor-access");
}
};
diff --git a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
index ca7cc6782f1e6..0c02ffc7306d1 100644
--- a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
@@ -25,6 +25,7 @@ add_clang_library(clangTidyAbseilModule STATIC
TimeComparisonCheck.cpp
TimeSubtractionCheck.cpp
UpgradeDurationConversionsCheck.cpp
+ UncheckedStatusOrAccessCheck.cpp
LINK_LIBS
clangTidy
diff --git a/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.cpp b/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.cpp
new file mode 100644
index 0000000000000..d8842cb2b55a2
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.cpp
@@ -0,0 +1,63 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UncheckedStatusOrAccessCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
+#include "clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Error.h"
+
+namespace clang::tidy::abseil {
+using ast_matchers::MatchFinder;
+using dataflow::statusor_model::UncheckedStatusOrAccessModel;
+using dataflow::statusor_model::UncheckedStatusOrAccessDiagnoser;
+
+static constexpr llvm::StringLiteral FuncID("fun");
+
+void UncheckedStatusOrAccessCheck::registerMatchers(MatchFinder *Finder) {
+ using namespace ast_matchers;
+ if (!getLangOpts().CPlusPlus) return;
+
+ auto has_statusor_call_descendant =
+ hasDescendant(callExpr(callee(cxxMethodDecl(ofClass(hasAnyName(
+ "absl::StatusOr", "absl::internal_statusor::OperatorBase"))))));
+ Finder->addMatcher(functionDecl(unless(isExpansionInSystemHeader()),
+ hasBody(has_statusor_call_descendant))
+ .bind(FuncID),
+ this);
+ Finder->addMatcher(
+ cxxConstructorDecl(hasAnyConstructorInitializer(
+ withInitializer(has_statusor_call_descendant)))
+ .bind(FuncID),
+ this);
+}
+void UncheckedStatusOrAccessCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ return;
+
+ const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>(FuncID);
+ if (FuncDecl->isTemplated())
+ return;
+
+ UncheckedStatusOrAccessDiagnoser Diagnoser;
+ if (llvm::Expected<llvm::SmallVector<SourceLocation>> Locs =
+ dataflow::diagnoseFunction<
+ UncheckedStatusOrAccessModel,
+ SourceLocation>(*FuncDecl, *Result.Context, Diagnoser))
+ for (const SourceLocation& Loc : *Locs)
+ diag(Loc, "unchecked access to 'absl::StatusOr' value");
+ else
+ llvm::consumeError(Locs.takeError());
+}
+
+} // namespace clang::tidy::abseil
diff --git a/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.h b/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.h
new file mode 100644
index 0000000000000..97ecd1befd6ee
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.h
@@ -0,0 +1,25 @@
+#ifndef DEVTOOLS_CYMBAL_CLANG_TIDY_RUNTIME_UPSTREAM_FRAMEWORK_UNCHECKED_STATUSOR_ACCESS_H_
+#define DEVTOOLS_CYMBAL_CLANG_TIDY_RUNTIME_UPSTREAM_FRAMEWORK_UNCHECKED_STATUSOR_ACCESS_H_
+
+#include <optional>
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang::tidy::abseil {
+
+// Warns when the code is unwrapping an absl::StatusOr<T> object without
+// assuring that it contains a value.
+//
+// For details on the dataflow analysis implemented in this check see:
+// http://google3/devtools/cymbal/nullability/statusor
+class UncheckedStatusOrAccessCheck : public ClangTidyCheck {
+ public:
+ using ClangTidyCheck::ClangTidyCheck;
+ void registerMatchers(ast_matchers::MatchFinder* Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult& Result) override;
+};
+
+} // namespace clang::abseil
+
+#endif // DEVTOOLS_CYMBAL_CLANG_TIDY_RUNTIME_UPSTREAM_FRAMEWORK_UNCHECKED_STATUSOR_ACCESS_H_
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code linter. |
Created using spr 1.3.7
fmayer
commented
Dec 9, 2025
vbvictor
reviewed
Dec 9, 2025
clang-tools-extra/clang-tidy/abseil/UncheckedStatusOrAccessCheck.cpp
Outdated
Show resolved
Hide resolved
jvoung
reviewed
Dec 11, 2025
Contributor
jvoung
left a comment
jvoung
left a comment
There was a problem hiding this comment.
mostly nits, otherwise nice work!
| False negatives | ||
| --------------- | ||
|
|
||
| This check generally does **not** generate false negatives. If it cannot |
Contributor
There was a problem hiding this comment.
Is there a better phrasing than "generate" false negatives? If the check has false negatives then it won't generate any report?
Also, the "If it cannot prove an access safe..." do you mean the opposite? "If it cannot prove an access unsafe..." ?
Contributor
Author
There was a problem hiding this comment.
Is there a better phrasing than "generate" false negatives? If the check has false negatives then it won't generate any report?
I don't know. It does not have false negatives, which means if a StatusOr access is deemed safe by the analysis, it is safe. If it is deemed unsafe, it could still be safe.
Also, the "If it cannot prove an access safe..." do you mean the opposite? "If it cannot prove an access unsafe..." ?
No; what is written is correct. This might also have caused the confusion for the other question?
Contributor
There was a problem hiding this comment.
I see, the second sentence is describing how the checker works:
!isSafeUnwrap(...) then issue warning that it is unsafe.
Whether that "unsafe" is true or not made me think more of the "false positive" category so was a bit confusing.
Perhaps you can clarify that and "note that if it is deemed unsafe, it could still be safe (false positive)."... or something to that effect.
| Use ``ABSL_CHECK_OK`` to signal that you knowingly want to crash on | ||
| non-OK values. | ||
|
|
||
| NOTE: Even though using ``.value()`` on a ``nullopt`` is defined to crash, |
Contributor
There was a problem hiding this comment.
Is nullopt the right term for StatusOr, or something more like "no value is present" / "status is not ok"?
| } | ||
| void f(Foo foo) { | ||
| if (foo.get().ok()) { | ||
| use(*get.get()); |
Contributor
There was a problem hiding this comment.
get.get() -> foo.get()
same below for the mutate() case?
| } | ||
|
|
||
| void function_template_user(const absl::StatusOr<int>& sor) { | ||
| // Instantiate the f3 function template so that it gets matched by the check. |
Contributor
There was a problem hiding this comment.
nit: f3 is renamed to "function_template_with_user" ?
| void multiple_unchecked_accesses(absl::StatusOr<int> sor1, | ||
| absl::StatusOr<int> sor2) { | ||
| for (int i = 0; i < 10; i++) { | ||
| sor1.ValueOrDie(); |
Contributor
There was a problem hiding this comment.
nit: could be simpler to use value() for these tests?
Created using spr 1.3.7
fmayer
commented
Dec 11, 2025
Contributor
Author
fmayer
left a comment
fmayer
left a comment
There was a problem hiding this comment.
also realised the test had the wrong name, removed the abseil from the file name because it's already in the directory name
| False negatives | ||
| --------------- | ||
|
|
||
| This check generally does **not** generate false negatives. If it cannot |
Contributor
Author
There was a problem hiding this comment.
Is there a better phrasing than "generate" false negatives? If the check has false negatives then it won't generate any report?
I don't know. It does not have false negatives, which means if a StatusOr access is deemed safe by the analysis, it is safe. If it is deemed unsafe, it could still be safe.
Also, the "If it cannot prove an access safe..." do you mean the opposite? "If it cannot prove an access unsafe..." ?
No; what is written is correct. This might also have caused the confusion for the other question?
| } | ||
| void f(Foo foo) { | ||
| if (foo.get().ok()) { | ||
| use(*get.get()); |
| } | ||
|
|
||
| void function_template_user(const absl::StatusOr<int>& sor) { | ||
| // Instantiate the f3 function template so that it gets matched by the check. |
| void multiple_unchecked_accesses(absl::StatusOr<int> sor1, | ||
| absl::StatusOr<int> sor2) { | ||
| for (int i = 0; i < 10; i++) { | ||
| sor1.ValueOrDie(); |
jvoung
approved these changes
Dec 11, 2025
| False negatives | ||
| --------------- | ||
|
|
||
| This check generally does **not** generate false negatives. If it cannot |
Contributor
There was a problem hiding this comment.
I see, the second sentence is describing how the checker works:
!isSafeUnwrap(...) then issue warning that it is unsafe.
Whether that "unsafe" is true or not made me think more of the "false positive" category so was a bit confusing.
Perhaps you can clarify that and "note that if it is deemed unsafe, it could still be safe (false positive)."... or something to that effect.
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.
The mock headers are copied from clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp.
Relevant RFC: https://discourse.llvm.org/t/rfc-abseil-unchecked-statusor-use-check/87998