Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a clear error message for when an integer enum doesn't have derive(Copy) #630

Closed
leighmcculloch opened this issue Sep 17, 2022 · 6 comments · Fixed by #1264 or #1293 · May be fixed by #1292
Closed

Add a clear error message for when an integer enum doesn't have derive(Copy) #630

leighmcculloch opened this issue Sep 17, 2022 · 6 comments · Fixed by #1264 or #1293 · May be fixed by #1292
Assignees

Comments

@leighmcculloch
Copy link
Member

Integer enums to be used as contracttype's need to have derive(Copy). This is so that we can convert a reference to an integer enum into a u32.

Right now the error message displayed is ambiguous because it is an error generated from generated code that uses the type in a way that requires a Copy.

We should try and detect from the other attributes on the type whether the type has Copy derived. If it hasn't, we should display a focused error message.

This is a follow up to #217

@jayz22
Copy link
Contributor

jayz22 commented Apr 23, 2024

Assigning myself to take a look at this one.

github-merge-queue bot pushed a commit that referenced this issue Apr 25, 2024
### What

Resolves #630

### Why

Previously missing `derive(Copy)` on an UDT enum integer shows up as a
cryptic error way down the stack (the contract itself will compile just
fine, you get the following error when trying to compile code (e.g. a
native unit test) using the generated contract client):

```
error[E0507]: cannot move out of `*self` which is behind a shared reference
 --> tests/udt_enum/src/lib.rs:4:1
  |
4 | #[contracttype]
  | ^^^^^^^^^^^^^^^ move occurs because `*self` has type `UdtEnum`, which does not implement the `Copy` trait
  |
  = note: this error originates in the attribute macro `contracttype` (in Nightly builds, run with -Z macro-backtrace for more info)
```

This change gives a clear error message during contract macro expansion
(before compile time):
```
error: enum integer UdtEnum must have `derive(Copy)`
 --> tests/udt_enum/src/lib.rs:6:10
  |
6 | pub enum UdtEnum {
  |          ^^^^^^^
```

### Known limitations

[TODO or N/A]
@jayz22
Copy link
Contributor

jayz22 commented Jun 25, 2024

The change has been reverted in #1283 due to uncovered corner cases. Reopening this issue.

@jayz22 jayz22 reopened this Jun 25, 2024
@leighmcculloch
Copy link
Member Author

I don't think we can overcome the uncovered corner cases unfortunately, as there's no way to catch a derive(Copy) if it is placed above the attribute, which was a limitation of the attributes I wasn't aware of at the time.

Another solution is we stop assuming that the enums should be copy in other macro generated code, then this becomes a non-issue for a slight probable loss of performance in some cases. This might not be realistic though.

@leighmcculloch
Copy link
Member Author

@jayz22 I've opened the following change that removes the need for an error message by making it so that errors no longer need to derive Copy:

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Jul 11, 2024

Another option from the friendly folks in Rust discord (link) is to insert this into the generated code:

const _: () = { fn assert_copy(v: #enum_ident) -> impl Copy { v } };

That results then in this very direct error:

error[E0277]: the trait bound `Error: core::marker::Copy` is not satisfied
 --> tests/errors/src/lib.rs:9:1
  |
9 | #[contracterror]
  | ^^^^^^^^^^^^^^^^ the trait `core::marker::Copy` is not implemented for `Error`
  |
help: consider annotating `Error` with `#[derive(Copy)]`
  |
11+ #[derive(Copy)]
12| pub enum Error {
  |

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Jul 11, 2024

@jayz22 I've opened a new change that uses the approach above to generate a nicer error message. Since this works I think it is preferred because it allows us to do conversions from the enum types to their integer types in the generated code without having to implement match tables between the values and integer constants.

github-merge-queue bot pushed a commit that referenced this issue Jul 11, 2024
…ints (#1293)

### What
Add nice compiler error when Copy is missing on error enums and enum
ints.

### Why
The existing compiler error is not very intuitive. The existing error
will still be outputted but a more direct error describing the problem
and the fix will be output first.

#### Before

```
error[E0507]: cannot move out of `*val` which is behind a shared reference
  --> tests/errors/src/lib.rs:15:1
   |
15 | #[contracterror]
   | ^^^^^^^^^^^^^^^^ move occurs because `*val` has type `Error`, which does not implement the `Copy` trait
   |
   = note: this error originates in the attribute macro `contracterror` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider cloning the value if the performance cost is acceptable
   |
15 | #[contracterror].clone()
   |                 ++++++++
```

#### After

```
error[E0277]: the trait bound `Error: core::marker::Copy` is not satisfied
  --> tests/errors/src/lib.rs:15:1
   |
15 | #[contracterror]
   | ^^^^^^^^^^^^^^^^ the trait `core::marker::Copy` is not implemented for `Error`
   |
help: consider annotating `Error` with `#[derive(Copy)]`
   |
17 + #[derive(Copy)]
18 | pub enum Error {
   |

error[E0507]: cannot move out of `*val` which is behind a shared reference
  --> tests/errors/src/lib.rs:15:1
   |
15 | #[contracterror]
   | ^^^^^^^^^^^^^^^^ move occurs because `*val` has type `Error`, which does not implement the `Copy` trait
   |
   = note: this error originates in the attribute macro `contracterror` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider cloning the value if the performance cost is acceptable
   |
15 | #[contracterror].clone()
   |                 ++++++++
```

Close #630
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment