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 DontImplCloneOnSqlType attribute. #94

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/workflows/CI.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ jobs:
args: --manifest-path tests/Cargo.toml --features sqlite

- name: Install Diesel-CLI
if: ${{ matrix.rust != 'stable' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this change is necessary

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If matrix.rust is not stable, rustc is an old version.
So the latest diesel-cli dependency packages cannot be compiled.
e.g. serde_spanned@v0.6.5 needs rustc 1.67 or newer

Then, I changed it as follows.
If matrix.rust is stable, use the latest dielse-cli.
If it is not stable, use the older version of diesel-cli.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I guess I'd want to just bump the MSRV then, but no strong opinion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I may, I would like to bump the MSRV.
However, I did this because I thought there might be some reason to keep the MSRV at 1.65.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to bump it: https://github.com/diesel-rs/diesel/blob/2.1.x/rust-toolchain is at 1.68.0
:)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your advice.
I will bump the MSRV to 1.68.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to install diesel-cli@2.1.1 with rust:1.68.0.
Then it failed to compile.

❯ docker run --rm -it rust:1.68.0 cargo install --features postgres --no-default-features diesel_cli
    Updating crates.io index
  Downloaded diesel_cli v2.1.1
  Downloaded 1 crate (53.8 KB) in 0.25s
  Installing diesel_cli v2.1.1
  Downloaded anstyle-query v1.0.2
  Downloaded autocfg v1.1.0
  Downloaded anstyle-parse v0.2.3
  Downloaded anstream v0.6.7
  Downloaded aho-corasick v1.1.2
  Downloaded unicode-normalization v0.1.22
  Downloaded unicode-bidi v0.3.14
  Downloaded diesel_migrations v2.1.0
  Downloaded serde v1.0.195
  Downloaded anstyle v1.0.4
  Downloaded unicode-ident v1.0.12
  Downloaded overload v0.1.1
  Downloaded clap_complete v4.4.6
  Downloaded tinyvec_macros v0.1.1
  Downloaded dotenvy v0.15.7
  Downloaded byteorder v1.5.0
  Downloaded toml_datetime v0.6.5
  Downloaded memchr v2.7.1
  Downloaded diffy v0.3.0
  Downloaded iana-time-zone v0.1.59
  Downloaded bitflags v2.4.1
  Downloaded pq-sys v0.4.8
  Downloaded itoa v1.0.10
  Downloaded form_urlencoded v1.2.1
  Downloaded diesel_table_macro_syntax v0.1.0
  Downloaded equivalent v1.0.1
  Downloaded migrations_internals v2.1.0
  Downloaded colorchoice v1.0.0
  Downloaded serde_spanned v0.6.5
  Downloaded utf8parse v0.2.1
  Downloaded migrations_macros v2.1.0
  Downloaded serde_regex v1.1.0
  Downloaded clap_lex v0.6.0
  Downloaded heck v0.4.1
  Downloaded strsim v0.10.0
  Downloaded nu-ansi-term v0.46.0
  Downloaded quote v1.0.35
  Downloaded percent-encoding v2.3.1
  Downloaded diesel_derives v2.1.2
  Downloaded tinyvec v1.6.0
  Downloaded num-traits v0.2.17
  Downloaded proc-macro2 v1.0.76
  Downloaded indexmap v2.1.0
  Downloaded serde_derive v1.0.195
  Downloaded clap v4.4.16
  Downloaded toml v0.7.8
  Downloaded url v2.5.0
  Downloaded toml_edit v0.19.15
  Downloaded hashbrown v0.14.3
  Downloaded clap_builder v4.4.16
  Downloaded winnow v0.5.34
  Downloaded chrono v0.4.31
  Downloaded regex v1.10.2
  Downloaded syn v2.0.48
  Downloaded idna v0.5.0
  Downloaded regex-syntax v0.8.2
  Downloaded regex-automata v0.4.3
  Downloaded diesel v2.1.4
  Downloaded 58 crates (4.4 MB) in 0.45s
error: failed to compile `diesel_cli v2.1.1`, intermediate artifacts can be found at `/tmp/cargo-install7WzKFd`

Caused by:
  package `clap v4.4.16` cannot be built because it requires rustc 1.70.0 or newer, while the currently active rustc version is 1.68.0
  Try re-running cargo install with `--locked`

Could I bump to 1.70 ?
I was able to compile it with 1.70.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion here, I'm going to let @adwhit decide on this :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adwhit What do you think about this topic?

run: |
cargo install --features postgres --no-default-features diesel_cli@2.0.1
diesel --help

- name: Install Diesel-CLI
if: ${{ matrix.rust == 'stable' }}
run: |
cargo install --features postgres --no-default-features diesel_cli
diesel --help
Expand Down
20 changes: 18 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,14 @@ use syn::*;
/// * `#[db_rename = "variant"]` specifies the db name for a specific variant.
#[proc_macro_derive(
DbEnum,
attributes(PgType, DieselType, ExistingTypePath, DbValueStyle, db_rename)
attributes(
PgType,
DieselType,
ExistingTypePath,
DbValueStyle,
db_rename,
DontImplCloneOnSqlType
)
)]
pub fn derive(input: TokenStream) -> TokenStream {
let input: DeriveInput = parse_macro_input!(input as DeriveInput);
Expand Down Expand Up @@ -70,6 +77,9 @@ pub fn derive(input: TokenStream) -> TokenStream {
.expect("ExistingTypePath is not a valid token")
});
let new_diesel_mapping = Ident::new(new_diesel_mapping.as_ref(), Span::call_site());

let with_clone = !contains_in_attrs(&input.attrs, "DontImplCloneOnSqlType");

if let Data::Enum(syn::DataEnum {
variants: data_variants,
..
Expand All @@ -81,6 +91,7 @@ pub fn derive(input: TokenStream) -> TokenStream {
&pg_internal_type,
case_style,
&input.ident,
with_clone,
&data_variants,
)
} else {
Expand Down Expand Up @@ -115,6 +126,10 @@ fn val_from_attrs(attrs: &[Attribute], attrname: &str) -> Option<String> {
None
}

fn contains_in_attrs(attrs: &[Attribute], attrname: &str) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just has_attr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imitated val_from_attrs, but that is a better idea.

fn val_from_attrs(attrs: &[Attribute], attrname: &str) -> Option<String> {
for attr in attrs {
if attr.path().is_ident(attrname) {
match &attr.meta {
Meta::NameValue(MetaNameValue {
value:
Expr::Lit(ExprLit {
lit: Lit::Str(lit_str),
..
}),
..
}) => return Some(lit_str.value()),
_ => panic!(
"Attribute '{}' must have form: {} = \"value\"",
attrname, attrname
),
}
}
}
None
}

I will change it to has_attr.

attrs.iter().any(|e| e.path().is_ident(attrname))
}

/// Defines the casing for the database representation. Follows serde naming convention.
#[derive(Copy, Clone, Debug, PartialEq)]
enum CaseStyle {
Expand Down Expand Up @@ -148,6 +163,7 @@ fn generate_derive_enum_impls(
pg_internal_type: &str,
case_style: CaseStyle,
enum_ty: &Ident,
with_clone: bool,
variants: &syn::punctuated::Punctuated<Variant, syn::token::Comma>,
) -> TokenStream {
let modname = Ident::new(&format!("db_enum_impl_{}", enum_ty), Span::call_site());
Expand Down Expand Up @@ -201,7 +217,7 @@ fn generate_derive_enum_impls(
match existing_mapping_path {
Some(path) => {
let common_impls_on_existing_diesel_mapping = generate_common_impls(path, enum_ty);
let postgres_impl = generate_postgres_impl(path, enum_ty, true);
let postgres_impl = generate_postgres_impl(path, enum_ty, with_clone);
Some(quote! {
#common_impls_on_existing_diesel_mapping
#postgres_impl
Expand Down
2 changes: 2 additions & 0 deletions tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ mod nullable;
#[cfg(feature = "postgres")]
mod pg_array;
#[cfg(feature = "postgres")]
mod pg_clone_impl;
#[cfg(feature = "postgres")]
mod pg_remote_type;
mod simple;
mod value_style;
35 changes: 35 additions & 0 deletions tests/src/pg_clone_impl.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
use diesel::prelude::*;

#[cfg(feature = "postgres")]
#[derive(diesel::sql_types::SqlType)]
#[diesel(postgres_type(name = "my_remote_enum"))]
pub struct MyRemoteEnumMapping;

table! {
use diesel::sql_types::Integer;
use super::MyRemoteEnumMapping;
test_remote {
id -> Integer,
my_enum -> MyRemoteEnumMapping,
}
}

#[derive(Insertable, Queryable, Identifiable, Debug, Clone, PartialEq)]
#[diesel(table_name = test_remote)]
struct Data {
id: i32,
my_enum: MyRemoteEnum,
}

#[derive(Debug, PartialEq, Clone, diesel_derive_enum::DbEnum)]
#[ExistingTypePath = "MyRemoteEnumMapping"]
pub enum MyRemoteEnum {
This,
That,
}

#[test]
fn clone_impl_on_sql_type() {
let x = MyRemoteEnumMapping {};
let _ = x.clone();
}