[libc++][modules] Remove a dependency of __type_traits/invoke.h on __utility

[ldionne]

We can use static_cast<T&&> instead of std::forward to break up a cyclic dependency between __type_traits and __utility. While we normally favour using std::forward instead of static_cast, in this case using std::forward kinda breaks the layering of __type_traits being a dependency-free "submodule" of the codebase.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

We can use static_cast<T&&> instead of std::forward to break up a cyclic dependency between __type_traits and __utility. While we normally favour using std::forward instead of static_cast, in this case using std::forward kinda breaks the layering of __type_traits being a dependency-free "submodule" of the codebase.

---

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

1 Files Affected:

• (modified) libcxx/include/__type_traits/invoke.h (+2-3)

diff --git a/libcxx/include/__type_traits/invoke.h b/libcxx/include/__type_traits/invoke.h
index 71db32ae6a3cef..f17da7669c4512 100644
--- a/libcxx/include/__type_traits/invoke.h
+++ b/libcxx/include/__type_traits/invoke.h
@@ -23,7 +23,6 @@
 #include <__type_traits/is_void.h>
 #include <__type_traits/nat.h>
 #include <__utility/declval.h>
-#include <__utility/forward.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
@@ -213,7 +212,7 @@ template <class _Ret, bool = is_void<_Ret>::value>
 struct __invoke_void_return_wrapper {
   template <class... _Args>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 static _Ret __call(_Args&&... __args) {
-    return std::__invoke(std::forward<_Args>(__args)...);
+    return std::__invoke(static_cast<_Args&&>(__args)...);
   }
 };
 
@@ -221,7 +220,7 @@ template <class _Ret>
 struct __invoke_void_return_wrapper<_Ret, true> {
   template <class... _Args>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 static void __call(_Args&&... __args) {
-    std::__invoke(std::forward<_Args>(__args)...);
+    std::__invoke(static_cast<_Args&&>(__args)...);
   }
 };
 

[philnik777]

What exactly is the benefit here? This feels a lot like making the code worse to make a tool happy.

[ldionne]

What exactly is the benefit here? This feels a lot like making the code worse to make a tool happy.

The benefit is that we no longer include anything from __utility/ from a header inside __type_traits/. This lays down the foundation for __type_traits being a dependency-free module in the modulemap, which allows us to make it a single module (instead of multiple separate modules like it is today).

Another option would be to move forward.h out of __utility/ into its own module (or into its own dependency-free module that could e.g. contain std::move, std::forward, std::declval and maybe a few others). But I went for the simplest change I could think about here.

[philnik777]

What exactly is the benefit here? This feels a lot like making the code worse to make a tool happy.

The benefit is that we no longer include anything from __utility/ from a header inside __type_traits/. This lays down the foundation for __type_traits being a dependency-free module in the modulemap, which allows us to make it a single module (instead of multiple separate modules like it is today).

Another option would be to move forward.h out of __utility/ into its own module (or into its own dependency-free module that could e.g. contain std::move, std::forward, std::declval and maybe a few others). But I went for the simplest change I could think about here.

I think I'd rather go for having an extra module for now. I have a feeling that we can absorb most of the modulemap into a single large module, which would make the cross-module dependencies not a problem.

[ldionne]

I think I'd rather go for having an extra module for now. I have a feeling that we can absorb most of the modulemap into a single large module, which would make the cross-module dependencies not a problem.

I just tried and this actually doesn't solve the problem after all. The problem is that:

• <__utility/forward.h> uses <__type_traits/is_reference.h> and <__type_traits/remove_reference.h>
• <__type_traits/something> uses <__utility/forward.h>

That's the cycle, and even if forward.h is alone in its own module, it still creates a loop with __type_traits. So either we break <__type_traits/invoke.h> out of __type_traits, or we remove the dependency (which can be done like this patch does, or by not including __type_traits from forward.h but that seems more complicated).

Also note that the moment we make __type_traits/ its own top-level module, this failure stops being theoretical -- we would catch it in the CI.

[philnik777]

What if we put type_traits and utility together into a single module?

[ldionne]

What if we put type_traits and utility together into a single module?

type_traits is a really fundamental module with basically no dependencies except __config (after I'm done). On the other hand, utility defines stuff like pair which has outside dependencies like __compare, __tuple or __concepts. I think they should stay separate modules both conceptually and for practical reasons.

[philnik777]

What if we put type_traits and utility together into a single module?

type_traits is a really fundamental module with basically no dependencies except __config (after I'm done). On the other hand, utility defines stuff like pair which has outside dependencies like __compare, __tuple or __concepts. I think they should stay separate modules both conceptually and for practical reasons.

I mean something like

module type_traits_and_some_utility {
  module type_traits {
    module whatever { header "__type_traits/whatever.h" }
  }

  module some_utility {
    module forward { header "__utility/forward.h" }
  }
}

I think that should work and avoids the dependency problem. The module names could probably be better.

[ldionne]

I think that should work and avoids the dependency problem. The module names could probably be better.

This is basically equivalent to moving __utility/forward.h into __type_traits. I could also do that, but it feels like forward.h doesn't belong there.

Alternatively, your suggestion can be seen as:

module core {
  module type_traits { ... }
  module basic_utilities {
    header "__utility/forward.h"
    header "__utility/declval.h"
    header "__utility/move.h"
  }
}

But then that begs the question of defining what the core module is, which seems a bit arbitrary to me. It would also mean that our directory hierarchy does not mirror the modulemap anymore, which I can live with but certainly would find nice if we could have that property.

[philnik777]

I think that should work and avoids the dependency problem. The module names could probably be better.

This is basically equivalent to moving __utility/forward.h into __type_traits. I could also do that, but it feels like forward.h doesn't belong there.

Alternatively, your suggestion can be seen as:

module core {
  module type_traits { ... }
  module basic_utilities {
    header "__utility/forward.h"
    header "__utility/declval.h"
    header "__utility/move.h"
  }
}

But then that begs the question of defining what the core module is, which seems a bit arbitrary to me. It would also mean that our directory hierarchy does not mirror the modulemap anymore, which I can live with but certainly would find nice if we could have that property.

I'd rather do that for now and I'm fine with having somewhat arbitrary modules for some time. If this is actually fundamentally necessary we can look into removing the cycle later, but I'm not yet convinced it actually is. Basically, I'd rather sacrifice a bit of modulemap simplicity than code readability at this point, since we don't yet really know where the modulemap journey will get us.

[ldionne]

I'd rather do that for now and I'm fine with having somewhat arbitrary modules for some time. If this is actually fundamentally necessary we can look into removing the cycle later, but I'm not yet convinced it actually is. Basically, I'd rather sacrifice a bit of modulemap simplicity than code readability at this point, since we don't yet really know where the modulemap journey will get us.

That suggestion seems to work (functionally). One downside is that we won't catch the reintroduction of cycles between these submodules, allowing us to regress. But if we're careful not to add too many headers into this "core" module, perhaps it's OK.

I'll close this for now since we won't be going for this approach.

[ldionne]

As I experiment with this more and more, the approach of having a monolithic top-level module to avoid actually disentangling our code dependencies doesn't really work well. You end up in cycles with things that are outside that monolithic module extremely fast.

IMO the only sane way of proceeding is to actually disentangle the dependencies in the code. If you forget about the modulemap itself, what we need to do is basically simplify the include graph to make it acyclic, and then the modulemap will follow naturally from that. I don't think there's any way around that.