Cannot derive DbEnum when schema.rs is separated into different crates

[yagince]

I have separated schema.rs into different crates as workspace because it takes time for macro expansion of diesel.
If I define an enum in the workspace that references that crate and try to derive DbEnum to that enum, I get a compile error.

error[E0117]: only traits defined in the current crate can be implemented for types defined outside of the crate
   |
   | #[derive(diesel_derive_enum::DbEnum)]
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |          |
   |          impl doesn't use only types from inside the current crate
   |          `XXXX` is not defined in the current crate
   |
   = note: define and implement a trait or new type instead
   = note: this error originates in the derive macro `diesel_derive_enum::DbEnum` (in Nightly builds, run with -Z macro-backtrace for more info)

diesel-derive-enum/src/lib.rs

Lines 389 to 393 in 36c44a2

	impl Clone for #diesel_mapping {
	fn clone(&self) -> Self {
	#diesel_mapping
	}
	}

Here, the reason was that diesel-derive-enum implements Clone for the type generated by diesel-cli.

Currently, you can add Clone to the types generated by diesel-cli by adding Clone to custom_derive_types, so you can add Clone to the types generated by diesel-cli.
I think the above code is no longer necessary.
What do you think?

[adwhit]

Interesting, thanks for the bug report. I made a test branch with the impl removed and the tests still passed - 5cb99b1 (ignore spurious failure for 1.57)

It seems the Clone impl was added during the refactor for 2.0 - here e2bad95. @Ten0 Do you remeber why this was added?

[Ten0]

IIRC Clone bound is sometimes necessary around boxed expressions and/or joins within Diesel. (If I added it it's surely because my own project didn't compile without and it was present before)

As far as I know it is true that it's now possible to add this implementation via this diesel.toml configuration instead.
I don't think that option was available at the time this was implemented, and it would seems indeed more appropriate to have it there than in DbEnum.

However since this is probably typically not set for existing projects or even the default for new projects (https://github.com/diesel-rs/diesel/blob/ce16e91bea0bd85342ff1cf1d683a8b8cb1a6ffc/diesel_cli/src/default_files/diesel.toml#L6) I don't think it would be reasonable to by default not generate it, as that would be a significant breaking change.
But opt in to disable would be OK I think.

Also I think diesel would probably accept to add the clone bound as a default in the default diesel.toml of the cli if you were to PR that there. Once that is released maybe changing the behavior of this crate would make more sense but it would still seem difficult to release it as non breaking.

That being said as far as I'm concerned I also have separate schema crates and I define my DbEnums in the same crate as I define my schema and I have no issue, and it also looks like it works better if several crates depend on the schema crate.

[adwhit]

Thanks, really useful info.

I agree that we can't wholesale remove the Clone impl without causing breakage, but adding a feature flag (e.g. no-clone-impl) is possible. It would break the Rust rule for feature composition, however (features should be strictly additive).

Alternatively could have clone-impl as a default feature. Not sure about the semver impliations of that.

I define my DbEnums in the same crate as I define my schema and I have no issue

This does sound like the most practical solution

[Ten0]

adding a feature flag

I was thinking adding an attribute rather, since this is pretty edge-casey. It allows the thing, just makes it slightly more verbose. (But not sure that's worth the added complexity considering the available alternate solution)

[yagince]

@adwhit @Ten0

Thank you for considering my proposal.

My project is separated into three workspaces.

1. schema
2. core
3. web-api

The dependencies between crates are as follows:
web-api -> core -> schema

The schema crate only has schema definitions,
structs that are mapped to schemas are in core .
These structs is also used in GraphQL, and derive definitions related to GraphQL are also implemented in core.
So if I define a DbEnum in the schema crate, requires to depend on GraphQL's crate.
In that case, the schema crate has meaning other than the schema definitions.
It will be a very wide-meaning crate, so I wanted to somehow define DbEnum in the core crate.
(I though we have to separate db models and web-api models, but It's difficult because the scope of influence is too wide than what I want to do now.)

Indeed, as you said, I think this is a pretty edge case.

However, depending on the situation where diesel is used, clone-impl may not be necessary, so whether clone-impl is necessary or not depends on the situation.
This is a problem on the side of using diesel, and I thought it might be possible to solve it outside of diesel-derive-enum, so I made this proposal.

adding a feature flag (e.g. no-clone-impl)

I got the feeling that the solution is not a very good solution.

Maybe the only solution is to define the DbEnum within the same crate?

[Ten0]

That does seem like a legit use-case.

I think the best solution is to:

• Add attribute to DbEnum to not generate clone impl
• Add clone derive to default attributes for new diesel projects (diesel pr to the default diesel.toml file I linked above, feel free to @ me)
• Add the derive to the list of derives in your existing diesel.toml, and set the attribute in your DbEnum calls.

[yagince]

@Ten0

thank you for your reply.

Add attribute to DbEnum to not generate clone impl

Is it better to add DbEnum attribute instead of feature flag?

#[derive(diesel_derive_enum::DbEnum)]
#[NoClone]
pub enum SomeEnum {
    A,
    B,
}

Does it look like the above?

[Ten0]

Is it better to add DbEnum attribute instead of feature flag?

I think so, for the reason that features generally should generally be additive (enabling a feature should not break existing code), and this is still edge-case enough that we probably don't want to break that rule

Does it look like the above?

The convention is generally to use snake_case for attributes but it seems so far in this crate we're using Pascal (e.g. DieselType, ExistingTypePath...), so it's probably better to stay consistent. However I think we could be slightly more explicit in case there are more derives:

#[derive(diesel_derive_enum::DbEnum, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
#[DontImplCloneOnSqlType]
pub enum SomeEnum {
    A,
    B,
}

Then it's just about passing false here if the attribute is specified:

diesel-derive-enum/src/lib.rs

Line 204 in 36c44a2

let postgres_impl = generate_postgres_impl(path, enum_ty, true);

[yagince]

Thank you.
I understand everything now.

I will consider how to fix.

[nstolmaker]

Bumping this request, as I ran into this same limitation after a full day of trying to get Enums working in Diesel. My use case is a shared crate, where as many types as possible are defined, like the core package that @yagince mentioned.

[fiadliel]

In addition to the patch by @yagince, could there be a 3.0.0 with Clone removed? Since diesel 2.2 is out for a while now, it should become the default.