Page MenuHomePhabricator

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

Authored by mehdi_amini on Apr 12 2022, 1:10 AM.

Details

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.

Diff Detail

Event Timeline

mehdi_amini created this revision.Apr 12 2022, 1:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 1:10 AM
mehdi_amini requested review of this revision.Apr 12 2022, 1:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 1:10 AM
csigg added inline comments.Apr 12 2022, 1:33 AM
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 marked 2 inline comments as done.

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

csigg accepted this revision.Apr 12 2022, 2:08 AM
This revision is now accepted and ready to land.Apr 12 2022, 2:08 AM

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)

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)

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.

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

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.