[Clang][Sema]: Diagnose lambda to bool implicit casts #83152

vinayakdsci · 2024-02-27T16:47:13Z

Adds diagnostics for lambda expressions being cast to boolean values, which results in the expression always evaluating to true.
Earlier, Clang allowed compilation of such erroneous programs, but now emits a warning through -Wpointer-bool-conversion.

llvmbot · 2024-02-27T16:47:41Z

@llvm/pr-subscribers-clang

Author: Vinayak Dev (vinayakdsci)

Changes

Fixes #82512

Adds diagnostics for lambda expressions being cast to boolean values, which results in the expression always evaluating to true.
Earlier, Clang allowed compilation of such erroneous programs, but now emits a warning through -Wpointer-bool-conversion.

Full diff: https://github.com/llvm/llvm-project/pull/83152.diff

1 Files Affected:

(modified) clang/lib/Sema/SemaChecking.cpp (+21)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 0de76ee119cf81..3b03036587baeb 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -16538,6 +16538,27 @@ void Sema::DiagnoseAlwaysNonNullPointer(Expr *E,
     }
   }
 
+  // Complain if we are converting a lambda expression to a boolean value
+  if (auto *MCallExpr = dyn_cast<CXXMemberCallExpr>(E)) {
+    if (MCallExpr->getObjectType()->isRecordType()) {
+      if (auto *MRecordDecl = MCallExpr->getRecordDecl()) {
+        if (MRecordDecl->isLambda()) {
+          std::string Str;
+          llvm::raw_string_ostream S(Str);
+          const unsigned DiagID = diag::warn_impcast_pointer_to_bool;
+          // For lambdas, the pointer type is function, which corresponds to 1.
+          const unsigned FunctionPointerType = 1;
+          // Pretty print the diagnostic for the warning
+          E->printPretty(S, nullptr, getPrintingPolicy());
+          Diag(E->getExprLoc(), DiagID)
+              << FunctionPointerType << S.str() << E->getSourceRange() << Range
+              << IsEqual;
+          return;
+        }
+      }
+    }
+  }
+
   // Expect to find a single Decl.  Skip anything more complicated.
   ValueDecl *D = nullptr;
   if (DeclRefExpr *R = dyn_cast<DeclRefExpr>(E)) {

AaronBallman

Thank you for the fix!

There are some related test failures caught by precommit CI that need to be addressed.

The issue in dr18xx.cpp looks to be a case where we should replace bool b = with auto b = (it doesn't change the testing for what's discussed in http://wg21.link/cwg1837).

The issues in blocks.mm should get new expected warning markings for the improved diagnostics.

You should also add a release note to clang/docs/ReleaseNotes.rst so users know about the fix (and mention the issue number as well).

The changes should come with new test coverage, probably in one of the existing tests in clang/test/SemaCXX/.

AaronBallman · 2024-02-27T19:06:48Z

clang/lib/Sema/SemaChecking.cpp

+  if (auto *MCallExpr = dyn_cast<CXXMemberCallExpr>(E)) {
+    if (MCallExpr->getObjectType()->isRecordType()) {
+      if (auto *MRecordDecl = MCallExpr->getRecordDecl()) {


Suggested change

if (auto *MCallExpr = dyn_cast<CXXMemberCallExpr>(E)) {

if (MCallExpr->getObjectType()->isRecordType()) {

if (auto *MRecordDecl = MCallExpr->getRecordDecl()) {

if (const auto *MCallExpr = dyn_cast<CXXMemberCallExpr>(E)) {

if (MCallExpr->getObjectType()->isRecordType()) {

if (const auto *MRecordDecl = MCallExpr->getRecordDecl()) {

Aiming for better const correctness, but if this doesn't work, don't worry about making the changes.

AaronBallman · 2024-02-27T19:14:40Z

clang/lib/Sema/SemaChecking.cpp

+          std::string Str;
+          llvm::raw_string_ostream S(Str);
+          const unsigned DiagID = diag::warn_impcast_pointer_to_bool;
+          // For lambdas, the pointer type is function, which corresponds to 1.
+          const unsigned FunctionPointerType = 1;
+          // Pretty print the diagnostic for the warning
+          E->printPretty(S, nullptr, getPrintingPolicy());
+          Diag(E->getExprLoc(), DiagID)
+              << FunctionPointerType << S.str() << E->getSourceRange() << Range
+              << IsEqual;
+          return;


Suggested change

std::string Str;

llvm::raw_string_ostream S(Str);

const unsigned DiagID = diag::warn_impcast_pointer_to_bool;

// For lambdas, the pointer type is function, which corresponds to 1.

const unsigned FunctionPointerType = 1;

// Pretty print the diagnostic for the warning

E->printPretty(S, nullptr, getPrintingPolicy());

Diag(E->getExprLoc(), DiagID)

<< FunctionPointerType << S.str() << E->getSourceRange() << Range

<< IsEqual;

return;

Diag(E->getExprLoc(), diag::warn_impcast_pointer_to_bool)

<< /*FunctionPointerType*/ 1 << S.str() << E->getSourceRange() << Range

<< IsEqual;

return;

Two changes happening here: 1) moves some constant variables inline, 2) removes the call to printPretty().

(2) is because we expect diagnostic output to all be piped through the diagnostics engine, so that we can do things like format it differently (e.g., perhaps print it as SARIF rather than text)

OK, understood. I will make the required changes, and force push again. Thanks for the review!

Removing the call to printPretty will not populate S, so the diagnostic will not have the name for the identifier describing the error-causing function but instead display ' '. Is there a workaround for this? Or should I leave it be? Thanks!

Oh gosh, this was a think-o on my part, you're right, that is being passed through the diagnostics engine. Sorry for the noise, I must have been tired. :-)

vinayakdsci · 2024-02-28T08:49:53Z

There are some related test failures caught by precommit CI that need to be addressed.

The issue in dr18xx.cpp looks to be a case where we should replace bool b = with auto b = (it doesn't change the testing for what's discussed in http://wg21.link/cwg1837).

Changing bool b = to auto b = does not compile, as the compiler complains that auto is not allowed in non-static struct members. b cannot be declared as static either, which would cause an invalid use of this outside of a non-static member function. It looks like type deduction using auto for member variables is not allowed.

AaronBallman · 2024-02-28T12:35:09Z

There are some related test failures caught by precommit CI that need to be addressed.
The issue in dr18xx.cpp looks to be a case where we should replace bool b = with auto b = (it doesn't change the testing for what's discussed in http://wg21.link/cwg1837).

Changing bool b = to auto b = does not compile, as the compiler complains that auto is not allowed in non-static struct members. b cannot be declared as static either, which would cause an invalid use of this outside of a non-static member function. It looks like type deduction using auto for member variables is not allowed.

Yeah, another think-o on my part. I think it's fine to add the expected diagnostic to the file instead of silencing the warning.

Endilll · 2024-02-28T14:50:07Z

clang/test/CXX/drs/dr18xx.cpp

@@ -287,6 +287,11 @@ namespace dr1837 { // dr1837: 3.3
      };
    };
  };
+  /* since-cxx11-warning@-6{{address of function '[] {


Can you convert this to use a marker? We don't want readers to count relative offsets. In other words, it should read since-cxx11-warning@#dr1837-lambda {{address of function ...

You can see an example in the test for 1890.

AaronBallman · 2024-02-28T14:36:51Z

clang/lib/Sema/SemaChecking.cpp

+  if (const auto *MCallExpr = dyn_cast<CXXMemberCallExpr>(E)) {
+    if (MCallExpr->getObjectType()->isRecordType()) {
+      if (const auto *MRecordDecl = MCallExpr->getRecordDecl()) {
+        if (MRecordDecl->isLambda()) {
+          std::string Str;
+          llvm::raw_string_ostream S(Str);
+
+          E->printPretty(S, nullptr, getPrintingPolicy());
+          Diag(E->getExprLoc(), diag::warn_impcast_pointer_to_bool)
+              << /*FunctionPointerType*/ 1 << S.str() << E->getSourceRange()
+              << Range << IsEqual;
+          return;
+        }
+      }
+    }
+  }


Do you actually need the call to isRecordType()? I would have thought this was equivalent.

Suggested change

if (const auto *MCallExpr = dyn_cast<CXXMemberCallExpr>(E)) {

if (MCallExpr->getObjectType()->isRecordType()) {

if (const auto *MRecordDecl = MCallExpr->getRecordDecl()) {

if (MRecordDecl->isLambda()) {

std::string Str;

llvm::raw_string_ostream S(Str);

E->printPretty(S, nullptr, getPrintingPolicy());

Diag(E->getExprLoc(), diag::warn_impcast_pointer_to_bool)

<< /*FunctionPointerType*/ 1 << S.str() << E->getSourceRange()

<< Range << IsEqual;

return;

}

}

}

}

if (const auto *MCallExpr = dyn_cast<CXXMemberCallExpr>(E)) {

if (const auto *MRecordDecl = MCallExpr->getRecordDecl(); MRecordDecl && MRecordDecl->isLambda()) {

std::string Str;

llvm::raw_string_ostream S(Str);

E->printPretty(S, nullptr, getPrintingPolicy());

Diag(E->getExprLoc(), diag::warn_impcast_pointer_to_bool)

<< /*FunctionPointerType*/ 1 << S.str() << E->getSourceRange()

<< Range << IsEqual;

return;

}

}

Actually, I'm not convinced we should print the entire expression in this case. Lambdas usually aren't nearly as trivial as the ones we have in our test cases, and can be arbitrarily long. e.g.,

C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:2:7: warning: address of function '[]() { int foo = 0; return foo; }' will always evaluate to 'true' [-Wpointer-bool-conversion] 2 | if ([](){ | ~~ ^~~~~ 3 | // This is a comment | ~~~~~~~~~~~~~~~~~~~~ 4 | // intended to make the lambda | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 5 | // longer so that it shows | ~~~~~~~~~~~~~~~~~~~~~~~~~~ 6 | // what the diagnostic behavior is | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 7 | int foo = 0; | ~~~~~~~~~~~~ 8 | // of really large lambdas, and whether | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 9 | // it is worth it to print out the entire | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 10 | // expression in this case. | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ 11 | return foo; | ~~~~~~~~~~~ 12 | }) { | ~ 1 warning generated.

This has two problems: 1) it repeats the user's code in both the diagnostic and in the highlighting, 2) the amount of context is huge and distracts from the diagnostic.

I think we should modify warn_impcast_pointer_to_bool to something along these lines:

def warn_impcast_pointer_to_bool : Warning< "address of%select{| function| array| lambda function pointer conversion operator}0 '%1' will always evaluate to 'true'">, InGroup<PointerBoolConversion>;

(Will need to be reformatted for the usual 80-col limits.) And when passing the source range for the lambda expression, I think we should pass only the lambda introducer as the source range so that we highlight far less code.

WDYT?

That is actually a great idea! and then the diagnostic would simply say address of lambda function {..} will always evaluate to true. Would actually be more informative than printing the entire expression in my opinion. Should I modify the code to do this instead??

Should I modify the code to do this instead??

Yes please!

vinayakdsci · 2024-02-28T15:53:13Z

Done! I added the diagnostic to the TableGen file, and passed in only the Record's source range instead of the whole expression's. Now only the lambda's initializer is highlighted in the diagnostic. I hope this does it!

AaronBallman · 2024-02-28T16:58:41Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+    "address of%select{| function| array| lambda function pointer conversion operator}0 '%1' "
+    "will always evaluate to 'true'">,


Suggested change

"address of%select{| function| array| lambda function pointer conversion operator}0 '%1' "

"will always evaluate to 'true'">,

"address of %select{'%1'|function '%1'|array '%1'|lambda function pointer "

"conversion operator}0 will always evaluate to 'true'">,

Wrapping to 80-col limits, moves %1 so it's not used for lambda conversion operators.

AaronBallman · 2024-02-28T17:02:21Z

clang/lib/Sema/SemaChecking.cpp

+      std::string Str;
+      llvm::raw_string_ostream S(Str);
+
+      E->printPretty(S, nullptr, getPrintingPolicy());
+      Diag(E->getExprLoc(), diag::warn_impcast_pointer_to_bool)
+          << /*LambdaPointerConversionOperatorType*/ 3 << S.str()
+          << MRecordDecl->getSourceRange() << Range << IsEqual;


Suggested change

std::string Str;

llvm::raw_string_ostream S(Str);

E->printPretty(S, nullptr, getPrintingPolicy());

Diag(E->getExprLoc(), diag::warn_impcast_pointer_to_bool)

<< /*LambdaPointerConversionOperatorType*/ 3 << S.str()

<< MRecordDecl->getSourceRange() << Range << IsEqual;

Diag(E->getExprLoc(), diag::warn_impcast_pointer_to_bool)

<< /*LambdaPointerConversionOperatorType*/ 3

<< MRecordDecl->getSourceRange() << Range << IsEqual;

This way we don't pretty print the entire lambda body in the diagnostic message.

vinayakdsci · 2024-02-28T17:41:02Z

@AaronBallman sorry for repeating the same mistake twice, I was unable to correctly interpret what you wanted to say when you meant removing the lambda body from the diagnostic.
I have made all the suggested changes and updated the tests. I hope I don't come across as dumb :-) ! I will be more mindful of what changes I have made before pushing them to the remote in the future.

Thanks!

AaronBallman · 2024-02-28T17:58:54Z

@AaronBallman sorry for repeating the same mistake twice, I was unable to correctly interpret what you wanted to say when you meant removing the lambda body from the diagnostic. I have made all the suggested changes and updated the tests. I hope I don't come across as dumb :-) ! I will be more mindful of what changes I have made before pushing them to the remote in the future.

No worries at all, your contribution is very appreciated! Also, it's not like I have room to judge -- I suggested breaking code and making a useless test! We all make mistakes. ;-)

AaronBallman

LGTM! @Endilll, do you have any concerns?

Endilll

DR test changes look good, save for a nit.

Endilll · 2024-02-28T19:12:57Z

clang/test/CXX/drs/dr18xx.cpp

@@ -281,7 +281,7 @@ namespace dr1837 { // dr1837: 3.3

  struct A {
    int f();
-    bool b = [] {
+    bool b = [] { // since-cxx11-warning{{address of lambda function pointer conversion operator will always evaluate to 'true'}}


Convert this to // since-cxx11-warning@-1 style, to follow the rest of the file.

shafik

Thank you for this fix. I am happy the changes were not too different from my suggestion. Sometimes problems end up being more difficult then they seem initially.

shafik · 2024-02-29T02:01:23Z

clang/lib/Sema/SemaChecking.cpp

+    if (const auto *MRecordDecl = MCallExpr->getRecordDecl();
+        MRecordDecl && MRecordDecl->isLambda()) {
+      Diag(E->getExprLoc(), diag::warn_impcast_pointer_to_bool)
+          << /*LambdaPointerConversionOperatorType*/ 3


Suggested change

<< /*LambdaPointerConversionOperatorType*/ 3

<< /*LambdaPointerConversionOperatorType=*/ 3

nitpick just to be consistent w/ bugprone-argument-comment, although I don't think it actually catches these cases.

github-actions · 2024-02-29T05:05:17Z

✅ With the latest revision this PR passed the C/C++ code formatter.

vinayakdsci · 2024-02-29T05:12:24Z

@shafik I have made the change in the code as you suggested. If everything seems alright, could you land this for me?

Thanks a lot!

MaskRay · 2024-03-01T01:39:37Z

Created #83497 as a follow-up to suppress the diagnostic for certain template instantiation uses.

I made one change to unblock our internal users

#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wpointer-bool-conversion"
#endif
    // functor_ may be a lambda, which is convertible to bool, leading to a
    // -Wpointer-bool-conversion warning because the value is always true.
    return functor_ == nullptr;
#ifdef __clang__
#pragma clang diagnostic pop
#endif

and then (when I saw another instance of wrapping a callable object) realized we probably should suppress the diagnostic for instantiation. This is similar to how we suppress the diagnostic for instantiations for -Wtautological-compare

…nversion diagnostic during instantiation (#83497) I have seen two internal pieces of code that uses a template type parameter to accept any callable type (function pointer, std::function, closure type, etc). The diagnostic added in #83152 would require adaptation to the template, which is difficult and also seems unnecessary. Example: ```cpp template <typename... Ts> static bool IsFalse(const Ts&...) { return false; } template <typename T, typename... Ts, typename = typename std::enable_if<std::is_constructible<bool, const T&>::value>::type> static bool IsFalse(const T& p, const Ts&...) { return p ? false : true; } template <typename... Args> void Init(Args&&... args) { if (IsFalse(absl::implicit_cast<const typename std::decay<Args>::type&>( args)...)) { // A callable object convertible to false is either a null pointer or a // null functor (e.g., a default-constructed std::function). empty_ = true; } else { empty_ = false; new (&factory_) Factory(std::forward<Args>(args)...); } } ```

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 27, 2024

AaronBallman requested changes Feb 27, 2024

View reviewed changes

vinayakdsci force-pushed the pointer-bool-address-warning-fix branch from 859ce0b to 0e9d7e7 Compare February 28, 2024 14:26

vinayakdsci requested a review from Endilll as a code owner February 28, 2024 14:26

vinayakdsci force-pushed the pointer-bool-address-warning-fix branch from 0e9d7e7 to c65c6a3 Compare February 28, 2024 14:27

vinayakdsci requested a review from AaronBallman February 28, 2024 14:29

Endilll requested changes Feb 28, 2024

View reviewed changes

AaronBallman reviewed Feb 28, 2024

View reviewed changes

vinayakdsci force-pushed the pointer-bool-address-warning-fix branch from c65c6a3 to 613e7c0 Compare February 28, 2024 15:45

vinayakdsci requested a review from AaronBallman February 28, 2024 16:40

AaronBallman reviewed Feb 28, 2024

View reviewed changes

vinayakdsci force-pushed the pointer-bool-address-warning-fix branch from 613e7c0 to 640cbf9 Compare February 28, 2024 17:38

AaronBallman approved these changes Feb 28, 2024

View reviewed changes

Endilll approved these changes Feb 28, 2024

View reviewed changes

shafik reviewed Feb 29, 2024

View reviewed changes

vinayakdsci force-pushed the pointer-bool-address-warning-fix branch from 640cbf9 to 60620d1 Compare February 29, 2024 05:03

[Clang][Sema]: Diagnose lambda to bool implicit casts

3d6100a

vinayakdsci force-pushed the pointer-bool-address-warning-fix branch from 60620d1 to 3d6100a Compare February 29, 2024 05:08

AaronBallman merged commit 6f7d824 into llvm:main Feb 29, 2024

MaskRay mentioned this pull request Feb 29, 2024

[Sema] -Wpointer-bool-conversion: suppress lambda function pointer conversion diagnostic during instantiation #83497

Merged

		"address of%select{\| function\| array\| lambda function pointer conversion operator}0 '%1' "
		"will always evaluate to 'true'">,

	<< /LambdaPointerConversionOperatorType/ 3
	<< /LambdaPointerConversionOperatorType=/ 3

[Clang][Sema]: Diagnose lambda to bool implicit casts #83152

[Clang][Sema]: Diagnose lambda to bool implicit casts #83152

Uh oh!

Conversation

vinayakdsci commented Feb 27, 2024

Uh oh!

llvmbot commented Feb 27, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinayakdsci commented Feb 28, 2024

Uh oh!

AaronBallman commented Feb 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinayakdsci commented Feb 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinayakdsci commented Feb 28, 2024

Uh oh!

AaronBallman commented Feb 28, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Endilll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vinayakdsci commented Feb 29, 2024

Uh oh!

MaskRay commented Mar 1, 2024

Uh oh!

Uh oh!

github-actions bot commented Feb 29, 2024 •

edited

Loading