From a770110438328833535b4e35238aeaa50e7a14a2 Mon Sep 17 00:00:00 2001 From: Starlight220 <53231611+Starlight220@users.noreply.github.com> Date: Sat, 9 Dec 2023 19:45:02 +0200 Subject: [PATCH] [commands] CommandCompositionError: Include stacktrace of original composition (#5984) --- .../wpilibj2/command/CommandScheduler.java | 32 +++++++++++-------- .../main/native/cpp/frc2/command/Command.cpp | 15 +++++++-- .../native/cpp/frc2/command/CommandPtr.cpp | 9 +++++- .../cpp/frc2/command/CommandScheduler.cpp | 8 +++-- .../native/include/frc2/command/Command.h | 12 ++++++- .../native/include/frc2/command/CommandPtr.h | 4 ++- 6 files changed, 57 insertions(+), 23 deletions(-) diff --git a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java index 41c969e7f1..b869f4e086 100644 --- a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java +++ b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java @@ -61,7 +61,7 @@ public final class CommandScheduler implements Sendable, AutoCloseable { private static final Optional kNoInterruptor = Optional.empty(); - private final Set m_composedCommands = Collections.newSetFromMap(new WeakHashMap<>()); + private final Map m_composedCommands = new WeakHashMap<>(); // A set of the currently-running commands. private final Set m_scheduledCommands = new LinkedHashSet<>(); @@ -586,7 +586,11 @@ public final class CommandScheduler implements Sendable, AutoCloseable { public void registerComposedCommands(Command... commands) { var commandSet = Set.of(commands); requireNotComposed(commandSet); - m_composedCommands.addAll(commandSet); + var exception = new Exception("Originally composed at:"); + exception.fillInStackTrace(); + for (var command : commands) { + m_composedCommands.put(command, exception); + } } /** @@ -615,14 +619,18 @@ public final class CommandScheduler implements Sendable, AutoCloseable { /** * Requires that the specified command hasn't been already added to a composition. * - * @param command The command to check + * @param commands The commands to check * @throws IllegalArgumentException if the given commands have already been composed. */ - public void requireNotComposed(Command command) { - if (m_composedCommands.contains(command)) { - throw new IllegalArgumentException( - "Commands that have been composed may not be added to another composition or scheduled " - + "individually!"); + public void requireNotComposed(Command... commands) { + for (var command : commands) { + var exception = m_composedCommands.getOrDefault(command, null); + if (exception != null) { + throw new IllegalArgumentException( + "Commands that have been composed may not be added to another composition or scheduled " + + "individually!", + exception); + } } } @@ -633,11 +641,7 @@ public final class CommandScheduler implements Sendable, AutoCloseable { * @throws IllegalArgumentException if the given commands have already been composed. */ public void requireNotComposed(Collection commands) { - if (!Collections.disjoint(commands, getComposedCommands())) { - throw new IllegalArgumentException( - "Commands that have been composed may not be added to another composition or scheduled " - + "individually!"); - } + requireNotComposed(commands.toArray(Command[]::new)); } /** @@ -651,7 +655,7 @@ public final class CommandScheduler implements Sendable, AutoCloseable { } Set getComposedCommands() { - return m_composedCommands; + return m_composedCommands.keySet(); } @Override diff --git a/wpilibNewCommands/src/main/native/cpp/frc2/command/Command.cpp b/wpilibNewCommands/src/main/native/cpp/frc2/command/Command.cpp index 66de555ca9..de673f2c41 100644 --- a/wpilibNewCommands/src/main/native/cpp/frc2/command/Command.cpp +++ b/wpilibNewCommands/src/main/native/cpp/frc2/command/Command.cpp @@ -4,6 +4,7 @@ #include "frc2/command/Command.h" +#include #include #include @@ -31,7 +32,7 @@ Command::~Command() { } Command& Command::operator=(const Command& rhs) { - m_isComposed = false; + SetComposed(false); return *this; } @@ -156,11 +157,19 @@ bool Command::HasRequirement(Subsystem* requirement) const { } bool Command::IsComposed() const { - return m_isComposed; + return GetPreviousCompositionSite().has_value(); } void Command::SetComposed(bool isComposed) { - m_isComposed = isComposed; + if (isComposed) { + m_previousComposition = wpi::GetStackTrace(1); + } else { + m_previousComposition.reset(); + } +} + +std::optional Command::GetPreviousCompositionSite() const { + return m_previousComposition; } void Command::InitSendable(wpi::SendableBuilder& builder) { diff --git a/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandPtr.cpp b/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandPtr.cpp index cbbcc14640..6fa86d3003 100644 --- a/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandPtr.cpp +++ b/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandPtr.cpp @@ -27,10 +27,17 @@ CommandPtr::CommandPtr(std::unique_ptr&& command) AssertValid(); } +CommandPtr::CommandPtr(CommandPtr&& rhs) { + m_ptr = std::move(rhs.m_ptr); + AssertValid(); + rhs.m_moveOutSite = wpi::GetStackTrace(1); +} + void CommandPtr::AssertValid() const { if (!m_ptr) { throw FRC_MakeError(frc::err::CommandIllegalUse, - "Moved-from CommandPtr object used!"); + "Moved-from CommandPtr object used!\nMoved out at:\n{}", + m_moveOutSite); } } diff --git a/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp b/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp index b424533106..63d1e3bf14 100644 --- a/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp +++ b/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp @@ -453,11 +453,13 @@ void CommandScheduler::OnCommandFinish(Action action) { } void CommandScheduler::RequireUngrouped(const Command* command) { - if (command->IsComposed()) { + auto stacktrace = command->GetPreviousCompositionSite(); + if (stacktrace.has_value()) { throw FRC_MakeError(frc::err::CommandIllegalUse, "Commands that have been composed may not be added to " - "another composition or scheduled " - "individually!"); + "another composition or scheduled individually!" + "\nOriginally composed at:\n{}", + stacktrace.value()); } } diff --git a/wpilibNewCommands/src/main/native/include/frc2/command/Command.h b/wpilibNewCommands/src/main/native/include/frc2/command/Command.h index a1b5dc170f..7bb6213e75 100644 --- a/wpilibNewCommands/src/main/native/include/frc2/command/Command.h +++ b/wpilibNewCommands/src/main/native/include/frc2/command/Command.h @@ -6,11 +6,13 @@ #include #include +#include #include #include #include #include +#include #include #include "frc2/command/Requirements.h" @@ -387,6 +389,14 @@ class Command : public wpi::Sendable, public wpi::SendableHelper { */ void SetComposed(bool isComposed); + /** + * Get the stacktrace of where this command was composed, or an empty + * optional. Intended for internal use. + * + * @return optional string representation of the composition site stack trace. + */ + std::optional GetPreviousCompositionSite() const; + /** * Whether the given command should run when the robot is disabled. Override * to return true if the command should run when disabled. @@ -424,7 +434,7 @@ class Command : public wpi::Sendable, public wpi::SendableHelper { */ virtual std::unique_ptr TransferOwnership() && = 0; - bool m_isComposed = false; + std::optional m_previousComposition; }; /** diff --git a/wpilibNewCommands/src/main/native/include/frc2/command/CommandPtr.h b/wpilibNewCommands/src/main/native/include/frc2/command/CommandPtr.h index ed11d5defc..2610684b09 100644 --- a/wpilibNewCommands/src/main/native/include/frc2/command/CommandPtr.h +++ b/wpilibNewCommands/src/main/native/include/frc2/command/CommandPtr.h @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -35,7 +36,7 @@ class CommandPtr final { : CommandPtr( std::make_unique>(std::forward(command))) {} - CommandPtr(CommandPtr&&) = default; + CommandPtr(CommandPtr&&); CommandPtr& operator=(CommandPtr&&) = default; explicit CommandPtr(std::nullptr_t) = delete; @@ -327,6 +328,7 @@ class CommandPtr final { private: std::unique_ptr m_ptr; + std::string m_moveOutSite{""}; void AssertValid() const; };