Fix #58322: Handlers for debug actions with equal parameter types must not override each other

[tlongeri]

Summary

Also clean up redundant public access specifiers.

[tlongeri]

mlir/include/mlir/Support/DebugAction.h

198

TypeID::get<Handler> fails to build in GCC. It works fine in Clang and MSVC. GCC thinks Handler can also be a template for some reason (bug?), and complains the call is ambiguous because it can match the template <template <typename> class> TypeID::get overload.

[tlongeri]

Fix TypeID errors

[tlongeri]

Actually fix the TypeID errors

[rriddle]

Nice. Thanks for fixing this! Can you also update the documentation (DebugActions.md)?

[tlongeri]

Updating documentation (DebugActions.md)

mlir/docs/DebugActions.md

 rewrite patterns.
 
 ```c++
 /// A debug action that allows for controlling the application of patterns.
 /// A new action type can be defined by inheriting from `DebugAction`.
 /// * The Tag is specified via a static `StringRef getTag()` method.
 /// * The Description is specified via a static `StringRef getDescription()`
 ///   method.
-/// * The parameters for the action are provided via template parameters when
-///   inheriting from `DebugAction`.
+/// * `DebugAction` is a CRTP class, so the first template parameter is the
+///   action type class itself.
+/// * The parameters for the action are provided via additional template
+///   parameters when inheriting from `DebugAction`.
 struct ApplyPatternAction
-    : public DebugAction<Operation *, const Pattern &> {
+    : public DebugAction<ApplyPatternAction, Operation *, const Pattern &> {
   static StringRef getTag() { return "apply-pattern"; }
   static StringRef getDescription() {
     return "Control the application of rewrite patterns";
   }
 };
 ```
 
 ## Debug Action Manager

 ```
 
 Building on the example from the [previous section](#debug-action), an example
 usage of the `shouldExecute` query is shown below:
 
 ```c++
 /// A debug action that allows for controlling the application of patterns.
 struct ApplyPatternAction
-    : public DebugAction<Operation *, const Pattern &> {
+    : public DebugAction<ApplyPatternAction, Operation *, const Pattern &> {
   static StringRef getTag() { return "apply-pattern"; }
   static StringRef getDescription() {
     return "Control the application of rewrite patterns";
   }
 };
 
 // ...
 

mlir/include/mlir/Support/DebugAction.h

 ///     - This method returns a unique string identifier, similar to a command
 ///       line flag or DEBUG_TYPE.
 ///   * static llvm::StringRef getDescription()
 ///     - This method returns a short description of what the action represents.
 ///
 /// This class provides a handler class that can be derived from to handle
 /// instances of this action. The parameters to its query methods map 1-1 to the
 /// types on the action type.
-template <typename... ParameterTs>
+template <typename Derived, typename... ParameterTs>
 class DebugAction {
 public:
   class Handler : public DebugActionManager::HandlerBase {
   public:
-    Handler()
-        : HandlerBase(
-              TypeID::get<typename DebugAction<ParameterTs...>::Handler>()) {}
+    Handler() : HandlerBase(TypeID::get<Derived>()) {}
 
     /// This hook allows for controlling whether an action should execute or
     /// not. `parameters` correspond to the set of values provided by the
     /// action as context. It should return failure if the handler could not
     /// process the action, passing it to the next registered handler.
     virtual FailureOr<bool> shouldExecute(ParameterTs... parameters) {
       return failure();
     }
 
     /// Provide classof to allow casting between handler types.
     static bool classof(const DebugActionManager::HandlerBase *handler) {
-      return handler->getHandlerID() ==
-             TypeID::get<typename DebugAction<ParameterTs...>::Handler>();
+      return handler->getHandlerID() == TypeID::get<Derived>();
     }
   };
 
 private:
   /// Returns true if the action can be handled within the given set of
   /// parameter types.
   template <typename... CallerParameterTs>
   static constexpr bool canHandleWith() {

mlir/unittests/Support/DebugActionTest.cpp

 //===- DebugActionTest.cpp - Debug Action Tests ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Support/DebugAction.h"
+#include "mlir/Support/TypeID.h"
 #include "gmock/gmock.h"
 
 // DebugActionManager is only enabled in DEBUG mode.
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
 
 using namespace mlir;
 
 namespace {
-struct SimpleAction : public DebugAction<> {
+struct SimpleAction : DebugAction<SimpleAction> {
+  MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(SimpleAction)
   static StringRef getTag() { return "simple-action"; }
   static StringRef getDescription() { return "simple-action-description"; }
 };
-struct ParametricAction : public DebugAction<bool> {
+struct OtherSimpleAction : DebugAction<OtherSimpleAction> {
+  MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(OtherSimpleAction)
+  static StringRef getTag() { return "other-simple-action"; }
+  static StringRef getDescription() {
+    return "other-simple-action-description";
+  }
+};
+struct ParametricAction : DebugAction<ParametricAction, bool> {
+  MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(ParametricAction)
   static StringRef getTag() { return "param-action"; }
   static StringRef getDescription() { return "param-action-description"; }
 };
 
 TEST(DebugActionTest, GenericHandler) {
   DebugActionManager manager;
 
   // A generic handler that always executes the simple action, but not the
   // parametric action.
-  struct GenericHandler : public DebugActionManager::GenericHandler {
+  struct GenericHandler : DebugActionManager::GenericHandler {
     FailureOr<bool> shouldExecute(StringRef tag, StringRef desc) final {
       if (tag == SimpleAction::getTag()) {
         EXPECT_EQ(desc, SimpleAction::getDescription());
         return true;
       }
 
       EXPECT_EQ(tag, ParametricAction::getTag());
       EXPECT_EQ(desc, ParametricAction::getDescription());
       return false;
     }
   };
   manager.registerActionHandler<GenericHandler>();
 
   EXPECT_TRUE(manager.shouldExecute<SimpleAction>());
   EXPECT_FALSE(manager.shouldExecute<ParametricAction>(true));
 }
 
 TEST(DebugActionTest, ActionSpecificHandler) {
   DebugActionManager manager;
 
   // Handler that simply uses the input as the decider.
-  struct ActionSpecificHandler : public ParametricAction::Handler {
+  struct ActionSpecificHandler : ParametricAction::Handler {
     FailureOr<bool> shouldExecute(bool shouldExecuteParam) final {
       return shouldExecuteParam;
     }
   };
   manager.registerActionHandler<ActionSpecificHandler>();
 
   EXPECT_TRUE(manager.shouldExecute<ParametricAction>(true));
   EXPECT_FALSE(manager.shouldExecute<ParametricAction>(false));
 
   // There is no handler for the simple action, so it is always executed.
   EXPECT_TRUE(manager.shouldExecute<SimpleAction>());
 }
 
 TEST(DebugActionTest, DebugCounterHandler) {
   DebugActionManager manager;
 
   // Handler that uses the number of action executions as the decider.
-  struct DebugCounterHandler : public SimpleAction::Handler {
+  struct DebugCounterHandler : SimpleAction::Handler {
     FailureOr<bool> shouldExecute() final { return numExecutions++ < 3; }
     unsigned numExecutions = 0;
   };
   manager.registerActionHandler<DebugCounterHandler>();
 
   // Check that the action is executed 3 times, but no more after.
   EXPECT_TRUE(manager.shouldExecute<SimpleAction>());
   EXPECT_TRUE(manager.shouldExecute<SimpleAction>());
   EXPECT_TRUE(manager.shouldExecute<SimpleAction>());
   EXPECT_FALSE(manager.shouldExecute<SimpleAction>());
   EXPECT_FALSE(manager.shouldExecute<SimpleAction>());
 }
 
+TEST(DebugActionTest, NonOverlappingActionSpecificHandlers) {
+  DebugActionManager manager;
+
+  // One handler returns true and another returns false
+  struct SimpleActionHandler : SimpleAction::Handler {
+    FailureOr<bool> shouldExecute() final { return true; }
+  };
+  struct OtherSimpleActionHandler : OtherSimpleAction::Handler {
+    FailureOr<bool> shouldExecute() final { return false; }
+  };
+  manager.registerActionHandler<SimpleActionHandler>();
+  manager.registerActionHandler<OtherSimpleActionHandler>();
+  EXPECT_TRUE(manager.shouldExecute<SimpleAction>());
+  EXPECT_FALSE(manager.shouldExecute<OtherSimpleAction>());
+}
+
 } // namespace
 
 #endif

mlir/unittests/Support/DebugCounterTest.cpp

 //===- DebugCounterTest.cpp - Debug Counter Tests -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Support/DebugCounter.h"
+#include "mlir/Support/TypeID.h"
 #include "gmock/gmock.h"
 
 using namespace mlir;
 
 // DebugActionManager is only enabled in DEBUG mode.
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
 
 namespace {
 
-struct CounterAction : public DebugAction<> {
+struct CounterAction : public DebugAction<CounterAction> {
+  MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(CounterAction)
   static StringRef getTag() { return "counter-action"; }
   static StringRef getDescription() { return "Test action for debug counters"; }
 };
 
 TEST(DebugCounterTest, CounterTest) {
   std::unique_ptr<DebugCounter> counter = std::make_unique<DebugCounter>();
   counter->addCounter(CounterAction::getTag(), /*countToSkip=*/1,
                       /*countToStopAfter=*/3);