Support constructing empty function_ref from other callables that can be "empty"

[mehdi_amini]

Summary

This fixes a surprising behavior where changing an existing function_ref
to std::function can break some code in a subtle manner. Here is the
pattern:

void foo(llvm::function_ref<void()) callback) {
  if (callback) callback();
}

void bar(llvm::function_ref<void()) callback) {
  foo(callback);
  // do something else with callback, accept an empty callback.
}

Assuming bar needs to change to take a std::function (for example
to be able to store it), the code in foo() would crash if callback
is a default constructed function.

[csigg]

llvm/include/llvm/ADT/STLFunctionalExtras.h

76

Instead of exposing this as an extra c'tor template parameter, would it make sense to split this out?

  // Handles construction from an empty callable (for example a default
  // constructed std::function) and ensure that bool conversion to `false` is
  // propagated.
  template <typename Callable,
            std::enable_if_t<std::is_constructible<bool, Callable>::value> * =
                nullptr>
  static Ret get_callback(const Callable &callable) {
    return callable ? callback_fn<std::remove_reference_t<Callable>> : nullptr;
  }
  template <typename Callable> static Ret get_callback(...) {
    return callback_fn<std::remove_reference_t<Callable>>;
  }

public:
  ...

and then, on line 66:

: callback(get_callback<Callable>(callable)),

90

I thought this wouldn't apply to std::function because it's only explicitly convertible to bool?

https://stackoverflow.com/questions/66438225/how-should-i-check-whether-a-type-is-contextually-convertible-to-bool

[mehdi_amini]

Address comments

llvm/include/llvm/ADT/STLFunctionalExtras.h

90

You're right! I ran ninja check-mlir locally by force of habit, but this won't run the LLVM test...

[mehdi_amini]

@dexonsmith thoughts on this?

[dblaikie]

I worry this is overly general (it's not obvious that any functor with boolean conversion has the semantics this patch relies on) & inconsistent with the standard library (std::function for instance doesn't provide this (or a narrower version of it) functionality - so we'll still have the bugs for any API that takes a function_ref and passes it to std::function)

I don't know that there's /no/ useful direction here, but I have my reservations.

(@rsmith as the original author of function_ref)

[dexonsmith]

In D123576#3467855, @dblaikie wrote:

I worry this is overly general (it's not obvious that any functor with boolean conversion has the semantics this patch relies on) & inconsistent with the standard library (std::function for instance doesn't provide this (or a narrower version of it) functionality - so we'll still have the bugs for any API that takes a function_ref and passes it to std::function)

Seems nice to make it safer than the STL. Would you be more comfortable if it were special-cased to known functions that behave this way (std::function and llvm::unique_function)? For your other concern, I'm wondering if we could also make it symmetric (add operator std::function() that adds this check somehow) or if that would lead to ambiguities.

I don't know that there's /no/ useful direction here, but I have my reservations.

(@rsmith as the original author of function_ref)

[dblaikie]

In D123576#3469352, @dexonsmith wrote:

In D123576#3467855, @dblaikie wrote:

I worry this is overly general (it's not obvious that any functor with boolean conversion has the semantics this patch relies on) & inconsistent with the standard library (std::function for instance doesn't provide this (or a narrower version of it) functionality - so we'll still have the bugs for any API that takes a function_ref and passes it to std::function)

Seems nice to make it safer than the STL. Would you be more comfortable if it were special-cased to known functions that behave this way (std::function and llvm::unique_function)?

Would certainly be more comfortable (guess we could have a static_assert that fails if the functor is boolean convertible but not in the list - suggesting they be added to the list (& if someone comes up with a functor that can't be added to the list because its boolean testability doesn't have the desired semantics, we can keep some explicit allow-list of "convertible-to-function_ref without boolean testing despite being boolean testable for some other semantics")
For your other concern, I'm wondering if we could also make it symmetric (add operator std::function() that adds this check somehow) or if that would lead to ambiguities.

I don't know that there's /no/ useful direction here, but I have my reservations.

(@rsmith as the original author of function_ref)

Wouldn't mind hearing from @rsmith still, but wouldn't block on that if this is restricted to only known-good cases.

[mehdi_amini]

How do you see the list declared?
I mean we can list std::function because we include the header, but would you / could you forward déclaré absl, folly, boost, etc. equivalent here? I feel I missed something?

By the way an alternative it to do what absl does and remove the Boolean operator and force llvm::function_ref to always wrap something “non null” on construction

[dblaikie]

In D123576#3475821, @mehdi_amini wrote:

How do you see the list declared?
I mean we can list std::function because we include the header, but would you / could you forward déclaré absl, folly, boost, etc. equivalent here? I feel I missed something?

Oh, I don't think it could/should be made compatible with those - only standard and llvm things.

By the way an alternative it to do what absl does and remove the Boolean operator and force llvm::function_ref to always wrap something “non null” on construction

Sort of has the same issue, though - it'd require either ignoring the boolean testable semantics of the source functor (which would leave the same bugs in the program - we'd produce unqualified calls to the functor and crash at runtime) or we build in the assumption that any (or some subset of) boolean testable functor is expressing nullability and assert that the functor is true/non-null on conversion.

llvm/include/llvm/ADT/STLFunctionalExtras.h

class function_ref<Ret(Params...)> {
   intptr_t callable;
 
   template<typename Callable>
   static Ret callback_fn(intptr_t callable, Params ...params) {
     return (*reinterpret_cast<Callable*>(callable))(
         std::forward<Params>(params)...);
   }
 
+  // Handles construction from an empty callable (for example a default
+  // constructed std::function) and ensure that bool conversion to `false` is
+  // propagated.
+  template <typename Callable,
+            std::enable_if_t<std::is_constructible<bool, Callable>::value> * =
+                nullptr>
+  static decltype(callback) get_callback(const Callable &callable) {
+    return callable ? callback_fn<std::remove_reference_t<Callable>> : nullptr;
+  }
+  template <typename Callable,
+            std::enable_if_t<!std::is_constructible<bool, Callable>::value> * =
+                nullptr>
+  static decltype(callback) get_callback(const Callable &callable) {
+    return callback_fn<std::remove_reference_t<Callable>>;
+  }
+
 public:
   function_ref() = default;
   function_ref(std::nullptr_t) {}
 
   template <typename Callable>
   function_ref(
       Callable &&callable,
       // This is not the copy-constructor.
       std::enable_if_t<!std::is_same<remove_cvref_t<Callable>,
                                      function_ref>::value> * = nullptr,
       // Functor must be callable and return a suitable type.
       std::enable_if_t<std::is_void<Ret>::value ||
                        std::is_convertible<decltype(std::declval<Callable>()(
                                                std::declval<Params>()...)),
                                            Ret>::value> * = nullptr)
-      : callback(callback_fn<typename std::remove_reference<Callable>::type>),
+      : callback(get_callback<Callable>(callable)),
         callable(reinterpret_cast<intptr_t>(&callable)) {}
 
   Ret operator()(Params ...params) const {
     return callback(callable, std::forward<Params>(params)...);
   }
 
   explicit operator bool() const { return callback; }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_ADT_STLFUNCTIONALEXTRAS_H

llvm/unittests/ADT/FunctionRefTest.cpp

 std::string returns(function_ref<StringRef()> F) { return "string"; }
 
 TEST(FunctionRefTest, SFINAE) {
   EXPECT_EQ("not a function", returns("boo!"));
   EXPECT_EQ("number", returns([] { return 42; }));
   EXPECT_EQ("string", returns([] { return "hello"; }));
 }
 
+TEST(FunctionRefTest, std_function) {
+  // Check that function_ref constructed from an empty std::function is empty.
+  std::function<void()> f;
+  function_ref<void()> f_ref(f);
+  ASSERT_FALSE((bool)f_ref);
+}
+
 } // namespace