[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[Anton Dukeman via Phabricator via cfe-commits]

adukeman created this revision.
adukeman added reviewers: njames93, ymandel, sgatev.
Herald added subscribers: PiotrZSL, carlosgalvezp, xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
adukeman requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

The unchecked-optional-access check identifies attempted value
unwrapping without checking if the value exists. These changes extend
that support to checking folly::Optional.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155890

Files:
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -56,9 +56,10 @@
   }
 
   if (RD.getName() == "Optional") {
-// Check whether namespace is "::base".
+// Check whether namespace is "::base" or "::folly".
 const auto *N = dyn_cast_or_null(RD.getDeclContext());
-return N != nullptr && isTopLevelNamespaceWithName(*N, "base");
+return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
+isTopLevelNamespaceWithName(*N, "folly"));
   }
 
   return false;
@@ -109,15 +110,15 @@
 }
 
 auto isMakeOptionalCall() {
-  return callExpr(
-  callee(functionDecl(hasAnyName(
-  "std::make_optional", "base::make_optional", "absl::make_optional"))),
-  hasOptionalType());
+  return callExpr(callee(functionDecl(hasAnyName(
+  "std::make_optional", "base::make_optional",
+  "absl::make_optional", "folly::make_optional"))),
+  hasOptionalType());
 }
 
 auto nulloptTypeDecl() {
-  return namedDecl(
-  hasAnyName("std::nullopt_t", "absl::nullopt_t", "base::nullopt_t"));
+  return namedDecl(hasAnyName("std::nullopt_t", "absl::nullopt_t",
+  "base::nullopt_t", "folly::None"));
 }
 
 auto hasNulloptType() { return hasType(nulloptTypeDecl()); }
@@ -129,8 +130,8 @@
 }
 
 auto inPlaceClass() {
-  return recordDecl(
-  hasAnyName("std::in_place_t", "absl::in_place_t", "base::in_place_t"));
+  return recordDecl(hasAnyName("std::in_place_t", "absl::in_place_t",
+   "base::in_place_t", "folly::in_place_t"));
 }
 
 auto isOptionalNulloptConstructor() {
@@ -839,6 +840,11 @@
   isOptionalMemberCallWithName("has_value"),
   transferOptionalHasValueCall)
 
+  // optional::hasValue for folly::Optional
+  .CaseOfCFGStmt(
+  isOptionalMemberCallWithName("hasValue"),
+  transferOptionalHasValueCall)
+
   // optional::operator bool
   .CaseOfCFGStmt(
   isOptionalMemberCallWithName("operator bool"),
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -1,6 +1,7 @@
 // RUN: %check_clang_tidy %s bugprone-unchecked-optional-access %t -- -- -I %S/Inputs/unchecked-optional-access
 
 #include "absl/types/optional.h"
+#include "folly/types/Optional.h"
 
 void unchecked_value_access(const absl::optional &opt) {
   opt.value();
@@ -21,12 +22,34 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value
 }
 
+void folly_check_value_then_reset(folly::Optional opt) {
+  if (opt) {
+opt.reset();
+opt.value();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional value
+  }
+}
+
+void folly_value_after_swap(folly::Optional opt1, folly::Optional opt2) {
+  if (opt1) {
+opt1.swap(opt2);
+opt1.value();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional value
+  }
+}
+
 void checked_access(const absl::optional &opt) {
   if (opt.has_value()) {
 opt.value();
   }
 }
 
+void folly_checked_access(const folly::Optional &opt) {
+  if (opt.hasValue()) {
+opt.value();
+  }
+}
+
 template 
 void function_template_without_user(const absl::optional &opt) {
   opt.value(); // no-warning
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h
@@ -0,0 +1,56 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKE

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[Anton Dukeman via Phabricator via cfe-commits]

adukeman added a comment.

In D155890#4522266 , @ymandel wrote:

> In D155890#4521266 , @carlosgalvezp 
> wrote:
>
>> This should be a configuration option, we should not hardcore 
>> project-specific things in the source code.
>
> I agree, but we already are hardcoding specific types -- I think this is a 
> separate (and valid) critique of the design. I'd propose filing an issue on 
> the github tracker and we can follow up there.  I, for one, would love to 
> review such a change but don't have the time to write it.

Is moving these values to config an appropriate task for somebody like me new 
to working on clang-tidy? I'd be happy to merge this and then try the 
transition to a config assuming there's some similar examples I can borrow from 
elsewhere in the codebase.




Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:844-846
+  .CaseOfCFGStmt(
+  isOptionalMemberCallWithName("hasValue"),
+  transferOptionalHasValueCall)

ymandel wrote:
> A few concerns:
> 1. This will allow `hasValue` on *any* of the optional types. While we know 
> that the other types don't have this call, this is bad hygiene. At the least, 
> we should note this potential problem in the comments.
> 2. I don't think its worth duplicating the case above just to change the 
> name, given that the action is identical. Instead, please generalize 
> `isOptionalMemberCallWithName` to take a name matcher, and pass 
> `hasAnyName("has_value", "hasValue")` for this case.  The other calls to 
> `isOptionalMemberCallWithName` will need to be changed to pass just 
> `hasName(...)`.
Sure. I can make that change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155890/new/

https://reviews.llvm.org/D155890

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[Anton Dukeman via Phabricator via cfe-commits]

adukeman updated this revision to Diff 543619.
adukeman added a comment.

Update calls to OptionalMemberCall to take a name matcher

Supports treating calls to optional::has_value and optional::hasValue 
identically


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155890/new/

https://reviews.llvm.org/D155890

Files:
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -56,9 +56,10 @@
   }
 
   if (RD.getName() == "Optional") {
-// Check whether namespace is "::base".
+// Check whether namespace is "::base" or "::folly".
 const auto *N = dyn_cast_or_null(RD.getDeclContext());
-return N != nullptr && isTopLevelNamespaceWithName(*N, "base");
+return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
+isTopLevelNamespaceWithName(*N, "folly"));
   }
 
   return false;
@@ -87,8 +88,8 @@
 /// Matches any of the spellings of the optional types and sugar, aliases, etc.
 auto hasOptionalType() { return hasType(optionalOrAliasType()); }
 
-auto isOptionalMemberCallWithName(
-llvm::StringRef MemberName,
+auto isOptionalMemberCallWithNameMatcher(
+ast_matchers::internal::Matcher matcher,
 const std::optional &Ignorable = std::nullopt) {
   auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
 : cxxThisExpr());
@@ -96,7 +97,7 @@
   on(expr(Exception,
   anyOf(hasOptionalType(),
 hasType(pointerType(pointee(optionalOrAliasType())),
-  callee(cxxMethodDecl(hasName(MemberName;
+  callee(cxxMethodDecl(matcher)));
 }
 
 auto isOptionalOperatorCallWithName(
@@ -109,15 +110,15 @@
 }
 
 auto isMakeOptionalCall() {
-  return callExpr(
-  callee(functionDecl(hasAnyName(
-  "std::make_optional", "base::make_optional", "absl::make_optional"))),
-  hasOptionalType());
+  return callExpr(callee(functionDecl(hasAnyName(
+  "std::make_optional", "base::make_optional",
+  "absl::make_optional", "folly::make_optional"))),
+  hasOptionalType());
 }
 
 auto nulloptTypeDecl() {
-  return namedDecl(
-  hasAnyName("std::nullopt_t", "absl::nullopt_t", "base::nullopt_t"));
+  return namedDecl(hasAnyName("std::nullopt_t", "absl::nullopt_t",
+  "base::nullopt_t", "folly::None"));
 }
 
 auto hasNulloptType() { return hasType(nulloptTypeDecl()); }
@@ -129,8 +130,8 @@
 }
 
 auto inPlaceClass() {
-  return recordDecl(
-  hasAnyName("std::in_place_t", "absl::in_place_t", "base::in_place_t"));
+  return recordDecl(hasAnyName("std::in_place_t", "absl::in_place_t",
+   "base::in_place_t", "folly::in_place_t"));
 }
 
 auto isOptionalNulloptConstructor() {
@@ -752,7 +753,7 @@
 
 StatementMatcher
 valueCall(const std::optional &IgnorableOptional) {
-  return isOptionalMemberCallWithName("value", IgnorableOptional);
+  return isOptionalMemberCallWithNameMatcher(hasName("value"), IgnorableOptional);
 }
 
 StatementMatcher
@@ -834,19 +835,20 @@
  transferArrowOpCall(E, E->getArg(0), State);
})
 
-  // optional::has_value
+  // optional::has_value, optional::hasValue
+  // Of the supported optionals only folly::Optional uses hasValue, but this will also pass for other types
   .CaseOfCFGStmt(
-  isOptionalMemberCallWithName("has_value"),
+  isOptionalMemberCallWithNameMatcher(hasAnyName("has_value", "hasValue")),
   transferOptionalHasValueCall)
 
   // optional::operator bool
   .CaseOfCFGStmt(
-  isOptionalMemberCallWithName("operator bool"),
+  isOptionalMemberCallWithNameMatcher(hasName("operator bool")),
   transferOptionalHasValueCall)
 
   // optional::emplace
   .CaseOfCFGStmt(
-  isOptionalMemberCallWithName("emplace"),
+  isOptionalMemberCallWithNameMatcher(hasName("emplace")),
   [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
  LatticeTransferState &State) {
 if (AggregateStorageLocation *Loc =
@@ -858,7 +860,7 @@
 
   // optional::reset
   .CaseOfCFGStmt(
-  isOptionalMemberCallWithName("reset"),
+  isOptionalMemberCallWithNameMatcher(hasName("reset")),
   [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
  LatticeTr

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[Anton Dukeman via Phabricator via cfe-commits]

adukeman marked 3 inline comments as done.
adukeman added a comment.

Thanks for all the comments! I think I've got them all addressed and tests are 
passing locally.

In D155890#4528560 , @carlosgalvezp 
wrote:

> The review is marked as accepted, should we land it? Let me know if you need 
> help with that :)

Yes, will need help with that since I don't have commit access. Anybody with 
commit access, feel free to land this for me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155890/new/

https://reviews.llvm.org/D155890

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

[Anton Dukeman via Phabricator via cfe-commits]

adukeman updated this revision to Diff 543659.
adukeman added a comment.

Update docs and run clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155890/new/

https://reviews.llvm.org/D155890

Files:
  
clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h
  
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -56,9 +56,10 @@
   }
 
   if (RD.getName() == "Optional") {
-// Check whether namespace is "::base".
+// Check whether namespace is "::base" or "::folly".
 const auto *N = dyn_cast_or_null(RD.getDeclContext());
-return N != nullptr && isTopLevelNamespaceWithName(*N, "base");
+return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
+isTopLevelNamespaceWithName(*N, "folly"));
   }
 
   return false;
@@ -87,8 +88,8 @@
 /// Matches any of the spellings of the optional types and sugar, aliases, etc.
 auto hasOptionalType() { return hasType(optionalOrAliasType()); }
 
-auto isOptionalMemberCallWithName(
-llvm::StringRef MemberName,
+auto isOptionalMemberCallWithNameMatcher(
+ast_matchers::internal::Matcher matcher,
 const std::optional &Ignorable = std::nullopt) {
   auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
 : cxxThisExpr());
@@ -96,7 +97,7 @@
   on(expr(Exception,
   anyOf(hasOptionalType(),
 hasType(pointerType(pointee(optionalOrAliasType())),
-  callee(cxxMethodDecl(hasName(MemberName;
+  callee(cxxMethodDecl(matcher)));
 }
 
 auto isOptionalOperatorCallWithName(
@@ -109,15 +110,15 @@
 }
 
 auto isMakeOptionalCall() {
-  return callExpr(
-  callee(functionDecl(hasAnyName(
-  "std::make_optional", "base::make_optional", "absl::make_optional"))),
-  hasOptionalType());
+  return callExpr(callee(functionDecl(hasAnyName(
+  "std::make_optional", "base::make_optional",
+  "absl::make_optional", "folly::make_optional"))),
+  hasOptionalType());
 }
 
 auto nulloptTypeDecl() {
-  return namedDecl(
-  hasAnyName("std::nullopt_t", "absl::nullopt_t", "base::nullopt_t"));
+  return namedDecl(hasAnyName("std::nullopt_t", "absl::nullopt_t",
+  "base::nullopt_t", "folly::None"));
 }
 
 auto hasNulloptType() { return hasType(nulloptTypeDecl()); }
@@ -129,8 +130,8 @@
 }
 
 auto inPlaceClass() {
-  return recordDecl(
-  hasAnyName("std::in_place_t", "absl::in_place_t", "base::in_place_t"));
+  return recordDecl(hasAnyName("std::in_place_t", "absl::in_place_t",
+   "base::in_place_t", "folly::in_place_t"));
 }
 
 auto isOptionalNulloptConstructor() {
@@ -752,7 +753,8 @@
 
 StatementMatcher
 valueCall(const std::optional &IgnorableOptional) {
-  return isOptionalMemberCallWithName("value", IgnorableOptional);
+  return isOptionalMemberCallWithNameMatcher(hasName("value"),
+ IgnorableOptional);
 }
 
 StatementMatcher
@@ -834,19 +836,22 @@
  transferArrowOpCall(E, E->getArg(0), State);
})
 
-  // optional::has_value
+  // optional::has_value, optional::hasValue
+  // Of the supported optionals only folly::Optional uses hasValue, but this
+  // will also pass for other types
   .CaseOfCFGStmt(
-  isOptionalMemberCallWithName("has_value"),
+  isOptionalMemberCallWithNameMatcher(
+  hasAnyName("has_value", "hasValue")),
   transferOptionalHasValueCall)
 
   // optional::operator bool
   .CaseOfCFGStmt(
-  isOptionalMemberCallWithName("operator bool"),
+  isOptionalMemberCallWithNameMatcher(hasName("operator bool")),
   transferOptionalHasValueCall)
 
   // optional::emplace
   .CaseOfCFGStmt(
-  isOptionalMemberCallWithName("emplace"),
+  isOptionalMemberCallWithNameMatcher(hasName("emplace")),
   [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
  LatticeTransferState &State) {
 if (AggregateStorageLocation *Loc =
@@ -858,7 +863,7 @@
 
   // optional::reset
   .CaseOfCFGStmt(
-  isOptionalMemberCallWithName("reset"),
+  isOptionalMemberCallWithNameMatcher(hasName("reset")),
   [](const CXXMemberCallExpr *E, const Match