[mlir] explicitly delete copy ctor for DialectRegistry and OperationState

[makslevental]

Both of these classes have fields involving unique_ptrs and thus can't be copied.
Holding a unique_ptr should trigger implicit deletion of the copy/assignment constructors, however
not when held through a container:

struct Foo {  
  explicit Foo();
  std::vector<std::unique_ptr<Bar>> ptr; // Does not implicitly delete its cpy/assign ctors
};

This results in a complicated diagnostics when attempting to copy such a class (it'll point to the
inner implementation of the container), instead of a nicer error message about Foo not being able
to be copied/assigned. Explicitly deleting the ctors provides an immediate diagnostic about the issue.

Fixes #118388

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvmbot]

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Maksim Levental (makslevental)

Changes

Both of these classes have fields involving unique_ptrs and thus can't be copied.

Fixes #118388

---

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

2 Files Affected:

• (modified) mlir/include/mlir/IR/DialectRegistry.h (+2)
• (modified) mlir/include/mlir/IR/OperationSupport.h (+2-2)

diff --git a/mlir/include/mlir/IR/DialectRegistry.h b/mlir/include/mlir/IR/DialectRegistry.h
index d3d53488fe72d..7bcf1eda7c636 100644
--- a/mlir/include/mlir/IR/DialectRegistry.h
+++ b/mlir/include/mlir/IR/DialectRegistry.h
@@ -143,6 +143,8 @@ class DialectRegistry {
 
 public:
   explicit DialectRegistry();
+  DialectRegistry(const DialectRegistry &) = delete;
+  DialectRegistry &operator=(const DialectRegistry &other) = delete;
 
   template <typename ConcreteDialect>
   void insert() {
diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h
index 2d9fb2bc5859e..0046d977c68f4 100644
--- a/mlir/include/mlir/IR/OperationSupport.h
+++ b/mlir/include/mlir/IR/OperationSupport.h
@@ -985,9 +985,9 @@ struct OperationState {
                  BlockRange successors = {},
                  MutableArrayRef<std::unique_ptr<Region>> regions = {});
   OperationState(OperationState &&other) = default;
-  OperationState(const OperationState &other) = default;
   OperationState &operator=(OperationState &&other) = default;
-  OperationState &operator=(const OperationState &other) = default;
+  OperationState(const OperationState &other) = delete;
+  OperationState &operator=(const OperationState &other) = delete;
   ~OperationState();
 
   /// Get (or create) a properties of the provided type to be set on the

[makslevental]

cc @0xddom

[0xddom]

I checked OperationState's destructor and seems to be doing meaningful work so I retract my rule of zero comment in the issue. This lgtm

[joker-eph]

Is this expected that explicitly deleting these should change the behavior compared to have them implicitly deleted because of the unique_ptr member?

I feel like the title/description should elaborate bit more on what's going on here and what are we "fixing".

[0xddom]

Is this expected that explicitly deleting these should change the behavior compared to have them implicitly deleted because of the unique_ptr member?

For OperationState, at least, looks like having the compiler implicitly remove them should be enough. However, I'm also running into this issue with the StorageUniquer class, where the copy constructor is left implicit. I'm trying to create Rust bindings for MLIR via autocxx and the autogenerated code fails to compile because of calls to these copy constructors. I raised an issue in their repo about this in case it's a bug on the tool.

[makslevental]

Is this expected that explicitly deleting these should change the behavior compared to have them implicitly deleted because of the unique_ptr member?

the problem is they are not being implicitly deleted because (as the original issue documents) SmallVectorImpl defines a copy ctor that is not smart enough.

I feel like the title/description should elaborate bit more on what's going on here and what are we "fixing".

updated

[joker-eph]

the problem is they are not being implicitly deleted because (as the original issue documents)

I don't get it what you mean by "they are not being implicitly deleted", we can't copy DialectRegistry right now can we?

SmallVectorImpl defines a copy ctor that is not smart enough.

Maybe but neither the description/title here neither the bug are very clear to me on what's happening actually.

The title here says: "explicitly delete copy ctor to prevent copying DialectRegistry and OperationState", which implies that right now without explicitly deleting them we can copy them. But how do you copy them today?

Here is a minimal test cases showing why the current description of the issue is confusing to me:

#include <memory>
class Bar {};
struct Foo {  
  std::unique_ptr<Bar> ptr;
};
void copy(Foo f1, Foo f2) {
   f1 = f2;
}

Yield:

<source>:8:9: error: use of deleted function 'Foo& Foo::operator=(const Foo&)'
    8 |    f1 = f2;
      |         ^~
<source>:3:8: note: 'Foo& Foo::operator=(const Foo&)' is implicitly deleted because the default definition would be ill-formed:
    3 | struct Foo {
      |        ^~~

(same with the copy ctor).

Can you provide a test showing what was failing before and is failing now?

[joker-eph]

Or is this patch an "improve diagnostics" kind of thing? Having error: call to deleted constructor of 'mlir::DialectRegistry' instead of having it on the data member?

[joker-eph]

LG with the updated description.

[WillFroom]

I think this unintentionally changes the behavior to not allowing moving of DialectRegistry, I'll create a patch to add the explicit move constructor