[mlir][TableGen] Verify compatibility of tblgen::Method properties

[andrey-golubev]

Following a recent discovery of a method being defined both "inline" and "declaration" (declaration implying no definition), verify the method properties in general to fail early in the development and avoid accidental bugs (especially for "opt-in" features).

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Andrei Golubev (andrey-golubev)

Changes

Following a recent discovery of a method being defined both "inline" and "declaration" (declaration implying no definition), verify the method properties in general to fail early in the development and avoid accidental bugs (especially for "opt-in" features).

---

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

2 Files Affected:

• (modified) mlir/include/mlir/TableGen/Class.h (+12-2)
• (modified) mlir/lib/TableGen/Class.cpp (+32)

diff --git a/mlir/include/mlir/TableGen/Class.h b/mlir/include/mlir/TableGen/Class.h
index f750a34a3b2ba..8000c07b52b2e 100644
--- a/mlir/include/mlir/TableGen/Class.h
+++ b/mlir/include/mlir/TableGen/Class.h
@@ -332,13 +332,19 @@ class Method : public ClassDeclarationBase<ClassDeclaration::Method> {
       : properties(properties),
         methodSignature(std::forward<RetTypeT>(retType),
                         std::forward<NameT>(name), std::forward<Args>(args)...),
-        methodBody(properties & Declaration) {}
+        methodBody(properties & Declaration) {
+    assert(methodPropertiesAreCompatible(properties) &&
+           "invalid combination of properties specified");
+  }
   /// Create a method with a return type, a name, method properties, and a list
   /// of parameters.
   Method(StringRef retType, StringRef name, Properties properties,
          std::initializer_list<MethodParameter> params)
       : properties(properties), methodSignature(retType, name, params),
-        methodBody(properties & Declaration) {}
+        methodBody(properties & Declaration) {
+    assert(methodPropertiesAreCompatible(properties) &&
+           "invalid combination of properties specified");
+  }
 
   // Define move constructor and assignment operator to prevent copying.
   Method(Method &&) = default;
@@ -402,6 +408,10 @@ class Method : public ClassDeclarationBase<ClassDeclaration::Method> {
   MethodBody methodBody;
   /// Deprecation message if the method is deprecated.
   std::optional<std::string> deprecationMessage;
+
+  /// Utility method to verify method properties correctness.
+  [[maybe_unused]] static bool
+  methodPropertiesAreCompatible(Properties properties);
 };
 
 /// This enum describes C++ inheritance visibility.
diff --git a/mlir/lib/TableGen/Class.cpp b/mlir/lib/TableGen/Class.cpp
index c65f67d50a47d..81f1aee73a7f0 100644
--- a/mlir/lib/TableGen/Class.cpp
+++ b/mlir/lib/TableGen/Class.cpp
@@ -159,6 +159,38 @@ void Method::writeDefTo(raw_indented_ostream &os, StringRef namePrefix) const {
   os << "}\n\n";
 }
 
+bool Method::methodPropertiesAreCompatible(Properties properties) {
+  const bool isStatic = (properties & Method::Static);
+  const bool isConstructor = (properties & Method::Constructor);
+  // const bool isPrivate = (properties & Method::Private);
+  const bool isDeclaration = (properties & Method::Declaration);
+  const bool isInline = (properties & Method::Inline);
+  const bool isConstexprValue = (properties & Method::ConstexprValue);
+  const bool isConst = (properties & Method::Const);
+
+  // Note: assert to immediately fail and thus simplify debugging.
+  if (isStatic && isConstructor) {
+    assert(false && "constructor cannot be static");
+    return false;
+  }
+  if (isConstructor && isConst) { // albeit constexpr is fine
+    assert(false && "constructor cannot be const");
+    return false;
+  }
+  if (isDeclaration && isInline) {
+    assert(false &&
+           "declaration implies no definition and thus cannot be inline");
+    return false;
+  }
+  if (isDeclaration && isConstexprValue) {
+    assert(false &&
+           "declaration implies no definition and thus cannot be constexpr");
+    return false;
+  }
+
+  return true;
+}
+
 //===----------------------------------------------------------------------===//
 // Constructor definitions
 //===----------------------------------------------------------------------===//

[andrey-golubev]

Note: this does not fail currently in the place fixed here - #147979 because there are no tests :|

[joker-eph]

Can you please turn all the asserts into report_fatal_error?
TableGen checks should fire always and not only in debug mode.