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

2023-07-24 Thread Piotr Zegar via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2f0630f8bc91: [clang-tidy] Add folly::Optional to 
unchecked-optional-access (authored by Anton Dukeman 
antonduke...@fb.com, committed by PiotrZSL).

Changed prior to commit:
  https://reviews.llvm.org/D155890?vs=543659=543678#toc

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/ReleaseNotes.rst
  
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  = 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() {
@@ -750,7 +751,8 @@
 
 StatementMatcher
 valueCall(const std::optional ) {
-  return isOptionalMemberCallWithName("value", IgnorableOptional);
+  return isOptionalMemberCallWithNameMatcher(hasName("value"),
+ IgnorableOptional);
 }
 
 StatementMatcher
@@ -832,19 +834,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 ) {

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

2023-07-24 Thread Piotr Zegar via Phabricator via cfe-commits 
PiotrZSL accepted this revision.
PiotrZSL added a comment.

+- LGTM, I will correct those nits and push it.


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

2023-07-24 Thread Piotr Zegar via Phabricator via cfe-commits 
PiotrZSL added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst:11
 This check identifies unsafe accesses to values contained in
-``std::optional``, ``absl::optional``, or ``base::Optional``
+``std::optional``, ``absl::optional``, ``base::Optional``, or 
``folly::Optional``
 objects. Below we will refer to all these types collectively as 
``optional``.

80 characters limit, wrap it


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

2023-07-24 Thread 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  = 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 ) {
-  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 ) {
 if (AggregateStorageLocation *Loc =
@@ -858,7 +863,7 @@
 
   // optional::reset
   .CaseOfCFGStmt(
-  isOptionalMemberCallWithName("reset"),
+  isOptionalMemberCallWithNameMatcher(hasName("reset")),
   [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,

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

2023-07-24 Thread Piotr Zegar via Phabricator via cfe-commits 
PiotrZSL added a comment.

Some entry in release notes or/and check documentation would be welcome.


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

2023-07-24 Thread 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

2023-07-24 Thread 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  = 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 ) {
-  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 ) {
 if (AggregateStorageLocation *Loc =
@@ -858,7 +860,7 @@
 
   // optional::reset
   .CaseOfCFGStmt(
-  isOptionalMemberCallWithName("reset"),
+  isOptionalMemberCallWithNameMatcher(hasName("reset")),
   [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
  LatticeTransferState ) {
 if

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

2023-07-24 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits 
ymandel added a comment.

In D155890#4528560 , @carlosgalvezp 
wrote:

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

We're still waiting on some small changes: 
https://reviews.llvm.org/D155890#inline-1508661


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

2023-07-24 Thread Carlos Galvez via Phabricator via cfe-commits 
carlosgalvezp added a comment.

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


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

2023-07-24 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits 
ymandel added a comment.

In D155890#4525144 , @carlosgalvezp 
wrote:

> I have opened a refactoring ticket here: 
> https://github.com/llvm/llvm-project/issues/64037

Thank you!


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

2023-07-22 Thread Carlos Galvez via Phabricator via cfe-commits 
carlosgalvezp added a comment.

I have opened a refactoring ticket here: 
https://github.com/llvm/llvm-project/issues/64037


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

2023-07-21 Thread Carlos Galvez via Phabricator via cfe-commits 
carlosgalvezp added inline comments.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:62
+return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
+isTopLevelNamespaceWithName(*N, "folly"));
   }

gribozavr2 wrote:
> carlosgalvezp wrote:
> > If there's no need for `absl` here, why do we need to add `folly`?
> This is the branch for `RD.getName() == "Optional"` with a capital "O" 
> because `base::Optional`, `folly;:Optional` are spelled with a capital "O".
> 
> Abseil provides `absl::optional` and it is handled under `RD.getName() == 
> "optional"` together with `std::optional` above.
Got it, thanks!


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

2023-07-21 Thread Dmitri Gribenko via Phabricator via cfe-commits 
gribozavr2 added inline comments.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:62
+return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
+isTopLevelNamespaceWithName(*N, "folly"));
   }

carlosgalvezp wrote:
> If there's no need for `absl` here, why do we need to add `folly`?
This is the branch for `RD.getName() == "Optional"` with a capital "O" because 
`base::Optional`, `folly;:Optional` are spelled with a capital "O".

Abseil provides `absl::optional` and it is handled under `RD.getName() == 
"optional"` together with `std::optional` above.


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

2023-07-21 Thread Carlos Galvez via Phabricator via cfe-commits 
carlosgalvezp resigned from this revision.
carlosgalvezp added a comment.
This revision is now accepted and ready to land.

> I think it'd be good to add reviewers there

I realize the CodeOwners for Analysis are already in the list of reviewers, I 
won't interfere then :)


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

2023-07-21 Thread Carlos Galvez via Phabricator via cfe-commits 
carlosgalvezp added a comment.

In D155890#4523262 , @gribozavr2 
wrote:

> In D155890#4523243 , @adukeman 
> wrote:
>
>> 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.
>
> I think it can be a good starter task for a new engineer on the project. 
> However, don't underestimate this problem, it will require the code to be 
> refactored a little bit. For example, the function `hasOptionalClassName` 
> needs restructuring so that it can accept class names from a list. Not a lot 
> of work, but it isn't mechanically replacing string literals with a variable 
> either.

Indeed this is not "the standard" CT check, the core is part of Clang so I 
think it'd be good to add reviewers there as well in case this affects other 
parts of the codebase. In that sense it does not seen as trivial as I thought 
to make this user configurable, so perhaps opening a ticket and solve it there 
is a faster way forward.




Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:62
+return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
+isTopLevelNamespaceWithName(*N, "folly"));
   }

If there's no need for `absl` here, why do we need to add `folly`?


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

2023-07-21 Thread Dmitri Gribenko via Phabricator via cfe-commits 
gribozavr2 added a comment.

In D155890#4523243 , @adukeman wrote:

> 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.

I think it can be a good starter task for a new engineer on the project. 
However, don't underestimate this problem, it will require the code to be 
refactored a little bit. For example, the function `hasOptionalClassName` needs 
restructuring so that it can accept class names from a list. Not a lot of work, 
but it isn't mechanically replacing string literals with a variable either.


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

2023-07-21 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits 
ymandel added a comment.

In D155890#4523243 , @adukeman wrote:

> 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.

This is one of the most complex clang-tidy checks. So, if you're looking for a 
CT starter task, I wouldn't recommend this particular challenge. That said, I 
think the clang-tidy side will be relatively easy -- CT has a mature config 
system/API.  The harder part (and not CT relevant) is refactoring this code to 
consume that config. It's not terribly complicated but will require a bunch of 
changes and probably some design questions.


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

2023-07-21 Thread 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

2023-07-21 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits 
ymandel added a comment.

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.




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

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(...)`.


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

2023-07-20 Thread Carlos Galvez via Phabricator via cfe-commits 
carlosgalvezp requested changes to this revision.
carlosgalvezp added a comment.
This revision now requires changes to proceed.

This should be a configuration option, we should not hardcore project-specific 
things in the source code.


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

2023-07-20 Thread Dmitri Gribenko via Phabricator via cfe-commits 
gribozavr2 accepted this revision.
gribozavr2 added a comment.
This revision is now accepted and ready to land.

Thank you! Do you have commit access or do you need me to commit this patch for 
you?




Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:843
 
+  // optional::hasValue for folly::Optional
+  .CaseOfCFGStmt(




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

2023-07-20 Thread Piotr Zegar via Phabricator via cfe-commits 
PiotrZSL added a comment.

To be honest, all those things just should be configurable instead of 
hardcoding them.
For example, a project that I work for got 4 different custom optional 
implementations, and this check is useless for all of them.
And still no boost::optional support.


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

2023-07-20 Thread 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.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 ) {
   if (opt.has_value()) {
 opt.value();
   }
 }
 
+void folly_checked_access(const folly::Optional ) {
+  if (opt.hasValue()) {
+opt.value();
+  }
+}
+
 template 
 void function_template_without_user(const absl::optional ) {
   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