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

[MaskRay]

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:

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]

@llvm/pr-subscribers-clang

Author: Fangrui Song (MaskRay)

Changes

I have seen internal pieces of code that uses a template type parameter
to accept std::function, a closure type, or any callable type. The
diagnostic added in #83152 would require adaptation to the template,
which is difficult and also seems unnecessary.

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 Partial&, 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)...);
  }
}

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaChecking.cpp (+10-7)
• (modified) clang/test/SemaCXX/warn-bool-conversion.cpp (+13)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 690bdaa63d058b..3533b5c8e0d235 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -16586,13 +16586,16 @@ void Sema::DiagnoseAlwaysNonNullPointer(Expr *E,
   }
 
   // Complain if we are converting a lambda expression to a boolean value
-  if (const auto *MCallExpr = dyn_cast<CXXMemberCallExpr>(E)) {
-    if (const auto *MRecordDecl = MCallExpr->getRecordDecl();
-        MRecordDecl && MRecordDecl->isLambda()) {
-      Diag(E->getExprLoc(), diag::warn_impcast_pointer_to_bool)
-          << /*LambdaPointerConversionOperatorType=*/3
-          << MRecordDecl->getSourceRange() << Range << IsEqual;
-      return;
+  // outside of instantiation.
+  if (!inTemplateInstantiation()) {
+    if (const auto *MCallExpr = dyn_cast<CXXMemberCallExpr>(E)) {
+      if (const auto *MRecordDecl = MCallExpr->getRecordDecl();
+          MRecordDecl && MRecordDecl->isLambda()) {
+        Diag(E->getExprLoc(), diag::warn_impcast_pointer_to_bool)
+            << /*LambdaPointerConversionOperatorType=*/3
+            << MRecordDecl->getSourceRange() << Range << IsEqual;
+        return;
+      }
     }
   }
 
diff --git a/clang/test/SemaCXX/warn-bool-conversion.cpp b/clang/test/SemaCXX/warn-bool-conversion.cpp
index 9e8cf0e4f8944a..8b1bf80af79d26 100644
--- a/clang/test/SemaCXX/warn-bool-conversion.cpp
+++ b/clang/test/SemaCXX/warn-bool-conversion.cpp
@@ -92,6 +92,19 @@ void foo() {
   bool is_true = [](){ return true; };
   // expected-warning@-1{{address of lambda function pointer conversion operator will always evaluate to 'true'}}
 }
+
+template <typename... Ts>
+static bool IsFalse(const Ts&...) { return false; }
+template <typename T>
+static bool IsFalse(const T& p) {
+  bool b;
+  b = f7; // expected-warning {{address of lambda function pointer conversion operator will always evaluate to 'true'}}
+  return p ? false : true;
+}
+
+bool use_instantiation() {
+  return IsFalse([]() { return 0; });
+}
 #endif
 
 void bar() {

[MaskRay]

@vinayakdsci

[AaronBallman]

LGTM with a minor commenting nit. Thank you for catching this and the quick patch!

[AaronBallman]

Suggested change

	return p ? false : true;
	// Intentionally not warned on because p could be a lambda type in one instantiation, but a pointer
	// type in another.
	return p ? false : true;

(May need to adjust for 80-col limits.)

[MaskRay]

Thanks for the suggestion! Adjusted to 80-col.

[vinayakdsci]

Great catch! The usage of template instantiation didn't seem very obvious while adding the tests, and hence the missed test case.
Thanks a lot for the fix!

[dwblaikie]

Presumably similar things might show up in macros? But can cross that bridge when we come to it.

Perhaps we have some/could use some generic utility for this sort of contextual warning "if these things are literally written next to each other, warn, but if they came to be via template instantiation, macro, (something else?) then don't" because that sort of situation shows up pretty regularly in diagnostics, I think...

[MaskRay]

Presumably similar things might show up in macros? But can cross that bridge when we come to it.

Perhaps we have some/could use some generic utility for this sort of contextual warning "if these things are literally written next to each other, warn, but if they came to be via template instantiation, macro, (something else?) then don't" because that sort of situation shows up pretty regularly in diagnostics, I think...

I modeled the template instantiation suppression after diagnoseTautologicalComparison.

While some macro uses suppress diagnoseTautologicalComparison diagnostics, e.g.

// Don't issue a warning when either the left or right side of the comparison
// results from a macro expansion.
#define R8435950_A i
#define R8435950_B i

int R8435950(int i) {
  if (R8435950_A == R8435950_B) // no-warning
   return 0;
  return 1;
}

some have diagnostics (e.g. https://reviews.llvm.org/D70624). Yes, we can cross that bridge when we come to it.