C++ Subsystem::SetDefaultCommand no longer accepts l-value references

[bhall-ctre]

Describe the bug
In 2023, the C++ Subsystem::SetDefaultCommand function could accept an l-value reference to a command variable. However, the new usage of concepts breaks this use-case.

The function previously used std::enable_if_t<std::is_base_of_v<Command, std::remove_reference_t<T>>> to check if the type passed in was a subclass of Command, accepting both l-value and r-value references. Now, the function uses the concept std::derived_from<Command> T, which does not remove the reference. As a result, passing in an l-value reference results in std::derived_from<T&, Command>, and std::is_base_of_v<Command, T&> == false.

To Reproduce
Steps to reproduce the behavior:

1. Construct a RunCommand m_defaultCommand (any subclass of command also works) as a member variable of RobotContainer.
2. Construct a subsystem m_subsystem as a member variable of RobotContainer.
3. In the RobotContainer subsystem, call m_subsystem.SetDefaultCommand(m_defaultCommand);. This does not compile.

Example code from 2023

Expected behavior
This example should compile, just as it did in 2023 WPILib.

Screenshots
n/a

Desktop (please complete the following information):

• OS: Windows 11
• Project Information:

WPILib Information:
Project Version: 2024.1.1-beta-1
VS Code Version: 1.83.1
WPILib Extension Version: 2024.1.1-beta-1
C++ Extension Version: 1.17.5
Java Extension Version: 1.23.0
Java Debug Extension Version: 0.52.0
Java Dependencies Extension Version 0.23.0
Java Version: 17
Java Location: C:\Users\Public\wpilib\2024\jdk
Vendor Libraries:
    WPILib-New-Commands (1.0.0)

Additional context
One possible solution is to define a concept that handles this:

namespace wpi {
    template <class Derived, class Base>
    concept ref_derived_from = std::derived_from<std::remove_reference_t<Derived>, Base>;
}

Another solution is to use something like requires std::derived_from<std::remove_reference_t<T>, Command> instead.

[calcmogul]

This affects more than just this function, so we should look into defining a concept that unifies all our possible command types (e.g., std::unique_ptr, CommandPtr, derived class of Command), and apply that everywhere.

[Starlight220]

In 2023, the C++ Subsystem::SetDefaultCommand function could accept an l-value reference to a command variable.

That sounds quite contradictory to the design that default commands are owned by the scheduler -- if passing an lvalue compiled, then that was probably a bug and that is extremely prone to use-after-move.

I now tested this by making the following changes to the GearsBot example from v2023.4.3:

diff --git a/wpilibcExamples/src/main/cpp/examples/GearsBot/cpp/RobotContainer.cpp b/wpilibcExamples/src/main/cpp/examples/GearsBot/cpp/RobotContainer.cpp
index c08faee74..330be15f9 100644
--- a/wpilibcExamples/src/main/cpp/examples/GearsBot/cpp/RobotContainer.cpp
+++ b/wpilibcExamples/src/main/cpp/examples/GearsBot/cpp/RobotContainer.cpp
@@ -22,9 +22,7 @@ RobotContainer::RobotContainer()
   frc::SmartDashboard::PutData(&m_wrist);
   frc::SmartDashboard::PutData(&m_claw);

-  m_drivetrain.SetDefaultCommand(
-      TankDrive([this] { return -m_joy.GetLeftY(); },
-                [this] { return -m_joy.GetRightY(); }, m_drivetrain));
+  m_drivetrain.SetDefaultCommand(m_autonomousCommand);

   // Configure the button bindings
   ConfigureButtonBindings();

This doesn't compile, which is the correct behavior.

Perhaps some Command types have copy-constructors for some reason -- and that's causing some issues/inconsistency?

---

Regardless, unifying all the different command types we take is a good idea.
We'd need two concepts that basically boil down to CommandPtr (owned) and Command* (non-owned) respectively.
I have some experiments on a local fork, not yet ready for a PR -- if someone else wants to tackle it, a PR would be welcome.